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 #5288 #5299

Merged
merged 1 commit into from
Jun 17, 2017
Merged

Fix #5288 #5299

merged 1 commit into from
Jun 17, 2017

Conversation

kripken
Copy link
Member

@kripken kripken commented Jun 13, 2017

When adding from js libraries, if we see something was already implemented in compiled code, do not add from the library - it may be duplicate code if we do. If one is in asm.js and the other out, it's not harmful, but it's wasted space; if both are in asm.js, then it breaks asm.js validation and wasm compilation.

Instead, just export the compiled version so js libraries can reach it.

…ented in compiled code, do not add from the library - it may be duplicate code if we do. instead, make sure to export the compiled version so js libraries can reach it. fixes #5288
@kripken
Copy link
Member Author

kripken commented Jun 14, 2017

I ran the full test suite, looks good.

def test_js_lib_dep_memset(self):
open('lib.js', 'w').write(r'''
mergeInto(LibraryManager.library, {
depper__deps: ['memset'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, this interacts with what was observed in #2908. Isn't it still the case that JS code cannot add dependencies to C code unless these dependencies are specified in src/deps_info.json? (or was that fixed at some point?)

If that's correct, then, say for clarity, if we replace memset by any other function that user has implemented in C side myFunctionOnCSide, then it's incorrect to specify depper__deps: ['myFunctionOnCSide'] since that won't currently do what user intended due to #2908, but user would have to specify the dependency depper-> myFunctionOnCSide in the system file src/deps_info.json for it to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still the case that deps_info.json is the only way for a JS library to depend on something in a C system library.

About the second paragraph, if the user defined something on the C side, it wouldn't be needed as part of __deps anyhow - the deps are for system library code, it determines which system libs we link in. If myFunctionOnCSide is in one of the source or bitcode files the users gives as input to emcc, they will be linked in anyhow.

(Although the user may need to add KEEPALIVE if there is no other use of the function - in theory it might be nice if the deps handled that somehow, but it might also be more confusing, I'm not sure.)

@kripken kripken merged commit 7a5744d into incoming Jun 17, 2017
@kripken kripken deleted the double-memset-no branch June 17, 2017 01:15
kripken added a commit that referenced this pull request Jun 20, 2017
…in our js library, and we should not also include the musl C version
buu700 pushed a commit to buu700/emscripten that referenced this pull request Jul 20, 2017
…is implemented in our js library, and we should not also include the musl C version
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