Add internal RUSTC_INTERNAL_FORCE_PANIC_ABORT env var for libpanic_abort

[Aaron1011]

Currently, we require libpanic_abort to always be build with the
abort panic strategy, regardless of what panic strategy would
normally be passed to rustc (e.g. unwind when building libstd).

This ensures that when downstream crates build against libpanic_abort,
it is properly detected as the panic runtime for the abort strategy.

Previously, this was done by special-casing libpanic_abort in
bootstrap. This meant that building libpanic_abort without using
x.py (e.g by using xargo) would result in the wrong panic strategy
being applied, leading to an error when trying to build against it:

error: the crate `panic_abort` does not have the panic strategy `abort`

This is a problem for tools like Miri, which require a custom-build
libstd, and cannot use x.py.

To fix this, we add a special environment variable
RUSTC_INTERNAL_FORCE_PANIC_ABORT. This is set in the build.rs
for libpanic_abort, and checked by the compiler when determining the
panic strategy of the current crate. While this is still a hack, it's a
much better one - the special case is now represented in the
libpanic_abort crate itself, rather than in bootstrap.

Ideally, this would be an internal attribute (e.g.
#[rustc_panic_abort]) that we apply to libpanic_abort.

Unfortunately, emscripten targets require that we be able to determine
the panic strategy very early on, before we've even started parsing the
crate:

rust/src/librustc_codegen_llvm/llvm_util.rs

Line 77 in 3fc30d8

sess.panic_strategy() == PanicStrategy::Unwind {

To avoid invasive changes to emscripten and/or the codewgen backend
infrastructure, I chose to add a new environment variable.

Note that the hack in bootstrap needs to remain until this changes makes
its way into the bootstrap compiler, to ensure that we can still build a
correct libpanic_abort with a bootstrap compiler that doesn't know
about this special environment variable.

[rust-highfive]

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

[RalfJung]

@rust-lang/compiler @oli-obk @alexcrichton I'd be interested in what you think is the best approach to ensuring that libpanic_abort is always compiled with -C panic=abort, in a way that does not rely on x.py.

Cc @ehuss for wg-cargo-std-aware, this might also affect you I think.

[Aaron1011]

Note that we could switch back to my original attribute+based approach if we could make the emscripten check perform the query of the panic strategy at a later point in time.

I glanced over the relevant code, and it seemed like it would require moving the creation or initialization of CodegenBackend to after we've started parsing the crate. I'm not sure if that amount of churn would be be worth it, or if delaying CodegenBackend::init is even desirable

[Mark-Simulacrum]

r? @alexcrichton

But implementation looks fine overall, if a bit unfortunate.

[RalfJung]

Just to make sure I understand why we need this now... the reason is that with your PRs, when __rust_start_panic gets called we actually run that code instead of aborting execution, and with -C panic=abort that ends up in libpanic_abort which is compiled incorrectly?

Couldn't we just keep aborting execution in __rust_start_panic with -C panic=abort?

[Aaron1011]

@RalfJung: No, this issue occurs before any of my code runs. Run cargo +nightly miri run -- -C panic=abort on the current nightly, and note that you get an error.

We need this now because we actually want to start using -C panic=abort

[alexcrichton]

I would personally say we should not merge this PR, I think there's some misunderstandings going on here and this feels like it's just piling hacks on hacks for a tool that shouldn't necessarily be guiding the design of the compiler and injecting random environment variables to be read at various locations.

Currently, we require libpanic_abort to always be build with the
abort panic strategy

I don't actually believe that this, as-stated, is correct. Cargo, for example, in -Zbuild-std mode builds the panic_abort crate with the unwind strategy and it works fine. I think there's some other subtelties going on here which indicate an error that isn't fully understood. I think the first step is to fully understand the source of the error, and then we can try to work from there to tweaking the crate and/or compiler.

[Aaron1011]

I think there's some misunderstandings going on here and this feels like it's just piling hacks on hacks

This is swapping out one hack for a slightly better hack. We still override the panic strategy for libpanic_abort, but part of the logic now lives in libpanic_abort itself, rather than a fairly arbitrary part of bootstrap.

Cargo, for example, in -Zbuild-std mode builds the panic_abort crate with the unwind strategy and it works fine.

That's because cargo -Z build-std is doing something different from both Miri (technically Xargo) and normal Cargo.

Normally, libstd is built once in panic=unwind mode. The target crate (with either Miri or normal Cargo) is then built in either panic or unwind mode, but is always linked against a libstd that was built in unwind mode.

What cargo -Z build-std appears to be doing is to build the entire crate graph - libstd plus the user's crate - in either unwind or abort mode.

In the unwind case, nothing ever tries to use the panic abort runtime, so the error never shows up (despite libpanic_abort being built in unwind mode).

In the abort case, libpanic_abort gets built in abort mode like everything else, so it works fine.

I can't find a way to stop cargo -Z build-std from rebuilding libstd when I change my Cargo.toml from [profile.dev] panic = "unwind" to [profile.dev] panic = "abort". If there's a way to do that, this issue should show up.

I think the first step is to fully understand the source of the error

This error comes from here:

rust/src/librustc_metadata/creader.rs

Lines 598 to 600 in bc0e288

	self.sess.err(&format!("the crate `{}` does not have the panic \
	strategy `{}`",
	name, desired_strategy.desc()));

The PanicStrategy for a crate gets stored in the metadata here:

rust/src/librustc_metadata/rmeta/encoder.rs

Line 552 in 56237d7

panic_strategy: tcx.sess.panic_strategy(),

which calls the method that this PR modifies:

rust/src/librustc/session/mod.rs

Line 565 in bc0e288

pub fn panic_strategy(&self) -> PanicStrategy {

The hack in bootstrap is responsible for forcibly overriding the panic strategy passed to rustc, causing libpanic_abort to always be encoded with a panic strategy of Abort.

Fundamentally, this error is about mixing a panic-unwind libstd with a panic-abort user crate. It might be possible to refactor how we represent a crate PanicStrategy to eliminate this hack. However, I think this PR is an improvement on the existing hack - not only does it make things easier to understand, it allows users of Xargo (Miri + anything else) to function correctly.

[RalfJung]

To expand on what @Aaron1011 wrote, here's the existing hack:

rust/src/bootstrap/bin/rustc.rs

Lines 92 to 107 in e931f00

	// If we're compiling specifically the `panic_abort` crate then we pass
	// the `-C panic=abort` option. Note that we do not do this for any
	// other crate intentionally as this is the only crate for now that we
	// ship with panic=abort.
	//
	// This... is a bit of a hack how we detect this. Ideally this
	// information should be encoded in the crate I guess? Would likely
	// require an RFC amendment to RFC 1513, however.
	//
	// `compiler_builtins` are unconditionally compiled with panic=abort to
	// workaround undefined references to `rust_eh_unwind_resume` generated
	// otherwise, see issue https://github.com/rust-lang/rust/issues/43095.
	if crate_name == Some("panic_abort") ||
	crate_name == Some("compiler_builtins") && stage != "0" {
	cmd.arg("-C").arg("panic=abort");
	}

Notice in particular the comment that says "This... is a bit of a hack how we detect this. Ideally this information should be encoded in the crate I guess?" ;)

[alexcrichton]

Yes I understsand the literal locations this error message is coming from because I basically wrote all the code here (rustbuild, rustc, error messages, etc).

Again I'm not fully understanding the source of the error here though in terms of setup and environment, not so much in terms of where it comes out of rustc. It's not correct that the error comes from mixing a panic=unwind libstd with a panic=abort user crate. That happens all the time when you compile with panic=abort via Cargo.

It sounds like the error you're talking about is that this may be a bug in xargo? When xargo builds crates it builds everything with panic=unwind, including the panic_abort crate, and then if you later actually compile your normal crate with panic=abort that fails because the panic_abort crate itself was compiled incorrectly. If I understand that right then this is a bug in xargo, not a reason to pile on hacks in rustc. (or more generally a bug in the build, just like it's a "bug" in rustbuild today)

[Aaron1011]

It's not correct that the error comes from mixing a panic=unwind libstd with a panic=abort user crate. That happens all the time when you compile with panic=abort via Cargo.

In that scenario, we're using a libpanic_abort that has the abort strategy set (since the libstd normally used with Cargo is built from CI, via the x.py hack).

With Xargo, we build directly from the Cargo.toml, so we don't get the special handling of libpanic_abort.

If I understand that right then this is a bug in xargo, not a reason to pile on hacks in rustc.

It's not really a bug in Xargo, because Xargo has no idea that the libpanic_abort crate needs to be treated specially. By moving the special-casing out of bootstrap and into the compiler and libpanic_abort itself, both bootstrap and xargo will use the same logic for selecting the panic strategy of libpanic_abort.

f I understand that right then this is a bug in xargo, not a reason to pile on hacks in rustc

I disagree that this change is 'piling on hacks'. This is moving the location of an existing hack, and moving special handling out of bootstrap into the compiler itself. As long as we need special handling for libpanic_abort, I think it's much cleaner to have the crate itself declare its usage of the hack, rather than relying on the build system having special knowledge of certain crates beyond what is encoded in the Cargo.toml and build.rs

[Aaron1011]

I agree that the use of a magic environment variable is really unfortunate (though I still think it's preferrable to hardcoded logic in bootstrap). Ideally, someone familiar with emscripten and LLVM could figure out if there's a way to move the panic_strategy check to after we've parsed crate attributes (without needing a ton of refactoring). That would allow us to switch back to using the custom attribute that I originally had in #60026 (comment)

[RalfJung]

It sounds like the error you're talking about is that this may be a bug in xargo? When xargo builds crates it builds everything with panic=unwind, including the panic_abort crate, and then if you later actually compile your normal crate with panic=abort that fails because the panic_abort crate itself was compiled incorrectly. If I understand that right then this is a bug in xargo, not a reason to pile on hacks in rustc. (or more generally a bug in the build, just like it's a "bug" in rustbuild today)

Well, the alternative to this PR is to replicate the hack from rustbuild in xargo. This would, in particular, require copying the rustbuild scheme of having a rustc wrapper binary that messes with the environment and the cmdline flags before calling the real rustc. So far, xargo doesn't need that -- turns out that (modulo the panic strategy stuff) libstd builds just fine with plain cargo, once the Cargo.toml is set up appropriately.

So the alternative is to either have to replicate the wrapper-binary-based hack everywhere that builds libstd (this is what we do right now), or find a solution that does not require wrapper-binary-hacks (which this PR proposes).

[alexcrichton]

Everything about xargo and rustbuild is just a bunch of unstable code, which is otherwise read as "it works today but has no guarantees". I'm subjectively saying this is a bigger hack than what's there today, because an environment variable is extremely non-discoverable, there's zero gating here, and it's just a random change in the middle of the compiler to read an environment variable. This has little-to-no precedent for these sorts of compiler settings and seems very likely to be abused in the future.

The rustbuild changes are a local "hack" which only applies to the Rust build system itself. It cannot be exploited by other users and is contained entirely to Rust's build sytem, not the compiler.

Xargo is not a supported method of compiling the standard library. Issues that arise during that do not imply that you should change the compiler to work with a deprecated and no longer supported project. The -Zbuild-std mode of Cargo is the hopeful replacement for Xargo.

I'm pretty firmly against modifying the compiler in this way purely to cater to a use case of Xargo in a niche area. I'd recommend giving -Zbuild-std a try. I do not understand the intricacies of why an attribute isn't working, nor do I have time to review those intricacies at this time.

[Aaron1011]

I'm subjectively saying this is a bigger hack than what's there today, because an environment variable is extremely non-discoverable

I would argue that it's more discoverable than the current hack, as it's possible to find out that something special is going on by examining libpanic_abort. Anecdotally, I spent a lot of time trying to figure out how libpanic_abort was magically getting a different panic strategy until I thought to look in bootstrap.

The -Zbuild-std mode of Cargo is the hopeful replacement for Xargo.

The issue is that Miri currently relies on Xargo. I'm not very familiar with how far along cargo -Z build-std is, or to what extent Miri is relying on internals of Xargo. However, I don't think it will be trivial to switch Miri to -Z build-std (cc @RalfJung).

In any case, -Z build-std will not currently work with Miri, as it lacks the hack for libpanic_abort. Since Miri builds its custom libstd separately, this issue will still show up when we try to use -C panic=abort.

I understand your concern about adding magic environment variables to the compiler. However, I'm concerned that full unwinding support on Miri could become blocked indefinitely on:

1. Migrating Miri to -Z build-std (which at minimum needs the libpanic_abort hack, and possibly other things - I haven't tested). or
2. Moving the emscripten-specific lookup of panic_strategy until after parsing, so that we can use a proper attribute instead of an environement variable.

Note that without some kind of workaround, Miri will either lose the ability to perform abort panics (unless we add some awful hack to handle -C panic=abort), or the unwinding PR will become blocked. I would really like to avoid that if at all possible.

[RalfJung]

Hacks based on bootstrap's wrapper binaries are extremely hard to discover and debug, I find myself agreeing with @Aaron1011 there and I have also lost my own share of times wondering what the heck is going on until I realized that the wrapper binary is the culprit. On the other hand the env var is indeed a quite bad hack, too.

The best solution seems to be an attribute, which is blocked by that codegen question. Cc @tlively for emscripten; who would know things about codegen backend initialization?

@alexcrichton

there's zero gating here

That's fair, the env var should only work on nightly similar to -Z flags.

a deprecated and no longer supported project

xargo is maintained by me to the extend that Miri needs it. I am looking forward to using -Zbuild-std eventually instead, but I doubt it will solve this particular problem -- unlike -Zbuild-std, Miri would prefer to build libstd once and then use it for many projects.

@Aaron1011

The issue is that Miri currently relies on Xargo. I'm not very familiar with how far along cargo -Z build-std is, or to what extent Miri is relying on internals of Xargo. However, I don't think it will be trivial to switch Miri to -Z build-std (cc @RalfJung).

Miri doesn't care about Xargo internals. It just needs a libstd built with some non-standard flags.

However, as you said, using cargo -Zbuild-std to build that libstd (instead of Xargo) would have the exact same problem as Xargo, unless cargo -Zbuild-std replicates the rustbuild/bootstrap hack.

However, I'm concerned that full unwinding support on Miri could become blocked indefinitely on:

[2 options listed]

Well, there is a third option: adding a rustc wrapper binary to xargo and replicating the rustbuild/bootstrap hack.

@Aaron1011 did you try adjusting the Xargo.toml to make Xargo build libpanic_abort separately and with panic=abort? That might or might not work, but it seems worth a shot.

Note that without some kind of workaround, Miri will either lose the ability to perform abort panics (unless we add some awful hack to handle -C panic=abort), or the unwinding PR will become blocked. I would really like to avoid that if at all possible.

Personally I think I am fine with not supporting panic=abort in Miri. We can always add -Zmiri-panic-abort instead and "manually" abort on __rust_start_panic -- not pretty but it'd be an interim solution. As you said we already don't support panic=abort.

[Aaron1011]

@Aaron1011 did you try adjusting the Xargo.toml to make Xargo build libpanic_abort separately and with panic=abort? That might or might not work, but it seems worth a shot.

@RalfJung: Unfortunately, Xargo only appears to support setting a global RUSTFLAGS and panic strategy - there's no way to customize either one on a per-crate or per-stage basis, which is what we would need in order to handle libpanic_abort.

[Aaron1011]

Here's another option:

We add a flag -Z force-if-panic-abort to rustc. When enabled, this flag forces the current crate to use panic=abort if the crate is named libpanic_abort. Otherwise, it does nothing.

This flag is passed unconditionally by bootstrap and xargo. While the compiler now depends on libpanic_abort having the right name, normal users can't see this behavior, since it's behind a flag. This will allow Miri to continue to build libstd without needing to make yet another wrapper binary.

[alexcrichton]

I'm sorry but it's hard to read over these comments because they repeatedly have factual inaccuracies which in my opinion drastically changes the calculus of what this solution looks like:

However, as you said, using cargo -Zbuild-std to build that libstd (instead of Xargo) would have the exact same problem as Xargo, unless cargo -Zbuild-std replicates the rustbuild/bootstrap hack.

This is not true. -Zbuild-std works today and doesn't need this hack and/or env var. I explained this above.

xargo is maintained by me to the extend that Miri needs it.

That's good news to hear, but I find it pretty off-putting still. Xargo is an unsupported way to build libstd and has always been "best effort". Finding an issue and the pushing really hard for a random change in the compiler is not the right flow of effort here in my opinion. The compiler works the way it does and building libstd is not a trivial task for every single invocation. Sometimes some complications are necessary.

I understand that trickery in the build system is not discoverable, but I will state again that an environment variable for this random setting in the compiler is wildly inappropriate. I am personally not a fan of a random -Z flag for this either, since the only user will be xargo and the only reason xargo would do it is because you don't want to do any other work of having a shim, for example. Furthermore you also don't want to do the work of migrating to -Zbuild-std, which would also fix this.

I feel like I'm suggesting better solutions than modifying the compiler (because that's not trivial) and I'm being met with "yeah but I don't want to do that work so please land this random environment variable instead".

[Aaron1011]

This is not true. -Zbuild-std works today and doesn't need this hack and/or env var.

-Z build-std works because it rebuilds the entire standard library when the user changes the panic strategy. This is not how Miri uses its libstd - like a normal Cargo invocation, the libstd is built once in unwind mode, and then linked to a panic=abort or panic=unwind user crate.

I am personally not a fan of a random -Z flag for this either, since the only user will be xargo

No, bootstrap would be a user as well (once the change makes its way into the bootstrap compiler). This would replace the need to specifically check for libpanic_abort in both bootstrap and xargo

[RalfJung]

I feel like I'm suggesting better solutions than modifying the compiler (because that's not trivial) and I'm being met with "yeah but I don't want to do that work so please land this random environment variable instead".

You suggested using -Zbuild-std instead of xargo. We are trying to explain why that is (likely) not a great fit for Miri, not because it's work but because it degrades the user experience. It seems so far we failed to explain that.

With -Zbuild-std, from what I understand, every time someone does cargo miri test in a workspace for the first time, it would build libstd for this particular workspace, for the flags that are used there. This is worse than what currently happens with Miri, where Miri's libstd is stored globally, so once it is built, all workspaces share that libstd. Basically, Miri's use of libstd is more like the way rustc normally uses it, than what -Zbuild-std does. So, just like for normal rustc, that global libstd should be usable both with -C panic=unwind and -C panic=abort.

[tlively]

Unfortunately I do not have broad enough knowledge of rustc to say what would be involved in reordering things so that the codegen backend is initialized after parsing begins.

[eddyb]

I doubt you can reorder anything wrt the codegen backend, because the only reason I can come up with for initializing LLVM that early is to extract information for #[cfg]-gating.

But I hope I'm wrong, as I don't like the idea that LLVM controls early parts of the compilation.

[alexcrichton]

Well I think I've made my case here, and I do not have anything else to add.

• An env var in the compiler is inappropriate for this
• It is not correct that this sort of flag or logic is required for everyone building std
• This treatment is only required when dealing with precompiled binaries. The official releases are precompiled and sounds like miri effectively wants precompiled releases in your home directory.

If you're dealing with precompiled binaries the panic_abort runtime must be compiled with panic=abort. The bootstrap build system does this by using a shim. I think that the work to update xargo to use a shim needs to be done or a different patch needs to be proposed.

[Aaron1011]

I doubt you can reorder anything wrt the codegen backend, because the only reason I can come up with for initializing LLVM that early is to extract information for #[cfg]-gating.

@eddyb: You're right:

rust/src/librustc_interface/util.rs

Lines 49 to 71 in cefcc20

	/// Adds `target_feature = "..."` cfgs for a variety of platform
	/// specific features (SSE, NEON etc.).
	///
	/// This is performed by checking whether a whitelisted set of
	/// features is available on the target machine, by querying LLVM.
	pub fn add_configuration(
	cfg: &mut ast::CrateConfig,
	sess: &Session,
	codegen_backend: &dyn CodegenBackend,
	) {
	let tf = sym::target_feature;
	cfg.extend(
	codegen_backend
	.target_features(sess)
	.into_iter()
	.map(|feat| (tf, Some(feat))),
	);
	if sess.crt_static_feature() {
	cfg.insert((tf, Some(Symbol::intern("crt-static"))));
	}
	}

I think any re-ordering would have to take of moving the emscripten check, insted of moving LLVM initialzition.

[Aaron1011]

I've come up with a new approach in #66489, based on manually initializing the emscripten flag after we initialize LLVM. I'm closing this PR in favor of that one, since most of the discussion here centers around a different approach.