-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Implement #[rustc_align_static(N)] on statics
#146178
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
Conversation
The Miri subtree was changed
cc @rust-lang/miri
Some changes occurred in compiler/rustc_attr_parsing
Some changes occurred in compiler/rustc_passes/src/check_attr.rs
Some changes occurred to the CTFE / Miri interpreter
cc @rust-lang/miri
Some changes occurred in compiler/rustc_codegen_gcc
Some changes occurred to the CTFE machinery
we can't have two unstable features use the same unstable attribute
What do you mean by "use"? How does a feature "use" an attribute?
All rustc_ attributes are unstable anyway...
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.
@RalfJung this is where the attribute gets declared, and the feature name on this line (static_align in this case) must be unique
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.
That's very odd, why does it have to be unique? That's not how features work anywhere else in the compiler, AFAIK.
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.
ah, sorry, what I meant by unique is that an attribute must be tied to one unstable feature. #[rustc_align] is already tied to fn_align, so it can't also be used for static_align.
So, one unstable feature can "contain" multiple attributes, but an attribute always is tied to only one unstable 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.
Ah, by "attribute used for feature" you mean "attribute is gated by feature". That's a rather confusing and non-standard way of saying things.^^
So what you want is to have a separate feature gate for aligning functions vs aligning statics, and the attribute registration macro isn't flexible enough to allow that. rustc_align is for functions only, despite its name sounding more general.
Since these are all temporary names anyway, I guess it doesn't matter too much, but when it comes to stabilization we very much might need the ability to stabilize an attribute in some positions while leaving it unstable elsewhere. Otherwise we have no way of unstably extending where an existing stable attribute can be used. So this limitation of attribute feature gating seems like it could become problematic. Cc @jdonszelmann
This needs tests for:
- Thread locals (
#[thread_local]andthread_local!) externblock statics
This comment has been minimized.
This comment has been minimized.
extern
Is there any interesting behavior to test here? (I'll add a test for the attribute being valid).
thread_local!
hmm, the thread_local! macro actually applies the attribute to a constant
rust/library/std/src/sys/thread_local/native/mod.rs
Lines 107 to 110 in fd75a9c
190ad9c to
07f4381
Compare
#[rustc_align_static(N)] on statics (削除ここまで)#[rustc_align_static(N)] on statics (追記ここまで)
This comment has been minimized.
This comment has been minimized.
07f4381 to
d66847e
Compare
This comment has been minimized.
This comment has been minimized.
Here is an implementation for thread_local!: Jules-Bertholet@ad038e6
d66847e to
6f0825e
Compare
This comment has been minimized.
This comment has been minimized.
That's odd, the miri tests pass for me locally. Do those somehow cache the standard library even when you don't pass --keep-stage?
389dad1 to
602e757
Compare
Miri has its own standard library build but it should usually be recompiled automatically when anything changed.
The error might only occur after merging the branch with latest master (which happens implicitly in CI).
which happens implicitly in CI
Doesn’t that apply only to Bors CI? (E.g. Bors try or Bors r+)
No, Github merges the PR with the master branch for PR CI.
Pretty terrible IMO in terms of reproducibility, but well, it is what it is.
So would a miri sync fix this (or at least make it reproducible locally)?
If my theory is correct, rebasing over the latest master commit should let you reproduce the issue.
Oh, I just noticed the target. The failure is on x86_64-pc-windows-gnu. The new thread-local macro logic is probably just broken on Windows.
./x test miri --target x86_64-pc-windows-gnu -- align should reproduce this.
EDIT: And indeed, it does.
This comment has been minimized.
This comment has been minimized.
The new thread-local macro logic is probably just broken on Windows.
Yes, it is, I’ll fix it
Yes, it is, I’ll fix it
Here you go: Jules-Bertholet@e0cc8eb
(It’s not complete yet; I still need to add cfg_attr support, and a few more tests.)
given how complicated the thread_local! stuff is getting, maybe that should be its own PR as a follow-up?
cfg_attrsupport
Done: Jules-Bertholet@ed2f8cd
maybe that should be its own PR as a follow-up?
Sure, if that’s what you prefer.
602e757 to
428ef41
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.
I removed the thread_local! stuff for now, because it got quite complex and will need detailed review from T-libs I'd assume. This PR now has just the changes that are relevant to lang/compiler. Assuming CI makes it, I think this is ready, with the understanding that there will be a follow-up to add support for thread_local!.
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 MIR interpreter parts look fine, but I don't have time to dig into the entire thing, sorry.
r? @jdonszelmann maybe because there's a bunch of attribute logic?
jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.
We need a different attribute than `rustc_align` because unstable attributes are tied to their feature (we can't have two unstable features use the same unstable attribute). Otherwise this uses all of the same infrastructure as `#[rustc_align]`.
4fd600d to
af834f3
Compare
Tracking issue: #146177
We need a different attribute than
rustc_alignbecause unstable attributes are tied to their feature (we can't have two unstable features use the same unstable attribute). Otherwise this uses all of the same infrastructure as#[rustc_align].r? @traviscross