Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Restructuring emscripten_dlopen_js #21595

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+60 −47
Open

Restructuring emscripten_dlopen_js #21595

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4aa494d
restructuring emscripten_dlopen_js
impact-maker Mar 22, 2024
5877e57
restructed the code
impact-maker Mar 23, 2024
2d2f41f
changed promise to callback format
impact-maker Mar 27, 2024
de9e66b
dlopen revision
impact-maker Apr 15, 2024
6d3a264
added promisetoCallbacks and removed error
impact-maker Apr 18, 2024
0508b17
internal function restored
impact-maker Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
120 changes: 67 additions & 53 deletions src/library_dylink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,42 +1097,36 @@ var LibraryDylink = {
},

// void* dlopen(const char* filename, int flags);
$dlopenInternal__deps: ['$ENV', '$dlSetError', '$PATH'],
$dlopenInternal: (handle, jsflags) => {
// void *dlopen(const char *file, int mode);
// http://pubs.opengroup.org/onlinepubs/009695399/functions/dlopen.html
var filename = UTF8ToString(handle + {{{ C_STRUCTS.dso.name }}});
var flags = {{{ makeGetValue('handle', C_STRUCTS.dso.flags, 'i32') }}};
#if DYLINK_DEBUG
dbg(`dlopenInternal: ${filename}`);
#endif
filename = PATH.normalize(filename);
var searchpaths = [];

var global = Boolean(flags & {{{ cDefs.RTLD_GLOBAL }}});
var localScope = global ? null : {};

// We don't care about RTLD_NOW and RTLD_LAZY.
var combinedFlags = {
global,
nodelete: Boolean(flags & {{{ cDefs.RTLD_NODELETE }}}),
loadAsync: jsflags.loadAsync,
}

if (jsflags.loadAsync) {
return loadDynamicLibrary(filename, combinedFlags, localScope, handle);
}

try {
return loadDynamicLibrary(filename, combinedFlags, localScope, handle)
} catch (e) {
#if ASSERTIONS
err(`Error in loading dynamic library ${filename}: ${e}`);
#endif
dlSetError(`Could not load dynamic lib: ${filename}\n${e}`);
return 0;
}
},
$dlopenInternal__deps: ['$loadDynamicLibrary', '$dlSetError', '$PATH'],
$dlopenInternal: function(filename, flags) {

filename = UTF8ToString(filename);
filename = PATH.normalize(filename);

var global = Boolean(flags & {{{ cDefs.RTLD_GLOBAL }}});
var nodelete = Boolean(flags & {{{ cDefs.RTLD_NODELETE }}});

var loadAsync = !!(flags & {{{ cDefs.RTLD_NOW }}});

var promise = loadDynamicLibrary(filename, {
global: global,
nodelete: nodelete,
loadAsync: loadAsync
});

if (loadAsync) {
return promise.then(module => {
var handle = _malloc(4);
{{{ makeSetValue('handle', 0, 'module', 'i32') }}};
return handle;
}).catch(err => {
$dlSetError('Dlopen failed: ' + err.message);
return 0;
});
} else {
return Asyncify.handleSync(promise);
}
},

_dlopen_js__deps: ['$dlopenInternal'],
#if ASYNCIFY
Expand All @@ -1150,30 +1144,50 @@ var LibraryDylink = {
#endif
},

// Async version of dlopen.
_emscripten_dlopen_js__deps: ['$dlopenInternal', '$callUserCallback', '$dlSetError'],
_emscripten_dlopen_js: (handle, onsuccess, onerror, user_data) => {
/** @param {Object=} e */
// Utility function to encapsulate promise handling with callbacks
$promiseToCallback: function(promise, successFnPtr, errorFnPtr, user_data) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this utility being called anywhere

function errorCallback(e) {
var filename = UTF8ToString(handle + {{{ C_STRUCTS.dso.name }}});
dlSetError(`'Could not load dynamic lib: ${filename}\n${e}`);
{{{ runtimeKeepalivePop() }}}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the {{{ }}} are still removed from this. Were you able to run the tests locally? They should be failing because of this problem.

Copy link
Author

Choose a reason for hiding this comment

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

@tlively necessary changes were missed in the previous commit. I have now pushed the latest changes. I have gone through the documentation but still doubtful on what commands and test suites needs to run to confirm for the api restructuring changes.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at CI, it looks one the failing tests is test_pthread_dlopen_many in the core0 suite. To run just that test locally, you would run test/runner core0.test_pthread_dlopen_many. To run all the tests in core0 (which means the core tests with optimization level O0), you would run test/runner core0. You can also use the --verbose and --save-dir flags to get more information and to save the files so you can manually run and debug the test yourself.

callUserCallback(() => {{{ makeDynCall('vpp', 'onerror') }}}(handle, user_data));
var filename = UTF8ToString(handle + C_STRUCTS.dso.name);
dlSetError(`Could not load dynamic lib: ${filename}\n${e}`);
runtimeKeepalivePop();
if(errorFnPtr) {
callUserCallback(() => dynCall('vpp', errorFnPtr, [0, user_data]));
}
}
function successCallback() {
{{{ runtimeKeepalivePop() }}}
callUserCallback(() => {{{ makeDynCall('vpp', 'onsuccess') }}}(handle, user_data));
runtimeKeepalivePop();
if(successFnPtr) {
callUserCallback(() => dynCall('vpp', successFnPtr, [0, user_data]));
}
}

{{{ runtimeKeepalivePush() }}}
var promise = dlopenInternal(handle, { loadAsync: true });
if (promise) {
promise.then(successCallback, errorCallback);
} else {
errorCallback();
}
// Assert that the promise is provided
assert(promise, 'promiseToCallback was called without a valid promise.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put this line at the top of the function and wrap in #if ASSERTIONS check

promise.then(successCallback).catch(errorCallback);
},

_emscripten_dlopen_js__deps: ['$dlopenInternal', '$runtimeKeepalivePush', '$runtimeKeepalivePop', '$dlSetError', '$dynCall'],
_emscripten_dlopen_js: function(handle, promise, onsuccess, onerror,) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems a little odd to mix passing a promise here with passing callback. I would assume that we would want one or the other and not both.

In fact it looks like this code doesn't actually use the promise argument right now?


{{{ runtimeKeepalivePush() }}}

var filename = UTF8ToString(handle);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the first argument of this function now a filename and not a handle then perhaps it should be renamed.

var flags = {{{ makeGetValue('promise', 4, 'i32') }}};

dlopenInternal(filename, flags)
.then(module => {
{{{ makeDynCall('vpp', 'onsuccess') }}}(module, handle);
{{{ runtimeKeepalivePop() }}};
}).catch(err => {
{{{ makeDynCall('vpp', 'onerror') }}}(0, user_data);
{{{ runtimeKeepalivePop() }}};
});

},




_dlsym_catchup_js: (handle, symbolIndex) => {
#if DYLINK_DEBUG
dbg("_dlsym_catchup: handle=" + ptrToString(handle) + " symbolIndex=" + symbolIndex);
Expand Down
64 changes: 26 additions & 38 deletions system/lib/libc/dynlink.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,35 +586,11 @@ void* dlopen(const char* file, int flags) {

void emscripten_dlopen(const char* filename, int flags, void* user_data,
em_dlopen_callback onsuccess, em_arg_callback_func onerror) {
dbg("emscripten_dlopen: %s", filename);
if (!filename) {
onsuccess(user_data, head->dso);
return;
}
do_write_lock();
char buf[2*NAME_MAX+2];
filename = resolve_path(buf, filename, sizeof buf);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened to this call to resolve_path?

struct dso* p = find_existing(filename);
if (p) {
onsuccess(user_data, p);
return;
}
p = load_library_start(filename, flags);
if (!p) {
do_write_unlock();
onerror(user_data);
return;
}

// For async mode
struct async_data* d = malloc(sizeof(struct async_data));
d->user_data = user_data;
d->onsuccess = onsuccess;
d->onerror = onerror;

em_promise_t promise = emscripten_dlopen_promise(filename,flags);
emscripten_promise_then(promise, onsuccess, user_data);
emscripten_promise_catch(promise, onerror, user_data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This nice, it seems like you have implemented something like promiseToCallback in native code?

I think it might be fast it if was implemented JS though since using promises from the native does have some amount of overhead I think (just calling back and forth into JS has a cost).


dbg("calling emscripten_dlopen_js %p", p);
// Unlock happens in dlopen_onsuccess/dlopen_onerror
_emscripten_dlopen_js(p, dlopen_onsuccess, dlopen_onerror, d);
}

static void promise_onsuccess(void* user_data, void* handle) {
Expand All @@ -636,16 +612,28 @@ static void promise_onerror(void* user_data) {
// TODO(sbc): Consider inverting this and perhaps deprecating/removing
// the old API.
em_promise_t emscripten_dlopen_promise(const char* filename, int flags) {
// Create a promise that is resolved (and destroyed) once the operation
// succeeds.
em_promise_t p = emscripten_promise_create();
emscripten_dlopen(filename, flags, p, promise_onsuccess, promise_onerror);

// Create a second promise bound the first one to return the caller. It's
// then up to the caller to destroy this promise.
em_promise_t ret = emscripten_promise_create();
emscripten_promise_resolve(ret, EM_PROMISE_MATCH, p);
return ret;

if (!filename) {
return emscripten_promise_resolve(EM_PROMISE_FULFILL, RTLD_DEFAULT);
}

struct dso* p = find_existing(filename);

if (p) {
return emscripten_promise_resolve(EM_PROMISE_FULFILL, p);
}

em_promise_t promise = emscripten_promise_create();

p = load_library_start(filename, flags);

if (!p) {
emscripten_promise_reject(promise, NULL);
} else {
_emscripten_dlopen_js(p, promise, dlopen_onsuccess, dlopen_onerror);
}

return promise;
}

void* __dlsym(void* restrict p, const char* restrict s, void* restrict ra) {
Expand Down