-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Implement #[rustc_align_static(N)]
on static
s
#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
base: master
Are you sure you want to change the base?
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 |
What do you mean by "use"? How does a feature "use" an attribute? |
@@ -621,6 +621,7 @@ pub static BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[ | |||
), | |||
// FIXME(#82232, #143834): temporarily renamed to mitigate `#[align]` nameres ambiguity | |||
gated!(rustc_align, Normal, template!(List: &["alignment"]), DuplicatesOk, EncodeCrossCrate::No, fn_align, experimental!(rustc_align)), | |||
gated!(rustc_align_static, Normal, template!(List: &["alignment"]), DuplicatesOk, EncodeCrossCrate::No, static_align, experimental!(rustc_align_static)), |
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:
|
This comment has been minimized.
This comment has been minimized.
Is there any interesting behavior to test here? (I'll add a test for the attribute being valid).
hmm, the 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 static
s#[rustc_align_static(N)]
on static
s
This comment has been minimized.
This comment has been minimized.
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]`.
07f4381
to
d66847e
Compare
let alloc = Allocation::try_new(layout.size, layout.align.abi, AllocInit::Uninit, ())?; | ||
// Take alignment from attributes into account here so that there is one source | ||
// of truth for the alignment of this allocation. | ||
let align = match ecx.tcx.codegen_fn_attrs(static_def_id).alignment { | ||
Some(align_from_attribute) => Ord::max(align_from_attribute, layout.align.abi), | ||
None => layout.align.abi, | ||
}; |
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 we have the information here to retrieve the alignment. Or perhaps you still prefer passing it in as an argument?
The final line of this function
ecx.machine.static_root_ids = Some((alloc_id, static_def_id));
assert!(ecx.memory.alloc_map.insert(alloc_id, (MemoryKind::Stack, alloc)).is_none());
interp_ok(ecx.ptr_to_mplace(Pointer::from(alloc_id).into(), layout))
uses the passed-in layout
, which is not over-aligned. is that OK?
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.
hmm, what should we do here?
rust/compiler/rustc_middle/src/mir/interpret/mod.rs
Lines 366 to 391 in 0d6a806
GlobalAlloc::Static(def_id) => { | |
let DefKind::Static { nested, .. } = tcx.def_kind(def_id) else { | |
bug!("GlobalAlloc::Static is not a static") | |
}; | |
if nested { | |
// Nested anonymous statics are untyped, so let's get their | |
// size and alignment from the allocation itself. This always | |
// succeeds, as the query is fed at DefId creation time, so no | |
// evaluation actually occurs. | |
let alloc = tcx.eval_static_initializer(def_id).unwrap(); | |
(alloc.0.size(), alloc.0.align) | |
} else { | |
// Use size and align of the type for everything else. We need | |
// to do that to | |
// * avoid cycle errors in case of self-referential statics, | |
// * be able to get information on extern statics. | |
let ty = tcx | |
.type_of(def_id) | |
.no_bound_vars() | |
.expect("statics should not have generic parameters"); | |
let layout = tcx.layout_of(typing_env.as_query_input(ty)).unwrap(); | |
assert!(layout.is_sized()); | |
(layout.size, layout.align.abi) | |
} | |
} |
Obviously the type does not take the additional alignment into account. The comment also suggests that unconditionally using the allocation is incorrect. So do we check the attributes again here (thus kind of sacrificing the one source of truth)?
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.
Nested statics can't carry an attribute so I don't think you have to do anything 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.
we have the information here to retrieve the alignment.
Nice!
uses the passed-in layout, which is not over-aligned. is that OK?
Yes, that just determines the type of the place, which is not changed by the over-alignment.
This comment has been minimized.
This comment has been minimized.
Here is an implementation for |
i.e. no longer have each backend do that, which is error-prone
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 |
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). |
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. EDIT: And indeed, it does. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Yes, it is, I’ll fix it |
Here you go: Jules-Bertholet@e0cc8eb (It’s not complete yet; I still need to add |
Tracking issue: #146177
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]
.r? @traviscross