-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Move dlopen file operations into native code. NFC #19310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,8 @@ | |
"mem_size", | ||
"table_addr", | ||
"table_size", | ||
"file_data", | ||
"file_data_size", | ||
"name" | ||
] | ||
} | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
#define _GNU_SOURCE | ||
#include <assert.h> | ||
#include <dlfcn.h> | ||
#include <fcntl.h> | ||
#include <pthread.h> | ||
#include <threads.h> | ||
#include <stdarg.h> | ||
|
@@ -19,6 +20,7 @@ | |
#include <stdlib.h> | ||
#include <string.h> | ||
#include <sys/stat.h> | ||
#include <unistd.h> | ||
|
||
#include <emscripten/console.h> | ||
#include <emscripten/threading.h> | ||
|
@@ -81,6 +83,8 @@ static thread_local struct dlevent* thread_local_tail = &main_event; | |
static pthread_mutex_t write_lock = PTHREAD_MUTEX_INITIALIZER; | ||
static thread_local bool skip_dlsync = false; | ||
|
||
static void dlsync(); | ||
|
||
static void do_write_lock() { | ||
// Once we have the lock we want to avoid automatic code sync as that would | ||
// result in a deadlock. | ||
|
@@ -155,6 +159,14 @@ static void load_library_done(struct dso* p) { | |
p->table_addr, | ||
p->table_size); | ||
new_dlevent(p, -1); | ||
#ifdef _REENTRANT | ||
// Block until all other threads have loaded this module. | ||
dlsync(); | ||
#endif | ||
if (p->file_data) { | ||
free(p->file_data); | ||
p->file_data_size = 0; | ||
} | ||
} | ||
|
||
static struct dso* load_library_start(const char* name, int flags) { | ||
|
@@ -169,6 +181,29 @@ static struct dso* load_library_start(const char* name, int flags) { | |
p->flags = flags; | ||
strcpy(p->name, name); | ||
|
||
// If the file exists in the filesystem, load it here into linear memory which | ||
// makes the data available to JS, and to other threads. This data gets | ||
// free'd later once all threads have loaded the DSO. | ||
struct stat statbuf; | ||
if (stat(name, &statbuf) == 0 && S_ISREG(statbuf.st_mode)) { | ||
int fd = open(name, O_RDONLY); | ||
if (fd >= 0) { | ||
off_t size = lseek(fd, 0, SEEK_END); | ||
if (size != (off_t)-1) { | ||
lseek(fd, 0, SEEK_SET); | ||
p->file_data = malloc(size); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check for a failing |
||
if (p->file_data) { | ||
if (read(fd, p->file_data, size) == size) { | ||
p->file_data_size = size; | ||
} else { | ||
free(p->file_data); | ||
} | ||
} | ||
} | ||
close(fd); | ||
} | ||
} | ||
|
||
return p; | ||
} | ||
|
||
|
@@ -424,10 +459,6 @@ static void dlopen_onsuccess(struct dso* dso, void* user_data) { | |
dso->mem_addr, | ||
dso->mem_size); | ||
load_library_done(dso); | ||
#ifdef _REENTRANT | ||
// Block until all other threads have loaded this module. | ||
dlsync(); | ||
#endif | ||
do_write_unlock(); | ||
data->onsuccess(data->user_data, dso); | ||
free(data); | ||
|
@@ -526,10 +557,6 @@ static struct dso* _dlopen(const char* file, int flags) { | |
} | ||
dbg("dlopen_js: success: %p", p); | ||
load_library_done(p); | ||
#ifdef _REENTRANT | ||
// Block until all other threads have loaded this module. | ||
dlsync(); | ||
#endif | ||
end: | ||
dbg("dlopen(%s): done: %p", file, p); | ||
do_write_unlock(); | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
28151 | ||
28007 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a regression for us because we've been using the
fs
argument toloadDynamicLibrary
. Do you have a recommended alternative @sbc100?cc @ryanking13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call
_dlopen(...)
perhaps instead?loadDynamicLibrary
and the other symbols inlibrary_dylink.js
are not really part of the public API .. or at least I would prefer them not to be considered public.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear. What was the intended purpose of this hook if it's not supposed to be used downstream?
There's a few issues. One is asynchronous compilation:
dlopen
is synchronous I think, so it has to instantiate the module synchronously, which fails on the main thread for large modules.A second problem is resolving dynamic library dependencies. If we
dlopen
a librarya.so
that depends onb.so
then the lookup logic by default it does something we don't want.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can find a way to work around that. There are 3 different versions of dlopen in emscripten and 2 of them are async (
emscripten_dlopen_promise
andemscripten_dlopen
) . They are mostly designed to be called from native code though. There is you loading code happening? Is it originating in native code?Can you say little more about this. The logic for loading the dependencies of dynamic libraries is all embedded in loadWebAssemblyModule on the JS side so I would expect them to be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sadly we don't have much clarity about what is supposed to be internal vs public JS APIs. I wish we did. In this case we used that hook internally, but after this change it was no longer needed. I'd rather not add it back because the new method of loading the data from the filesystem once on the main thread and sharing it via shared memory has a lot of advantages (the alternative is that each thread would need to do FS operations of their own, which themselves get proxied back to main thread, which adds a up to load wasted work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue here is that we need to adjust how dependencies are looked up. If we call
FS.createPreloadedFile
directly, it will do its normal dependency resolution process without any way for us to control it. So we either need a hook or we need a way to work out the dependencies ahead of time and resolve them manually. As far as I can tellgetDylinkMetadata
is the only way for us to work out dependencies ahead of time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say "adjust how dependencies are looked up" do you mean controlling where .so files are found (like LD_LIBRARY_PATH)? Or something else?
Is that because you have DSO with the same name in different locations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be all we need, but I'm not 100% sure. Presumably there is some correct standard way to do this we could be using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping. Let me make it a little clearer
Yes, "adjust how dependencies are looked up" is why Pyodide uses fs.findObject and fs.readfile now.
If a.so depends on b.so, we need a way to locate b.so somewhere in the (virtual) file system. So we want a global paths (LD_LIBRARY_PATH) that dependencies can be searched from.
Additionally, It would be great to have a thing like RPATH (pyodide/pyodide#3854). To make Python packages portable, we are trying to bundle shared libraries into Python wheel, so we want a way to search those "local" paths, which is availble in Linux with RPATH.
It's similar, but a little different.
The reason we modify
LDSO.loadedLibsByName
is to handle cases when we pass an absolute path and just a lib name intoloadDynamicLibrary
: when two libraries are the same.For example, if we have a situation where we have previously called
loadDynamicLibrary (/usr/lib/b.so)
to load b.so, and we subsequently loada.so
that depends onb.so
, we want to know that theb.so
thata.so
depends on is/usr/lib/b.so
, which has already been loaded, so that we don't get a duplicate load.For now,
a.so
will callloadDynamicLibrary(b.so)
in the process of loading the dependent library, so we need to tell it that this is the same file as/usr/lib/b.so
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more specific, the reason why we are using
getDylinkMetadata
now in Pyodide is to handle the bug described in #18264.If a library is loaded locally with RTLD_LOCAL option,
the dependent libraries will also be loaded locally. On Linux, dependent libraries can resolve symbols even if they are loaded locally, but I think there is currently a bug in Emscripten that prevents this.
As a workaround, Pyodide is changing the option to force libraries that are dependencies of other libraries to be loaded globally, using the metadata value returned by
getDylinkMetadata
.