-
Notifications
You must be signed in to change notification settings - Fork 33
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
Test breaking out of a try #270
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't quite understand the focus of this test, it seems quite convoluted.
- Why the outermost
try
, that never fires. - Why the block argument, that's immediately dropped.
- How is the delegate relevant to the test?
oh wait did we copy the wrong one |
so, with the correct test: (func (export "break-throw") (result i32)
(try $outer (result i32)
(do
(try (result i32)
(do
(block $a
(try (do (br $a)) (delegate $outer))
)
(call $throw)
(i32.const 0)
)
(catch $e0 (i32.const 1))
)
)
(catch $e0 (i32.const 2))
) the outer the block is necessary because wasm2c has bugs. like not being able to it's also important to call |
In general it would be great to have test coverage of branching out of try and catch blocks (#210), including some simpler tests. This particular test seems valuable because it separates existing implementations that otherwise pass the current tests. |
+1 on adding tests for (module
(tag $e0)
...
(func (export "br-in-try")
(block $l
(try
(do
(br $l)
)
(catch_all)
)
(throw $e0)
)
)
...
)
(assert_return (invoke "br-in-try")) Because if |
hmm what happened in CI? |
we disagree. we don't wanna test whether the we wanna test whether the |
That means your test doesn't pass. Can you check locally and fix it? You can use |
I don't really understand what you mean by |
it's testing whether the for context: wasm2c returns 2. |
I agree that this test makes sense, but it seems a bit one-off. Can you perhaps extend it to a slightly more systematic bunch of tests for branching out of (different forms of) handlers? |
hmm, with |
not sure what else to throw into these but this seems like a good starting point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, these look good to me. Can we keep them in try-catch.wast and try-delegate.wast, respectively, however? (The test suite is mostly structured around test files dedicated to individual instructions/constructs.)
Bump -- it would be nice if these tests (or something like them) landed. It looks like @SoniEx2 has responded to the feedback. |
@aheejin does that break existing functionality? if not, then there's no reason to delay this - further tests can be added later. |
Sorry that this slipped from my notice and ended up not being merged for a long time. Can you fix the conflicts so we can merge this? |
oh. uh, is this still relevant? we don't know how the new EH system works, we just know we wrote these for the old system. |
Given that the old (Phase 3) proposal still has not been deprecated, I think it wouldn't hurt to merge this. By the way the legacy tests have moved into https://github.com/WebAssembly/exception-handling/tree/main/test/legacy/exceptions, so I think you should move these tests there too. |
oh good it just needed a rebase, nice |
I'm a little confused that this only ended up touching the |
I think we can write the corresponding tests for the new proposal too. Will add them if @SoniEx2 or anyone submits PRs for them. |
someone else will have to do it because we still haven't got around to figuring out how the new proposal works ^^' |
Tests that breaking out of a try resets the exception handler correctly.
See WebAssembly/wabt#2203