Skip to content
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

Adjust the alignment when passing a niche as a pointer #131739

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions compiler/rustc_abi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1489,6 +1489,29 @@ pub enum Variants<FieldIdx: Idx, VariantIdx: Idx> {
},
}

impl<FieldIdx: Idx, VariantIdx: Idx> Variants<FieldIdx, VariantIdx> {
// Returns niches for the discriminants.
pub fn niches(&self) -> Option<impl Iterator<Item = u128> + '_> {
match self {
&Variants::Multiple {
tag_encoding:
TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start },
ref variants,
..
} => Some(variants.iter_enumerated().filter_map(move |(variant_idx, variant)| {
if untagged_variant == variant_idx || variant.abi.is_uninhabited() {
None
} else {
let niche = ((variant_idx.index() - niche_variants.start().index()) as u128)
.wrapping_add(niche_start);
Comment on lines +1505 to +1506
Copy link
Member

Choose a reason for hiding this comment

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

This logic is wrong; it needs to take into account the size of the field that holds the niche, and wrap around accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I only want to calculate alignment, I think the actual value of niche is not necessary. I will add some comment.

Copy link
Member

Choose a reason for hiding this comment

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

This function is called niches and offers itself as a general-purpose function. If it's only correct for alignment, then it should be a private helper function inside the alignment logic.

Some(niche)
}
})),
_ => None,
}
}
}

// NOTE: This struct is generic over the VariantIdx for rust-analyzer usage.
#[derive(PartialEq, Eq, Hash, Clone, Debug)]
#[cfg_attr(feature = "nightly", derive(HashStable_Generic))]
Expand Down
23 changes: 17 additions & 6 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ where
}

_ => {
let mut data_variant = match this.variants {
let (mut data_variant, niche_align) = match this.variants {
// Within the discriminant field, only the niche itself is
// always initialized, so we only check for a pointer at its
// offset.
Expand All @@ -1016,9 +1016,16 @@ where
tag_field,
..
} if this.fields.offset(tag_field) == offset => {
Some(this.for_variant(cx, untagged_variant))
// When a non-null niche is passed in, we might pass an unaligned value.
// Calculates a maximum alignment that matches all niches.
let niche_align = this.variants.niches().map(|niches| {
niches.fold(Align::MAX, |align, niche| {
align.restrict_for_offset(Size::from_bytes(niche))
})
});
(Some(this.for_variant(cx, untagged_variant)), niche_align)
Copy link
Member

@RalfJung RalfJung Nov 8, 2024

Choose a reason for hiding this comment

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

This logic is still not right, we need something like #132745 or else there will be wrong dereferenceable flags.

The point of this function is to compute the pointee info, but with a type like MultipleAlign8 in your example, there is no pointee. More precisely, we are computing "pointee or null" info; null is the one and only special case that is allowed here.

}
_ => Some(this),
_ => (Some(this), None),
};

if let Some(variant) = data_variant {
Expand Down Expand Up @@ -1055,20 +1062,24 @@ where
}
}

// Fixup info for the first field of a `Box`. Recursive traversal will have found
// the raw pointer, so size and align are set to the boxed type, but `pointee.safe`
// will still be `None`.
if let Some(ref mut pointee) = result {
if offset.bytes() == 0
&& let Some(boxed_ty) = this.ty.boxed_ty()
{
// Fixup info for the first field of a `Box`. Recursive traversal will have found
// the raw pointer, so size and align are set to the boxed type, but `pointee.safe`
// will still be `None`.
debug_assert!(pointee.safe.is_none());
let optimize = tcx.sess.opts.optimize != OptLevel::No;
pointee.safe = Some(PointerKind::Box {
unpin: optimize && boxed_ty.is_unpin(tcx, cx.param_env()),
global: this.ty.is_box_global(tcx),
});
}
if let Some(align) = niche_align {
// Takes the minimum alignment of the pointer and niches to ensure that the alignment is legal.
pointee.align = pointee.align.min(align);
}
}

result
Expand Down
113 changes: 113 additions & 0 deletions tests/codegen/enum/enum-ptr-align-niche.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
//@ compile-flags: -Cno-prepopulate-passes -O
//@ only-64bit (I don't care about alignment under different bits)

// Testing different niches updates to the corresponding alignment.

#![crate_type = "lib"]
#![feature(rustc_attrs)]
#![feature(never_type)]

#[rustc_layout_scalar_valid_range_start(0)]
#[rustc_layout_scalar_valid_range_end(0x7fff)]
struct RestrictedAddress_0_0x7fff(&'static i64);

#[rustc_layout_scalar_valid_range_start(1)]
#[rustc_layout_scalar_valid_range_end(0x7fff)]
struct RestrictedAddress_1_0x7fff(&'static i64);

#[rustc_layout_scalar_valid_range_start(0)]
#[rustc_layout_scalar_valid_range_end(0xf000)]
struct RestrictedAddress_0_0xf000(&'static i64);

enum MultipleAlign8 {
Untag(RestrictedAddress_1_0x7fff),
Niche_32768,
Uninhabited_1(!),
Uninhabited_2(!),
Uninhabited_3(!),
Uninhabited_4(!),
Uninhabited_5(!),
Uninhabited_6(!),
Uninhabited_7(!),
Niche_32776,
}

// CHECK-LABEL: @multiple_niches_align_8
// CHECK-SAME: align 8 {{.*}}%a)
#[no_mangle]
#[inline(never)]
fn multiple_niches_align_8(a: MultipleAlign8) {}

// CHECK-LABEL: @call_multiple_niches_align_8
#[no_mangle]
fn call_multiple_niches_align_8() {
// CHECK: call void @multiple_niches_align_8(ptr {{.*}}align 8 {{.*}}(i64 32768 to ptr)
multiple_niches_align_8(MultipleAlign8::Niche_32768);
// CHECK: call void @multiple_niches_align_8(ptr {{.*}}align 8 {{.*}}(i64 32776 to ptr)
Copy link
Member

Choose a reason for hiding this comment

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

Specifically, among the attributes hidden by the {{.*}} is dereferenceable, and that is unsound.

We currently don't have a way to represent a pointer that is aligned but maybe non-dereferenceable. However, I am not convinced that is something worth having -- it will be extremely rare; IMO we should have evidence of real-world benefit for such a construction before we spend the effort of implementing and maintaining that in the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

Correction -- we do have a way to represent a non-dereferenceable pointer, and it is by setting size to 0. This is what we already do for Box.

But the rest of my point still stands.

multiple_niches_align_8(MultipleAlign8::Niche_32776);
}

enum MultipleAlign2 {
Untag(RestrictedAddress_1_0x7fff),
Niche_32768,
Uninhabited_1(!),
Niche_32770,
}

// CHECK-LABEL: @multiple_niches_align_2
// CHECK-SAME: align 2 {{.*}}%a)
#[no_mangle]
#[inline(never)]
fn multiple_niches_align_2(a: MultipleAlign2) {}

// CHECK-LABEL: @call_multiple_niches_align_2
#[no_mangle]
fn call_multiple_niches_align_2() {
// CHECK: call void @multiple_niches_align_2(ptr {{.*}}align 2 {{.*}}(i64 32768 to ptr)
multiple_niches_align_2(MultipleAlign2::Niche_32768);
// CHECK: call void @multiple_niches_align_2(ptr {{.*}}align 2 {{.*}}(i64 32770 to ptr)
multiple_niches_align_2(MultipleAlign2::Niche_32770);
}

enum SingleAlign8 {
Untag(RestrictedAddress_0_0x7fff),
Niche_32768,
}

// CHECK-LABEL: @single_niche_align_8
// CHECK-SAME: align 8 {{.*}}%a)
#[no_mangle]
#[inline(never)]
fn single_niche_align_8(a: SingleAlign8) {}

// CHECK-LABEL: @call_single_niche_align_8
#[no_mangle]
fn call_single_niche_align_8() {
// CHECK: call void @single_niche_align_8(ptr {{.*}}align 8 {{.*}}(i64 32768 to ptr)
single_niche_align_8(SingleAlign8::Niche_32768);
}

enum SingleAlign1 {
Untag(RestrictedAddress_0_0xf000),
Niche_61441,
}

// CHECK-LABEL: @single_niche_align_1
// CHECK-SAME: align 1 {{.*}}%a)
#[no_mangle]
#[inline(never)]
fn single_niche_align_1(a: SingleAlign1) {}

// CHECK-LABEL: @call_single_niche_align_1
#[no_mangle]
fn call_single_niche_align_1() {
// CHECK: call void @single_niche_align_1(ptr {{.*}}align 1 {{.*}}(i64 61441 to ptr)
single_niche_align_1(SingleAlign1::Niche_61441);
}

// Check that we only apply the new alignment on enum.

// CHECK-LABEL: @restricted_address
// CHECK-SAME: align 8 {{.*}}%a)
#[no_mangle]
fn restricted_address(a: RestrictedAddress_0_0x7fff) {}
Loading