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.

Sign up for GitHub

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

Jump to bottom

Add Tests for native wasm exceptions #113247

Merged
merged 5 commits into from
Jul 9, 2023
Merged

Add Tests for native wasm exceptions #113247

merged 5 commits into from
Jul 9, 2023

Conversation

mirkootter
Copy link
Contributor

Motivation

In PR #111322, I added support for native WASM exceptions. I was asked by @davidtwco to add some tests for it in a follow up PR, which seems like a very good idea.

This PR adds three tests for this feature:

  • codegen: ensure the correct LLVM instructions are used
  • assembly: ensure the correct WASM instructions are used
  • run-make: ensure the exception handling works; the WASM code is run using a small nodejs script which demonstrates the exception handling

Complications

There are a few changes beside adding the tests, which were necessary

  • Tests for the wasm32-unknown-unknown target are (as far as I know) only run on test-various. Its docker image uses nodejs-15, which is very old. Experimental support for wasm-exceptions was added in nodejs16. In nodejs 18.12 (LTS), they are stable.
    • --> increase nodejs to 18.12 in test-various
  • codegen/assembly tests are not performed for the wasm32-unknown-unknown target yet
    • --> add those to test-various as well

Due to the last point, some tests are run which have not run before (assembly+codegen tests for wasm32-unknown-unknown). I added // ignore wasm32-bare for those which failed

Local testing

I run all tests locally using both test-various and wasm32. As far as I know, none of the other systems run any test for wasm32 targets.

@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jul 1, 2023
@bjorn3
Copy link
Member

bjorn3 commented Jul 2, 2023

Can you also add a test that catch_unwind catches all exceptions, even those with an unknown exception tag? catch_unwind is required to catch all exceptions and currently aborts when it catched a foreign exception. (See __rust_foreign_exception)

@mirkootter
Copy link
Contributor Author

Can you also add a test that catch_unwind catches all exceptions, even those with an unknown exception tag? catch_unwind is required to catch all exceptions and currently aborts when it catched a foreign exception. (See __rust_foreign_exception)

Good point... unfortunately, this behaviour is currently not implemented :( LLVM only generates catch for catchpads, no catch_all instructions. The latter is only used for cleanuppads.

I will look into it. I believe I can make it work if I add a special cleanuppad which aborts.

However, this should not affect this PR, since adding a test for it would not work at this point

@Mark-Simulacrum
Copy link
Member

@bors r+

I'm not really familiar with the details here but the goal seems reasonable and this is adding tests we can modify later.

@bors
Copy link
Contributor

bors commented Jul 9, 2023

📌 Commit a0bd381 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 9, 2023
@matthiaskrgr
Rollup merge of rust-lang#113247 - mirkootter:test-wasm-exceptions-no…
1fcbe52 
…std, r=Mark-Simulacrum

Add Tests for native wasm exceptions

### Motivation
In PR rust-lang#111322, I added support for native WASM exceptions. I was asked by `@davidtwco` to add some tests for it in a follow up PR, which seems like a very good idea.

This PR adds three tests for this feature:
* codegen: ensure the correct LLVM instructions are used
* assembly: ensure the correct WASM instructions are used
* run-make: ensure the exception handling works; the WASM code is run using a small nodejs script which demonstrates the exception handling

### Complications
There are a few changes beside adding the tests, which were necessary
* Tests for the wasm32-unknown-unknown target are (as far as I know) only run on `test-various`. Its docker image uses nodejs-15, which is very old. Experimental support for wasm-exceptions was added in nodejs16. In nodejs 18.12 (LTS), they are stable.
  - --> increase nodejs to 18.12 in `test-various`
* codegen/assembly tests are not performed for the wasm32-unknown-unknown target yet
  - --> add those to `test-various` as well

Due to the last point, some tests are run which have not run before (assembly+codegen tests for wasm32-unknown-unknown). I added `// ignore wasm32-bare` for those which failed

### Local testing
I run all tests locally using both `test-various` and `wasm32`. As far as I know, none of the other systems run any test for wasm32 targets.
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 9, 2023
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 9, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 9, 2023
@bors
Auto merge of rust-lang#113508 - matthiaskrgr:rollup-xzrp4nt, r=matth…
a9eba8d 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#111618 (Always name the return place.)
 - rust-lang#113247 (Add Tests for native wasm exceptions)
 - rust-lang#113273 (Use String or Int to set the opt level)
 - rust-lang#113469 (Remove `default_free_fn` feature)
 - rust-lang#113493 (additional io::copy specializations)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6a20f68 into rust-lang:master Jul 9, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mirkootter @rustbot @bjorn3 @Mark-Simulacrum @bors