Merge pull request #7725 from yorickvP/check-coro-gc

Disable GC during coroutine execution + test

5 files changed, 213 insertions, 6 deletions

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 2721b6733..3e8857fc8 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -325,6 +325,22 @@ 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;
 
@@ -349,6 +365,15 @@ 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
diff --git a/src/libexpr/tests/coro-gc.cc b/src/libexpr/tests/coro-gc.cc
new file mode 100644
index 000000000..f848bc2f0
--- /dev/null
+++ b/src/libexpr/tests/coro-gc.cc
@@ -0,0 +1,147 @@
+#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
+}
diff --git a/src/libexpr/tests/local.mk b/src/libexpr/tests/local.mk
index 3e5504f71..c36d21bfe 100644
--- a/src/libexpr/tests/local.mk
+++ b/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
+libexpr-tests_LDFLAGS := $(GTEST_LIBS) -lgmock -lboost_context
diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc
index 7476e6f6c..6e53239f5 100644
--- a/src/libutil/serialise.cc
+++ b/src/libutil/serialise.cc
@@ -186,6 +186,22 @@ 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
@@ -206,7 +222,8 @@ 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()) {
@@ -223,17 +240,24 @@ std::unique_ptr<FinishSink> sourceToSink(std::function<void(Source &)> fun)
                     });
                     fun(source);
                 });
+            }
 
             if (!*coro) { abort(); }
 
-            if (!cur.empty()) (*coro)(false);
+            if (!cur.empty()) {
+                CoroutineContext ctx;
+                (*coro)(false);
+            }
         }
 
         void finish() override
         {
             if (!coro) return;
             if (!*coro) abort();
-            (*coro)(true);
+            {
+                CoroutineContext ctx;
+                (*coro)(true);
+            }
             if (*coro) abort();
         }
     };
@@ -264,18 +288,23 @@ 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()) (*coro)();
+                if (!cur.empty()) {
+                    CoroutineContext ctx;
+                    (*coro)();
+                }
                 cur = coro->get();
                 pos = 0;
             }
diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh
index 7da5b07fd..e99c5fcc7 100644
--- a/src/libutil/serialise.hh
+++ b/src/libutil/serialise.hh
@@ -501,4 +501,10 @@ 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)();
+
+
 }