Fix Issue 14650 - Destructors are not called on global variables #9151

RazvanN7 · 2018-12-27T13:01:00Z

The bug fix creates a static destructor that calls the destructors of static and global variables. Currently, this fix has exposed a bug (see [1], cc @MartinNowak). Also there might be breakage due to the fact that what this patch does might be done manually in user defined static destructors which will lead to double free errors. A solution might be to abandon the generation of static this when one already exists.

[1] https://issues.dlang.org/show_bug.cgi?id=16980#c8

dlang-bot · 2018-12-27T13:01:03Z

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	14650	normal	Destructors are not called on global variables

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9151"

thewilsonator · 2018-12-27T13:02:10Z

src/dmd/dmodule.d

+     * in the below defined array to be later used in a static
+     * destructor.
+     */
+    Statements dtors;


jacob-carlborg · 2018-12-27T14:17:54Z

I think the platform supports de/constructors for TLS variables. I'm guessing it's necessary for thread_local in C++.

andralex · 2018-12-27T14:33:06Z

Please take a look at the failures. Thanks!

rainers · 2018-12-27T15:58:44Z

I think the platform supports de/constructors for TLS variables. I'm guessing it's necessary for thread_local in C++.

The VC runtime has __tlregdtor that is a thread local version of atexit. As we don't have constructors executed for globals, the best we can do is to add one, but that's no different from executing a static destructor with the existing semantics.

rainers · 2018-12-27T16:00:04Z

src/dmd/dsymbolsem.d

+            // expression so it can be used in the static destructor of the
+            // module.
+            auto m = dsym.getModule();
+            if (dsym.isStatic || sc.parent == m)


I guess you will have to distinguish between TLS and shared/__gshared data here.

I think shared/__gshared is not as important as thread local destruction, as the process should clean up most of the stuff anyway.

It might be more important in a shared library that gets unloaded from the process.

Anyway, the shared globals should not be destroyed from within every thread.

Ah yes, I always forget about that case.

You mean that shared variables should be destroyed in a shared static destructor?

You mean that shared variables should be destroyed in a shared static destructor?

Yes, that should be the case. A non-shared static destructor will run on every thread termination. If that is destroying the shared variables, then you will have problems.

rainers · 2018-12-27T16:02:58Z

test/runnable/test14650.d

+
+static ~this()
+{
+    assert(a == 2);


I think the globals should be destroyed after the explicit static ~this() have run, so the test should verify that (a == 0).
A shared static ~this() can verify the destruction instead.

This reveals a dilemma. a shared static ~this() is going to currently expect it's own thread local storage to still be valid. Changing this will break that code. I've seen shared static destructors do something different with the TLS data because it knows it's in the main thread.

What we may have to do is only emit the static destructors if no manual static destructors exist.

Edit: the ones inside functions might be OK, but no telling if they are accessible through some inner struct.

Is it possible to tell if a static destructor accesses a particular variable? That probably would be a more complex fix.

This reveals a dilemma. a shared static ~this() is going to currently expect it's own thread local storage to still be valid. Changing this will break that code.

Maybe the main thread should run TLS destructors after shared static ~this(), but before shared destructors. The latter could contain the assertion above.

Isn't shared static ~this() a shared destructor?

This would be a breaking change as well.

Is it possible to tell if a static destructor accesses a particular variable? That probably would be a more complex fix.

It is possible to know if a particular variable is accessed, but it is hard to tell if the variable is destroyed or not:

struct A { ~this() {} } A a; void fun(ref A a) { a.__dtor(); } static ~this() { import std.stdio : writeln; writeln(a); // accessed but not destroyed; } static ~this() { fun(a); // need to examine fun to tell if it is destroyed or not }

Not to mention the overhead of re-analyzing all the function bodies that a static destructor calls.

Honestly, at this point I think that a better approach would be to document the existing behavior and guide the user to destroy its global variables using a static destructor. I'm not really sure how you can destroy static variables from different scopes (maybe use the mangled name?).

but before shared destructors.
Isn't shared static ~this() a shared destructor?

I meant destructors of shared variables.

You mean that shared variables should be destroyed in a shared static destructor?

yes, if it is the last to be executed for that module.

Honestly, at this point I think that a better approach would be to document the existing behavior and guide the user to destroy its global variables using a static destructor.

There are some issues there:

adding a static destructor to a module makes it participate in the cycle detection. It may not be possible to add such things. However, we can in the compiler make standalone static destructors which are free to execute in any order (that change needs to go into this PR).

any destructors should be run AFTER everything is done, doing it in the compiler the way this PR is done is much safer than someone adding a static destructor, possibly in the middle of the module, that will destroy the item early.

Maybe the main thread should run TLS destructors after shared static ~this(), but before shared destructors. The latter could contain the assertion above.

This means adding another entry point to moduleinfo, as all destructors are combined into one function. Which I'm OK with, BTW, but it's going to require more effort than this PR has done.

schveiguy · 2018-12-27T16:22:03Z

Also there might be breakage due to the fact that what this patch does might be done manually in user defined static destructors which will lead to double free errors

All destructors should be fine to run on items that are already destroyed. Classes should be on the heap, so only structs are what we need to worry about.

jacob-carlborg · 2018-12-27T20:32:55Z

The VC runtime has __tlregdtor that is a thread local version of atexit. As we don't have constructors executed for globals, the best we can do is to add one, but that's no different from executing a static destructor with the existing semantics.

@rainers Currently, as far as I know, TLS variables are initialized by the system, that means druntime is not necessary. With this change, druntime is required for TLS destructors to be run correctly.

rainers · 2018-12-28T07:00:31Z

@rainers Currently, as far as I know, TLS variables are initialized by the system, that means druntime is not necessary. With this change, druntime is required for TLS destructors to be run correctly.

On Windows, the system only makes a plain copy, anything else has to be done within the user code. I think we should avoid the C++ trouble of out-of-order con/destructors, and module dependencies are D's solution to that (note that this PR might add circular dependencies to existing code). Moving druntime into the compiler is not a great idea, when the alternative of adding a library to the command line is simple.

jacob-carlborg · 2018-12-28T10:17:49Z

note that this PR might add circular dependencies to existing code

That's not good.

Moving druntime into the compiler is not a great idea,

Not sure I follow.

RazvanN7 · 2018-12-28T11:22:21Z

@rainers

note that this PR might add circular dependencies to existing code)

I do not follow.

rainers · 2018-12-28T11:43:33Z

note that this PR might add circular dependencies to existing code)

I do not follow.

Module info contains infomation about imports that contain static ctors/dtors so they can execute in an appropriate order (those with no dependencies first for ctors, reversed for dtors). Adding a static dtor in a module that has none yet will add this kind of dependency and can cause cycles.

rainers · 2018-12-28T11:47:19Z

Moving druntime into the compiler is not a great idea,

Not sure I follow.

I meant replicating druntime functionality in the compiler (calling dtors for TLS variables and ensuring a proper order in this case).

schveiguy · 2018-12-28T16:27:48Z

Adding a static dtor in a module that has none yet will add this kind of dependency and can cause cycles.

Hm... but a module destructor can be marked as standalone, and so it won't be included in the cycle detection (i.e. it's done last, after all other static destructors are done). I think the way this PR is, that isn't happening, but it should be. I think type destructors are not normally dependent on other module variables. It's possible they are, but unlikely.

The main thing we need to handle (most important), is to destroy thread local items when the thread is no longer active. Since threads can be created and destroyed many times during a process, not doing this leads to leaks. Although there is the possibility of shared globals being leaky with dynamic library loading and unloading, it's less of a problem, and we can always go the route of saying shared data is never destroyed, but thread local is automatically destroyed, so handle the shared globals in shared destructors.

schveiguy · 2018-12-28T16:33:48Z

Another idea:

construct a fake standalone module that just has the static destructor, make it dependent on the current module, and it will naturally run its static destructors after all dependencies have run theirs.

Note, we will have to disable this functionality for betterC.

WalterBright · 2018-12-30T06:35:22Z

The reason this bug exists in D at all is because it was originally designed without RAII, and all initialization/teardown would be manually done in the static this() / static ~this(). This suggests another solution entirely - disallow objects with destructors as static data.

schveiguy · 2018-12-30T16:22:09Z

That means no more stdout?

WalterBright · 2018-12-31T03:45:40Z

That means no more stdout?

static shared ~this()

Anyhow, stdout worked long before D had RAII.

schveiguy · 2018-12-31T20:12:28Z

No, I mean stdout is a std.stdio.File, which has a destructor, so it will no longer be allowed as a static variable.

ghost · 2019-01-04T22:52:03Z

How about using scope to indicate that a global has to be auto destroyed ? This solution is not a breaking change (stdout continues working) and for now scope at the global scope is a noop.

RazvanN7 · 2019-01-07T10:11:34Z

The reason this bug exists in D at all is because it was originally designed without RAII, and all initialization/teardown would be manually done in the static this() / static ~this(). This suggests another solution entirely - disallow objects with destructors as static data.

@WalterBright I think that this is a bit restrictive. Currently, phobos and druntime have global/static
struct variables that have destructors; we would need to update that code and it might not be that obvious on how to do that and if we do it there's no telling how many projects rely on it. An alternative would be to document the current behavior and highlight the fact that the destruction of such variables needs to be done in module destructors.

RazvanN7 · 2019-01-07T10:12:40Z

How about using scope to indicate that a global has to be auto destroyed ? This solution is not a breaking change (stdout continues working) and for now scope at the global scope is a noop.

I like this.

schveiguy · 2019-01-07T15:10:53Z

for now scope at the global scope is a noop

I know that scope at the module level is a noop, but changing something from a noop to a yesop is still a change. If it was an error before, then this would be a change that wouldn't affect any code.

However, I think it will not break any code at all! If you destroy items after they are already destroyed, that is supposed to be allowed. So it would be probably OK to do this. However, @WalterBright should chime in on the effects of this before we merge. scope is getting a complete facelift with dip1000, and I'm not 100% sure this will not affect that, or be confusing.

andralex · 2019-01-07T18:07:46Z

Hamstringing the language to disallow globals would be a major breakage that is only justifiable as a copout. The right solution is to use and integrate with the current module dtors, as follows. Right after each top-level global declaration, insert this:

static shared T x;
// generated code {
static shared ~this() { x.__dtor(); }
// }

Then let the usual language rules take care of circular dependencies etc.

andralex · 2019-01-07T18:08:44Z

Similarly for static unshared top-level declarations, the code generation would go:

static T x;
// generated code {
static ~this() { x.__dtor(); }
// }

schveiguy · 2019-01-07T18:37:05Z

The right solution is to use and integrate with the current module dtors, as follows. Right after each top-level global declaration

Not right after, it needs to go at the end of the module. Otherwise, actual static destructors could be using already-destroyed data.

This is what the PR originally did I believe. There is still a problem with adding cycles to the runtime using this method. My previous suggestion of putting it into a dependent module would not work either, because the destructors would be run first.

So the best answer I can think of is to add those calls at the end of the module, and if the static destructor is ONLY added because of these inserted calls, mark it standalone, so it doesn't participate in cycles.

Fix Issue 14650 - Destructors are not called on global variables

6b9fb99

dlang-bot added the Severity:Bug Fix label Dec 27, 2018

thewilsonator reviewed Dec 27, 2018

View reviewed changes

src/dmd/dmodule.d

* in the below defined array to be later used in a static

* destructor.

*/

Statements dtors;

Copy link

Contributor

thewilsonator Dec 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headers.

rainers reviewed Dec 27, 2018

View reviewed changes

Create static shared dtor for gshared/shared variables

046ced0

Scope global/static variables are destroyed

35f52dd

Auto destroy globals only

1e1d7f4

andralex mentioned this pull request Feb 12, 2019

Replace module-level data with Meyers singletons dlang/druntime#2466

Merged

RazvanN7 closed this Mar 13, 2019

Fix Issue 14650 - Destructors are not called on global variables #9151

Fix Issue 14650 - Destructors are not called on global variables #9151

Conversation

RazvanN7 commented Dec 27, 2018

dlang-bot commented Dec 27, 2018

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

jacob-carlborg commented Dec 27, 2018

andralex commented Dec 27, 2018

rainers commented Dec 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schveiguy Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schveiguy Dec 28, 2018 • edited Loading

Choose a reason for hiding this comment

schveiguy commented Dec 27, 2018

jacob-carlborg commented Dec 27, 2018

rainers commented Dec 28, 2018

jacob-carlborg commented Dec 28, 2018

RazvanN7 commented Dec 28, 2018

rainers commented Dec 28, 2018

rainers commented Dec 28, 2018

schveiguy commented Dec 28, 2018

schveiguy commented Dec 28, 2018

WalterBright commented Dec 30, 2018

schveiguy commented Dec 30, 2018

WalterBright commented Dec 31, 2018

schveiguy commented Dec 31, 2018

ghost commented Jan 4, 2019

RazvanN7 commented Jan 7, 2019 • edited Loading

RazvanN7 commented Jan 7, 2019

schveiguy commented Jan 7, 2019

andralex commented Jan 7, 2019 • edited Loading

andralex commented Jan 7, 2019

schveiguy commented Jan 7, 2019

schveiguy Dec 27, 2018 •

edited

Loading

schveiguy Dec 28, 2018 •

edited

Loading

RazvanN7 commented Jan 7, 2019 •

edited

Loading

andralex commented Jan 7, 2019 •

edited

Loading