-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
bootstrap: don't apply -Ztls-model=initial-exec
to proc macros
#100536
Conversation
@bors try |
⌛ Trying commit 7ecc8c0e6b3cbebb9568e373def599e8932280d5 with merge b8faa934fa7fcac224925252dcd9884abf34a688... |
If you have |
☀️ Try build successful - checks-actions |
Ah the try build produced x86 linux assets 😅 Pushed a commit that I believe should make it use aarch64 temporarily |
@bors try |
⌛ Trying commit 7b941bb9f11cc75b463d5255528b1d9b179890a0 with merge ad757cfdaf165871015454fd840ff1400125fd79... |
☀️ Try build successful - checks-actions |
No such luck:
|
Right, |
Is that what you meant? My comment is lacking, I don't know enough about proc macros to explain why it's in |
Yes @bors try |
⌛ Trying commit de81d0661c0278a95fde22aa0a5b9f0eac1bb3e9 with merge 5618bbd2f9cff002b70877eb50368991de5e4ab7... |
☀️ Try build successful - checks-actions |
There's progress, before the following crates would give the error:
On 5618bbd2f9cff002b70877eb50368991de5e4ab7 there are fewer:
|
What is the exact error in those cases? |
(same error for all of them) |
chalk_derive depends on quote, syn and synstructure. I don't know which one is responsible, but adding all of them as exceptions just like proc-macro2 would work I think. |
@bors try |
⌛ Trying commit fd0327d0194cb9a6150f814ea84ec9973faee050 with merge d65f8b66932795e8cc8e3f099902dc2ad9813af5... |
☀️ Try build successful - checks-actions |
fd0327d
to
23abd59
Compare
-Ztls-model=initial-exec
to non-host crates-Ztls-model=initial-exec
to proc macros
Nice, that one compiled successfully! Thanks for walking me through it rebased + removed the ci try change |
LGTM, but given that I suggested this fix, I'm going to defer final review to someone else. r? @Mark-Simulacrum You also reviewed #78201. Would you mind taking a look? |
// 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")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure.
☀️ Test successful - checks-actions |
Finished benchmarking commit (f2858b5): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Potentially fixes #100530
r? @bjorn3