Skip to content

Reimplement resolve_path in JS NFC #23935

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 16 commits into from
Mar 19, 2025
Merged

Conversation

ryanking13
Copy link
Contributor

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.

@ryanking13
Copy link
Contributor Author

ryanking13 commented Mar 15, 2025

@hoodmane @sbc100 This is a split off from #23872, but I am having trouble making the function work in thread. Can I get some help with this? It seems like FS.lookupPath (JS) fails to locate the file in thread environment, while stat (C) does.

@ryanking13 ryanking13 marked this pull request as draft March 15, 2025 06:38
@hoodmane
Copy link
Collaborator

Is there a ci failure for this problem? Or which test should I run? I only see the sigs job failing which needs an update to the expected output.

@ryanking13
Copy link
Contributor Author

ryanking13 commented Mar 15, 2025

This one fails

./test/runner other.test_ld_library_path_pthread

while the same test (without pthread)

./test/runner other.test_ld_library_path

success

@hoodmane
Copy link
Collaborator

hoodmane commented Mar 15, 2025

Well applying this change, I can see that stat finds the file but lookupPath does not:

diff --git a/test/test_other.py b/test/test_other.py
index ad240f4a1..d33fdf0b4 100644
--- a/test/test_other.py
+++ b/test/test_other.py
@@ -7605,12 +7605,29 @@ Module.preRun = () => {
 #include <stdlib.h>
 #include <string.h>
 #include <dlfcn.h>
+#include "emscripten.h"
+#include <sys/stat.h>
+
+EM_JS(int, check_path, (char* cpath), {
+  const path = UTF8ToString(cpath);
+  console.log("check_path", path);
+  try {
+    const res = FS.lookupPath(path);
+    console.log("res", res);
+  } catch(e) {
+    console.log(e);
+  }
+});
 
 int main() {
   void *h;
   void (*f)();
   double (*f2)(double);
 
+  check_path("/lib/libhello1.wasm");
+  struct stat statbuf;
+  printf("res: %d\n", stat("/lib/libhello1.wasm", &statbuf));
+
   h = dlopen("libhello1.wasm", RTLD_NOW);
   assert(h);
   f = dlsym(h, "hello1");

@hoodmane
Copy link
Collaborator

Okay I think I fixed the problem in b805542.

@ryanking13
Copy link
Contributor Author

Okay I think I fixed the problem in b805542.

Indeed. Thanks for the fix! Didn't know about the proxy parameter. The remaining issue now seems to be handling BigInt in a MEMORY64 environment.

@hoodmane
Copy link
Collaborator

The memory64 test now passes for me locally.

@ryanking13 ryanking13 changed the title [DRAFT] Reimplement resolve_path in JS NFC Reimplement resolve_path in JS NFC Mar 16, 2025
@ryanking13 ryanking13 marked this pull request as ready for review March 16, 2025 07:08
@ryanking13
Copy link
Contributor Author

The memory64 test now passes for me locally.

@hoodmane Awesome, thanks for helping me fix the issues.

$locateLibraryFromFS: (filename, searchDirs, maxLength = Infinity) => {
// Find the library in the filesystem.
// returns null if not found.
if (typeof FS.lookupPath !== 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slightly shorter:

Suggested change
if (typeof FS.lookupPath !== 'function') {
if (!FS.lookupPath) {

}

var candidates = [];
if (filename.charAt(0) === '/') { // abs path
Copy link
Collaborator

Choose a reason for hiding this comment

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

shorter:

Suggested change
if (filename.charAt(0) === '/') { // abs path
if (filename[0] === '/') { // abs path

Copy link
Collaborator

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

It looks good to me, thanks @ryanking13. Let's wait for @sbc100 to review as well.

@hoodmane
Copy link
Collaborator

Hmm it seems the test I added breaks in CI. When I run it locally it works fine...

@hoodmane hoodmane requested a review from sbc100 March 18, 2025 19:52
@hoodmane hoodmane merged commit 66d2137 into emscripten-core:main Mar 19, 2025
28 checks passed
@hoodmane
Copy link
Collaborator

hoodmane commented Mar 19, 2025

I hit the merge button on this, but now looking back at the history I'm thinking it might have been premature. Ah well, we could either revert and reland or fix any problems in followups.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 19, 2025

Yes, lets revert this pending some more discussion.

sbc100 added a commit that referenced this pull request Mar 19, 2025
sbc100 added a commit that referenced this pull request Mar 19, 2025
@sbc100
Copy link
Collaborator

sbc100 commented Mar 19, 2025

Can you re-open this PR? (or perhaps a new one is needed?)

@ryanking13
Copy link
Contributor Author

ryanking13 commented Mar 20, 2025

@hoodmane @sbc100 Made a new PR #23958

@hoodmane
Copy link
Collaborator

Sorry about that!

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.

3 participants