Skip to content

peel_transparent_wrappers only works on non-1ZST #162000

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

Open
RalfJung wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −13
Open

peel_transparent_wrappers only works on non-1ZST #162000

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
19 changes: 15 additions & 4 deletions compiler/rustc_abi/src/layout/ty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,15 +259,25 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
where
Ty: TyAbiInterface<'a, C> + Copy,
{
let base = self.peel_transparent_wrappers(cx);
// FIXME: Peeling the wrappers above would not work correctly if we are a 1-ZST. So we make
// `#[rustc_pass_indirectly_in_non_rustic_abis]` a NOP on 1-ZST. In the future,
// `non_1zst_field` should become `non_trivial_abi_field` and
// `#[rustc_pass_indirectly_in_non_rustic_abis]` should make a type have non-trivial ABI.
if self.is_1zst() {
return false;
}

@RalfJung RalfJung Aug 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This basically says that the #[rustc_pass_indirectly_in_non_rustic_abis] attribute is a NOP on 1-ZST. I think that's reasonable. A better approach would be to have a non_trivial_abi_field and declare the attribute to make the ABI non-trivial, but before #157973 lands I don't think it makes sense to implement anything like that and anyway the attribute is perma-unstable.

But I am not sure where to document it, the attribute seems wholly undocumented?

View changes since the review

@beetrees beetrees Aug 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think leaving a FIXME for making this non_trivial_abi_field after #157973 is merged (since it's already in proposed FCP) is fine for now. I think #[rustc_pass_indirectly_in_non_rustic_abis] is primarily documented on TyAndLayout::pass_indirectly_in_non_rustic_abis and in the VaList implementation at the moment.

@folkertdev folkertdev Aug 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At the moment it's just an implementation detail of c-variadic functions.

@RalfJung RalfJung Aug 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At the moment it's just an implementation detail of c-variadic functions.

I know that's the intent, but even then it needs docs that say that. ;)
But we're generally not doing great regarding documentation of rustc attributes.

I can add a FIXME.


let base = self.peel_transparent_wrappers_from_non_1zst(cx);
Ty::is_pass_indirectly_in_non_rustic_abis_flag_set(base)
}

/// Recursively peel away transparent wrappers, returning the inner value.
/// Will not peel anything if `self` is a 1-ZST! Callers need to either check
/// that the result is not a 1-ZST, or have separate logic for that.
Comment on lines +275 to +276

@folkertdev folkertdev Aug 31, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we could debug_assert! to check?

View changes since the review

@RalfJung RalfJung Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not really, if you look at the callsites I have patched, some will call this on inputs that may be 1-ZST -- but they then check that the output is not a 1-ZST and that's why their use is correct.

///
/// The return value is not `repr(transparent)` and/or does
/// not have a non-1zst field.
pub fn peel_transparent_wrappers<C>(mut self, cx: &C) -> Self
pub fn peel_transparent_wrappers_from_non_1zst<C>(mut self, cx: &C) -> Self
where
Ty: TyAbiInterface<'a, C> + Copy,
{
Expand Down Expand Up @@ -308,12 +318,13 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
where
Ty: TyAbiInterface<'a, C> + Copy,
{
let complex = self.peel_transparent_wrappers(cx);
// We're checking for scalar repr below which excludes 1-ZST.
let complex = self.peel_transparent_wrappers_from_non_1zst(cx);
if !Ty::is_complex_number_lang_item(complex, cx) {
return None;
}

let part = complex.field(cx, 0).peel_transparent_wrappers(cx);
let part = complex.field(cx, 0).peel_transparent_wrappers_from_non_1zst(cx);

if let BackendRepr::Scalar(scalar) = part.backend_repr {
// Only Complex<{ float }> and Complex<{ integer }> have special layout.
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
bx: &mut Bx,
layout: TyAndLayout<'tcx>,
) -> Self {
if layout.peel_transparent_wrappers(bx).deref().is_scalable_vector() {
// Scalable vector are never 1-ZST. FIXME: is that correct?
if layout.peel_transparent_wrappers_from_non_1zst(bx).deref().is_scalable_vector() {
Comment on lines +115 to +116

@RalfJung RalfJung Aug 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@davidtwco is it correct that scalable vectors are never 1ZST?
What does is_sized even say for them?

View changes since the review

@folkertdev folkertdev Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

David is only back at the end of the week, do you want to wait for that?

@RalfJung RalfJung Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not in a rush with this PR, seems fine to wait.

I have a whole pile of PRs waiting for David it seems. ;)

Self::alloca_scalable(bx, layout)
} else {
Self::alloca_size(bx, layout.size, layout)
Expand Down Expand Up @@ -159,7 +160,8 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
layout: TyAndLayout<'tcx>,
) -> Self {
PlaceValue::new_sized(
bx.alloca_with_ty(layout.peel_transparent_wrappers(bx)),
// FIXME why is this peeling at all? And why is it redoing the work the caller just did?
bx.alloca_with_ty(layout.peel_transparent_wrappers_from_non_1zst(bx)),
layout.align.abi,
)
.with_type(layout)
Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_codegen_ssa/src/traits/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,12 +528,13 @@ pub trait BuilderMethods<'a, 'tcx>:
let tt = tt.add_indirection();
let fnc_tree = FncTree { args: vec![tt.clone(), tt], ret: TypeTree::new() };
let bytes = self.const_usize(layout.size.bytes());
let bytes = if layout.peel_transparent_wrappers(self).ty.is_scalable_vector() {
let vscale = self.vscale(self.type_i64());
self.mul(vscale, bytes)
} else {
bytes
};
let bytes =
if layout.peel_transparent_wrappers_from_non_1zst(self).ty.is_scalable_vector() {
let vscale = self.vscale(self.type_i64());
self.mul(vscale, bytes)
} else {
bytes
Comment on lines +532 to +536

@RalfJung RalfJung Aug 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This code seems quite fragile, why does it need to special-case scalable vectors and how do we know there aren't any other special cases that have been forgotten here?

But that's pre-existing... still, quite concerning IMO. Cc @davidtwco @ZuseZ4

View changes since the review

};
self.memcpy(dst.llval, dst.align, src.llval, src.align, bytes, flags, Some(fnc_tree));
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/hir_ty_lowering/cmse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ fn is_valid_cmse_output_layout<'tcx>(cx: LayoutCx<'tcx>, layout: TyAndLayout<'tc

// Accept (transparently wrapped) scalar 64-bit primitives.
matches!(
layout.peel_transparent_wrappers(&cx).ty.kind(),
layout.peel_transparent_wrappers_from_non_1zst(&cx).ty.kind(),

@RalfJung RalfJung Aug 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

HIR uses layout?!? That's a pretty stark layering violation, isn't it?

View changes since the review

@folkertdev folkertdev Aug 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With CMSE we can ensure that this works out. We need to know how many registers will be used by the signature to error (even in cargo check builds) when the arguments don't fit in the available space.

@RalfJung RalfJung Aug 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't doubt that it works, but it's a hack in terms of compiler layering. It's one more step towards an unmaintainable spaghetti mess.

@folkertdev folkertdev Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doing this post-mono massively degrades the usability of this feature. This code has been reviewed by several members of the types team over several PRs.

@RalfJung RalfJung Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree this should be done pre-mono. But this is in the HIR, it should use HIR types, not Layout.

Anyway this is more a little unpleasant surprise I found while writing this PR. Not really something I expect to be resolved in this PR. It seems to work but IMO it should be cleaned up before it turns into technical debt.

ty::Int(ty::IntTy::I64) | ty::Uint(ty::UintTy::U64) | ty::Float(ty::FloatTy::F64)
)
}
Expand Down
Loading