Look for orig$ functions from side modules too #9906
Conversation
tests/test_other.py
Outdated
| self.assertContained('''res32 - internal 32 | ||
| res32 - external 32 | ||
| res64 - internal 64 | ||
| res64 - external 64''', run_js('a.out.js')) |
There was a problem hiding this comment.
Can you not use the same helper function that other test_dylink_* function in this file use?
Can we reduce this test to something smaller that just tests for the exact problem? Why both to call the functions internally for example?
There was a problem hiding this comment.
I actually thought it might be good to write a new test here, as it uses Module.dynamicLibraries, and we have no coverage for that aside from a few in the browser suite. So it works differently than the existing dylink tests in core, and can't use the same infra.
There was a problem hiding this comment.
How does that work differently? I thought RUNTIME_LINKED_LIBS just ends up setting dynamicLibraries, no?
If you want to write a specific test for modifying dynamicLibraries I don't see that as related to fixing this i64 issue.
There was a problem hiding this comment.
I agree it's separate, but it was easier to do both at once.
If you want I can rewrite this test from scratch?
There was a problem hiding this comment.
I rewrote the test in core as you preferred. It works (i.e. it fails before the PR, passes with it) - not sure what I did wrong before.
tests/test_other.py
Outdated
| create_test_file('pre.js', r''' | ||
| if (typeof(Module) === "undefined") Module = {}; | ||
| Module.dynamicLibraries = ['side.wasm']; | ||
| ''') |
There was a problem hiding this comment.
Can you just do self.set_setting('RUNTIME_LINKED_LIBS', ['wide.wasm']) instead of this?
There was a problem hiding this comment.
That only works in core, I believe? (We call emcc directly here.)
There was a problem hiding this comment.
Ah, well I guess that begs the question why don't we have common patter for dylink testing in "other"?
For now, why not just add a quick, minimal dylink-style test to 'core'?
If you think that this doesn't belong in core, the same can probably be said for a lot of the other "dylink" tests too and we can do a followup change to migrate them out of core. I think this test might actually belong in core because the JS interaction stuff is effected by things like optimization levels.
Basically I think there is way too much boiler plate in this test which it seems to me should just test for i64 function being exported from a side module. But maybe I'm missing something?
There was a problem hiding this comment.
Not directly, you're right, this was killing two birds with one stone 😄
Note that I actually tried to write a quick, minimal test - and failed. After 15 minutes I felt it wasn't a good use of time, and used the existing testcase, again, given I had realized I wanted to write a new testcase for Module.dynamicLibraries anyhow which it uses.
I'm not sure why I failed, as the function pointer issue should be separable from how the library is loaded - but maybe that assumption is wrong? When we dlopen vs Module.dynamicLibraries do we add the side module symbols differently to main somehow?
There was a problem hiding this comment.
Hmm, I'm not suggesting you use dlopen which I agree is a different use case.
My understanding was/is that RUNTIME_LINKED_LIBS and Module.dynamicLibraries do exactly the same thing. I see the former as the official way to set the later.
The tests in core that use dylink_test use RUNTIME_LINKED_LIBS and not dlopen IIRC so they should behave exactly like adding to Module.dynamicLibraries.. which is to say they do library loading before main is called.
| // If there is no wasm function, this may be a JS library function or | ||
| // something from another module. | ||
| // Try an original version from a side module. | ||
| if (!func) func = Module['_%(original)s']; |
There was a problem hiding this comment.
But why do side module symbols have an _? Oh I guess they don't but when we add them to "Module" we artificially at one maybe? Maybe we should add them to 'asm' too without the "_"? since they are "native code symbols" too?
There was a problem hiding this comment.
Yes, I think when we add stuff to module, the support.js code adds a _ to everything.
I don't think we can add them to asm - asm is an exports object from a wasm file, it's not a normal JS object.
I do think this asymmetry is not great, though - we should probably not use asm here, and have the main module write its symbols to Module like side modules do. But as the usage of asm was pre-existing here, I wasn't sure it makes sense to try to do a large refactoring here. But maybe you know that code better and have an idea?
|
friendly ping @sbc100 on the questions above |
|
I still think the test should just use We should be using a common helper for all dylib tests but that be done a followup perhaps. |
Fixes #9901