From 1e75c5011b010b94e402e5c0cd24a96f3b768d3d Mon Sep 17 00:00:00 2001 From: Nemanja Boric <4burgos@gmail.com> Date: Thu, 13 Jul 2017 17:03:43 +0000 Subject: [PATCH 1/4] Make thread_attachThis nothrow In case any of the GC operations in the thread_attachThis fail with the Error, it's likely that GC.enable will also throw an error. This is not allowed for the statically allocated errors (such as InvalidMemoryOperationError, commonly thrown by GC), and it will cause deadlock where the t->next will be same as t. In order to prevent this, we need to make scope(exit) GC.enable() scope(success) GC.enable(), but then to make sure there will be no exception which will cause GC.enable to be skiped, we will make entire thread_attachThis nothrow. --- src/core/thread.d | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/thread.d b/src/core/thread.d index 4f99cbbc31..3cefd6acbb 100644 --- a/src/core/thread.d +++ b/src/core/thread.d @@ -2073,7 +2073,7 @@ extern (C) bool thread_isMainThread() nothrow @nogc * * extern (C) void rt_moduleTlsCtor(); */ -extern (C) Thread thread_attachThis() +extern (C) Thread thread_attachThis() nothrow { GC.disable(); scope(exit) GC.enable(); From 4c97b07c389f25abb9bf48594df77cd84cbad560 Mon Sep 17 00:00:00 2001 From: Nemanja Boric <4burgos@gmail.com> Date: Thu, 13 Jul 2017 17:07:23 +0000 Subject: [PATCH 2/4] Enable GC in thread_attachThis in scope(success) instead scope(exit) Since thread_attachThis is nothrow, this is the same as scope(exit), just it will avoid using GC when the Error is thrown, possibly by the GC itself. --- src/core/thread.d | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/thread.d b/src/core/thread.d index 3cefd6acbb..e050c1c553 100644 --- a/src/core/thread.d +++ b/src/core/thread.d @@ -2075,7 +2075,7 @@ extern (C) bool thread_isMainThread() nothrow @nogc */ extern (C) Thread thread_attachThis() nothrow { - GC.disable(); scope(exit) GC.enable(); + GC.disable(); scope(success) GC.enable(); if (auto t = Thread.getThis()) return t; From d5088da1f0c1517b50954f5527f98c23f88c1644 Mon Sep 17 00:00:00 2001 From: Nemanja Boric <4burgos@gmail.com> Date: Thu, 13 Jul 2017 17:11:04 +0000 Subject: [PATCH 3/4] Rename ConservativeGC._in_Finalizer to _is_Locked Since this field was used to prevent recursive calls to GC to itself while is locked, not just when running inside finalizer, this field is renamed to better describe its purpose. --- src/gc/impl/conservative/gc.d | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/gc/impl/conservative/gc.d b/src/gc/impl/conservative/gc.d index 10726554a0..5fea6be4ca 100644 --- a/src/gc/impl/conservative/gc.d +++ b/src/gc/impl/conservative/gc.d @@ -259,12 +259,12 @@ class ConservativeGC : GC import core.internal.spinlock; static gcLock = shared(AlignedSpinLock)(SpinLock.Contention.lengthy); - static bool _inFinalizer; + static bool _isLocked; // lock GC, throw InvalidMemoryOperationError on recursive locking during finalization static void lockNR() @nogc nothrow { - if (_inFinalizer) + if (_isLocked) onInvalidMemoryOperationError(); gcLock.lock(); } @@ -817,7 +817,7 @@ class ConservativeGC : GC void free(void *p) nothrow { - if (!p || _inFinalizer) + if (!p || _isLocked) { return; } @@ -1120,7 +1120,7 @@ class ConservativeGC : GC bool inFinalizer() nothrow { - return _inFinalizer; + return _isLocked; } @@ -1500,8 +1500,8 @@ struct Gcx */ void runFinalizers(in void[] segment) nothrow { - ConservativeGC._inFinalizer = true; - scope (failure) ConservativeGC._inFinalizer = false; + ConservativeGC._isLocked = true; + scope (failure) ConservativeGC._isLocked = false; foreach (pool; pooltable[0 .. npools]) { @@ -1518,7 +1518,7 @@ struct Gcx spool.runFinalizers(segment); } } - ConservativeGC._inFinalizer = false; + ConservativeGC._isLocked = false; } Pool* findPool(void* p) pure nothrow @@ -2408,12 +2408,12 @@ struct Gcx start = stop; } - ConservativeGC._inFinalizer = true; + ConservativeGC._isLocked = true; size_t freedLargePages=void; { - scope (failure) ConservativeGC._inFinalizer = false; + scope (failure) ConservativeGC._isLocked = false; freedLargePages = sweep(); - ConservativeGC._inFinalizer = false; + ConservativeGC._isLocked = false; } if (config.profile) From 8a61b43a0e01ef0ea177df5265c1e1448cb5ba84 Mon Sep 17 00:00:00 2001 From: Nemanja Boric <4burgos@gmail.com> Date: Thu, 13 Jul 2017 17:15:37 +0000 Subject: [PATCH 4/4] Fix issue 17413: Prevent deadlock in the GC init If the program during initialization in the pressing memory environment died with the Error thrown by the GC, GC would not be usable anymore. However, dso registry unregistration will try to removeRanges. This would cause a deadlock in the GC, preventing the exit of the program. This commit prevents GC entering the recursive lock from the chain GC.fullcollect() -> Error -> dso_registry -> GC.removeRange. --- src/gc/impl/conservative/gc.d | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/gc/impl/conservative/gc.d b/src/gc/impl/conservative/gc.d index 5fea6be4ca..7582380efa 100644 --- a/src/gc/impl/conservative/gc.d +++ b/src/gc/impl/conservative/gc.d @@ -1093,7 +1093,7 @@ class ConservativeGC : GC void removeRange(void *p) nothrow @nogc { - if (!p) + if (!p || _isLocked) { return; } @@ -2374,12 +2374,14 @@ struct Gcx { // lock roots and ranges around suspending threads b/c they're not reentrant safe + ConservativeGC._isLocked = true; rangesLock.lock(); rootsLock.lock(); scope (exit) { rangesLock.unlock(); rootsLock.unlock(); + ConservativeGC._isLocked = false; } thread_suspendAll(); @@ -2395,6 +2397,7 @@ struct Gcx markAll(nostack); thread_processGCMarks(&isMarked); + ConservativeGC._isLocked = false; thread_resumeAll(); }