ldso: Switch dlopen to use loadDynamicLibrary instead of duplicating it by navytux · Pull Request #7571 · emscripten-core/emscripten

navytux · 2018-11-27T09:39:59Z

Currently dlopen duplicate loadDynamicLibrary in many ways. Since
logic to load dynamic libraries will be soon reworked to support DSO ->
DSO linking, instead of adding more duplication, as a preparatory step
dlopen is first reworked to use loadDynamicLibrary itself.

This moves functionality to keep track of loaded DSO, their handles,
refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now
accepting various flags (global/nodelete) to handle e.g.
RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE
semantic is needed for initially-linked-in libraries).

Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was
previously using Module['read'] and friends, loadDynamicLibrary now also
accepts fs interface, which if provided, is used as FS-like interface to load
library data, and if not - native loading capabilities of the environment
are still used.

Another aspect related to deduplication is that loadDynamicLibrary now also
uses preloaded/precompiled wasm modules, that were previously only used by
dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async
(#6663)").

navytux · 2018-11-27T09:40:25Z

( spawned from #7512 )

sbc100 · 2018-11-27T18:15:24Z

lgtm, what do you think @kripken

navytux · 2018-11-28T08:54:49Z

@sbc100, thanks for feedback.

@kripken, I noticed you did 59f1078 on dlclosure branch (#7572), which both conflicts and will be not needed after here-present patch.

I thus suggest we review/merge the patch present in current PR first. It is already approved by @sbc100, and it would be a pity to have a long wait for when you could have a chance to review and get conflicts then.

So would you please have a look here first, before proceeding with #7572?

Thanks beforehand,
Kirill

kripken · 2018-11-29T00:31:48Z

src/library.js

  },
  // void* dlopen(const char* filename, int flag);
-  dlopen__deps: ['$DLFCN', '$FS', '$ENV'],
+  dlopen__deps: ['$LDSO', '$DLFCN', '$FS', '$ENV'],


Note that $LDSO means: depend on the object LDSO in the library (the $ prefix is due to a limitation in the library system). There is no need to add a dependency on something not in the libraries, so as the PR is written now these deps additions can be removed. (However, see above question on where this should be.)

(If we end up putting LDSO in the library, then the prefix $ is needed as I mentioned for technical reasons. It's only needed in the deps and in the declaration, but when JS code calls it, it should not appear.)

Thanks for clarifying this. Indeed, since LDSO is staying always present in the runrime (in support.js - see the other reply) we don't need to put it into __deps here and we don't need to use $. I'm amending the patch accordingly.

kripken · 2018-11-29T00:33:37Z

src/library.js

+  dlsym:   'dlopen',
+  dlerror: 'dlopen',
+  dladdr:  'dlopen',
+#else


// MAIN_MODULE == 0 on the else and the endif, please

ok, I've added // MAIN_MODULE != 0 after the #else (note the negation). For #endif it already was

#endif // MAIN_MODULE != 0

I put it that way because endif is very far from if/else, and by just looking at the endif it is not clear whether it was only if/endif, or it was if/else/endif, and this way the condition comment on the endif indicates the condition under which ... endif block is actually covered. For else it follows the same logic - it show the condition under which code block covered by else is activated.

We aleady have several cases where such approach is used, e.g. here:

https://github.com/kripken/emscripten/blob/1.38.20-11-gfc1844db2/src/library.js#L526-L539
https://github.com/kripken/emscripten/blob/1.38.20-11-gfc1844db2/src/library_html5.js#L2179-L2238

I hope it is ok.

kripken · 2018-11-29T00:34:57Z

src/library.js

+  dladdr:  'dlopen',
+#else
+
  $DLFCN: {


We should either have both DLFCN and LDSO objects in the library here, or both in support.js there. Unless there is a reason to keep them separate. Would LDSO be used without dlopen?

Yes, LDSO (ld.so stands for dynamic linker/loader, see e.g. http://man7.org/linux/man-pages/man8/ld.so.8.html) is always used in loadDynamicLibrary who anyway needs it to load dynamic libraries that were linked at compile time to the program (or to another library after my next patch).

DLFCN is used only by dlopen & friends, which are in -ldl (libdl), delegates most of the work to the dynamic linker, but still needs to store some libdl specific bits somewhere (e.g. error to return from dlerror) - thus DLFCN.

This way LDSO should be alway present (e.g. in support.js) and DLFCN should be present only when libdl is in use (thus in library.js).

kripken · 2018-11-29T00:38:44Z

src/support.js


 #if RELOCATABLE
+// dynamic linking
+var $LDSO = {


What does LDSO stand for?

It stands for ld.so which is known as "dynamic linker/loader" (see e.g. http://man7.org/linux/man-pages/man8/ld.so.8.html).

kripken · 2018-11-29T00:39:53Z

Agreed to wait on #7572.

Currently dlopen duplicate loadDynamicLibrary in many ways. Since logic to load dynamic libraries will be soon reworked to support DSO -> DSO linking, instead of adding more duplication, as a preparatory step dlopen is first reworked to use loadDynamicLibrary itself. This moves functionality to keep track of loaded DSO, their handles, refcounts, etc into the dynamic linker itself, with loadDynamicLibrary now accepting various flags (global/nodelete) to handle e.g. RTLD_LOCAL/RTLD_GLOBAL and RTLD_NODELETE dlopen cases (RTLD_NODELETE semantic is needed for initially-linked-in libraries). Also, since dlopen was using FS to read libraries, and loadDynamicLibrary was previously using Module['read'] and friends, loadDynamicLibrary now also accepts fs interface, which if provided, is used as FS-like interface to load library data, and if not - native loading capabilities of the environment are still used. Another aspect related to deduplication is that loadDynamicLibrary now also uses preloaded/precompiled wasm modules, that were previously only used by dlopen (see a5866a5 "Add preload plugin to compile wasm side modules async (emscripten-core#6663)").

navytux · 2018-11-29T10:19:19Z

Thanks a lot for coming here and for feedback. I've amended the patch with the following interdiff:

diff --git a/src/library.js b/src/library.js
index 1f5fa4787..37686f19f 100644
--- a/src/library.js
+++ b/src/library.js
@@ -1722,14 +1722,14 @@ LibraryManager.library = {
   dlsym:   'dlopen',
   dlerror: 'dlopen',
   dladdr:  'dlopen',
-#else
+#else // MAIN_MODULE != 0
 
   $DLFCN: {
     error: null,
     errorMsg: null,
   },
   // void* dlopen(const char* filename, int flag);
-  dlopen__deps: ['$LDSO', '$DLFCN', '$FS', '$ENV'],
+  dlopen__deps: ['$DLFCN', '$FS', '$ENV'],
   dlopen__proxy: 'sync',
   dlopen__sig: 'iii',
   dlopen: function(filenameAddr, flag) {
@@ -1783,29 +1783,29 @@ LibraryManager.library = {
     return handle;
   },
   // int dlclose(void* handle);
-  dlclose__deps: ['$LDSO', '$DLFCN'],
+  dlclose__deps: ['$DLFCN'],
   dlclose__proxy: 'sync',
   dlclose__sig: 'ii',
   dlclose: function(handle) {
     // int dlclose(void *handle);
     // http://pubs.opengroup.org/onlinepubs/009695399/functions/dlclose.html
-    if (!$LDSO.loadedLibs[handle]) {
+    if (!LDSO.loadedLibs[handle]) {
       DLFCN.errorMsg = 'Tried to dlclose() unopened handle: ' + handle;
       return 1;
     } else {
-      var lib_record = $LDSO.loadedLibs[handle];
+      var lib_record = LDSO.loadedLibs[handle];
       if (--lib_record.refcount == 0) {
         if (lib_record.module.cleanups) {
           lib_record.module.cleanups.forEach(function(cleanup) { cleanup() });
         }
-        delete $LDSO.loadedLibNames[lib_record.name];
-        delete $LDSO.loadedLibs[handle];
+        delete LDSO.loadedLibNames[lib_record.name];
+        delete LDSO.loadedLibs[handle];
       }
       return 0;
     }
   },
   // void* dlsym(void* handle, const char* symbol);
-  dlsym__deps: ['$LDSO', '$DLFCN'],
+  dlsym__deps: ['$DLFCN'],
   dlsym__proxy: 'sync',
   dlsym__sig: 'iii',
   dlsym: function(handle, symbol) {
@@ -1813,11 +1813,11 @@ LibraryManager.library = {
     // http://pubs.opengroup.org/onlinepubs/009695399/functions/dlsym.html
     symbol = Pointer_stringify(symbol);
 
-    if (!$LDSO.loadedLibs[handle]) {
+    if (!LDSO.loadedLibs[handle]) {
       DLFCN.errorMsg = 'Tried to dlsym() from an unopened handle: ' + handle;
       return 0;
     } else {
-      var lib = $LDSO.loadedLibs[handle];
+      var lib = LDSO.loadedLibs[handle];
       symbol = '_' + symbol;
       if (!lib.module.hasOwnProperty(symbol)) {
         DLFCN.errorMsg = ('Tried to lookup unknown symbol "' + symbol +
@@ -1854,7 +1854,7 @@ LibraryManager.library = {
     }
   },
   // char* dlerror(void);
-  dlerror__deps: ['$LDSO', '$DLFCN'],
+  dlerror__deps: ['$DLFCN'],
   dlerror__proxy: 'sync',
   dlerror__sig: 'i',
   dlerror: function() {
diff --git a/src/support.js b/src/support.js
index a57faab81..249c4c1ef 100644
--- a/src/support.js
+++ b/src/support.js
@@ -85,7 +85,7 @@ var asm2wasmImports = { // special asm2wasm imports
 
 #if RELOCATABLE
- // dynamic linking
-var $LDSO = {
+// dynamic linker/loader (a-la ld.so on ELF systems)
+var LDSO = {
   // next free handle to use for a loaded dso.
   // (handle=0 is avoided as it means "error" in dlopen)
   nextHandle: 1,
@@ -143,14 +143,14 @@ function loadDynamicLibrary(lib, flags) {
   // when loadDynamicLibrary did not have flags, libraries were loaded globally & permanently
   flags = flags || {global: true, nodelete: true}
 
-  var handle = $LDSO.loadedLibNames[lib];
+  var handle = LDSO.loadedLibNames[lib];
   var dso;
   if (handle) {
     // the library is being loaded or has been loaded already.
     //
     // however it could be previously loaded only locally and if we get
     // load request with global=true we have to make it globally visible now.
-    dso = $LDSO.loadedLibs[handle];
+    dso = LDSO.loadedLibs[handle];
     if (flags.global && !dso.global) {
       dso.global = true;
       if (dso.module !== 'loading') {
@@ -167,15 +167,15 @@ function loadDynamicLibrary(lib, flags) {
   }
 
   // allocate new DSO & handle
-  handle = $LDSO.nextHandle++;
+  handle = LDSO.nextHandle++;
   dso = {
     refcount: flags.nodelete ? Infinity : 1,
     name:     lib,
     module:   'loading',
     global:   flags.global,
   };
-  $LDSO.loadedLibNames[lib] = handle;
-  $LDSO.loadedLibs[handle] = dso;
+  LDSO.loadedLibNames[lib] = handle;
+  LDSO.loadedLibs[handle] = dso;
 
   // libData <- lib
   function loadLibData() {

Hope it is ok.

kripken · 2018-11-29T18:03:13Z

Thanks for the clarifications! Looks good.

navytux · 2018-11-29T19:38:45Z

@kripken, thanks for feedback and for merging. @sbc100 thanks again for your review.

navytux mentioned this pull request Nov 27, 2018

Teach dynamic linking to handle library -> library dependencies #7512

Merged

sbc100 requested a review from kripken November 27, 2018 18:15

kripken reviewed Nov 29, 2018

View reviewed changes

navytux force-pushed the y/ldso-needed.4 branch from b38b6c5 to 3d53e5b Compare November 29, 2018 10:18

kripken merged commit 538f958 into emscripten-core:incoming Nov 29, 2018

navytux deleted the y/ldso-needed.4 branch November 29, 2018 19:38

Comments

Conversation

navytux commented Nov 27, 2018

Uh oh!

navytux commented Nov 27, 2018

Uh oh!

sbc100 commented Nov 27, 2018

Uh oh!

navytux commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Nov 29, 2018

Uh oh!

navytux commented Nov 29, 2018

Uh oh!

kripken commented Nov 29, 2018

Uh oh!

navytux commented Nov 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants