Skip to content

Make pops valid in tests #3282

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

Merged
merged 2 commits into from
Oct 23, 2020
Merged

Make pops valid in tests #3282

merged 2 commits into from
Oct 23, 2020

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Oct 23, 2020

These two tests had pops in invalid locations; pops are valid only after
catch. This fixes those invalid wasm files.
This removes pops from Os_print-stack-ir_all-features.wast too.

Fixes #3213 and #3283.

These two tests had pops in invalid locations; pops are valid only after
`catch`. This fixes those invalid wasm files.

This leaves only one test file with invalid pops:
https://github.com/WebAssembly/binaryen/blob/master/test/passes/Os_print-stack-ir_all-features.wast
But I'm not sure if we can remove this now, because this tests pops for
different types of values than `exnref`. I guess we can fix it after we
implement the new spec for EH, which removes `exnref` and there `catch`
can extract other types of values.
@aheejin aheejin requested review from kripken and tlively October 23, 2020 20:55
@@ -1273,7 +1273,6 @@
(try
(do
;; Expressions that can throw should NOT be taken out of 'try' scope.
(local.set $exn (pop exnref))
Copy link
Member Author

@aheejin aheejin Oct 23, 2020

Choose a reason for hiding this comment

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

I'm not sure why we had this line here; I think this was a copy-paste error.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM, but for what it's worth, I would also be fine just deleting these parts of the tests. They are from back when I originally added Pop with the idea that it could be used anywhere (along with Push, which we've since removed).

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.

lgtm.

Removing them would be even better, but I'm not sure myself if these don't help somehow. If you you think it's ok to remove then sgtm.

@aheejin aheejin merged commit aaf7825 into WebAssembly:master Oct 23, 2020
@aheejin aheejin deleted the valid_pop branch October 23, 2020 22:33
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.

pop()s in test/passes/instrument-locals_all-features.wast are not written correctly to binary
3 participants