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.

Sign up for GitHub

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 Fill in GOT entries when libs are loaded with allowUndefined #22053

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

FIX Fill in GOT entries when libs are loaded with allowUndefined #22053

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/library_dylink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,16 @@ var LibraryDylink = {
// correctly.
rtn.required = true;
}
var value = resolveGlobalSymbol(symName, true).sym;
if (!value) {
return rtn;
}
if (typeof value == 'function') {
/** @suppress {checkTypes} */
rtn.value = addFunction(value, value.sig);
} else if (typeof value == {{{ POINTER_JS_TYPE }}}) {
rtn.value = value;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this code is very similar to existing code in both reportUndefinedSymbols and updateGOT. I wonder if we can factor this better?

Also, I think there is probably some reason we delay updated the GOT at GOTHandler time and instead do it later on. Perhaps we could instead modify the existing updateGOT function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good I'll try factoring out the shared logic and moving the change to updateGot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is __cxa_throw effected because its a JS symbol?

The problem specifically seems to be that updateGOT fills in the GOT table for all wasm exports from the loaded library. So it misses JavaScript symbols exported from the main module. Perhaps it would fix it to add GOT entries for all of the JS-implemented library funcs in the main module?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about if we rename reportUndefinedSymbols to handleUndefinedSymbols and have it take an reportErrors flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well it seems to me that the only reason we need this GOT update behavior in reportUndefinedSymbols is because of JS symbols from the main module which are not handled by updateGOT. So they only get added to the GOT if first we have a library that needs them. Why not just stick all JS symbols in the GOT at startup and not worry about having something like handleUndefinedSymbols?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry, yes, strong symbols should always win, although I'm not sure how important that edge case is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But you think the case where a JS symbol is overridden by a native symbol is more important? Are the JS symbols automatically counted as weak?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, not necessarily. I was just trying to imagine/remember the reason for factoring the code this way.

I do think we have good test coverage here, so I think its reasonably safe to try refactoring this code and if the tests pass it should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also a non-zero cost to adding JS functions to the wasm table.. see the addFunction helper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, in the future where WebAssembly.Function is available it will be better. Is it possible we could just import and reexport all these JS functions from the main module so that they would all get added by updateGOT in the normal way?

return rtn;
}
},
Expand Down
Loading