Compile rustc crates with the initial-exec TLS model

[joshtriplett]

This should produce more efficient code, with fewer calls to
__tls_get_addr. The tradeoff is that libraries using it won't work with
dlopen, but that shouldn't be a problem for rustc's internal libraries.

[rust-highfive]

r? @Mark-Simulacrum

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

[joshtriplett]

cc @eddyb

[joshtriplett]

Trying this out to see if it provides a performance improvement.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 328401119c35ff10758630aafe5eb7a04925c399 with merge a612e191b94bc6e23963dbadbab26b6486a51dc3...

[eddyb]

I wonder if we should also check that the path includes compiler/rustc_ because there's some stuff on crates.io that also starts with rustc_ or rustc- and it might not be strictly a rustc_driver dependency.

[joshtriplett]

We need a much better way to handle this, yes; this was just the simplest way to test the impact of this change.

[Mark-Simulacrum]

Best bet is probably to modify builder.rs to set RUSTC_IS_COMPILING_THE_COMPILER for cargo and then read that here.

[eddyb]

This may be silly, but, if we want to exactly track which crates are using TLS, so that we can list them out in the wrapper, we could:

• have an allow-by-default lint against using #[thread_local] (including from thread_local!)
• pass -Fthread-local from the bootstrap/bin/rustc.rs wrapper by default for all crates
  • specifically using "forbid" means #[allow(thread_local)] wouldn't work so it couldn't be accidentally used
• for an explicit list of crates, we would omit -Fthread-local
  • the subset of that which ends up linked into librustc_driver-*.so, also get -Ztls-model=initial-exec

This could get confusing so we might want to make it an internal rustc::untracked_tls_crate lint that has a custom message telling you to what list to add the crate etc.

[joshtriplett]

@eddyb If we're going to go that far, I would rather just see this mechanism fixed to make the TLS model depend on the top-level artifact(s), such as "if you're building a binary use local-exec, if you're building a cdylib use global-dynamic for pub and local-dynamic for private". This shouldn't depend on the individual library crate, it should depend on the top-level crate.

[eddyb]

If we're going to go that far, I would rather just see this mechanism fixed to make the TLS model depend on the top-level artifact(s)

Oh yeah I forgot I wanted that too (should be possible, based on my experiments an rlib crate is undecided between static and dynamic TLS until it's linked into a bin crate, or a dylib crate, respectively) and went down the rabbit hole of thinking about tracking which crates define #[thread_local] statics.

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: a612e191b94bc6e23963dbadbab26b6486a51dc3 (a612e191b94bc6e23963dbadbab26b6486a51dc3)

[rust-timer]

Queued a612e191b94bc6e23963dbadbab26b6486a51dc3 with parent 1d27267, future comparison URL.

[rust-timer]

Finished benchmarking try commit (a612e191b94bc6e23963dbadbab26b6486a51dc3): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[joshtriplett]

Looks like this is definitely worth doing, and we just need to figure out the best way to capture the set of crates we can safely do it for.

Ideally, we'd apply this to every crate that isn't a proc macro or compiled into a dynamically linkable version of std. How can we most easily capture that set here?

[bjorn3]

If you tell the linker about the storage model instead of LLVM it should rewrite all TLS accesses to use that storage model I think. You could do that using a build.rs for rustc_driver. That will apply it to exactly the dylib that it should be applied to.

[Mark-Simulacrum]

Hm, so when you ask "Ideally, we'd apply this to every crate that isn't a proc macro or compiled into a dynamically linkable version of std. How can we most easily capture that set here?" -- do you mean to do that as an end-user visible change (rather than just to crates compiled by rustbuild? It seems to me that rustc shouldn't really be special here in some sense. That's probably not quite true because we have the somewhat odd case of compiling to dylibs that aren't intended to be dlopen'd (rather just linked directly), but I'm not sure that's important.

I think the best thing is probably to start out with just a change to how we compile rustc which should be somewhat easier to land and see impacts of though. To do so we probably want to set the -Z flag via rustflags if possible, something like this diff. This will set it for pretty much everything except std workspace and crates that don't get rustflags (e.g., proc macros and other build deps). Note that this includes e.g. cargo and clippy too, but I think that's the right thing?

diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs
index 707c1ff3efa..0f6a74b01ce 100644
--- a/src/bootstrap/builder.rs
+++ b/src/bootstrap/builder.rs
@@ -1114,6 +1114,10 @@ impl<'a> Builder<'a> {
             }
         }
 
+        if mode != Mode::Std {
+            rustflags.arg("-Ztls-model=initial-exec");
+        }
+
         if self.config.incremental {
             cargo.env("CARGO_INCREMENTAL", "1");
         } else {

[joshtriplett]

@Mark-Simulacrum Yeah, that looks perfect. That's exactly the set of crates that should work with this.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 15068d492e48e3aefc563baa5aa9bd6ee9223a80 with merge e33e3c8a267474be0fabf07c0eb7b6e3f7e00e37...

[bors]

☀️ Try build successful - checks-actions
Build commit: e33e3c8a267474be0fabf07c0eb7b6e3f7e00e37 (e33e3c8a267474be0fabf07c0eb7b6e3f7e00e37)

[jyn514]

Looks spurious?

------------------------------------------
stderr:
------------------------------------------
error: need to add support for DW_TAG_base_type '()' encoded with DW_ATE = 0x7, bit_size = 0
clang: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options
clang: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

------------------------------------------

[Mark-Simulacrum]

@bors retry - agreed that this seems spurious

error: need to add support for DW_TAG_base_type '()' encoded with DW_ATE = 0x7, bit_size = 0
clang: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options
clang: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine **options*

[bors]

⌛ Testing commit 0328e69 with merge e012fa10bed4afe82574679760549a76eb053951...

[bors]

💔 Test failed - checks-actions

[mati865]

The same error again on apple.
Maybe it should be enabled only on Linux?

[Aaron1011]

This is a known issue which is unrelated to this PR: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/apple-x86_64.20gha.20checks

[joshtriplett]

@bors retry

[bors]

⌛ Testing commit 0328e69 with merge 25f6938...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 25f6938 to master...

[glaubitz]

This change broke the Rust compiler on 32-bit PowerPC, see: #81334