allow -Ldependency search paths for panic runtimes

[Bryanskiy]

View all comments

Part of build-std=always RFC.

This PR supports -Ldependency= search paths for panic runtimes.

To do so, both panic runtimes (or one if the other hasn't been built) are injected as conditional dependencies of std during crate resolution. Therefore, the panic runtimes will be present in the std metadata and loaded like regular transitive dependencies, rather than being injected as a direct dependency. The activation of one particular runtime happens later in dependency_format.rs, based on the desired strategy.

This will allow Cargo to treat panic runtimes as regular transitive dependencies, rather than special-casing them.

cc @adamgemmell

[rustbot]

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 74 candidates
• Random selection from 18 candidates

[Bryanskiy]

@rustbot blocked on #159717

[bjorn3]

This would risk accidentally getting a crates.io crate replacing compiler-builtins or a panic runtime from the sysroot even when it isn't a direct dependency, right? Currently -Ldependency is only used for crates that are indirect dependencies and thus we already know the crate hash for to prevent such accidental inclusion.

[petrochenkov]

One possible alternative is using something like force (ExternEntry::force) + no-link (CrateDepKind::MacrosOnly) for dependencies like panic runtime. Then the panic runtime will be present in the dependency tree, its crate hash will be recorded, and it will be naturally found in the dependency directories.

inject_panic_runtime and friends will just need to promote such dependencies from "no-link" to "link".

[bjorn3]

Wouldn't --extern panic_abort=... --extern panic_unwind=... already be enough? I would expect the panic runtime injection code to already look at --extern.

[petrochenkov]

--extern panic_abort=... --extern panic_unwind=... exist when building std, but they are not not actually used by std so the crates are not loaded and the options are lost. (#160007 (comment) would make them loaded and not lost, just not linked).

When cargo builds some final binary depending on libstd, panic runtime crates are already indirect dependencies.
So cargo doesn't pass them with --extern, and as Kirill explained to me, cargo doesn't want to pass them with --extern at this stage, because it would contradict the cargo's internal logic and design.
Cargo wants library\std\Cargo.toml to be the only place where these dependencies are specified.

[bjorn3]

--extern panic_abort=... --extern panic_unwind=... exist when building std

I mean when building user crates.

but they are not not actually used by std so the crates are not loaded and the options are lost.

At panic_unwind is already listed as conditional dependency of std, see rustc +nightly -Zls=root $(rustc +nightly --print target-libdir)/libstd-*.rmeta:

[...]
18 panic_unwind-aff6afe86ac8b0f1 hash 362c7180b46c57babeedc50f4e9b00d0 host_hash None kind Conditional private linkage Some(RequireStatic)

panic_abort isn't though.

[Bryanskiy]

I mean when building user crates.

From my understanding, we want to avoid special cases for compiler_builtins/panic runtimes in Cargo as much as possible. But @adamgemmell might know the motivation better.

[adamgemmell]

That's indeed the reason, we'd like Cargo to only have to be aware of stable crates (plus test)

This would risk accidentally getting a crates.io crate replacing compiler-builtins or a panic runtime from the sysroot even when it isn't a direct dependency, right?

rustc does report an error when it finds more than one candidate, including if one of these candidates is in the sysroot. So for this to happen, rustc would need to want to inject a crate that cargo hasn't built as part of build-std (and doesn't exist in the sysroot), which I don't think is possible right now.

If it is possible I think you'd have to pass a panic strategy via RUSTFLAGS rather than the profile, and we do have this line in the RFC to fall back on:

In line with Cargo’s stance on not parsing the RUSTFLAGS environment variable, it will not be checked for compilation flags that would require additional crates to be built for compilation to succeed.

[bjorn3]

rustc does report an error when it finds more than one candidate, including if one of these candidates is in the sysroot.

Sysroot crates should have lower priority than user crates. Otherwise it wouldn't be possible to use libc from crates.io.

Would it work if we make rustc add a conditional dependency on both panic_unwind and panic_abort when compiling libstd rather than only on panic_unwind?

[adamgemmell]

Sorry, I'm just referring to injected dependencies, for which Kirill found that rustc threw an error when he added -Ldependency args to their search paths. That's why this patch now depends on the one for -Zimplicit-sysroot-deps.

Always building both is an option, but the Cargo team expressed a preference for not building panic_unwind if it can be avoided. Hence we don't pass the panic-unwind feature if the profile panic is set to abort. We have a future possibility to get the target's default panic strategy to avoid building it when the panic strategy is set to "unwind" (aka target default in the profile).

Sorry, I'm clearly not awake yet!

[Bryanskiy]

Sysroot crates should have lower priority than user crates. Otherwise it wouldn't be possible to use libc from crates.io.

There is no such thing as priority from the crate locator perspective:

rust/compiler/rustc_metadata/src/locator.rs

Lines 502 to 505 in dfbea5b

	match libraries.len() {
	0 => Ok(None),
	1 => Ok(Some(libraries.into_iter().next().unwrap().1)),
	_ => {

The right version of libc is chosen based on svh, I think.

Would it work if we make rustc add a conditional dependency on both panic_unwind and panic_abort when compiling libstd rather than only on panic_unwind?

When compiling libstd as rlib, we don't inject the panic runtime at all. So the panic runtime is not present in std metadata as a dependency and therefore is not loaded like regular transitive dependencies.

[Bryanskiy]

Always building both is an option

It's not about building a panic runtime. It's about rustc representation of dependencies in the crate graph. Here is what conditional dependency mean:

rust/compiler/rustc_session/src/cstore.rs

Lines 42 to 44 in ad0c9dc

	/// A dependency that is injected into the crate graph but which only
	/// sometimes needs to actually be linked in, e.g., the injected panic runtime.
	Conditional,

[bjorn3]

The right version of libc is chosen based on svh, I think.

For indirect dependencies yes. For direct dependencies the --extern libc=/path/to/rlib passed by cargo has a higher priority than the version in the sysroot. If they had identical priority, any attempt to use libc from crates.io would result in rustc emitting an error that it doesn't know if it should pick the one passed by cargo or the one in the sysroot.

When compiling libstd as rlib, we don't inject the panic runtime at all. So the panic runtime is not present in std metadata as a dependency and therefore is not loaded like regular transitive dependencies.

panic_unwind is absolutely present as conditional dependency in the libstd crate metadata:

rustc +nightly -Zls=root $(rustc +nightly --print target-libdir)/libstd-*.rmeta | rg "Dependencies|panic_unwind"
=External Dependencies=
18 panic_unwind-aff6afe86ac8b0f1 hash 362c7180b46c57babeedc50f4e9b00d0 host_hash None kind Conditional private linkage Some(RequireStatic)

The problem is that the panic runtime injection code attempts to include it as direct dependency. My suggestion is to include both panic runtimes (or one if the other hasn't been built) as conditional dependency of libstd replacing the code at

rust/compiler/rustc_metadata/src/creader.rs

Lines 956 to 1025 in ad0c9dc

	fn inject_panic_runtime(&mut self, tcx: TyCtxt<'_>, krate: &ast::Crate) {
	// If we're only compiling an rlib, then there's no need to select a
	// panic runtime, so we just skip this section entirely.
	let only_rlib = tcx.crate_types().iter().all(|ct| *ct == CrateType::Rlib);
	if only_rlib {
	info!("panic runtime injection skipped, only generating rlib");
	return;
	}
	// If we need a panic runtime, we try to find an existing one here. At
	// the same time we perform some general validation of the DAG we've got
	// going such as ensuring everything has a compatible panic strategy.
	let mut needs_panic_runtime = attr::contains_name(&krate.attrs, sym::needs_panic_runtime);
	for (_cnum, data) in self.iter_crate_data() {
	needs_panic_runtime |= data.needs_panic_runtime();
	}
	// If we just don't need a panic runtime at all, then we're done here
	// and there's nothing else to do.
	if !needs_panic_runtime {
	return;
	}
	// By this point we know that we need a panic runtime. Here we just load
	// an appropriate default runtime for our panic strategy.
	//
	// We may resolve to an already loaded crate (as the crate may not have
	// been explicitly linked prior to this), but this is fine.
	//
	// Also note that we have yet to perform validation of the crate graph
	// in terms of everyone has a compatible panic runtime format, that's
	// performed later as part of the `dependency_format` module.
	let desired_strategy = tcx.sess.panic_strategy();
	let name = match desired_strategy {
	PanicStrategy::Unwind => sym::panic_unwind,
	PanicStrategy::Abort => sym::panic_abort,
	PanicStrategy::ImmediateAbort => {
	// Immediate-aborting panics don't use a runtime.
	return;
	}
	};
	info!("panic runtime not found -- loading {}", name);
	// This has to be conditional as both panic_unwind and panic_abort may be present in the
	// crate graph at the same time. One of them will later be activated in dependency_formats.
	let Some(cnum) = self.resolve_crate(
	tcx,
	name,
	DUMMY_SP,
	CrateDepKind::Conditional,
	CrateOrigin::Injected,
	) else {
	return;
	};
	let cdata = self.get_crate_data(cnum);
	// Sanity check the loaded crate to ensure it is indeed a panic runtime
	// and the panic strategy is indeed what we thought it was.
	if !cdata.is_panic_runtime() {
	tcx.dcx().emit_err(diagnostics::CrateNotPanicRuntime { crate_name: name });
	}
	if cdata.required_panic_strategy() != Some(desired_strategy) {
	tcx.dcx().emit_err(diagnostics::NoPanicStrategy {
	crate_name: name,
	strategy: desired_strategy,
	});
	}
	self.injected_panic_runtime = Some(cnum);
	}

This way we don't have to inject it as dependency for any user of libstd. We already have code to activate exactly one panic runtime later on in the dependency format code:

rust/compiler/rustc_metadata/src/dependency_format.rs

Lines 253 to 260 in ad0c9dc

	// We've gotten this far because we're emitting some form of a final
	// artifact which means that we may need to inject dependencies of some
	// form.
	//
	// Things like panic runtimes may not have been activated quite yet, so do so here.
	activate_injected_dep(CStore::from_tcx(tcx).injected_panic_runtime(), &mut ret, &|cnum| {
	tcx.is_panic_runtime(cnum)
	});

[petrochenkov]

We already have code to activate exactly one panic runtime later on in the dependency format code

Then it seems like CrateDepKind::Conditional already mostly implements what I suggested in #160007 (comment).

[Bryanskiy]

I'm curious what should we do with compiler_builtins.

On the one hand, as Adam already said, we want to use only stable names in Cargo and therefore remove compiler_builtins from the build-std implementation:

https://github.com/rust-lang/cargo/blob/0158e40d8638a7de292b7242b1533caaf48cbe5f/src/compiler/standard_lib.rs#L31
+
https://github.com/rust-lang/cargo/blob/0158e40d8638a7de292b7242b1533caaf48cbe5f/src/compiler/standard_lib.rs#L41

On the other hand compiler_builtins is not an explicit dependency of core(and should not be) in Cargo.toml. This means that after removal, Cargo will not be able to figure out whether it needs to build compiler_builtins for -Zbuild-std=core.

[adamgemmell]

I was planning on making it a dependency of sysroot and using that crate as the sort of "global entry point" when needed (such as for -Zbuild-std-features), but not generate an actual unit to build it in Cargo

[Bryanskiy]

For panic runtimes I implemented #160007 (comment).
For compiler_builtins crate -Ldependency= paths are not supported in the case of -Zbuild-std=core, but it's blocked on the mechanism mentioned in #160007 (comment).

[petrochenkov]

r? @bjorn3

[Bryanskiy]

@rustbot ready

[bjorn3]

@bors r+

[rust-bors]

📌 Commit ab78e6a has been approved by bjorn3

It is now in the queue for this repository.

[JonathanBrouwer]

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #160278 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#160278), which was unapproved.

View changes since this unapproval

[Bryanskiy]

LINK : error LNK2001: unresolved external symbol __DllMainCRTStartup@12

My test with #![no_std]/#![no_core] is just to check crate resolution. Maybe we could pass --emit=llvm-ir to avoid running the linker?

[Bryanskiy]

implemented #160007 (comment)