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

side module calls main module with a 64bit param #8291

Closed

Conversation

waterlike86
Copy link
Contributor

@waterlike86 waterlike86 commented Mar 13, 2019

}

void callPassBigInt(){
passBigInt(1152921504606846975);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we expect that this would get translated into a call_indirect, but this gets translated into a $env._passBigInt instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The indirection here is handled on the JS side. The imported function is a JS stub that gets filled in as symbols are resolved by loading modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right and this JS stub is causing problems when we turn off JS legalization.

args = ['-s', 'LEGALIZE_JS_FFI=0', '-s', 'SIDE_MODULE=1', '-O3', '-s', 'DISABLE_EXCEPTION_CATCHING=0']
print(args)
cmd = [PYTHON, EMCC, path_from_root('tests', 'other', 'noffi-side.cpp'), '-g', '-o', 'side.wasm'] + args
print(' '.join(cmd))
Copy link
Member

Choose a reason for hiding this comment

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

I do see this fail in the node command, with function signature mismatch - but this emcc command is never executed here. So side.wasm does not exist. Is that the cause of the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kripken sorry about that. i have pushed a fix to the test so it fails properly now. the error is
exception thrown: TypeError: invalid type,TypeError: invalid type

@kripken
Copy link
Member

kripken commented Mar 25, 2019

Thanks, now I think I see what's going on here.

In src/support.js, in our dynamic loader code we create a JS indirection for calls between wasm modules. That indirection helps with circular dependencies (adding an import to a wasm module not yet loaded). But it will fail on JS not being able to handle a wasm type.

As a fix, we'd need either BigInts in JS, or to implement the new dynamic loading idea we've discussed in #8268

@sbc100 sbc100 closed this Jan 30, 2020
@sbc100
Copy link
Collaborator

sbc100 commented Jan 30, 2020

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

@sbc100 sbc100 reopened this Jan 30, 2020
@sbc100 sbc100 changed the base branch from incoming to master January 30, 2020 22:02
@stale
Copy link

stale bot commented Jan 30, 2021

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Jan 30, 2021
Base automatically changed from master to main March 8, 2021 23:49
@stale stale bot removed the wontfix label Mar 8, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 16, 2022
@stale stale bot closed this Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants