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

Compiletest no longer respects -Cpanic=abort in TARGET_RUSTCFLAGS after #108905 #110850

Closed
djkoloski opened this issue Apr 26, 2023 · 3 comments · Fixed by #111472
Closed

Compiletest no longer respects -Cpanic=abort in TARGET_RUSTCFLAGS after #108905 #110850

djkoloski opened this issue Apr 26, 2023 · 3 comments · Fixed by #111472
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@djkoloski
Copy link
Contributor

Prior to #108905, compiletest would take TARGET_RUSTCFLAGS into account when determining whether a test target can unwind. It did so by calling rustc --print=cfg --target=$TARGET $TARGET_RUSTCFLAGS, which changes the reported panic strategy depending on the flags:

$ rustc --print=cfg --target=x86_64-unknown-fuchsia
...
panic="unwind"
...

$ rustc --print=cfg --target=x86_64-unknown-fuchsia -Cpanic=abort
...
panic="abort"
...

After the PR, compiletest calls rustc --print=all-target-specs-json -Zunstable-options which always reports the default panic strategy for the target (even if TARGET_RUSTCFLAGS are passed):

$ rustc --print=all-target-specs-json -Zunstable-options
...
  "x86_64-unknown-fuchsia": {
    ... (note that no "panic-strategy" is specified, it defaults to "unwind")
  }
...

$ rustc --print=all-target-specs-json -Zunstable-options -Cpanic=abort
...
  "x86_64-unknown-fuchsia": {
    ... ("panic-strategy" is still not specified)
  }
...

This causes a problem for us because we run the compiler test suite against a toolchain built with -Cpanic=abort. Compiletest will run tests marked with // needs-unwind even though they panic (and thus fail).

I see a few options for fixing this issue:

  • Have --print=all-target-specs-json respect any additional flags passed.
  • Parse target_rustcflags specifically for -Cpanic=abort and modify the return value of Config::can_unwind() to take it into account.
  • Add some new configuration flags to compiletest to control panic strategy for the selected target.

cc @pietroalbini who authored the original PR

@djkoloski djkoloski added the C-bug Category: This is a bug. label Apr 26, 2023
@jyn514
Copy link
Member

jyn514 commented Apr 26, 2023

Have --print=all-target-specs-json respect any additional flags passed.

This seems like the best solution to me.

@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Apr 26, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 26, 2023
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 27, 2023
@pietroalbini
Copy link
Member

Agree with jyn here on the solution. Unfortunately I won't have much time myself in the coming weeks to address this :(

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 15, 2023
…target, r=compiler-errors

Get current target config from` --print=cfg`

Compiletest was switched to querying all targets using `--print=all-target-specs-json` and `--print=target-spec-json` in rust-lang#108905. This unintentionally prevented codegen flags like `-Cpanic` from being reflected in the current target configuration. This change gets the current compiletest target config using `--print=cfg` like it was previously while still using the faster prints for getting information on all other targets.

Fixes rust-lang#110850.

`@jyn514` might be interested in reviewing since they commented on the issue.
cc `@tmandry` since this issue is affecting Fuchsia.
bors added a commit to rust-lang-ci/rust that referenced this issue May 16, 2023
…rget, r=compiler-errors

Get current target config from` --print=cfg`

Compiletest was switched to querying all targets using `--print=all-target-specs-json` and `--print=target-spec-json` in rust-lang#108905. This unintentionally prevented codegen flags like `-Cpanic` from being reflected in the current target configuration. This change gets the current compiletest target config using `--print=cfg` like it was previously while still using the faster prints for getting information on all other targets.

Fixes rust-lang#110850.

`@jyn514` might be interested in reviewing since they commented on the issue.
cc `@tmandry` since this issue is affecting Fuchsia.
@bors bors closed this as completed in 9dffb52 May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants