Skip to content

Reimplement resolve_path in JS Take 2 NFC #23958

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

Closed
wants to merge 16 commits into from

Conversation

ryanking13
Copy link
Contributor

Reopening #23935


As discussed in #23872, this PR reimplements resolve_path function, which is used to locate the library using LD_LIBRARY_PATH in JavaScript, so that it can be reused inside libdylink.js.

Currently, this code doesn't work in thread.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 20, 2025

One concern I have here is generally moving code from native to JS is that opposite direction that we've been trying to move a long time in emscripten.

Would it be possible to invert this have the JS code call into native code to do the LD_LIBRARY_PATH and filesystem searching?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 20, 2025

I guess on problem is that the dyanmic linking code can sometime run before the native code is ready to execute?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 20, 2025

On the other hand with wasmfs we require the native code to be running in order to even use the filesystem.

@hoodmane
Copy link
Collaborator

I can try going the opposite direction. I see no reason that it wouldn't work.

@ryanking13
Copy link
Contributor Author

I guess on problem is that the dyanmic linking code can sometime run before the native code is ready to execute?

When will that happen? Does preloading files happen before the native code is ready to execute?

@hoodmane
Copy link
Collaborator

Well it happens before malloc is ready to execute. I think if the code doesn't allocate anything it should work fine.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 21, 2025

I guess on problem is that the dyanmic linking code can sometime run before the native code is ready to execute?

When will that happen? Does preloading files happen before the native code is ready to execute?

Yes, if you have main module with a set of side modules (in the needed section, or specified via Module.dynamicLibraries) then we load the main module and all these dependencies before we run any native code (I think).

So I guess I'm going back and forth on this... it would be nice if it could be native, but also maybe there are reasons why it cannot be. I'm actually concerned now that the design of wasmfs is going to be a problem here since the FS itself is implemented in native code. @tlively @kripken

@kripken
Copy link
Member

kripken commented Mar 24, 2025

I'm having trouble following the issue here, as explanations refer to a long previous discussion, and I don't see a concrete testcase in the PR that I could learn from. Can someone perhaps summarize things?

@hoodmane
Copy link
Collaborator

The goal is to implement RPATH support. So we need a function that checks for a library first in zero or more RPATH entries, and then in zero or more LD_LIBRARY_PATH entries. The solution that @ryanking13 has currently implemented reimplements the existing C path_find code which does the LD_LIBRARY_PATH lookup into JS. @sbc100 is asking whether we could just move all the code into C or whether that might cause trouble with preloaded dynamic libraries.

If the LD_LIBRARY_PATH is already searched for preloaded dynamic libraries, then it seems like we are already calling into C in the preloader. We can't allocate, but as long as the new code also doesn't allocate, it seems like it might be okay.

@kripken
Copy link
Member

kripken commented Mar 24, 2025

Thanks @hoodmane !

We do have some LD_LIBRARY_PATH support (shown in the diff in this PR too)

// Resolve filename using LD_LIBRARY_PATH
static const char* resolve_path(char* buf, const char* file, size_t buflen) {
if (!strchr(file, '/')) {
const char* env_path = getenv("LD_LIBRARY_PATH");
if (env_path && path_find(file, env_path, buf, buflen) == 0) {
dbg("dlopen: found in LD_LIBRARY_PATH: %s", buf);
return buf;
}
}
return file;
}

So it seems natural to extend that. As @sbc100 mentioned, though, we should see about WasmFS - this PR uses FS, but WasmFS supports only part of that API. It would be good to test in WasmFS too, to make sure.

@kripken
Copy link
Member

kripken commented Mar 24, 2025

When we have a test here, we can test WasmFS by just adding the @also_with_wasmfs decorator.

@hoodmane
Copy link
Collaborator

Closing because #23872 went a different way.

@hoodmane hoodmane closed this Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants