Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bootstrap: don't apply -Ztls-model=initial-exec to proc macros #100536

Merged
merged 1 commit into from
Aug 21, 2022
Merged

bootstrap: don't apply -Ztls-model=initial-exec to proc macros #100536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/bootstrap/bin/rustc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ use std::time::Instant;

fn main() {
let args = env::args_os().skip(1).collect::<Vec<_>>();
let arg = |name| args.windows(2).find(|args| args[0] == name).and_then(|args| args[1].to_str());

// Detect whether or not we're a build script depending on whether --target
// is passed (a bit janky...)
let target = args.windows(2).find(|w| &*w[0] == "--target").and_then(|w| w[1].to_str());
let target = arg("--target");
let version = args.iter().find(|w| &**w == "-vV");

let verbose = match env::var("RUSTC_VERBOSE") {
Expand Down Expand Up @@ -59,8 +60,7 @@ fn main() {
cmd.args(&args).env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());

// Get the name of the crate we're compiling, if any.
let crate_name =
args.windows(2).find(|args| args[0] == "--crate-name").and_then(|args| args[1].to_str());
let crate_name = arg("--crate-name");

if let Some(crate_name) = crate_name {
if let Some(target) = env::var_os("RUSTC_TIME") {
Expand Down Expand Up @@ -106,6 +106,15 @@ fn main() {
{
cmd.arg("-C").arg("panic=abort");
}

// `-Ztls-model=initial-exec` must not be applied to proc-macros, see
// issue https://github.com/rust-lang/rust/issues/100530
if env::var("RUSTC_TLS_MODEL_INITIAL_EXEC").is_ok()
&& arg("--crate-type") != Some("proc-macro")
&& !matches!(crate_name, Some("proc_macro2" | "quote" | "syn" | "synstructure"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test or something else we can add that validates this list is enough? For example, I'm thinking that maybe our invocations of cargo metadata can help here?

I don't think this is something we should do in rustc.rs, probably too much overhead, but some kind of out of band check seems like a good idea to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the actual set is something like proc macros dep and contains TLS symbols, or so?

For example, rustc_macros depends on multiple other crates:

https://github.com/rust-lang/rust/blob/master/compiler/rustc_macros/Cargo.toml#L10

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the actual set is something like proc macros dep and contains TLS symbols, or so?

Yes, although a blanket include of all proc macro deps would be correct though. It may just cause a perf regression.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try it; my guess is that TLS access for these crates isn't hot (even at runtime if they're pulled in there).

Or we can go a different direction and try to test this, but it sounds like doing that isn't trivial?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think checking all proc macro dylibs shipped with rustc-dev for the R_X86_64_TPOFF64 relocation kind (on x86_64, aarch64 has a different relocation name) would suffice. R_X86_64_DTPOFF64 is the only TLS relocation kind that is used for the global dynamic tls model (the most general tls model) I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference before/after appears to be R_AARCH64_TLS_TPREL64

Is there a nice way to get a list of the proc macro dylibs in a run-make-fulldeps test? (or a better way to test for that)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure.

{
cmd.arg("-Ztls-model=initial-exec");
}
} else {
// FIXME(rust-lang/cargo#5754) we shouldn't be using special env vars
// here, but rather Cargo should know what flags to pass rustc itself.
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1850,7 +1850,7 @@ impl<'a> Builder<'a> {
// so we can't use it by default in general, but we can use it for tools
// and our own internal libraries.
if !mode.must_support_dlopen() && !target.triple.starts_with("powerpc-") {
rustflags.arg("-Ztls-model=initial-exec");
cargo.env("RUSTC_TLS_MODEL_INITIAL_EXEC", "1");
}

if self.config.incremental {
Expand Down