[commit: ghc] master: Don't call DEAD_WEAK finalizer again on shutdown (#7170) (dfdc50d)

[git at git.haskell.org]

Repository : ssh://git@git.haskell.org/ghc

On branch  : master
Link       : http://ghc.haskell.org/trac/ghc/changeset/dfdc50d666498c5a1118557d67209fe067c61cc1/ghc

>---------------------------------------------------------------

commit dfdc50d666498c5a1118557d67209fe067c61cc1
Author: Simon Marlow <marlowsd at gmail.com>
Date:   Mon Jun 1 21:34:02 2015 +0100

    Don't call DEAD_WEAK finalizer again on shutdown (#7170)
    
    Summary:
    There's a race condition like this:
    
      # A foreign pointer gets promoted to the last generation
      # It has its finalizer called manually
      # We start shutting down the runtime in `hs_exit_` from the main
        thread
      # A minor GC starts running (`scheduleDoGC`) on one of the threads
      # The minor GC notices that we're in `SCHED_INTERRUPTING` state and
        advances to `SCHED_SHUTTING_DOWN`
      # The main thread tries to do major GC (with `scheduleDoGC`), but it
        exits early because we're in `SCHED_SHUTTING_DOWN` state
      # We end up with a `DEAD_WEAK` left on the list of weak pointers of
        the last generation, because it relied on major GC removing it from
        that list
    
    This change:
      * Ignores DEAD_WEAK finalizers when shutting down
      * Makes the major GC on shutdown more likely
      * Fixes a bogus assert
    
    Test Plan:
    before this diff https://ghc.haskell.org/trac/ghc/ticket/7170#comment:5
    reproduced and after it doesn't
    
    Reviewers: ezyang, austin, simonmar
    
    Reviewed By: simonmar
    
    Subscribers: bgamari, thomie
    
    Differential Revision: https://phabricator.haskell.org/D921
    
    GHC Trac Issues: #7170


>---------------------------------------------------------------

dfdc50d666498c5a1118557d67209fe067c61cc1
 rts/Schedule.c    |  7 +++++--
 rts/Weak.c        | 11 ++++++++++-
 rts/sm/MarkWeak.c |  3 ++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/rts/Schedule.c b/rts/Schedule.c
index 957aa4b..f81fc0e 100644
--- a/rts/Schedule.c
+++ b/rts/Schedule.c
@@ -251,7 +251,7 @@ schedule (Capability *initialCapability, Task *task)
     case SCHED_INTERRUPTING:
         debugTrace(DEBUG_sched, "SCHED_INTERRUPTING");
         /* scheduleDoGC() deletes all the threads */
-        scheduleDoGC(&cap,task,rtsFalse);
+        scheduleDoGC(&cap,task,rtsTrue);
 
         // after scheduleDoGC(), we must be shutting down.  Either some
         // other Capability did the final GC, or we did it above,
@@ -1458,6 +1458,7 @@ scheduleDoGC (Capability **pcap, Task *task USED_IF_THREADS,
     Capability *cap = *pcap;
     rtsBool heap_census;
     nat collect_gen;
+    rtsBool major_gc;
 #ifdef THREADED_RTS
     nat gc_type;
     nat i, sync;
@@ -1476,6 +1477,7 @@ scheduleDoGC (Capability **pcap, Task *task USED_IF_THREADS,
     // Figure out which generation we are collecting, so that we can
     // decide whether this is a parallel GC or not.
     collect_gen = calcNeeded(force_major || heap_census, NULL);
+    major_gc = (collect_gen == RtsFlags.GcFlags.generations-1);
 
 #ifdef THREADED_RTS
     if (sched_state < SCHED_INTERRUPTING
@@ -1618,8 +1620,9 @@ delete_threads_and_gc:
      * We now have all the capabilities; if we're in an interrupting
      * state, then we should take the opportunity to delete all the
      * threads in the system.
+     * Checking for major_gc ensures that the last GC is major.
      */
-    if (sched_state == SCHED_INTERRUPTING) {
+    if (sched_state == SCHED_INTERRUPTING && major_gc) {
         deleteAllThreads(cap);
 #if defined(THREADED_RTS)
         // Discard all the sparks from every Capability.  Why?
diff --git a/rts/Weak.c b/rts/Weak.c
index f8faa4e..92f1bdb 100644
--- a/rts/Weak.c
+++ b/rts/Weak.c
@@ -43,7 +43,16 @@ runAllCFinalizers(StgWeak *list)
     }
 
     for (w = list; w; w = w->link) {
-        runCFinalizers((StgCFinalizerList *)w->cfinalizers);
+        // We need to filter out DEAD_WEAK objects, because it's not guaranteed
+        // that the list will not have them when shutting down.
+        // They only get filtered out during GC for the generation they
+        // belong to.
+        // If there's no major GC between the time that the finalizer for the
+        // object from the oldest generation is manually called and shutdown
+        // we end up running the same finalizer twice. See #7170.
+        if (w->header.info != &stg_DEAD_WEAK_info) {
+            runCFinalizers((StgCFinalizerList *)w->cfinalizers);
+        }
     }
 
     if (task != NULL) {
diff --git a/rts/sm/MarkWeak.c b/rts/sm/MarkWeak.c
index c5a107c..60ac53f 100644
--- a/rts/sm/MarkWeak.c
+++ b/rts/sm/MarkWeak.c
@@ -348,7 +348,8 @@ static void checkWeakPtrSanity(StgWeak *hd, StgWeak *tl)
 {
     StgWeak *w, *prev;
     for (w = hd; w != NULL; prev = w, w = w->link) {
-        ASSERT(INFO_PTR_TO_STRUCT(UNTAG_CLOSURE((StgClosure*)w)->header.info)->type == WEAK);
+        ASSERT(INFO_PTR_TO_STRUCT(UNTAG_CLOSURE((StgClosure*)w)->header.info)->type == WEAK
+            || UNTAG_CLOSURE((StgClosure*)w)->header.info == &stg_DEAD_WEAK_info);
         checkClosure((StgClosure*)w);
     }
     if (tl != NULL) {