Skip to content

wasm2c is hardcoded to depend on dynCall_xx exports from native code #12065

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

Closed
sbc100 opened this issue Aug 27, 2020 · 3 comments · Fixed by #12070
Closed

wasm2c is hardcoded to depend on dynCall_xx exports from native code #12065

sbc100 opened this issue Aug 27, 2020 · 3 comments · Fixed by #12070
Assignees

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Aug 27, 2020

As of #12059 we are moving away form the native dynCall functions but because of wasm2c's dependency we currently use the legacy mode when WASM2C is enabled.

Instead it should use direct wasm table access in all cases (unlike JS which still needs to use dynCalls for i64 signatures if WASM_BIGINT is missing).

Do wew enable WASM_BIGINT when WASM2C is enabled? Perhaps we should.

See #12002

@kripken
Copy link
Member

kripken commented Aug 27, 2020

I don't think WASM_BIGINT makes a difference here. The issue is that if the program uses exceptions or longjmp, it needs invokes and dynCalls, one per signature basically, and it assumes those are in the wasm.

When #12059 lands I can debug this, but I assume we just need to teach the output to call the wasm2c-emitted Table directly, instead of a dynCall export.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 27, 2020

I don't think WASM_BIGINT makes a difference here. The issue is that if the program uses exceptions or longjmp, it needs invokes and dynCalls, one per signature basically, and it assumes those are in the wasm.

When #12059 lands I can debug this, but I assume we just need to teach the output to call the wasm2c-emitted Table directly, instead of a dynCall export.

Yes, exactly.

I guess what I mean is, with wasm2c we don't want binaryen to generate any dyncCalls, not even i64 once, because we can't call the i64 ones direclty.

So the code the tells binaryen to generate the dynCalls should do the same thing that the WASM_BIGINT=1 path does... which makes me thing make all the places we check for bigint support would want to return true on wasm2c, right?

@kripken
Copy link
Member

kripken commented Aug 27, 2020

@sbc100 Oh right, now I understand. Makes sense, I opened #12069

kripken added a commit that referenced this issue Aug 27, 2020
We have no problem with i64s on the FFI boundary, which is effectively
the same as WASM_BIGINT for JS.

Also, we always want to be in standalone mode, as in wasm2c there is
no JS, just wasm and then C.

As suggested in #12065
kripken added a commit that referenced this issue Aug 28, 2020
Instead, this calls the Table directly.

This takes a little more effort at compile time because we need to find
the index of the function type for the signature we are calling with
(so that we trap if the target function has the wrong type). To do that,
scan the wasm2c output - which is at risk of breaking if that output
changes, sadly (another option might be to disassemble the wasm
and scan that, which would at least be a stable format - but this
seems fine for now).

Helps #12059

Fixes #12065
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 a pull request may close this issue.

2 participants