Remove the interface function fullCollectNoStack from the gcinterface

changelog/druntime.removenostackcollect.dd

@@ -0,0 +1,38 @@
+Remove all `collectNoStack` functions and API from druntime.
+
+The function `collectNoStack` in the D garbage collector performed a
+collection, without using any roots from thread stacks or thread-local-storage.
+The danger of running this mechanism is that any blocks of memory which only
+have a reference from a thread might be collected, while the thread is still
+running and possibly using the memory.
+
+The only time this function was called was at GC termination. At GC
+termination, the GC is about to be destroyed, and so we want to run as many
+destructors as possible. However, if some thread is using GC-allocated memory,
+cleaning up that memory isn't going to help matters. Either it will crash after
+the GC cleans the memory, or it will crash after the GC is destroyed.
+
+The original purpose of this function (from D1) was to ensure simple uses of
+the GC were cleaned up in small test programs, as this mechanism was only used
+on single-threaded programs (and of course, at program exit). Also note at the
+time, D1 was 32-bit, and false pointers where much more common. Avoiding
+scanning stacks would aid in avoiding seemingly random behavior in cleanup.
+However, as shown below, there are more deterministic ways to ensure data is
+always cleaned up.
+
+Today, the dangers are much greater that such a function is even callable --
+any call to such a function would immediately start use-after-free memory
+corruption in any thread that is still running. Therefore, we are removing the
+functionality entirely, and simply doing a standard GC cleanup (scanning stacks
+and all). One less footgun is the benefit for having less guaranteed GC clean
+up at program exit.
+
+In addition, the GC today is a bit smarter about where the valid stack is, so
+there is even less of a chance of leaving blocks unfinalized.
+
+As always, the GC is *not* guaranteed to clean up any block at the end of
+runtime. Any change in behavior with code that had blocks clean up before, but
+no longer are cleaned up is still within specification. And if you want the
+behavior that absolutely cleans all blocks, you can use the
+`--DRT-gcopt=cleanup:finalize` druntime configuration option, which will clean
+up all blocks without even scanning.

druntime/src/core/gc/gcinterface.d

@@ -53,11 +53,6 @@ interface GC
      */
     void collect() nothrow;
 
-    /**
-     *
-     */
-    void collectNoStack() nothrow;
-
     /**
      * minimize free space usage
      */

druntime/src/core/internal/gc/impl/conservative/gc.d

@@ -1252,12 +1252,6 @@ class ConservativeGC : GC
     }
 
 
-    void collectNoStack() nothrow
-    {
-        fullCollectNoStack();
-    }
-
-
     /**
      * Begins a full collection, scanning all stack segments for roots.
      *
@@ -1290,21 +1284,6 @@ class ConservativeGC : GC
     }
 
 
-    /**
-     * Begins a full collection while ignoring all stack segments for roots.
-     */
-    void fullCollectNoStack() nothrow
-    {
-        // Since a finalizer could launch a new thread, we always need to lock
-        // when collecting.
-        static size_t go(Gcx* gcx) nothrow
-        {
-            return gcx.fullcollect(true, true, true); // standard stop the world
-        }
-        runLocked!go(gcx);
-    }
-
-
     /**
      * Minimize free space usage.
      */
@@ -2556,14 +2535,11 @@ struct Gcx
     }
 
     // collection step 2: mark roots and heap
-    void markAll(alias markFn)(bool nostack) nothrow
+    void markAll(alias markFn)() nothrow
     {
-        if (!nostack)
-        {
-            debug(COLLECT_PRINTF) printf("\tscan stacks.\n");
-            // Scan stacks and registers for each paused thread
-            thread_scanAll(&markFn);
-        }
+        debug(COLLECT_PRINTF) printf("\tscan stacks.\n");
+        // Scan stacks registers, and TLS for each paused thread
+        thread_scanAll(&markFn);
 
         // Scan roots[]
         debug(COLLECT_PRINTF) printf("\tscan roots[]\n");
@@ -2584,14 +2560,11 @@ struct Gcx
     }
 
     version (COLLECT_PARALLEL)
-    void collectAllRoots(bool nostack) nothrow
+    void collectAllRoots() nothrow
     {
-        if (!nostack)
-        {
-            debug(COLLECT_PRINTF) printf("\tcollect stacks.\n");
-            // Scan stacks and registers for each paused thread
-            thread_scanAll(&collectRoots);
-        }
+        debug(COLLECT_PRINTF) printf("\tcollect stacks.\n");
+        // Scan stacks registers and TLS for each paused thread
+        thread_scanAll(&collectRoots);
 
         // Scan roots[]
         debug(COLLECT_PRINTF) printf("\tcollect roots[]\n");
@@ -2920,7 +2893,7 @@ struct Gcx
     }
 
     version (COLLECT_FORK)
-    ChildStatus markFork(bool nostack, bool block, bool doParallel) nothrow
+    ChildStatus markFork(bool block, bool doParallel) nothrow
     {
         // Forking is enabled, so we fork() and start a new concurrent mark phase
         // in the child. If the collection should not block, the parent process
@@ -2936,11 +2909,11 @@ struct Gcx
         int child_mark() scope
         {
             if (doParallel)
-                markParallel(nostack);
+                markParallel();
             else if (ConservativeGC.isPrecise)
-                markAll!(markPrecise!true)(nostack);
+                markAll!(markPrecise!true)();
             else
-                markAll!(markConservative!true)(nostack);
+                markAll!(markConservative!true)();
             return 0;
         }
 
@@ -2999,11 +2972,11 @@ struct Gcx
                     // do the marking in this thread
                     disableFork();
                     if (doParallel)
-                        markParallel(nostack);
+                        markParallel();
                     else if (ConservativeGC.isPrecise)
-                        markAll!(markPrecise!false)(nostack);
+                        markAll!(markPrecise!false)();
                     else
-                        markAll!(markConservative!false)(nostack);
+                        markAll!(markConservative!false)();
                 } else {
                     assert(r == ChildStatus.done);
                     assert(r != ChildStatus.running);
@@ -3016,7 +2989,7 @@ struct Gcx
      * Return number of full pages free'd.
      * The collection is done concurrently only if block and isFinal are false.
      */
-    size_t fullcollect(bool nostack = false, bool block = false, bool isFinal = false) nothrow
+    size_t fullcollect(bool block = false, bool isFinal = false) nothrow
     {
         // It is possible that `fullcollect` will be called from a thread which
         // is not yet registered in runtime (because allocating `new Thread` is
@@ -3098,7 +3071,7 @@ Lmark:
             {
                 version (COLLECT_FORK)
                 {
-                    auto forkResult = markFork(nostack, block, doParallel);
+                    auto forkResult = markFork(block, doParallel);
                     final switch (forkResult)
                     {
                         case ChildStatus.error:
@@ -3125,14 +3098,14 @@ Lmark:
             else if (doParallel)
             {
                 version (COLLECT_PARALLEL)
-                    markParallel(nostack);
+                    markParallel();
             }
             else
             {
                 if (ConservativeGC.isPrecise)
-                    markAll!(markPrecise!false)(nostack);
+                    markAll!(markPrecise!false)();
                 else
-                    markAll!(markConservative!false)(nostack);
+                    markAll!(markConservative!false)();
             }
 
             thread_processGCMarks(&isMarked);
@@ -3184,7 +3157,7 @@ Lmark:
 
         updateCollectThresholds();
         if (doFork && isFinal)
-            return fullcollect(true, true, false);
+            return fullcollect(true, false);
         return freedPages;
     }
 
@@ -3300,10 +3273,10 @@ Lmark:
     shared uint stoppedThreads;
     bool stopGC;
 
-    void markParallel(bool nostack) nothrow
+    void markParallel() nothrow
     {
         toscanRoots.clear();
-        collectAllRoots(nostack);
+        collectAllRoots();
         if (toscanRoots.empty)
             return;
 

druntime/src/core/internal/gc/impl/manual/gc.d

@@ -79,10 +79,6 @@ class ManualGC : GC
     {
     }
 
-    void collectNoStack() nothrow
-    {
-    }
-
     void minimize() nothrow
     {
     }

druntime/src/core/internal/gc/impl/proto/gc.d

@@ -76,10 +76,6 @@ class ProtoGC : GC
     {
     }
 
-    void collectNoStack() nothrow
-    {
-    }
-
     void minimize() nothrow
     {
     }

druntime/src/core/internal/gc/proxy.d

@@ -106,18 +106,7 @@ extern (C)
                 case "none":
                     break;
                 case "collect":
-                    // NOTE: There may be daemons threads still running when this routine is
-                    //       called.  If so, cleaning memory out from under then is a good
-                    //       way to make them crash horribly.  This probably doesn't matter
-                    //       much since the app is supposed to be shutting down anyway, but
-                    //       I'm disabling cleanup for now until I can think about it some
-                    //       more.
-                    //
-                    // NOTE: Due to popular demand, this has been re-enabled.  It still has
-                    //       the problems mentioned above though, so I guess we'll see.
-
-                    instance.collectNoStack();  // not really a 'collect all' -- still scans
-                                                // static data area, roots, and ranges.
+                    instance.collect();
                     break;
                 case "finalize":
                     instance.runFinalizers((cast(ubyte*)null)[0 .. size_t.max]);

druntime/test/exceptions/src/invalid_memory_operation.d

@@ -8,5 +8,6 @@ struct S
 
 void main()
 {
-    new S;
+    foreach(i; 0 .. 100)
+        new S;
 }

druntime/test/valgrind/src/no_use_after_gc.d

@@ -16,11 +16,24 @@ struct S
 
 __gshared int result; // Trick the optimizer
 
-int main()
+int makeBadInstances()
 {
     auto a = new S;
     auto b = new S;
     a.other = b;
     b.other = a;
     return result;
 }
+
+int destroyStack()
+{
+    int[1000] x = result;
+    return x[123];
+}
+
+int main()
+{
+    int x = makeBadInstances();
+    x += destroyStack();
+    return x;
+}