Skip to content

Conversation

@ryanking13
Copy link
Contributor

Updates webassembly.py to parse and store WASM_DYLINK_RUNTIME_PATH that were suggested in: WebAssembly/tool-conventions#246

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm.. I guess we should add a test though? Perhaps we need a test that adds and -rpath to the link comment?

@hoodmane
Copy link
Collaborator

hoodmane commented Mar 2, 2025

We also need to drop the code that filters out -Wl,rpath=... and warns that it is unsupported.
https://github.com/emscripten-core/emscripten/blob/main/tools%2Flink.py#L75

@hoodmane
Copy link
Collaborator

hoodmane commented Mar 2, 2025

And to teach the runtime function getDylinkMetadata about this:
https://github.com/emscripten-core/emscripten/blob/main/src%2Flib%2Flibdylink.js#L466

# wasm-ld doesn't support soname or other dynamic linking flags (yet). Ignore them
# in order to aid build systems that want to pass these flags.
'-allow-shlib-undefined': False,
'-rpath': True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should also add this to SUPPORTED_LINKER_FLAGS above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not? @sbc100 it's not entirely clear to me from reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I added -rpath there.

@ryanking13
Copy link
Contributor Author

Okay, I teached rpath to linker and libdylink.js, and added some tests that check the RUNTIME_PATH subsection.

The follow-up work would be actually utilizing this runtime path when locating the libraries.

@ryanking13 ryanking13 changed the title Update webassembly.py to parse WASM_DYLINK_RUNTIME_PATH Enable -rpath flag Mar 2, 2025
@ryanking13
Copy link
Contributor Author

Can I ask for you help to understand the core2 failure?

test_dylink_raii_exceptions_reversed_wasm (test_core.core2) ... FAIL
test_dylink_raii_exceptions_reversed_wasm_legacy (test_core.core2) ... ok (7.02s)
test_dylink_raii_exceptions_reversed_emscripten (test_core.core2) ... ok (6.78s)

Among the three sibling tests, the one using wasm-eh fails (I guess), while other one passes. I am not sure how they are related to the flag.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 4, 2025

Can I ask for you help to understand the core2 failure?

test_dylink_raii_exceptions_reversed_wasm (test_core.core2) ... FAIL
test_dylink_raii_exceptions_reversed_wasm_legacy (test_core.core2) ... ok (7.02s)
test_dylink_raii_exceptions_reversed_emscripten (test_core.core2) ... ok (6.78s)

Among the three sibling tests, the one using wasm-eh fails (I guess), while other one passes. I am not sure how they are related to the flag.

If you look at the test output you see:

building:ERROR: /tmp/emtest_bs3d1h4g/emscripten_temp_pd8wnjjg/main.jso1.js:681:10: ERROR - [JSC_UNDEFINED_VARIABLE] variable path is undeclared
  681|           path = getString();

I guess you are missing a var somewhere?

} else if (subsectionType === WASM_DYLINK_RUNTIME_PATH) {
var runtimePathsCount = getLEB();
for (var i = 0; i < runtimePathsCount; ++i) {
path = getString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing var here.

Also how about just while (runtimePathsCount--) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks for the catch. I missed the error message in the log.

Also how about just while (runtimePathsCount--) here?

Sounds good. I also updated the similar codes in this file.

This is an automatic change generated by tools/maint/rebaseline_tests.py.

The following (2) test expectation files were updated by
running the tests with `--rebaseline`:

```
other/codesize/test_codesize_hello_dylink.gzsize: 5926 => 5911 [-15 bytes / -0.25%]
other/codesize/test_codesize_hello_dylink.jssize: 13112 => 13085 [-27 bytes / -0.21%]

Average change: -0.23% (-0.25% - -0.21%)
```
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.

Thanks @ryanking13!

@hoodmane hoodmane enabled auto-merge (squash) March 6, 2025 12:08
@hoodmane hoodmane merged commit 28051fb into emscripten-core:main Mar 6, 2025
29 checks passed
@sbc100
Copy link
Collaborator

sbc100 commented Mar 6, 2025

Thanks for working on this!

I wonder if we should add this to the ChangeLog.

hoodmane added a commit that referenced this pull request Apr 16, 2025
This updates `loadDynamicLibrary` to locate shared libraries from the file system
if possible.

When locating a shared library with a relative path, first attempt to find it in
the `LD_LIBRARY_PATH` directories. If it is not found there, search for it in the
runtime path directories. If it is not found, fall back to the original
`Module['locateFile']`.

Followup to #23805.

---------

Co-authored-by: Hood Chatham <[email protected]>
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