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 needs-unwind to tests that depend on panicking #91373

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

djkoloski
Copy link
Contributor

@djkoloski djkoloski commented Nov 29, 2021

These tests were found by running the test suite on fuchsia which compiles with panic=abort by default, then picking through the failures manually to locate the tests that require unwinding support.

Most of these tests are already opted-out on platforms that compile with panic=abort by default. This just generalizes it a bit more so that fuchsia tests can be run properly. Currently, the needs-unwind directive needs to be manually passed to compiletest (e.g. via --test-args '--target-panic=abort'). Eventually, I would like x.py or compiletest to determine whether the directive should be used automatically based on the target panic settings.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2021
@tmandry
Copy link
Member

tmandry commented Nov 29, 2021

You should also be able to remove the existing ignore-(platform) lines that are there because the platform is built without unwinding support.

@djkoloski
Copy link
Contributor Author

I did that originally but the flag in compiletest needs a command-line argument (i.e. it's not inherently set for specific target configurations). Should we be pulling that setting from a config?

@tmandry
Copy link
Member

tmandry commented Nov 30, 2021

Ah right, the plan was to add a flag to bootstrap that enables panic=abort and tells compiletest about it. That should be a separate change. I think this is fine for now.

@rust-log-analyzer

This comment has been minimized.

@djkoloski djkoloski force-pushed the fuchsia_test_suite branch 2 times, most recently from a485fc8 to b3c1e65 Compare November 30, 2021 20:22
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Dec 3, 2021

I did that originally but the flag in compiletest needs a command-line argument (i.e. it's not inherently set for specific target configurations). Should we be pulling that setting from a config?

I forget what PR added needs-unwind (maybe my bad there), but if it's currently just not being used at all (which seems to be the case), it seems a little misleading to go through annotating tests with this flag, particularly when it "seems" like some of the ignore- flags would be removable with this flag added.

I also noted that the current target specs for fuchsia don't have a panic strategy of abort, so it seems like in general unwinding is supported?

I am not strictly opposed to merging this, but I would like a better understanding of the expectation on your end for integrating needs-unwind into actual invocations of compile test from our build system (rough sense of timeline), and maybe some commentary in the PR description for how these tests were found (did you run with a panic=abort flag and just blanket added for where it failed?). My review comment suggests that it's possible some unrelated tests were also adjusted in this PR, which seems best to avoid and is part of the reason I'd like the test selection rationale better documented in the PR.

(If I miss your comment, please @rustbot ready so the S-waiting-on-review label is set).

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2021
@djkoloski
Copy link
Contributor Author

if it's currently just not being used at all (which seems to be the case), it seems a little misleading to go through annotating tests with this flag

There is a flag and it is functional, but it has to be manually passed (e.g. ./x.py test --test-args '--target-panic=abort'). It should really be pulled from the target specification instead, but I haven't done any work to get that up and running yet. Once the flag is automatically specified, I would be comfortable removing the target-specific ignore- directives.

I also noted that the current target specs for fuchsia don't have a panic strategy of abort, so it seems like in general unwinding is supported?

Although I believe it is now possible to build fuchsia with unwinding, I believe this was not always the case and the standard library is compiled with panic=abort by default. I can check with @tmandry on this.

I would like a better understanding of the expectation on your end for integrating needs-unwind into actual invocations of compile test

I expect that eventually x.py will automatically determine whether the needs-unwind directive applies to a target. Getting the Rust test suite running on fuchsia is my primary objective.

maybe some commentary in the PR description for how these tests were found

Updated.

My review comment suggests that it's possible some unrelated tests were also adjusted in this PR

Ack, you are correct. Those are fixes for other tests that failed on fuchsia, but are unrelated to this PR. They were added by mistake and I've removed them from this PR.

@djkoloski
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 3, 2021
@tmandry
Copy link
Member

tmandry commented Dec 3, 2021

For context, my plan was to add an explicit flag to x.py for building with panic=abort (ideally with defaults for the platforms that don't support it). This would modify compilation flags (including -Zpanic-abort-tests in the case of testing) as well as cause --target-panic=abort to be passed to compiletest.

It's true that adding the needs-unwind header while it's not fully functional could be misleading, but at least for now there wouldn't be any examples where it was there without the right platforms also being ignored explicitly. Eventually we would take those out.

@Mark-Simulacrum
Copy link
Member

I'm okay proceeding here -- it sounds like we're on a good path toward this being integrated, and it's already ~usable directly, when needed.

@djkoloski it looks like an unrelated commit and a couple merges landed on this branch -- can you rebase those away? After that's done I expect to r+

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2021
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 9, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This directive isn't automatically set by compiletest or x.py, but can
be turned on manually for targets that require it.
@djkoloski
Copy link
Contributor Author

Sorry about that, accidentally pulled in an unrelated commit. @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 9, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2021

📌 Commit ea68758 has been approved by Mark-Simulacrum

@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 Dec 9, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021
…k-Simulacrum

Add needs-unwind to tests that depend on panicking

These tests were found by running the test suite on fuchsia which compiles with `panic=abort` by default, then picking through the failures manually to locate the tests that require unwinding support.

Most of these tests are already opted-out on platforms that compile with `panic=abort` by default. This just generalizes it a bit more so that fuchsia tests can be run properly. Currently, the `needs-unwind` directive needs to be manually passed to compiletest (e.g. via `--test-args '--target-panic=abort'`). Eventually, I would like `x.py` or compiletest to determine whether the directive should be used automatically based on the target panic settings.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2021
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#91127 (Add `<*{const|mut} T>::{to|from}_bits`)
 - rust-lang#91310 (Add --out-dir flag for rustdoc)
 - rust-lang#91373 (Add needs-unwind to tests that depend on panicking)
 - rust-lang#91426 (Make IdFunctor::try_map_id panic-safe)
 - rust-lang#91515 (Add rsplit_array variants to slices and arrays)
 - rust-lang#91553 (socket ancillary data implementation for dragonflybsd.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f7278cf into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants