Uh oh!
There was an error while loading. Please reload this page.
Add new rustc_panic_abort_runtime attribute for libpanic_abort - #66489
Add new rustc_panic_abort_runtime attribute for libpanic_abort#66489Aaron1011 wants to merge 2 commits into
rustc_panic_abort_runtime attribute for libpanic_abort#66489Conversation
rust-highfive
commented
Nov 17, 2019
(rust_highfive has picked a reviewer for you, use r? to override) |
bjorn3
left a comment
There was a problem hiding this comment.
Would it be possible to move codegen_backend.init(sess) itself after parsing? Having two places to initialize a codegen backend can lead to confusion about when to use which place when adding some new thing to init.
Uh oh!
There was an error while loading. Please reload this page.
rust-highfive
commented
Nov 17, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Aaron1011
commented
Nov 17, 2019
@bjorn3: Unfortunately, no. Sadly, parsing the crate actually depends on LLVM being initialized, since we need to know LLVM's supported features for cfg-gating: rust/src/librustc_interface/util.rs Lines 49 to 71 in cefcc20 This whole situation is far from ideal. However, I believe that this solution allows us to remove the bootstrap hack with a minimum of changes to the compiler. |
Aaron1011
commented
Nov 17, 2019
I had to change my approach slightly. Checking for |
Uh oh!
There was an error while loading. Please reload this page.
Aaron1011
commented
Nov 18, 2019
cc @alexcrichton: I believe this approach avoids your concerns with a custom environment variable/ |
alexcrichton
commented
Nov 19, 2019
I personally believe that this is still not necessary and it's better to fix the build system rather than add features to the compiler which are likely to be somewhat difficult to maintain over time. This, unlike the previous PR, however is gated on nightly so we can change this later, and it seems fine to land if it's approved to me. |
By 'fix the build system', do you mean moving the hack to Xargo, or adjusting how we determine if If it's the former, I'm strongly opposed to perpetuating this kind of hack. I think any special handling of a crate should require something in the crate itself, whether it's an attribute or something in If it's the latter, I think that sounds fine, but I'm not sure the best way to go about that. |
Aaron1011
commented
Nov 19, 2019
I'd appreciate it if someone more familiar with LLVM could weigh in on my approach to 'late-modification' of LLVM command line arguments. In my local testing, it appears to be doing the right thing w.r.t emscripten (I initially had it wrong, and the panic-related tests failed in weird ways). However, I don't know how likely this is to break in future LLVM releases, or if there's something subtle I'm overlooking. |
bjorn3
commented
Nov 19, 2019
Using something in |
Alexendoo
commented
Nov 27, 2019
Ping from triage, any updates? @nikomatsakis |
bors
commented
Nov 29, 2019
☔ The latest upstream changes (presumably #66697) made this pull request unmergeable. Please resolve the merge conflicts. |
ae55910 to
d2c2252Comparerust-highfive
commented
Nov 30, 2019
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
commented
Dec 1, 2019
☔ The latest upstream changes (presumably #66908) made this pull request unmergeable. Please resolve the merge conflicts. |
1704697 to
f6d8137Comparebors
commented
Dec 3, 2019
☔ The latest upstream changes (presumably #66947) made this pull request unmergeable. Please resolve the merge conflicts. |
f6d8137 to
3ab9acfComparebors
commented
Dec 3, 2019
☔ The latest upstream changes (presumably #66997) made this pull request unmergeable. Please resolve the merge conflicts. |
Aaron1011
commented
Dec 4, 2019
This is constantly getting broken due to changes to |
Dylan-DPC-zz
commented
Dec 10, 2019
r? @oli-obk |
oli-obk
left a comment
There was a problem hiding this comment.
I've read through the superseded PRs comments and am still unclear on some things.
Is the main statement here that it will never be possible to move miri (and other xargo users) to cargo -Zbuild-std? If so, what could be changed with -Zbuild-std to support this use case? This does seem like a cargo issue and not a rustc issue (I was under the impression the goal is to retire xargo when cargo supports everything needed).
There was a problem hiding this comment.
Won't this in turn break cargo's -Zbuild-std which builds this crate with the unwind strategy?
There was a problem hiding this comment.
Building this crate with the unwind strategy is the wrong behavior - it should always be built with the abort strategy. So, this will correctly change how -Zbuild-std builds this crate - but it's a bugfix, not breakage.
Aaron1011
commented
Dec 10, 2019
This change is correct regardless of what any In While this change allows |
3ab9acf to
d93892dCompareAaron1011
commented
Dec 21, 2019
@oli-obk: Rebased |
ehuss
commented
Dec 21, 2019
I don't know what this means, I also never saw an answer to my question above. Is there anything (other than time) blocking miri from using |
@ehuss: Do you mean that
Miri is currently using a pre-built libstd with Xargo. Switching away from a pre-built libstd would increase Miri run times and disk space usage (since every crate you run |
ehuss
commented
Dec 21, 2019
Hm, I think I may be a little confused, as what I think of "pre-built libstd" is what ships with Currently, If I'm understanding that performance concern correctly, would it be sufficient for miri if Cargo had a shared dependency cache in a common location? Are there any other fundamental concerns? (FYI, I know next to nothing about miri or how it works, so I have no idea what it needs concerning the standard library crates.) |
bors
commented
Dec 21, 2019
☔ The latest upstream changes (presumably #67485) made this pull request unmergeable. Please resolve the merge conflicts. |
Sorry, my use of 'pre-built' may have been unclear. I'm referring to you're calling 'shared artifact caching' - that is, building a
That's correct
Are there any plans to change this (even if it's far in the future)? I completely understand that there are more pressing concerns for Assuming that a 're-use a built
Ideally, Miri would continue to be able to build |
Supersedes rust-lang#66311 This replaces the hack in `bootstrap`, allowing the special handling for `libpanic_abort` to be encoded into the crate source itself, rather than existing as special knowledge in the build system. This will allow Miri (and any other users of Xargo) to correctly build `libpanic_abort` without relying on `bootstrap` or custom wrapepr hacks. The trickeist part of this PR is how we handle LLVM. The `emscripten` target family requires the "-enable-emscripten-cxx-exceptions" flag to be passed to LLVM when the panic strategy is set to "unwind". Unfortunately, the location of this emscripten-specific check ends up creating a circular dependency between LLVM and attribute resoltion. When we check the panic strategy, we need to have already parsed crate attributes, so that we determine if `rustc_panic_abort_runtime` was set. However, attribute parsing requires LLVM to be initialized, so that we can proerly handle cfg-gating of target-specific features. However, the whole point of checking the panic strategy is to determinne which flags to use during LLVM initialization! To break this circular dependency, we explicitly set the "-enable-emscripten-cxx-exceptions" in LLVM's argument-parsing framework (using public but poorly-documented APIs). While this approach is unfortunate, it only affects emscripten, and only modifies a command-line flag which is never used until much later (when we create a `PassManager`).
b4b2523 to
6ea7226Compareehuss
commented
Dec 21, 2019
Yea, we want to get to a point where a shared artifact cache works well in Cargo. Some work was done over the past year, but there are some difficult things to deal with.
This I don't agree with. There aren't any hacks currently in use, and AFAIK cargo doesn't need this. With |
Aaron1011
commented
Dec 21, 2019
@ehuss: See my comment from before:
|
ehuss
commented
Dec 21, 2019
Caching seems unrelated to whether or not Cargo needs this new attribute. If the artifacts are shared, it's a one-time cost. So if project A uses unwind, and B uses abort, you'll end up with two different copies of the standard library. If there is a shared cache, then project C will reuse either A or B's copy, depending on which it needs. |
Aaron1011
commented
Dec 21, 2019
@ehuss: I see. So, there would be an initial cost to changing settings in your I believe that Miri will still want to build a If Miri ends up switching to Regardless of what Miri ends up doing, I don't think this kind of crate-specific logic belongs in |
oli-obk
commented
Dec 21, 2019
Imo once So basically the choices we have that would allow miri to work with
|
bors
commented
Dec 23, 2019
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
joelpalmer
commented
Jan 6, 2020
Ping from Triage: any updates @Aaron1011? |
Aaron1011
commented
Jan 9, 2020
@joelpalmer: This is blocked on a decision from @oli-obk, @ehuss, @alexcrichton and others on whether this is the right approach. |
RalfJung
commented
Feb 24, 2020
@Aaron1011 does the fact that crates compiled for Miri no longer are codegen'd help with this? |
Aaron1011
commented
Feb 24, 2020
@RalfJung: This PR addresses a different issue - the metadata that we write out for |
oli-obk
commented
Mar 18, 2020
The current status is
So, my suggestion is to a) close this PR |
RalfJung
commented
Mar 18, 2020
I agree leaving this PR open will not lead anywhere. Thus, let's close it. The problem of supporting panic=abort in Miri is tracked at rust-lang/miri#1058. |
Supersedes #66311
This replaces the hack in
bootstrap, allowing the special handling forlibpanic_abortto be encoded into the crate source itself, rather thanexisting as special knowledge in the build system. This will allow Miri
(and any other users of Xargo) to correctly build
libpanic_abortwithout relying on
bootstrapor custom wrapepr hacks.The trickeist part of this PR is how we handle LLVM. The
emscriptentarget family requires the "-enable-emscripten-cxx-exceptions" flag
to be passed to LLVM when the panic strategy is set to "unwind".
Unfortunately, the location of this emscripten-specific check ends up
creating a circular dependency between LLVM and attribute resoltion.
When we check the panic strategy, we need to have already parsed crate
attributes, so that we determine if
rustc_panic_abort_runtimewas set.However, attribute parsing requires LLVM to be initialized, so that we
can proerly handle cfg-gating of target-specific features. However, the
whole point of checking the panic strategy is to determinne which flags
to use during LLVM initialization!
To break this circular dependency, we explicitly set the
"-enable-emscripten-cxx-exceptions" in LLVM's argument-parsing framework
(using public but poorly-documented APIs). While this approach is
unfortunate, it only affects emscripten, and only modifies a
command-line flag which is never used until much later (when we create a
PassManager).