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 a panic-strategy field to the target specification #36794

Merged
merged 2 commits into from Sep 29, 2016

Conversation

Projects
None yet
6 participants
@japaric
Copy link
Member

japaric commented Sep 28, 2016

Now a target can define its panic strategy in its specification. If a
user doesn't specify a panic strategy via the command line, i.e. '-C
panic', then the compiler will use the panic strategy defined by the
target specification.

Custom targets can pick their panic strategy via the "panic-strategy"
field of their target specification JSON file. If omitted in the
specification, the strategy defaults to "unwind".

closes #36647


I checked that compiling an executable for a custom target with "panic-strategy" set to "abort" doesn't need the "eh_personality" lang item and also that standard crates compiled for that custom target didn't contained undefined symbols to _Unwind_Resume. But this needs an actual unit test, any suggestion on how to test this?

Most of the noise in the diff is due to moving PanicStrategy from the rustc to the rustc_back crate.

r? @alexcrichton
cc @phil-opp

add a panic-strategy field to the target specification
Now a target can define its panic strategy in its specification. If a
user doesn't specify a panic strategy via the command line, i.e. '-C
panic', then the compiler will use the panic strategy defined by the
target specification.

Custom targets can pick their panic strategy via the "panic-strategy"
field of their target specification JSON file. If omitted in the
specification, the strategy defaults to "unwind".

closes #36647
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 28, 2016

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Sep 28, 2016

travis failed, I think you need some stage1 cfg attributes

Testing librustc stage2 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
   Compiling fmt_macros v0.0.0 (file:///build/src/libfmt_macros)
   Compiling syntax_pos v0.0.0 (file:///build/src/libsyntax_pos)
   Compiling rustc_errors v0.0.0 (file:///build/src/librustc_errors)
   Compiling syntax v0.0.0 (file:///build/src/libsyntax)
   Compiling build_helper v0.1.0 (file:///build/src/build_helper)
   Compiling serialize v0.0.0 (file:///build/src/libserialize)
   Compiling flate v0.0.0 (file:///build/src/libflate)
   Compiling rustc_llvm v0.0.0 (file:///build/src/librustc_llvm)
   Compiling rustc_const_math v0.0.0 (file:///build/src/librustc_const_math)
   Compiling rustc_macro v0.0.0 (file:///build/src/librustc_macro)
   Compiling rustc_data_structures v0.0.0 (file:///build/src/librustc_data_structures)
   Compiling rustc_platform_intrinsics v0.0.0 (file:///build/src/librustc_platform_intrinsics)
   Compiling rustc_bitflags v0.0.0 (file:///build/src/librustc_bitflags)
   Compiling syntax_ext v0.0.0 (file:///build/src/libsyntax_ext)
   Compiling rustc_back v0.0.0 (file:///build/src/librustc_back)
   Compiling arena v0.0.0 (file:///build/src/libarena)
   Compiling log v0.0.0 (file:///build/src/liblog)
   Compiling rustc v0.0.0 (file:///build/src/librustc)
   Compiling rustc_save_analysis v0.0.0 (file:///build/src/librustc_save_analysis)
error[E0308]: mismatched types
    --> src/librustc/session/config.rs:2311:25
     |
2311 |         opts.cg.panic = PanicStrategy::Abort;
     |                         ^^^^^^^^^^^^^^^^^^^^ expected enum `core::option::Option`, found enum `rustc_back::PanicStrategy`
     |
     = note: expected type `core::option::Option<rustc_back::PanicStrategy>`
     = note:    found type `rustc_back::PanicStrategy`

error: aborting due to previous error
@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Sep 28, 2016

@oli-obk Thanks for the heads up. I don't think we need a cfg here because this is a test which can't be run against stage0 (stage1 already has the change to Option).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 28, 2016

@bors: r+

Thanks @japaric!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 28, 2016

📌 Commit 8a46e78 has been approved by alexcrichton

jonathandturner added a commit to jonathandturner/rust that referenced this pull request Sep 29, 2016

Rollup merge of rust-lang#36794 - japaric:target-panic, r=alexcrichton
add a panic-strategy field to the target specification

Now a target can define its panic strategy in its specification. If a
user doesn't specify a panic strategy via the command line, i.e. '-C
panic', then the compiler will use the panic strategy defined by the
target specification.

Custom targets can pick their panic strategy via the "panic-strategy"
field of their target specification JSON file. If omitted in the
specification, the strategy defaults to "unwind".

closes rust-lang#36647

---

I checked that compiling an executable for a custom target with "panic-strategy" set to "abort" doesn't need the "eh_personality" lang item and also that standard crates compiled for that custom target didn't contained undefined symbols to _Unwind_Resume. But this needs an actual unit test, any suggestion on how to test this?

Most of the noise in the diff is due to moving `PanicStrategy` from the `rustc` to the `rustc_back` crate.

r? @alexcrichton
cc @phil-opp

bors added a commit that referenced this pull request Sep 29, 2016

Auto merge of #36818 - jonathandturner:rollup, r=jonathandturner
Rollup of 12 pull requests

- Successful merges: #35286, #35892, #36460, #36704, #36741, #36760, #36787, #36789, #36794, #36803, #36811, #36813
- Failed merges:

@bors bors merged commit 8a46e78 into rust-lang:master Sep 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Oct 2, 2016

What happens when a Cargo file and target both specify the panic strategy? I'd prefer it if Cargo files specified a set of allowed panic strategies and then the target specification alone picked the one used.

[Also, would be convenient to specify a map of names to targets in a (virtual) workspace root.]

@japaric japaric deleted the japaric:target-panic branch Oct 2, 2016

@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Oct 2, 2016

@Ericson2314 Under the current implementation of Cargo, profile.*.panic will be preferred over what the target specifies because Cargo passes -C panic=$PROFILE to its rustc invocations.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Oct 2, 2016

Ok, that seems good enough for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.