"Handle" calls to upstream monomorphizations in compiler_builtins

[saethlin]

This is pretty cooked, but I think it works.

compiler-builtins has a long-standing problem that at link time, its rlib cannot contain any calls to core. And yet, in codegen we love inserting calls to symbols in core, generally from various panic entrypoints.

I intend this PR to attack that problem as completely as possible. When we generate a function call, we now check if we are generating a function call from compiler_builtins and whether the callee is a function which was not lowered in the current crate, meaning we will have to link to it.

If those conditions are met, actually generating the call is asking for a linker error. So we don't. If the callee diverges, we lower to an abort with the same behavior as core::intrinsics::abort. If the callee does not diverge, we produce an error. This means that compiler-builtins can contain panics, but they'll SIGILL instead of panicking. I made non-diverging calls a compile error because I'm guessing that they'd mostly get into compiler-builtins by someone making a mistake while working on the crate, and compile errors are better than linker errors. We could turn such calls into aborts as well if that's preferred.

[rustbot]

r? @pnkfelix

rustbot has assigned @pnkfelix.
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

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[bjorn3]

cg_clif will need this change too.

[saethlin]

If I apply this change to cranelift, how would I verify that it works?

[bjorn3]

https://github.com/rust-lang/rustc_codegen_cranelift/#building-and-testing-with-changes-in-rustc-code has instructions.

[bjorn3]

And the new code would likely be somewhere around https://github.com/rust-lang/rustc_codegen_cranelift/blob/4cf4ffc6ba514f171b3f52d1c731063e4fc45be3/src/abi/mod.rs#L374

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[Amanieu]

I like this approach since it removes the need for existing hacks like disabling debug assertions and disabling overflow checks.

[saethlin]

That's also why I like it! For the record, @dpaoliello deserves some credit for saying this:

If we can't rely on libcore, would it be possible to implement a "mini panic" in compiler-builtins instead?

Which taken literally is not possible, but it's a very good line of thinking that led me to this.

[pnkfelix]

Hmm.

Is there any way to test the behavior being introduced here as part of our CI test suite?

It seems like testing it would require either:

1. substituting in a custom "buggy" version of compiler_builtins solely for testing purposes. Which does not sound feasible, or at least not a good use of effort if its solely to support testing this PR. Or,

2. maybe there is a way to make a custom variant of libcore and link to that, and have that custom libcore provide items that would normally return true for should_codegen_locally but now they return false for this hypothetical custom libcore..., or

3. add a new item to compiler_builtins that is itself designed to exercise the behavior being added here, but otherwise serves no purpose.

... so after some reflection, I am guessing that Options 1 and 2 are impossible or non-desirable; option 3 might be doable but isn't really attractive from an overall development trajectory standpoint.

And instead, we just say "we do not intend to regression test this PR as part of our test suite. One can test it in a local fashion by making local changes to compiler-builtins that expose the behavior in question."

Does that all sound right?

[pnkfelix]

This looks fine to me as it stands, assuming that I'm correct about it not being reasonable to try to test it in CI.

But I also am inferring that it should not land without the corresponding changes to cg_clif

@rustbot author

[bjorn3]

But I also am inferring that it should not land without the corresponding changes to cg_clif

This has been added to the PR since I made that comment.

[pnkfelix]

This has been added to the PR since I made that comment.

Oh, whoops! Thanks!

[pnkfelix]

@bors r+

[bors]

📌 Commit 5f4f252 has been approved by pnkfelix

It is now in the queue for this repository.

[saethlin]

I agree that we should have some tests for this. I'll open an issue with some ideas.

[saethlin]

@bors rollup=never

[rust-timer]

Finished benchmarking commit (b3df0d7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.5%	[5.7%, 7.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.525s -> 669.004s (-0.08%)
Artifact size: 314.91 MiB -> 314.95 MiB (0.02%)

[RalfJung]

Why is this extra check needed, given the check during codegen? How can it even fail?

[saethlin]

There are compiler changes in the commit that I only discovered are necessary by adding this test. Normally we always build compielr_builtins with optimizations enabled. So those changes would probably have been build failures in the -nopt jobs, but that's such a late failure. Here's that commit: 2f6fb23 And those changes are:

Not only is -Copt-level=0 not optimized, it enables -Zshare-generics by default. So compiler_builtins needs to opt out of this, because sharing the generics of core also makes it inherit panics.

The check in codegen is awfully fiddly to get right. I forgot to apply the "abort on panic" logic inside terminate_block; compiler_builtins uses some rustc_nounwind functions, and that attribute makes us insert UnwindTerminate blocks just in case there is an unwind. Those blocks tend to be optimized out, but if optimizations are off, they become calls into core.

[RalfJung]

Okay so this kind of sanity-checks the codegen-time checks? Makes sense, thanks. (That may be worth a comment in the code though so it's also clear to a future reader.)

[Amanieu]

While trying to solve the CI issues in compiler_builtins, I noticed that -C embed-bitcode=yes causes references to panicking functions in core to be emitted.

That's not what's causing the currently issues in CI (x86_64-apple-darwin seems to still be generating references to panic), but it might be worth fixing.

[saethlin]

While trying to solve the CI issues in compiler_builtins, I noticed that -C embed-bitcode=yes causes references to panicking functions in core to be emitted.

I just noticed this as well. I'm starting to look into it.

[saethlin]

Ah no what I was looking at is another case of #118770, at least this time it's just me causing trouble for me. You probably don't care to support -Zcross-crate-inline-threshold=yes.

I'll extract a reproducer of the embed-bitcode issue from the compiler-builtins CI.

[petrochenkov]

@saethlin
The added run-make test fails with this message for me:

   Compiling core v0.0.0 (E:\msys64\home\we\rust\build\x86_64-pc-windows-gnu\stage1\lib\rustlib\src\rust\library\core)
   Compiling compiler_builtins v0.1.109
error: couldn't create a temp dir: Access is denied. (os error 5) at path "C:\\Windows\\rustcKMWsUo"

error: could not compile `compiler_builtins` (build script) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: couldn't create a temp dir: Access is denied. (os error 5) at path "C:\\Windows\\rustclRyY6G"

error: could not compile `core` (lib) due to 1 previous error
thread 'main' panicked at E:\msys64\home\we\rust\tests\run-make\compiler-builtins\rmake.rs:71:5:
assertion failed: status.success()
stack backtrace:
   0: rust_begin_unwind
             at E:\msys64\home\we\rust\library\std\src\panicking.rs:652:5
   1: core::panicking::panic_fmt
             at E:\msys64\home\we\rust\library\core\src\panicking.rs:72:14
   2: core::panicking::panic
             at E:\msys64\home\we\rust\library\core\src\panicking.rs:146:5
   3: rmake::main
   4: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Looks like the test suite uses a wrong temporary directory on Windows in some cases.
Or MSYS2 is to blame, or the fact that my Rust repo is on disk E:, I haven't investigated yet.
cc @ChrisDenton

[jieyouxu]

Looks like the test suite uses a wrong temporary directory on Windows in some cases. Or MSYS2 is to blame, or the fact that my Rust repo is on disk E:, I haven't investigated yet.

@petrochenkov Judging from the error message, it's emitted from codegen:

rust/compiler/rustc_codegen_ssa/src/back/link.rs

Lines 99 to 102 in 22f5bdc

	let tmpdir = TempFileBuilder::new()
	.prefix("rustc")
	.tempdir()
	.unwrap_or_else(|error| sess.dcx().emit_fatal(errors::CreateTempDir { error }));

Where somehow env::temp_dir looks messed up -- I think this might be a msys2 weird interaction with $TMPDIR problem -- because apparently compiletest uses and modifies $TMPDIR (even though that env var influences what env::temp_dir returns!)

[ChrisDenton]

Oh it's using env_clear, which is always risky on Windows because there's at least a few environment variables that are assumed to always be set.

In this case either TMP or TEMP needs to be set otherwise getting the temp dir will fallback to USERPROFILE and if that's not found either then the windows directory is used as a last resort. Which will be unwritable.

[ChrisDenton]

I think the helper lib could have a env_reset that preserves some necessary environment variables rather than fully clearing everything.

[jieyouxu]

I think the helper lib could have a env_reset that preserves some necessary environment variables rather than fully clearing everything.

I'll work on a patch for this

[jieyouxu]

I think the helper lib could have a env_reset that preserves some necessary environment variables rather than fully clearing everything.

@ChrisDenton I opened #125426, for now I only preserve TMP and TEMP on Windows, TMPDIR on non-Windows. Let me know if you know of any more "necessary" env vars.

[RalfJung]

In this case either TMP or TEMP needs to be set otherwise getting the temp dir will fallback to USERPROFILE and if that's not found either then the windows directory is used as a last resort. Which will be unwritable.

Isn't that a terrible fallback? If that's happening in our implementation, it seems better to error out than provide an unwriteable directory.

[ChrisDenton]

It's not our fallback. See https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppath2w

[RalfJung]

Ah, unfortunate. I wonder if that's worth working around on our side. But temp_dir doesn't even return a Result so there's no good way we can indicate failure either...

[jieyouxu]

Ah, unfortunate. I wonder if that's worth working around on our side. But temp_dir doesn't even return a Result so there's no good way we can indicate failure either...

We could still describe this in the docs, because it is very surprising...

EDIT: filed #125439 to not forget.