Skip to content
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

Fix test_dlfcn_self test under wasm and wasm backend #8531

Merged
merged 2 commits into from
May 14, 2019
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 2, 2019

No description provided.

@sbc100 sbc100 requested a review from kripken May 2, 2019 00:05
src/library.js Outdated Show resolved Hide resolved
var result = lib.module[mod_symbol];
#if WASM
// Attempt to get the real "unwrapped" symbol so we have more chance of
// getting wasm function which can be added to a table.
Copy link
Member

Choose a reason for hiding this comment

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

maybe I don't understand the comment, but there seem to be two issues here - first, that we look on Module.asm and not just on Module, and second, that this is important for getting a wasm function instead of a JS wrapper (I guess of those that we have in ASSERTIONS builds, async compilation, etc.?) Are the issues separate, or is the last the real issue and the first is how we achieve it?

If so, it might be cleaner to have those functions mark their actual wasm function on them, so

var jsWrapperFunction = function() {
  ..call the wasm..
};
jsWrapperFunction.wasmFunction = ..the wasm..

Then we could just check for result.wasmFunction, and this would not be specific to main modules, and could work for others too. Although, perhaps we don't add those wrappers currently for non-main modules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its just so that we can get a real wasm function for the table. I will try your approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that the jswrappers are generated statically at compile time. When they are called they dynamically call the wasm functions. I tried doing something where the wrappers would replace themselves on first use, but that didn't work for this test where thee function in question was not called before the wasm function was needed. I suppose I could fixup with wrappers during post wasm instantiate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mind if I do this as a followup? I have an idea for a bigger change to how Module['foo'] passthrough wrappers work. Actually.. i can do it a precursor too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On further investigation this looks like the cleanest way to do this for now.

The wasm functions are not available that time of js wrapper creation so we would have to do something like this:

var jsWrapperFunction = function() {
  return Module['asm'][foo'].apply(...)
};
jsWrapperFunction.getWasmFunction() = function() {
  return Module['asm'][foo'];
}

This would add another wrapper for every wasm function .. which is a lot more expensive that the 3 lines above.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the current code may be simplest and best.

var result = lib.module[mod_symbol];
#if WASM
// Attempt to get the real "unwrapped" symbol so we have more chance of
// getting wasm function which can be added to a table.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the current code may be simplest and best.

@sbc100 sbc100 merged commit 41f2bee into incoming May 14, 2019
@sbc100 sbc100 deleted the fix_dlfnc_self branch May 14, 2019 16:14
VirtualTim pushed a commit to VirtualTim/emscripten that referenced this pull request May 21, 2019
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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.

2 participants