add support for linux dynamic dll loading #583

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Owner

WalterBright commented Aug 23, 2013

First stab at supporting dynamically loadable shared libraries under Linux. It's magical, because (thanks to @dawgfoto's ideas) you don't have to do anything other than use the Linux API to load/unload DLLs. They initialize themselves!

(Currently won't work if you try to load DLLs at the same time from different threads, needs some synchronization.)

Also needs some thought towards what should happen if an exception is thrown during module construction/destruction.

Once we get thoroughly satisfied that this is right, we can extend it to the other platforms.

Owner

WalterBright commented Aug 23, 2013

Looks like something is going wrong in the thread shutdown. I'll look into it tomorrow.

@alexrp alexrp and 1 other commented on an outdated diff Aug 23, 2013

src/rt/dmain2.d
@@ -243,7 +243,7 @@ extern (C) bool rt_init(ExceptionHandler dg = null)
initStaticDataGC();
rt_moduleCtor();
rt_moduleTlsCtor();
- runModuleUnitTests();
+ //runModuleUnitTests();
@alexrp

alexrp Aug 23, 2013

Member

Wait what? Debug leftover?

@WalterBright

WalterBright Aug 23, 2013

Owner

Running unit tests is not really part of initialization. I moved the call to line 567.

@alexrp alexrp and 3 others commented on an outdated diff Aug 23, 2013

src/rt/dmain2.d
@@ -286,6 +286,8 @@ extern (C) bool rt_term(ExceptionHandler dg = null)
{
if (dg)
dg(e);
+ else
+ throw e; // rethrow, don't silently ignore error
@alexrp

alexrp Aug 23, 2013

Member

Fine with me, but should be separate commit.

@complexmath

complexmath Aug 23, 2013

Member

Fine even though rt_init() is being called from C code?

@WalterBright

WalterBright Aug 23, 2013

Owner

Note that I adjusted the behavior of rt_term() to match that of rt_init() in regards to exceptions.

@WalterBright

WalterBright Aug 23, 2013

Owner

Also, when this propagates to all platforms, rt_init() will no longer be called by C code. The initialization will happen automatically, just by loading the DLL using the operating system call to do so.

@andralex

andralex Aug 25, 2013

Owner

I'd write this as

if (!dg) 
    throw e;
dg();

i.e. minimize flow control.

@alexrp alexrp commented on the diff Aug 23, 2013

src/rt/dmain2.d
@@ -560,27 +559,27 @@ extern (C) int _d_run_main(int argc, char **argv, MainFunc mainFunc)
void runAll()
{
- initSections();
- gc_init();
- initStaticDataGC();
- rt_moduleCtor();
- rt_moduleTlsCtor();
+ version (linux)
@alexrp

alexrp Aug 23, 2013

Member

Should probably copy the comment here as this is more likely the place people will look and wonder "what's going on here?"

@WalterBright

WalterBright Aug 23, 2013

Owner

Copy what comment?

Member

alexrp commented Aug 23, 2013

Module-related code is beyond me; cc @dawgfoto

Member

MartinNowak commented Aug 23, 2013

Among other things we cannot initialize a shared library for another thread but the loading one. This means we must not use one globally shared data structure (_static_dsos). Instead it should contain only the linked libraries. For the dynamically loaded libraries we should use a thread local data structure.

Member

MartinNowak commented Aug 23, 2013

While handling rt.dmain2's constructor specially works, we could simply run rt_init before the first module constructor (here) and rt_term after the last one. I think that would be a cleaner solution.

@complexmath complexmath commented on the diff Aug 23, 2013

src/rt/dmain2.d
+ _STI_monitor_staticctor();
+ _STI_critical_init();
+ initSections();
+ gc_init();
+ initStaticDataGC();
+}
+
+shared static ~this()
+{
+ //printf("~dmain2\n");
+ thread_joinAll();
+ gc_term();
+ finiSections();
+ _STD_critical_term();
+ _STD_monitor_staticdtor();
+}
@complexmath

complexmath Aug 23, 2013

Member

Will this really work the way we want? Since there's no dependency tree here, it seems like this static dtor will be run in a random order with the others, and so some static dtors will be run after the GC and runtime are completely shut down.

@WalterBright

WalterBright Aug 23, 2013

Owner

In minfo.d, I added code specifically to run this ctor first, and this dtor last.

@MartinNowak

MartinNowak Aug 26, 2013

Member

Just call rt_init within the first call to _d_dso_registry and rt_term when the last library unregisters. This is dead simple and you can scratch all the code to treat rt.dmain2 specially.

Owner

WalterBright commented Aug 23, 2013

Among other things we cannot initialize a shared library for another thread but the loading one. This means we must not use one globally shared data structure (_static_dsos). Instead it should contain only the linked libraries. For the dynamically loaded libraries we should use a thread local data structure.

I know _static_dsos[] needs to be protected with a mutex. Other than that, I believe the design is sound.

Owner

WalterBright commented Aug 23, 2013

While handling rt.dmain2's constructor specially works, we could simply run rt_init before the first module constructor (here) and rt_term after the last one. I think that would be a cleaner solution.

You may be right. But I'd like to get everything working first, and then we can look at it again and perhaps refactor it.

Owner

braddr commented Aug 23, 2013

This whole area is in need of a comprehensive set of tests. Have you been building them or sticking to ad-hoc tests?

Owner

WalterBright commented Aug 23, 2013

A great deal of it is tested already with the existing suite.

For the more specific tests for loading/unloading, yes I have some, and I've been working up in parallel a guide to writing such DLLs. But I was going to ask for your help in integrating them into the auto tester.

Owner

WalterBright commented Aug 24, 2013

Hopefully got the std.concurrency bug fixed. Turns out that atexit() runs before the .ctors, which caused the mutexes used by the GC to be prematurely destroyed.

Member

MartinNowak commented Aug 24, 2013

I know _static_dsos[] needs to be protected with a mutex. Other than that, I believe the design is sound.

This is a bigger issue than it might initially appear. Say that thread A loads a shared library libfoo.so which contains module foo. Because we can only initialize that module for thread A one cannot use that module in thread B.
Using a thread local array for dynamically loaded libraries is necessary to keep track of which library is initialized for which thread.
One consequence of this is that foreach (m; ModuleInfo) might list different modules in different threads.

Owner

WalterBright commented Aug 24, 2013

I don't see a fundamental reason why a dll cannot be initialized for all threads.

Owner

WalterBright commented Aug 25, 2013

I added a mutex around access to _static_dsos[].

Member

MartinNowak commented Aug 25, 2013

I don't see a fundamental reason why a dll cannot be initialized for all threads.

Because native TLS access can't be redirected, you'd need to alter the GS register to do so.
Furthermore it would require to pause other threads which makes loading a library much more expensive than it ought to be.

Owner

WalterBright commented Aug 25, 2013

Suppose Thread 1 loads DLL A. Then Thread 2 loads DLL A. It isn't going to be loaded or initialized again.

Also, there's nothing stopping Thread 3 from calling functions in A.

Owner

WalterBright commented Aug 25, 2013

@dawgfoto Ah, I see what you mean. Thread 1 cannot initialize TLS in Thread 2, i.e. cannot call the module tls ctors for Thread 2.

In general it may be a problem we're stuck with. Dynamically loading/unloading DLLs does have some safety issues that I don't see a reasonable way around, such as unloading a DLL while still having function pointers into the DLL.

But in the meantime, this pull does not make things worse, nor does it preclude other designs.

Member

MartinNowak commented Aug 26, 2013

Suppose Thread 1 loads DLL A. Then Thread 2 loads DLL A. It isn't going to be loaded or initialized again.

We can perform initialization for Thread 2 in Runtime.loadLibrary. That would need a mapping from the dlopened handle to the DSO struct and knowing library dependencies. It's something to do after we get a single threaded version to run.

Also, there's nothing stopping Thread 3 from calling functions in A.

That's true and as you said certain programming errors remain possible.
At the same time we shouldn't list uninitialized modules.
A thread local array for dynamically loaded libraries solves this and requires no synchronization.

@andralex andralex commented on an outdated diff Aug 26, 2013

src/rt/minfo.d
@@ -14,6 +14,7 @@ module rt.minfo;
import core.stdc.stdlib; // alloca
import core.stdc.string; // memcpy
+import core.stdc.stdio; // printf
@andralex

andralex Aug 26, 2013

Owner

since there are comments for what functions get used, at best the : notation should be used

@andralex andralex commented on the diff Aug 26, 2013

src/rt/minfo.d
@@ -203,6 +220,19 @@ struct ModuleGroup
void runCtors()
{
+ version (none)
@andralex

andralex Aug 26, 2013

Owner

version (debug_something)?

@WalterBright

WalterBright Aug 26, 2013

Owner

I prefer to turn printf's on individually as desired. Turning on a bunch with a global flag tends to produce a blizzard of senseless output.

@andralex andralex commented on the diff Aug 26, 2013

src/rt/minfo.d
@@ -245,6 +278,8 @@ private:
ModuleInfo*[] _modules;
ModuleInfo*[] _ctors;
ModuleInfo*[] _tlsctors;
+public:
@andralex

andralex Aug 26, 2013

Owner

why public I wonder

@WalterBright

WalterBright Aug 26, 2013

Owner

Because sections_linux.d accesses first.

@andralex andralex commented on an outdated diff Aug 26, 2013

src/rt/sections_linux.d
- _static_dsos.popBack();
+
+ // Run module destructors for DSO
+ pdso._moduleGroup.runTlsDtors();
+ if (pdso._moduleGroup.first)
+ thread_joinAll();
+ pdso._moduleGroup.runDtors();
+
+ /* If DSOs come from dynamically loaded DLLs, they can be unloaded
+ * in any order. Most of the time, however, they'll be unloaded in
+ * reverse order. So search backwards for pdso in _static_dsos[].
+ * Once we find it, ripple the trailing entries over it, and shorten
+ * _static_dsos[] by one.
+ */
+ DsoMutex.lock();
+ for (size_t i = _static_dsos.length; 1; )
@andralex

andralex Aug 26, 2013

Owner

no need for 1

Owner

andralex commented Aug 26, 2013

@braddr @dawgfoto OK to pull this in the interest of time? If it doesn't lock us out of good designs etc. it would be great. Please advise, thanks.

WalterBright referenced this pull request in dlang/dlang.org Aug 26, 2013

Merged

add dll-linux.html #375

Owner

WalterBright commented Aug 26, 2013

The corresponding documentation:

D-Programming-Language/dlang.org#375

@MartinNowak MartinNowak commented on the diff Aug 26, 2013

src/rt/sections_linux.d
+ static void term()
+ {
+ pthread_mutex_destroy(&mutex);
+ }
+
+ static void lock()
+ {
+ pthread_mutex_lock(&mutex);
+ }
+
+ static void unlock()
+ {
+ pthread_mutex_unlock(&mutex);
+ }
+}
+
@MartinNowak

MartinNowak Aug 26, 2013

Member

You don't actually need a mutex because calls to dlopen/dlclose are already globally serialized by the runtime linker, i.e. there is always only one thread calling _d_dso_registry.

@WalterBright

WalterBright Aug 26, 2013

Owner

But there are references to _static_dsos[] outside of _d_dso_registry which do need to be synchronized with the ones inside _d_dso_registry. Otherwise the code may wind up pointing to memory that has been free()'d.

Member

MartinNowak commented Aug 26, 2013

OK to pull this in the interest of time?

I'd rather spend two more days to sort out the remaining issues with this pull.
Also splitting off some changes (i.e. #587, thread_term) would make sense.

Owner

WalterBright commented Aug 26, 2013

Also splitting off some changes (i.e. #587, thread_term) would make sense.

I had that as a separate pull already,
https://github.com/D-Programming-Language/druntime/pull/581/files
but merged it in with this one as there seemed no further point of making it separate.

Member

MartinNowak commented Aug 26, 2013

I had that as a separate pull already, #581

Which wasn't immediately merged because it introduced two unrelated behavioral changes.
Those are still debatable within this much bigger pull, so splitting off orthogonal changes will help to focus the discussion and proceed with the merge.

Member

MartinNowak commented Aug 26, 2013

I added #589 to #587.

@MartinNowak MartinNowak commented on the diff Aug 26, 2013

src/rt/dmain2.d
@@ -593,3 +589,30 @@ extern (C) int _d_run_main(int argc, char **argv, MainFunc mainFunc)
return result;
}
+
+version (linux)
+{
+/* Startup and shutdown is done with static construction/destruction
+ */
+
+//import core.stdc.stdio;
+shared static this()
+{
+ //printf("dmain2\n");
+ _STI_monitor_staticctor();
+ _STI_critical_init();
+ initSections();
+ gc_init();
@MartinNowak

MartinNowak Aug 26, 2013

Member

This call chain comes to early gc_init=>thread_init=>thread_attachThis=>rt.sections_linux.initTLSRanges, because it makes the implicit assumption that all libraries are already loaded. It will miss the TLS ranges from anything but libphobos2.so.

Member

MartinNowak commented Aug 26, 2013

I tried to make an alternative pull for the auto initialization during loading (https://github.com/dawgfoto/druntime/tree/autoInit) and it turns out, that this requires more work because druntime's initialization is pretty messy and contains a lot of implicit assumptions which break when we perform the initialization earlier.
As this is not strictly required to get dynamic loading to work we could defer this.

Member

MartinNowak commented Aug 26, 2013

Got it to work #590.

Owner

WalterBright commented Aug 26, 2013

Which wasn't immediately merged because it introduced two unrelated behavioral changes.

They were related because the initialization in rt_init and otherwise was different. They needed to be the same. It would not pass the test suite otherwise.

Member

MartinNowak commented Aug 27, 2013

And dynamic loading #593.

Owner

WalterBright commented Sep 7, 2013

This is superceded by #593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment