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

Add test for EM_JS and i64 arguments #15916

Merged
merged 1 commit into from
May 4, 2022
Merged

Add test for EM_JS and i64 arguments #15916

merged 1 commit into from
May 4, 2022

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 7, 2022

See #15871

@sbc100 sbc100 requested a review from kripken January 7, 2022 15:12
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

It looks like we have other.test_EM_ASM_i64 which checks that this fails (without WASM_BIGINT). Maybe this PR can add to that test, or otherwise keep the two things close together?

Btw, that test shows that we fail to compile with EM_ASM. I wasn't aware that EM_ASM and EM_JS differed there...

@sbc100 sbc100 force-pushed the em_js_i64 branch 2 times, most recently from c24adde to 3f0f6e7 Compare May 3, 2022 04:00
@sbc100
Copy link
Collaborator Author

sbc100 commented May 3, 2022

Added a check and error

@sbc100
Copy link
Collaborator Author

sbc100 commented May 3, 2022

It makes sense to me to (a) have this test in core since it effect the JS/wasm boundary and (b) grouping em_js tests together makes sense to me.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Am I correct that EM_JS with i64 never worked, because while in theory the JS side could have two i32 params (the legalized i64), there is a single place defining the function for both JS and C, so the C would not have an i64 in that case? So writing an i64 there would not be possible with JS getting two legalized parameters. (Just checking this new error does not prevent anything that worked before.)

@sbc100
Copy link
Collaborator Author

sbc100 commented May 3, 2022

Am I correct that EM_JS with i64 never worked, because while in theory the JS side could have two i32 params (the legalized i64), there is a single place defining the function for both JS and C, so the C would not have an i64 in that case? So writing an i64 there would not be possible with JS getting two legalized parameters. (Just checking this new error does not prevent anything that worked before.)

Yes, you are mostly correct in your assessment I believe. Using int64 with EM_JS results in the JS signature that doesn't match the one used in C, or that one called at runtime. However its conceivable that you could ignore the JS arguments that instead just peek into arguments to make it work. So its not completely impossible today.

I do plan to make this work in the long run.. so perhaps I should just make this into a warning rather than an error for now?

@sbc100
Copy link
Collaborator Author

sbc100 commented May 3, 2022

I went with the warning instead..

@sbc100 sbc100 requested a review from kripken May 3, 2022 17:52
@sbc100 sbc100 force-pushed the em_js_i64 branch 2 times, most recently from 5c2f79e to 5437421 Compare May 3, 2022 20:39
@sbc100 sbc100 enabled auto-merge (squash) May 3, 2022 20:44
Ideally we should error out if WASM_BIGINT is not passed by I have
yet to find a good place to do that.

See #15871
@sbc100 sbc100 merged commit a23058a into main May 4, 2022
@sbc100 sbc100 deleted the em_js_i64 branch May 4, 2022 02:15
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.

None yet

2 participants