Open ehuss opened 4 years ago
My opinion on this is that Cargo should largely be able to handle all of this transparently without users having to configure anything. My thinking is that users specify a panic
setting in their profile, and then Cargo would automatically respect that and handle everything else, so you'd just say build-std
and everything would auto-magically work.
Now obviously like everything else the devil is in the details, so let me be a bit more clear about how I think all this can work.
Cargo is in control of every single crate in the crate graph, unlike today where libstd is precompiled. This gives Cargo the ability to compile every piece of code in the exact same panic profile. This, for example, lifts the restriction that panic_abort
must be compiled with -C panic=abort
. That's only done because we ship a precompiled copy, and the whole point of linking to it is to be a panic=abort
implementation. If Cargo compiles everything with panic=unwind
then if the compilation of the panic_abort
crate happens it's technically just wasted time since it'll never be used.
Eventually (hopefully soon) Cargo will be able to take a much more pure stance with respect to the test
crate, assuming everything is abort
or everything is panic
, no questions asked. This is still a work in progress but if that's the last remaining issue here and it still isn't over the finish line I don't mind finishing the implementation myself :)
Ideally we're compiling precisely one panic_*
crate, and I think that's ideally configured via some sort of feature on the std
crate itself. Right now there's a panic-unwind
feature which controls whether panic_unwind
is made available and linked in. If you compile libstd with panic=unwind
you're required to compile in the panic-unwind
feature/crate. Ideally with this feature we'd skip the panic=abort
crate. Conversely if you're compiling with panic=abort
then we can disable this feature and just build the panic_abort
crate. Overall, I'm envisioning some sort of contract between libstd and rust-lang/rust's manifests, and we'd both invent the contract and uphold it when building with Cargo.
The point about target-specific abort settings is pretty tricky. Almost all use cases of building libstd from source are panic=abort targets with no support for unwinding to boot. I think in these cases we should probably fix this at the source level in rust-lang/rust to make panic=unwind and panic=abort basically the same thing. By this I mean that on these targets the panic-unwind
crate should do nothing but just depend on panic_abort
and reexport it where panic_abort
is the actual panic runtime. That way whether or not Cargo thinks it's compiling in panic=abort or panic=unwind mode you get the same code. (the thinking here is we just want "things to work by default")
How's that sound? Does that cover the concerns you were thinking of @ehuss?
So testing this out a bit today, you can actually get this sort of working today by configuring panic = 'abort'
in your profile and passing -Z build-std=panic_abort,std
. The compiler will resolve the panic_abort
crate at compile time, which means that this only actually works if Cargo passes --extern panic_abort=...
. That's sort of unfortunate because it does mean that Cargo would have to hardcode the names panic_unwind
and panic_abort
which I'd ideally prefer to leave as internal details of libstd/rustc...
Sorry if this is a dumb question. Are there targets that have a panic strategy of "unwind", but do not want to use libunwind? Such as libunwind does not compile on the target, or the user wants to provide their own panic handler?
Alex, you mentioned having a feature to control whether the "panic_unwind" crate is enabled, but I'm hoping to support this without the user specifying features=["panic-unwind"]
. I was wondering if just looking at the profile setting would be sufficient to know if Cargo should build panic_abort
or panic_unwind
. I'm guessing if the strategy needs to be independent of the actual handler used, that might cause a problem.
If looking at the profile is not sufficient, are there any other options than using features?
Another question: The cross_custom
test cannot build either panic_unwind or panic_abort, because neither of those crates support "os": "none"
. How should that be handled?
In general a panic=unwind strategy requires so much compiler support that's basically impossible to implement a custom unwinding panic strategy. In that sense only targets officially supported by rustc (likely tier 1) and shipped with binaries will support unwinding panics, and yeah I think everything there wants to use the libunwind crate and C library in general for unwinding. (MSVC is a bit odd but "eh")
I think that we can use the panic
mode configured in the profile to configure which feature of libstd is compiled, whether it has panic-unwind
or not. (or at least that's what I was thinking). I believe that this should be sufficient for us to get everything working, so long as we also do a bit of legwork for the source code in libpanic_unwind
to compile on all platforms (but abort).
The reason I ask is because I have a branch that is switching back to using --extern
instead of --sysroot
. However, Cargo needs to know if it should pass --extern panic_unwind=…
and/or --extern panic_abort=…
or neither. With the current nightly, you can control this with the list of crates passed to -Zbuild-std
. However, in my branch I also have explicit std deps in Cargo.toml
instead of passing the crates on the command-line. But I assume we don't want people to be required to list which panic crate they want in [dependencies]
, so Cargo needs to have some way of knowing which to pass in.
Looking at the profile is fine if either unwind or abort is what you want, but what if you want neither? Such as the case mentioned above in cross_custom
, where not even panic_abort
will build.
Should panic_abort
have some kind of fallback that at least compiles on all platforms? I assume it is not possible, since the abort
intrinsic isn't available everywhere? And I assume an infinite loop would also be bad…
Oh ok so if we're passing --extern
it's a lot more tricky. Agreed that we can only "succeed" here if no one writes down panic_abort = { sysroot = true }
in their manifest.
There's a subtle difference, but we can't look at the profile for --extern
, but we can look at it for features enabled with libstd. With libstd today and the sysroot approach panic=unwind
means may use the unwinding strategy, but actually uses the default for the target. Using panic=abort
hardwires it to abort. By enabling the panic_unwind feature of std
it makes it a candidate for usage, but the compiler may end up using the panic_abort
crate anyway (which IIRC is always built). Using --extern
we're trying to take the decision into Cargo's hands, but Cargo doesn't know what the default panic strategy is.
The only real solution I can think of for this is to just always compile both panic_abort
and panic_unwind
. We'd then pass --extern
for both of them and basically leave it up to the compiler to figure out which it wants to use. This strategy would require that panic_abort
and panic_unwind
have fallbacks to compile on all platforms. The fallback would be always aborting, and I think we could probably get by with intrinsics::abort
to be "good enough", and then select platforms would have ways of improving on that. (there's also panic implementations and such nowadays to handle all this).
If you're in a case where neither is wanted, I think we can automatically detect that based on whether std
is used or not. This is the #![needs_panic_runtime]
attribute, which is only attached to std
, so if you don't compile std
we could probably auto-infer to avoid panic_abort
and panic_unwind
.
Overall that feels like a lot of custom logic in Cargo for the panic runtimes, which I'm not really overly comfortable with. It's not necessarily the end of the world though?
This is the
#![needs_panic_runtime]
attribute
Oh, I didn't know about that. I think that does make it easier. And if it is possible to have a fallback for all platforms in panic_abort
, that also might help.
I'll try to update my prototype and see how it goes. Thanks!
So it seems to be working great except for one wrinkle. If you have a project with panic="abort"
, it builds the standard library with the abort
strategy. This causes a problem for running tests, since they require all dependencies to be unwind
. What do you think of these options:
-Zpanic-abort-tests
to run tests with abort
strategy.I like 3, since it is simple and straightforward, and already works. I think 2 is doable, but I think will be very messy (it will need to create multiple std graphs, know which one to use, etc.), and is also really slow. I don't really know the consequence of 1.
Thoughts?
Definitely (3) in my opinion!
I suspect that'll stabilize long before -Zbuild-std
anyway, and for now I think it'd be fine if -Zbuild-std
basically implied -Zpanic-abort-tests
@ehuss Can you recall if anything new came up since this discussion? It seems like all that's needed is to plumb panic=abort
from the profile into building panic-abort automatically and using -Zpanic-abort-tests
(still unstable)
I don't think anything new has come up. I think the comments above still capture all the questions that need investigation and experimentation.
I'd glossed over the trickiness resulting from using --extern
. Thanks!
The current implementation does not support different panic strategies, and is hard-coded for
unwind
. This is a tricky and difficult issue, and there will need to be some investigation and experimentation to figure out all the details. Some things to think about:unwind
andabort
?-C panic=abort
must be passed for thepanic_abort
crate.abort
, need to figure that out.abort
crates, cargo currently rebuilds them withunwind
when built with the libtest harness. Does this mean the entire standard library would need to be built twice? This needs more thought, as it is already subtle and finicky.There is also some hard-coded knowledge in Cargo about building the panic-unwind/abort crates, due to the way the compiler works (just relying on the std feature flags wasn't enough). Alex mentioned a desire to remove this, but I'm not sure how.