Merge pull request #8012 from NixOS/revert-7725-check-coro-gc

Revert "Disable GC during coroutine execution + test"

boehmgc-coroutine-sp-fallback.diff

@@ -1,8 +1,8 @@
 diff --git a/darwin_stop_world.c b/darwin_stop_world.c
-index 0468aaec..b348d869 100644
+index 3dbaa3fb..36a1d1f7 100644
 --- a/darwin_stop_world.c
 +++ b/darwin_stop_world.c
-@@ -356,6 +356,7 @@ GC_INNER void GC_push_all_stacks(void)
+@@ -352,6 +352,7 @@ GC_INNER void GC_push_all_stacks(void)
    int nthreads = 0;
    word total_size = 0;
    mach_msg_type_number_t listcount = (mach_msg_type_number_t)THREAD_TABLE_SZ;
@@ -10,7 +10,7 @@ index 0468aaec..b348d869 100644
    if (!EXPECT(GC_thr_initialized, TRUE))
      GC_thr_init();
  
-@@ -411,6 +412,19 @@ GC_INNER void GC_push_all_stacks(void)
+@@ -407,6 +408,19 @@ GC_INNER void GC_push_all_stacks(void)
              GC_push_all_stack_sections(lo, hi, p->traced_stack_sect);
            }
            if (altstack_lo) {
@@ -30,22 +30,6 @@ index 0468aaec..b348d869 100644
              total_size += altstack_hi - altstack_lo;
              GC_push_all_stack(altstack_lo, altstack_hi);
            }
-diff --git a/include/gc.h b/include/gc.h
-index edab6c22..f2c61282 100644
---- a/include/gc.h
-+++ b/include/gc.h
-@@ -2172,6 +2172,11 @@ GC_API void GC_CALL GC_win32_free_heap(void);
-         (*GC_amiga_allocwrapper_do)(a,GC_malloc_atomic_ignore_off_page)
- #endif /* _AMIGA && !GC_AMIGA_MAKINGLIB */
- 
-+#if !__APPLE__
-+/* Patch doesn't work on apple */
-+#define NIX_BOEHM_PATCH_VERSION 1
-+#endif
-+
- #ifdef __cplusplus
-   } /* extern "C" */
- #endif
 diff --git a/pthread_stop_world.c b/pthread_stop_world.c
 index b5d71e62..aed7b0bf 100644
 --- a/pthread_stop_world.c

src/libexpr/eval.cc

@@ -325,22 +325,6 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env)
     }
 }
 
-#if HAVE_BOEHMGC
-/* Disable GC while this object lives. Used by CoroutineContext.
- *
- * Boehm keeps a count of GC_disable() and GC_enable() calls,
- * and only enables GC when the count matches.
- */
-class BoehmDisableGC {
-public:
-    BoehmDisableGC() {
-        GC_disable();
-    };
-    ~BoehmDisableGC() {
-        GC_enable();
-    };
-};
-#endif
 
 static bool gcInitialised = false;
 
@@ -365,15 +349,6 @@ void initGC()
 
     StackAllocator::defaultAllocator = &boehmGCStackAllocator;
 
-
-#if NIX_BOEHM_PATCH_VERSION != 1
-    printTalkative("Unpatched BoehmGC, disabling GC inside coroutines");
-    /* Used to disable GC when entering coroutines on macOS */
-    create_coro_gc_hook = []() -> std::shared_ptr<void> {
-        return std::make_shared<BoehmDisableGC>();
-    };
-#endif
-
     /* Set the initial heap size to something fairly big (25% of
        physical RAM, up to a maximum of 384 MiB) so that in most cases
        we don't need to garbage collect at all.  (Collection has a

src/libexpr/tests/coro-gc.cc

@@ -1,147 +0,0 @@
-#include <gtest/gtest.h>
-#if HAVE_BOEHMGC
-#include <gc/gc.h>
-
-#include "eval.hh"
-#include "serialise.hh"
-
-#endif
-
-
-namespace nix {
-#if HAVE_BOEHMGC
-
-    static void finalizer(void *obj, void *data) {
-        *((bool*)data) = true;
-    }
-
-    static bool* make_witness(volatile void* obj) {
-        /* We can't store the witnesses on the stack,
-           since they might be collected long afterwards */
-        bool* res = (bool*)GC_MALLOC_UNCOLLECTABLE(1);
-        *res = false;
-        GC_register_finalizer((void*)obj, finalizer, res, nullptr, nullptr);
-        return res;
-    }
-
-    // Generate 2 objects, discard one, run gc,
-    // see if one got collected and the other didn't
-    // GC is disabled inside coroutines on __APPLE__
-    static void testFinalizerCalls() {
-        volatile void* do_collect = GC_MALLOC_ATOMIC(128);
-        volatile void* dont_collect = GC_MALLOC_ATOMIC(128);
-
-        bool* do_collect_witness = make_witness(do_collect);
-        bool* dont_collect_witness = make_witness(dont_collect);
-        GC_gcollect();
-        GC_invoke_finalizers();
-
-        ASSERT_TRUE(GC_is_disabled() || *do_collect_witness);
-        ASSERT_FALSE(*dont_collect_witness);
-        ASSERT_NE(nullptr, dont_collect);
-    }
-
-    TEST(CoroGC, BasicFinalizers) {
-        initGC();
-        testFinalizerCalls();
-    }
-
-    // Run testFinalizerCalls inside a coroutine
-    // this tests that GC works as expected inside a coroutine
-    TEST(CoroGC, CoroFinalizers) {
-        initGC();
-
-        auto source = sinkToSource([&](Sink& sink) {
-            testFinalizerCalls();
-
-            // pass control to main
-            writeString("foo", sink);
-        });
-
-        // pass control to coroutine
-        std::string foo = readString(*source);
-        ASSERT_EQ(foo, "foo");
-    }
-
-#if __APPLE__
-    // This test tests that GC is disabled on darwin
-    // to work around the patch not being sufficient there,
-    // causing crashes whenever gc is invoked inside a coroutine
-    TEST(CoroGC, AppleCoroDisablesGC) {
-        initGC();
-        auto source = sinkToSource([&](Sink& sink) {
-            ASSERT_TRUE(GC_is_disabled());
-            // pass control to main
-            writeString("foo", sink);
-
-            ASSERT_TRUE(GC_is_disabled());
-
-            // pass control to main
-            writeString("bar", sink);
-        });
-
-        // pass control to coroutine
-        std::string foo = readString(*source);
-        ASSERT_EQ(foo, "foo");
-        ASSERT_FALSE(GC_is_disabled());
-        // pass control to coroutine
-        std::string bar = readString(*source);
-        ASSERT_EQ(bar, "bar");
-
-        ASSERT_FALSE(GC_is_disabled());
-    }
-#endif
-
-    // This test tests that boehm handles coroutine stacks correctly
-    // This test tests that coroutine stacks are registered to the GC,
-    // even when the coroutine is not running. It also tests that
-    // the main stack is still registered to the GC when the coroutine is running.
-    TEST(CoroGC, CoroutineStackNotGCd) {
-        initGC();
-
-        volatile void* do_collect = GC_MALLOC_ATOMIC(128);
-        volatile void* dont_collect = GC_MALLOC_ATOMIC(128);
-
-        bool* do_collect_witness = make_witness(do_collect);
-        bool* dont_collect_witness = make_witness(dont_collect);
-
-        do_collect = nullptr;
-
-        auto source = sinkToSource([&](Sink& sink) {
-            volatile void* dont_collect_inner = GC_MALLOC_ATOMIC(128);
-            volatile void* do_collect_inner = GC_MALLOC_ATOMIC(128);
-
-            bool* do_collect_inner_witness = make_witness(do_collect_inner);
-            bool* dont_collect_inner_witness = make_witness(dont_collect_inner);
-
-            do_collect_inner = nullptr;
-
-            // pass control to main
-            writeString("foo", sink);
-
-            ASSERT_FALSE(*dont_collect_inner_witness);
-            ASSERT_TRUE(*do_collect_inner_witness);
-            ASSERT_NE(nullptr, dont_collect_inner);
-
-            // pass control to main
-            writeString("bar", sink);
-        });
-
-        // pass control to coroutine
-        std::string foo = readString(*source);
-        ASSERT_EQ(foo, "foo");
-
-        ASSERT_FALSE(GC_is_disabled());
-        GC_gcollect();
-        GC_invoke_finalizers();
-
-        // pass control to coroutine
-        std::string bar = readString(*source);
-        ASSERT_EQ(bar, "bar");
-
-        ASSERT_FALSE(*dont_collect_witness);
-        ASSERT_TRUE(*do_collect_witness);
-        ASSERT_NE(nullptr, dont_collect);
-    }
-#endif
-}

src/libexpr/tests/local.mk

@@ -16,4 +16,4 @@ libexpr-tests_CXXFLAGS += -I src/libexpr -I src/libutil -I src/libstore -I src/l
 
 libexpr-tests_LIBS = libstore-tests libutils-tests libexpr libutil libstore libfetchers
 
-libexpr-tests_LDFLAGS := $(GTEST_LIBS) -lgmock -lboost_context
+libexpr-tests_LDFLAGS := $(GTEST_LIBS) -lgmock

src/libutil/serialise.cc

@@ -186,22 +186,6 @@ static DefaultStackAllocator defaultAllocatorSingleton;
 StackAllocator *StackAllocator::defaultAllocator = &defaultAllocatorSingleton;
 
 
-std::shared_ptr<void> (*create_coro_gc_hook)() = []() -> std::shared_ptr<void> {
-    return {};
-};
-
-/* This class is used for entry and exit hooks on coroutines */
-class CoroutineContext {
-    /* Disable GC when entering the coroutine without the boehm patch,
-     * since it doesn't find the main thread stack in this case.
-     * std::shared_ptr<void> performs type-erasure, so it will call the right
-     * deleter. */
-    const std::shared_ptr<void> coro_gc_hook = create_coro_gc_hook();
-public:
-    CoroutineContext() {};
-    ~CoroutineContext() {};
-};
-
 std::unique_ptr<FinishSink> sourceToSink(std::function<void(Source &)> fun)
 {
     struct SourceToSink : FinishSink
@@ -222,8 +206,7 @@ std::unique_ptr<FinishSink> sourceToSink(std::function<void(Source &)> fun)
             if (in.empty()) return;
             cur = in;
 
-            if (!coro) {
+            if (!coro)
-                CoroutineContext ctx;
                 coro = coro_t::push_type(VirtualStackAllocator{}, [&](coro_t::pull_type & yield) {
                     LambdaSource source([&](char *out, size_t out_len) {
                         if (cur.empty()) {
@@ -240,24 +223,17 @@ std::unique_ptr<FinishSink> sourceToSink(std::function<void(Source &)> fun)
                     });
                     fun(source);
                 });
-            }
 
             if (!*coro) { abort(); }
 
-            if (!cur.empty()) {
+            if (!cur.empty()) (*coro)(false);
-                CoroutineContext ctx;
-                (*coro)(false);
-            }
         }
 
         void finish() override
         {
             if (!coro) return;
             if (!*coro) abort();
-            {
-                CoroutineContext ctx;
             (*coro)(true);
-            }
             if (*coro) abort();
         }
     };
@@ -288,23 +264,18 @@ std::unique_ptr<Source> sinkToSource(
 
         size_t read(char * data, size_t len) override
         {
-            if (!coro) {
+            if (!coro)
-                CoroutineContext ctx;
                 coro = coro_t::pull_type(VirtualStackAllocator{}, [&](coro_t::push_type & yield) {
                     LambdaSink sink([&](std::string_view data) {
                         if (!data.empty()) yield(std::string(data));
                     });
                     fun(sink);
                 });
-            }
 
             if (!*coro) { eof(); abort(); }
 
             if (pos == cur.size()) {
-                if (!cur.empty()) {
+                if (!cur.empty()) (*coro)();
-                    CoroutineContext ctx;
-                    (*coro)();
-                }
                 cur = coro->get();
                 pos = 0;
             }

src/libutil/serialise.hh

@@ -501,10 +501,4 @@ struct StackAllocator {
     static StackAllocator *defaultAllocator;
 };
 
-/* Disabling GC when entering a coroutine (without the boehm patch).
-   mutable to avoid boehm gc dependency in libutil.
- */
-extern std::shared_ptr<void> (*create_coro_gc_hook)();
-
-
 }