[dylink] Fix rpath calculation in nested dependencies #24234
Conversation
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: 11735 => 11758 [+23 bytes / +0.20%] other/codesize/test_codesize_hello_dylink.jssize: 27774 => 27799 [+25 bytes / +0.09%] Average change: +0.14% (+0.09% - +0.20%) ```
sbc100
changed the title
src/lib/libdylink.js
Outdated
| @@ -937,6 +939,7 @@ var LibraryDylink = { | |||
| var dso = { | |||
| refcount: Infinity, | |||
| name, | |||
There was a problem hiding this comment.
So does that mean that name is always just the basename / so_name? Is is name sometimes absolute too here?
There was a problem hiding this comment.
No, only the dependencies of the libraries will have only the so_name in name. The parent library called from dlopen would have absolute path in the name.
There was a problem hiding this comment.
No, only the dependencies of the libraries will have only the so_name in name.
So, I think the alternative approach would be ensuring loadDynamicLibrary always take absolute path. Let me try that approach instead.
src/lib/libdylink.js
Outdated
| flags = {...flags, rpath: { parentLibPath: libName, paths: metadata.runtimePaths }} | ||
| var dso = LDSO.loadedLibsByName[libName]; | ||
| var libPath = dso?.path ?? libName; | ||
| flags = {...flags, rpath: { parentLibPath: libPath, paths: metadata.runtimePaths }} |
There was a problem hiding this comment.
Should parentLibPath be just the dirname part of the parent? i.e. is there any point in passing a value if its just a basename?
There was a problem hiding this comment.
I think it is possible to calculate the dirname here.
For now, the dirname is calculated inside replaceORIGIN function, and the reason I did that was to isolate the code that depends on FS or PATH as much as possible.
If we calculate the dirname here, I think it can be something like:
var dso = LDSO.loadedLibsByName[libName];
#if FILESYSTEM
var libPath = PATH.dirname(dso?.path ?? libName);
#endif
flags = {...flags, rpath: { parentLibPath: libPath, paths: metadata.runtimePaths }}
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: 11758 => 11757 [-1 bytes / -0.01%] other/codesize/test_codesize_hello_dylink.jssize: 27799 => 27823 [+24 bytes / +0.09%] Average change: +0.04% (-0.01% - +0.09%) ```
This fixes a bug in rpath calculation in nested dependencies.
Basically, it fixes a case when a relative path is passed to the
loadDynamicLibrary.Think of the following scenario:
loadDynamicLibrary('/usr/lib/libhello.wasm')(_dlopen_js ==> dlopenInternal ==> loadDynamicLibrary)loadDynamicLibrary('/usr/lib/libhello.wasm'), it loads the dependencylibhello_dep.so. When locatinglibhello_dep.so, it usesparentLibPath: /usr/lib/libhello.wasmit locates the library correctly from/usr/lib/libhello_dep.so.loadDynamicLibrary('libhello_dep.so'), and it loads its dependency 'libhello_nested_dep.so' first. In this case it usesparentLibPath: libhello_dep.soso it fails to locate the library.