-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Support #[rustc_align_static]
inside thread_local!
#146281
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
base: master
Are you sure you want to change the base?
Support #[rustc_align_static]
inside thread_local!
#146281
Conversation
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri The Miri subtree was changed cc @rust-lang/miri |
67db3cb
to
dd3475c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@Jules-Bertholet would you mind rebasing? #146178 merged |
Reminder, once the PR becomes ready for a review, use |
dd3475c
to
9cdfc77
Compare
This comment has been minimized.
This comment has been minimized.
([$($prev_align_attrs:tt)*] [$($prev_other_attrs:tt)*]; | ||
#[doc $($doc_rhs_1:tt)*] #[doc $($doc_rhs_2:tt)*] #[doc $($doc_rhs_3:tt)*] #[doc $($doc_rhs_4:tt)*] | ||
#[doc $($doc_rhs_5:tt)*] #[doc $($doc_rhs_6:tt)*] #[doc $($doc_rhs_7:tt)*] #[doc $($doc_rhs_8:tt)*] | ||
$($rest:tt)*) => ( |
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 don't think we could use a regular proc macro (IIRC, std can't use proc macros), we'd have to make it a part of rustc.
#![crate_type = "lib"] | ||
|
||
thread_local! { | ||
//~^ ERROR the `#[rustc_align_static]` attribute is an experimental feature |
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.
Are we (implicitly) stabilizing support for cfg_attr
on the static in this PR? Can we test other forms of the attribute to verify we didn't allow other attributes (e.g., no_mangle or used(linker), etc.) and what the diagnostics are like?
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.
Are we (implicitly) stabilizing support for cfg_attr on the static in this PR?
It was already stable.
Can we test other forms of the attribute to verify we didn't allow other attributes (e.g., no_mangle or used(linker), etc.) and what the diagnostics are like?
I’ve added some tests for this. The error spans are not great, but that is a pre-existing issue (#146567), this PR doesn’t make it any worse than before.
Another pre-existing issue I just discovered is that unsafe(no_mangle)
is erroneously accepted (but has no effect). I am looking into that now… (Edit: #146557)
475178f
to
b7fa550
Compare
I realized that I had neglected to support |
This is a T-libs issue but to me this macro looks very scary and I'm not sure it needs to be a decl macro. Maybe a compiler provided macro like |
if self.state.get() == State::Alive { | ||
self.value.get() as *const T | ||
} else { | ||
self.initialize(i, f) |
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.
This will hurt codegen for thread locals that otherwise had option optimization (e.g., if you store NonNull pointer today, this would optimize to a single load + test, rather than loading from two different memory locations).
I'm not sure how much that matters in practice, but it seems unfortunate to pay the cost for all users. Ideally, we'd only do so for the case where alignof(Option.Some.0) != alignof(Option), but I'm not sure if that's possible to check in macros / const-fn to optimize this.
Does this code get used only in the no_threads case? Or does it end up exported for the actually threaded cases too?
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.
Only the no_threads
case. I suppose we could have two impls depending on whether any rustc_align_static
is present or not…
(@align) => (1), | ||
|
||
// `rustc_align_static` attributes are present, | ||
// translate them into a `const` block that computes the alignment |
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 wonder if this could be simplified by attaching the attributes to some static and then checking the alignment of that static (via align_of_val)? That way we presumably get most of the error checking from rustc and we don't nee to manually compute the alignment or deal with cfgs.
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.
We would have to translate rustc_align_static(…)
into repr(align(…))
, so just as much complexity as before.
We do rely on rustc for error checking, via static DUMMY
below.
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.
Perhaps this was meant as a comment on the struct idea below? For this comment using a static shouldn't require any translation - presumably align_of_val(&DUMMY) gives us the alignment we need?
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.
With regards to structs, I wonder if we should teach rustc to support rustc_align_static on fields/structs as a forever unstable extension just to simplify the impl here....
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.
presumably align_of_val(&DUMMY) gives us the alignment we need?
No, that would give the alignment of the type of DUMMY
, which is 1.
// Note that to prevent an infinite loop we reset it back to null right | ||
// before we return from the destructor ourselves. | ||
abort_on_dtor_unwind(|| { | ||
let ptr = unsafe { Box::from_raw(ptr as *mut Value<T>) }; |
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.
Similarly to the above thought: is it plausible that for aligned thread locals we could generate a repr(C) struct that bumped the alignment of our stored value to what we need and then we'd alter our code to &struct.0
when actually returning pointers out to userspace? Presumably that would avoid the majority of the internal changes and avoid storing alignments into memory (wastefully).
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 realized that we can just use a const generic. Switched to that
This comment has been minimized.
This comment has been minimized.
f12ddd7
to
fca42ed
Compare
This comment has been minimized.
This comment has been minimized.
I have no idea why the Miri floating point test failed in #146281 (comment), or why fca42ed fixes it. @RalfJung do you know what is going on here? |
Well, it's a randomized test, so it always has a small chance of failing. |
fca42ed
to
8d9ddfd
Compare
☔ The latest upstream changes (presumably #146931) made this pull request unmergeable. Please resolve the merge conflicts. |
8d9ddfd
to
ad8a3b4
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
fe4eac4
to
3434c1b
Compare
3434c1b
to
ca8d8b3
Compare
This comment has been minimized.
This comment has been minimized.
ca8d8b3
to
ff97f2a
Compare
ff97f2a
to
a4e87e9
Compare
This comment has been minimized.
This comment has been minimized.
52fd64d
to
4d32b9a
Compare
Tracking issue: #146177
This increases the amount of recursion the macro performs (once per attribute in addition to the previous once per item), making it easier to hit the recursion limit. I’ve added workarounds to limit the impact in the case of long doc comments, but this still needs a crater run just in case.
r? libs
@rustbot label A-attributes A-macros A-thread-locals F-static_align T-libs