From eb7b53c7e06490990cba437e4a0bd631111f3592 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Fri, 30 Jul 2021 14:19:37 +0000 Subject: [PATCH 1/8] test/gc/Makefile: Add missing dependency --- test/gc/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/gc/Makefile b/test/gc/Makefile index 566e7403c2..5c7b926fd2 100644 --- a/test/gc/Makefile +++ b/test/gc/Makefile @@ -35,7 +35,7 @@ $(ROOT)/precise: $(SRC) $(DMD) -debug -debug=INVARIANT -debug=MEMSTOMP $(UDFLAGS) -main -of$@ $(SRC) $(ROOT)/precise.done: RUN_ARGS += --DRT-gcopt=gc:precise -$(ROOT)/precisegc: $(SRC) +$(ROOT)/precisegc: $(SRC) precisegc.d $(DMD) $(UDFLAGS) -gx -of$@ $(SRC) precisegc.d $(ROOT)/attributes: attributes.d From 5ba0aa26b8e0ea05c7dc055995f6bb29e34a6b33 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Sat, 24 Jul 2021 14:19:43 +0000 Subject: [PATCH 2/8] Fix compilation without COLLECT_PARALLEL Move the fork handlers out of the version(COLLECT_PARALLEL): section, and only version-out the code dealing with parallel scanning. --- src/core/internal/gc/impl/conservative/gc.d | 83 +++++++++++---------- 1 file changed, 43 insertions(+), 40 deletions(-) diff --git a/src/core/internal/gc/impl/conservative/gc.d b/src/core/internal/gc/impl/conservative/gc.d index ba12ea0d31..49b8487295 100644 --- a/src/core/internal/gc/impl/conservative/gc.d +++ b/src/core/internal/gc/impl/conservative/gc.d @@ -2723,6 +2723,49 @@ struct Gcx return IsMarked.unknown; } + version (Posix) + { + // A fork might happen while GC code is running in a different thread. + // Because that would leave the GC in an inconsistent state, + // make sure no GC code is running by acquiring the lock here, + // before a fork. + + extern(C) static void _d_gcx_atfork_prepare() + { + if (instance) + ConservativeGC.lockNR(); + } + + extern(C) static void _d_gcx_atfork_parent() + { + if (instance) + ConservativeGC.gcLock.unlock(); + } + + extern(C) static void _d_gcx_atfork_child() + { + if (instance) + { + ConservativeGC.gcLock.unlock(); + + // make sure the threads and event handles are reinitialized in a fork + version (COLLECT_PARALLEL) + { + if (Gcx.instance.scanThreadData) + { + cstdlib.free(Gcx.instance.scanThreadData); + Gcx.instance.numScanThreads = 0; + Gcx.instance.scanThreadData = null; + Gcx.instance.busyThreads = 0; + + memset(&Gcx.instance.evStart, 0, Gcx.instance.evStart.sizeof); + memset(&Gcx.instance.evDone, 0, Gcx.instance.evDone.sizeof); + } + } + } + } + } + /* ============================ Parallel scanning =============================== */ version (COLLECT_PARALLEL): import core.sync.event; @@ -2955,46 +2998,6 @@ struct Gcx } debug(PARALLEL_PRINTF) printf("scanBackground thread %d done\n", threadId); } - - version (Posix) - { - // A fork might happen while GC code is running in a different thread. - // Because that would leave the GC in an inconsistent state, - // make sure no GC code is running by acquiring the lock here, - // before a fork. - - extern(C) static void _d_gcx_atfork_prepare() - { - if (instance) - ConservativeGC.lockNR(); - } - - extern(C) static void _d_gcx_atfork_parent() - { - if (instance) - ConservativeGC.gcLock.unlock(); - } - - extern(C) static void _d_gcx_atfork_child() - { - if (instance) - { - ConservativeGC.gcLock.unlock(); - - // make sure the threads and event handles are reinitialized in a fork - if (Gcx.instance.scanThreadData) - { - cstdlib.free(Gcx.instance.scanThreadData); - Gcx.instance.numScanThreads = 0; - Gcx.instance.scanThreadData = null; - Gcx.instance.busyThreads = 0; - - memset(&Gcx.instance.evStart, 0, Gcx.instance.evStart.sizeof); - memset(&Gcx.instance.evDone, 0, Gcx.instance.evDone.sizeof); - } - } - } - } } /* ============================ Pool =============================== */ From 9e1fbefe8baa785bfe67f5b24cc72abb79f7601c Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Fri, 30 Jul 2021 15:25:58 +0000 Subject: [PATCH 3/8] Introduce debug=GC_RECURSIVE_LOCK When a debug GC build fails an assertion, this manifests as the program freezing. This is because the failed assertion causes an allocation (to allocate memory for AssertError), however, as the GC lock was held at the time that the assertion failed, the allocation attempt results in another attempt at acquiring the GC lock. As the locking mechanism used by the GC does not support same-thread re-entry, the program hangs in a deadlock. This is acceptable for casual debugging, as it's easy enough to attach a debugger and examine the stack trace. However, it does make it complicated to write automated tests which verify that the GC code is asserting when it should. For this purpose, introduce debug=GC_RECURSIVE_LOCK, which adds runtime checking if we are attempting to re-acquire the GC lock in the same thread, and call onInvalidMemoryOperationError if so. --- src/core/internal/gc/impl/conservative/gc.d | 26 +++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/core/internal/gc/impl/conservative/gc.d b/src/core/internal/gc/impl/conservative/gc.d index 49b8487295..55366532a4 100644 --- a/src/core/internal/gc/impl/conservative/gc.d +++ b/src/core/internal/gc/impl/conservative/gc.d @@ -25,6 +25,7 @@ module core.internal.gc.impl.conservative.gc; //debug = PTRCHECK2; // thorough but slow pointer checking //debug = INVARIANT; // enable invariants //debug = PROFILE_API; // profile API calls for config.profile > 1 +//debug = GC_RECURSIVE_LOCK; // check for recursive locking on the same thread /***************************************************/ version = COLLECT_PARALLEL; // parallel scanning @@ -213,10 +214,17 @@ class ConservativeGC : GC runLocked!(go, otherTime, numOthers)(gcx); } + debug (GC_RECURSIVE_LOCK) static bool lockedOnThisThread; auto runLocked(alias func, Args...)(auto ref Args args) { debug(PROFILE_API) immutable tm = (config.profile > 1 ? currTime.ticks : 0); + debug(GC_RECURSIVE_LOCK) + { + if (lockedOnThisThread) + onInvalidMemoryOperationError(); + lockedOnThisThread = true; + } lockNR(); scope (failure) gcLock.unlock(); debug(PROFILE_API) immutable tm2 = (config.profile > 1 ? currTime.ticks : 0); @@ -229,6 +237,12 @@ class ConservativeGC : GC debug(PROFILE_API) if (config.profile > 1) lockTime += tm2 - tm; gcLock.unlock(); + debug(GC_RECURSIVE_LOCK) + { + if (!lockedOnThisThread) + onInvalidMemoryOperationError(); + lockedOnThisThread = false; + } static if (!is(typeof(func(args)) == void)) return res; @@ -238,6 +252,12 @@ class ConservativeGC : GC auto runLocked(alias func, alias time, alias count, Args...)(auto ref Args args) { debug(PROFILE_API) immutable tm = (config.profile > 1 ? currTime.ticks : 0); + debug(GC_RECURSIVE_LOCK) + { + if (lockedOnThisThread) + onInvalidMemoryOperationError(); + lockedOnThisThread = true; + } lockNR(); scope (failure) gcLock.unlock(); debug(PROFILE_API) immutable tm2 = (config.profile > 1 ? currTime.ticks : 0); @@ -255,6 +275,12 @@ class ConservativeGC : GC time += now - tm2; } gcLock.unlock(); + debug(GC_RECURSIVE_LOCK) + { + if (!lockedOnThisThread) + onInvalidMemoryOperationError(); + lockedOnThisThread = false; + } static if (!is(typeof(func(args)) == void)) return res; From 3d4500f3029400be1519b6eb17867f9215740a7d Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Fri, 30 Jul 2021 13:48:08 +0000 Subject: [PATCH 4/8] Move disabled debug=SENTINEL test to own file Because this test involves crashing the GC and catching an Error, this requires it to be a stand-alone test, so that the undefined state of the GC left behind by the thrown Error does not interfere with other tests. --- src/core/internal/gc/impl/conservative/gc.d | 12 ++-------- test/gc/Makefile | 6 ++++- test/gc/sentinel1.d | 26 +++++++++++++++++++++ 3 files changed, 33 insertions(+), 11 deletions(-) create mode 100644 test/gc/sentinel1.d diff --git a/src/core/internal/gc/impl/conservative/gc.d b/src/core/internal/gc/impl/conservative/gc.d index 55366532a4..c951e5f3ed 100644 --- a/src/core/internal/gc/impl/conservative/gc.d +++ b/src/core/internal/gc/impl/conservative/gc.d @@ -4425,16 +4425,8 @@ unittest auto p = cast(ubyte*)GC.malloc(1); assert(p[-1] == 0xF4); assert(p[ 1] == 0xF5); -/* - p[1] = 0; - bool thrown; - try - GC.free(p); - catch (Error e) - thrown = true; - p[1] = 0xF5; - assert(thrown); -*/ + + // See also stand-alone tests in test/gc } unittest diff --git a/test/gc/Makefile b/test/gc/Makefile index 5c7b926fd2..dbeb957ccb 100644 --- a/test/gc/Makefile +++ b/test/gc/Makefile @@ -1,6 +1,7 @@ include ../common.mak -TESTS := attributes sentinel printf memstomp invariant logging precise precisegc forkgc forkgc2 \ +TESTS := attributes sentinel sentinel1 printf memstomp invariant logging \ + precise precisegc forkgc forkgc2 \ sigmaskgc startbackgc recoverfree nocollect SRC_GC = ../../src/core/internal/gc/impl/conservative/gc.d @@ -19,6 +20,9 @@ $(ROOT)/%.done: $(ROOT)/% $(ROOT)/sentinel: $(SRC) $(DMD) -debug=SENTINEL $(UDFLAGS) -main -of$@ $(SRC) +$(ROOT)/sentinel1: $(SRC) sentinel1.d + $(DMD) -debug=SENTINEL -debug=GC_RECURSIVE_LOCK $(DFLAGS) sentinel1.d -of$@ $(SRC) + $(ROOT)/printf: $(SRC) $(DMD) -debug=PRINTF -debug=PRINTF_TO_FILE -debug=COLLECT_PRINTF $(UDFLAGS) -main -of$@ $(SRC_GC) diff --git a/test/gc/sentinel1.d b/test/gc/sentinel1.d new file mode 100644 index 0000000000..7443d04f86 --- /dev/null +++ b/test/gc/sentinel1.d @@ -0,0 +1,26 @@ +debug (SENTINEL) +void main() +{ + import core.stdc.stdio : printf; + import core.sys.posix.unistd : _exit; + import core.memory : GC; + + auto p = cast(ubyte*)GC.malloc(1); + assert(p[ 1] == 0xF5); + + p[1] = 0; + try + { + GC.free(p); + + printf("Clobbered sentinel not detected by GC.free!\n"); + _exit(1); + } + catch (Error e) + { + printf("Clobbered sentinel successfully detected by GC.free.\n"); + _exit(0); + } +} +else + static assert(false); From 6558d800f8b4db57bb8954511b7c5987590a0480 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Sat, 24 Jul 2021 14:58:16 +0000 Subject: [PATCH 5/8] Fix debug hooks not being run when sweeping finalizer-less bins The `if (hasFinalizer)` condition wraps a lot more than just calling finalizers. Update the list of conditions, and rename the boolean variable to a more descriptive name, with the hope that this will help avoid future bitrot. --- src/core/internal/gc/impl/conservative/gc.d | 26 +++++++++++++-------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/core/internal/gc/impl/conservative/gc.d b/src/core/internal/gc/impl/conservative/gc.d index c951e5f3ed..9bd6283079 100644 --- a/src/core/internal/gc/impl/conservative/gc.d +++ b/src/core/internal/gc/impl/conservative/gc.d @@ -2456,22 +2456,28 @@ struct Gcx static foreach (w; 0 .. PageBits.length) recoverPage = recoverPage && (~freebitsdata[w] == toFree[w]); - bool hasFinalizer = false; - debug(COLLECT_PRINTF) // need output for each onject - hasFinalizer = true; - else debug(LOGGING) - hasFinalizer = true; - else debug(MEMSTOMP) - hasFinalizer = true; - if (pool.finals.data) + // We need to loop through each object if any have a finalizer, + // or, if any of the debug hooks are enabled. + bool doLoop = false; + debug (SENTINEL) + doLoop = true; + else version (assert) + doLoop = true; + else debug (COLLECT_PRINTF) // need output for each object + doLoop = true; + else debug (LOGGING) + doLoop = true; + else debug (MEMSTOMP) + doLoop = true; + else if (pool.finals.data) { // finalizers must be called on objects that are about to be freed auto finalsdata = pool.finals.data + pn * PageBits.length; static foreach (w; 0 .. PageBits.length) - hasFinalizer = hasFinalizer || (toFree[w] & finalsdata[w]) != 0; + doLoop = doLoop || (toFree[w] & finalsdata[w]) != 0; } - if (hasFinalizer) + if (doLoop) { immutable size = binsize[bin]; void *p = pool.baseAddr + pn * PAGESIZE; From 6dde9c7408f6988219164c2d695d4330db2a4d90 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Fri, 30 Jul 2021 14:14:23 +0000 Subject: [PATCH 6/8] Add debug=SENTINEL test for sweep-time checking Tests the fix in the previous commit. --- test/gc/Makefile | 5 ++++- test/gc/sentinel2.d | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 test/gc/sentinel2.d diff --git a/test/gc/Makefile b/test/gc/Makefile index dbeb957ccb..9a1af125c8 100644 --- a/test/gc/Makefile +++ b/test/gc/Makefile @@ -1,6 +1,6 @@ include ../common.mak -TESTS := attributes sentinel sentinel1 printf memstomp invariant logging \ +TESTS := attributes sentinel sentinel1 sentinel2 printf memstomp invariant logging \ precise precisegc forkgc forkgc2 \ sigmaskgc startbackgc recoverfree nocollect @@ -23,6 +23,9 @@ $(ROOT)/sentinel: $(SRC) $(ROOT)/sentinel1: $(SRC) sentinel1.d $(DMD) -debug=SENTINEL -debug=GC_RECURSIVE_LOCK $(DFLAGS) sentinel1.d -of$@ $(SRC) +$(ROOT)/sentinel2: $(SRC) sentinel2.d + $(DMD) -debug=SENTINEL -debug=GC_RECURSIVE_LOCK $(DFLAGS) sentinel2.d -gx -of$@ $(SRC) + $(ROOT)/printf: $(SRC) $(DMD) -debug=PRINTF -debug=PRINTF_TO_FILE -debug=COLLECT_PRINTF $(UDFLAGS) -main -of$@ $(SRC_GC) diff --git a/test/gc/sentinel2.d b/test/gc/sentinel2.d new file mode 100644 index 0000000000..d75f719a36 --- /dev/null +++ b/test/gc/sentinel2.d @@ -0,0 +1,37 @@ +debug (SENTINEL) +void main() +{ + import core.stdc.stdio : printf; + import core.sys.posix.unistd : _exit; + import core.thread : Thread, thread_detachThis; + import core.memory : GC; + + // Create a new thread and immediately detach it from the runtime, + // so that the pointer p will not be visible to the GC. + auto t = new Thread({ + thread_detachThis(); + + auto p = cast(ubyte*)GC.malloc(1); + assert(p[-1] == 0xF4); + assert(p[ 1] == 0xF5); + + p[1] = 0; + try + { + GC.collect(); + + printf("Clobbered sentinel not detected by GC.collect!\n"); + _exit(1); + } + catch (Error e) + { + printf("Clobbered sentinel successfully detected by GC.collect.\n"); + _exit(0); + } + }); + t.start(); + t.join(); + assert(false, "Unreachable"); +} +else + static assert(false); From 063dd46d931b14796dfc689679a963e9335b4459 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Sat, 24 Jul 2021 15:24:13 +0000 Subject: [PATCH 7/8] Add size parameter to LeakDetector.log_free This function is a good site to add debug hooks to trace GC deallocations. However, though the size of the allocation is known to the GC in all deallocation circumstances, this information is not passed along. Add a parameter to log_free to thus aid debugging. Without debug=LOGGING, the value is unused (and therefore optimized out). With debug=LOGGING, we can additionally print the allocation size in case LeakDetector detects a double free. --- src/core/internal/gc/impl/conservative/gc.d | 22 +++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/core/internal/gc/impl/conservative/gc.d b/src/core/internal/gc/impl/conservative/gc.d index 9bd6283079..59293eeb0c 100644 --- a/src/core/internal/gc/impl/conservative/gc.d +++ b/src/core/internal/gc/impl/conservative/gc.d @@ -747,7 +747,9 @@ class ConservativeGC : GC return; sentinel_Invariant(p); + auto q = p; p = sentinel_sub(p); + size_t ssize; if (pool.isLargeObject) // if large alloc { @@ -757,7 +759,9 @@ class ConservativeGC : GC // Free pages size_t npages = lpool.bPageOffsets[pagenum]; - debug (MEMSTOMP) memset(p, 0xF2, npages * PAGESIZE); + auto size = npages * PAGESIZE; + ssize = sentinel_size(q, size); + debug (MEMSTOMP) memset(p, 0xF2, size); lpool.freePages(pagenum, npages); lpool.mergeFreePageOffsets!(true, true)(pagenum, npages); } @@ -769,7 +773,9 @@ class ConservativeGC : GC // Add to free list List *list = cast(List*)p; - debug (MEMSTOMP) memset(p, 0xF2, binsize[bin]); + auto size = binsize[bin]; + ssize = sentinel_size(q, size); + debug (MEMSTOMP) memset(p, 0xF2, size); // in case the page hasn't been recovered yet, don't add the object to the free list if (!gcx.recoverPool[bin] || pool.binPageChain[pagenum] == Pool.PageRecovered) @@ -782,7 +788,7 @@ class ConservativeGC : GC } pool.clrBits(biti, ~BlkAttr.NONE); - gcx.leakDetector.log_free(sentinel_add(p)); + gcx.leakDetector.log_free(sentinel_add(p), ssize); } @@ -2382,7 +2388,7 @@ struct Gcx pool.clrBits(biti, ~BlkAttr.NONE ^ BlkAttr.FINALIZE); debug(COLLECT_PRINTF) printf("\tcollecting big %p\n", p); - leakDetector.log_free(q); + leakDetector.log_free(q, sentinel_size(q, npages * PAGESIZE - SENTINEL_EXTRA)); pool.pagetable[pn..pn+npages] = B_FREE; if (pn < pool.searchStart) pool.searchStart = pn; freedLargePages += npages; @@ -2502,7 +2508,7 @@ struct Gcx assert(core.bitop.bt(toFree.ptr, i)); debug(COLLECT_PRINTF) printf("\tcollecting %p\n", p); - leakDetector.log_free(sentinel_add(p)); + leakDetector.log_free(q, sentinel_size(q, size)); debug (MEMSTOMP) memset(p, 0xF3, size); } @@ -4243,13 +4249,13 @@ debug (LOGGING) } - private void log_free(void *p) nothrow @nogc + private void log_free(void *p, size_t size) nothrow @nogc { //debug(PRINTF) printf("+log_free(%p)\n", p); auto i = current.find(p); if (i == OPFAIL) { - debug(PRINTF) printf("free'ing unallocated memory %p\n", p); + debug(PRINTF) printf("free'ing unallocated memory %p (size %zu)\n", p, size); } else current.remove(i); @@ -4323,7 +4329,7 @@ else { static void initialize(Gcx* gcx) nothrow { } static void log_malloc(void *p, size_t size) nothrow { } - static void log_free(void *p) nothrow @nogc { } + static void log_free(void *p, size_t size) nothrow @nogc {} static void log_collect() nothrow { } static void log_parent(void *p, void *parent) nothrow { } } From 4b401a8bd929a5111efad567f333393da786c584 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Sat, 31 Jul 2021 06:48:11 +0000 Subject: [PATCH 8/8] Fix debug=MEMSTOMP unittest Broken in c1169182acaaafd18bd030797d4538fbd1454400. --- src/core/internal/gc/impl/conservative/gc.d | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/internal/gc/impl/conservative/gc.d b/src/core/internal/gc/impl/conservative/gc.d index 59293eeb0c..3ed6ec5fa7 100644 --- a/src/core/internal/gc/impl/conservative/gc.d +++ b/src/core/internal/gc/impl/conservative/gc.d @@ -4423,11 +4423,11 @@ debug (MEMSTOMP) unittest { import core.memory; - auto p = cast(uint*)GC.malloc(uint.sizeof*5); - assert(*p == 0xF0F0F0F0); + auto p = cast(size_t*)GC.malloc(size_t.sizeof*3); + assert(*p == cast(size_t)0xF0F0F0F0F0F0F0F0); p[2] = 0; // First two will be used for free list GC.free(p); - assert(p[4] == 0xF2F2F2F2); // skip List usage, for both 64-bit and 32-bit + assert(p[2] == cast(size_t)0xF2F2F2F2F2F2F2F2); } debug (SENTINEL)