Skip to content

pointee_info_at: fix logic for recursing into enums #132745

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

Merged
merged 1 commit into from
Nov 9, 2024
Merged
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
16 changes: 12 additions & 4 deletions compiler/rustc_abi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1743,15 +1743,23 @@ pub enum PointerKind {
Box { unpin: bool, global: bool },
}

/// Note that this information is advisory only, and backends are free to ignore it.
/// It can only be used to encode potential optimizations, but no critical information.
/// Encodes extra information we have about a pointer.
/// Note that this information is advisory only, and backends are free to ignore it:
/// if the information is wrong, that can cause UB, but if the information is absent,
/// that must always be okay.
#[derive(Copy, Clone, Debug)]
pub struct PointeeInfo {
pub size: Size,
pub align: Align,
/// If this is `None`, then this is a raw pointer, so size and alignment are not guaranteed to
/// be reliable.
pub safe: Option<PointerKind>,
/// If `safe` is `Some`, then the pointer is either null or dereferenceable for this many bytes.
/// On a function argument, "dereferenceable" here means "dereferenceable for the entire duration
/// of this function call", i.e. it is UB for the memory that this pointer points to to be freed
/// while this function is still running.
/// The size can be zero if the pointer is not dereferenceable.
pub size: Size,
/// If `safe` is `Some`, then the pointer is aligned as indicated.
pub align: Align,
}

impl<FieldIdx: Idx, VariantIdx: Idx> LayoutData<FieldIdx, VariantIdx> {
Expand Down
38 changes: 27 additions & 11 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1011,25 +1011,41 @@ where
}

_ => {
let mut data_variant = match this.variants {
let mut data_variant = match &this.variants {
// Within the discriminant field, only the niche itself is
// always initialized, so we only check for a pointer at its
// offset.
//
// If the niche is a pointer, it's either valid (according
// to its type), or null (which the niche field's scalar
// validity range encodes). This allows using
// `dereferenceable_or_null` for e.g., `Option<&T>`, and
// this will continue to work as long as we don't start
// using more niches than just null (e.g., the first page of
// the address space, or unaligned pointers).
// Our goal here is to check whether this represents a
// "dereferenceable or null" pointer, so we need to ensure
// that there is only one other variant, and it must be null.
// Below, we will then check whether the pointer is indeed
// dereferenceable.
Variants::Multiple {
tag_encoding: TagEncoding::Niche { untagged_variant, .. },
tag_encoding:
TagEncoding::Niche { untagged_variant, niche_variants, niche_start },
tag_field,
variants,
..
} if this.fields.offset(tag_field) == offset => {
Some(this.for_variant(cx, untagged_variant))
} if variants.len() == 2 && this.fields.offset(*tag_field) == offset => {
let tagged_variant = if untagged_variant.as_u32() == 0 {
VariantIdx::from_u32(1)
} else {
VariantIdx::from_u32(0)
};
assert_eq!(tagged_variant, *niche_variants.start());
if *niche_start == 0 {
// The other variant is encoded as "null", so we can recurse searching for
// a pointer here. This relies on the fact that the codegen backend
// only adds "dereferenceable" if there's also a "nonnull" proof,
// and that null is aligned for all alignments so it's okay to forward
// the pointer's alignment.
Some(this.for_variant(cx, *untagged_variant))
} else {
None
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, this align will always be 1. I adjusted align at #131739, and I think it's sound.

Copy link
Member

Choose a reason for hiding this comment

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

How about adding a dereferenceable field to PointeeInfo? Or something similar?

Copy link
Member Author

@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.

No, align here can be larger than 1. This is checked by the existing test:

// CHECK: @option_borrow(ptr noalias noundef readonly align 4 dereferenceable_or_null(4) %_x)
#[no_mangle]
pub fn option_borrow(_x: Option<&i32>) {}

// CHECK: @option_borrow_mut(ptr noalias noundef align 4 dereferenceable_or_null(4) %_x)
#[no_mangle]
pub fn option_borrow_mut(_x: Option<&mut i32>) {}

Notice the align 4. Not sure why you are saying align will be 1 here.

I don't see why adding a "dereferenceable" field would help. The entire PointeeInfo struct is the "derefernceable-or-null" flag; a non-dereferenceable type must return None. The "or-null" part is easy to deal with later since we can use the scalar range to check whether null is allowed or not; it would be redundant to also compute that here.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps my comment was a bit misplaced. See https://github.com/rust-lang/rust/pull/131739/files#diff-a93329b2efc9dabaf147bfdb806fb52501ed7889eb08e26e1a2714bb43862531R22-R39. In this case, we need to drop dereferenceable, but we can keep align 8.

Copy link
Member Author

@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.

I don't think that's worth the effort. We shouldn't implement complicated logic like that unless it actually has real-world benefit.

In particular, I think it is actually impossible for stable code to even potentially benefit from this.

}
}
Variants::Multiple { .. } => None,
_ => Some(this),
};

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_target/src/callconv/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ pub struct ArgAttributes {
pub regular: ArgAttribute,
pub arg_ext: ArgExtension,
/// The minimum size of the pointee, guaranteed to be valid for the duration of the whole call
/// (corresponding to LLVM's dereferenceable and dereferenceable_or_null attributes).
/// (corresponding to LLVM's dereferenceable_or_null attributes, i.e., it is okay for this to be
/// set on a null pointer, but all non-null pointers must be dereferenceable).
pub pointee_size: Size,
pub pointee_align: Option<Align>,
}
Expand Down
24 changes: 20 additions & 4 deletions tests/codegen/function-arguments.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//@ compile-flags: -O -C no-prepopulate-passes
#![crate_type = "lib"]
#![feature(rustc_attrs)]
#![feature(dyn_star)]
#![feature(allocator_api)]

Expand Down Expand Up @@ -143,13 +144,28 @@ pub fn indirect_struct(_: S) {}
#[no_mangle]
pub fn borrowed_struct(_: &S) {}

// CHECK: @option_borrow(ptr noalias noundef readonly align 4 dereferenceable_or_null(4) %x)
// CHECK: @option_borrow(ptr noalias noundef readonly align 4 dereferenceable_or_null(4) %_x)
#[no_mangle]
pub fn option_borrow(x: Option<&i32>) {}
pub fn option_borrow(_x: Option<&i32>) {}

// CHECK: @option_borrow_mut(ptr noalias noundef align 4 dereferenceable_or_null(4) %x)
// CHECK: @option_borrow_mut(ptr noalias noundef align 4 dereferenceable_or_null(4) %_x)
#[no_mangle]
pub fn option_borrow_mut(x: Option<&mut i32>) {}
pub fn option_borrow_mut(_x: Option<&mut i32>) {}

// Function that must NOT have `dereferenceable` or `align`.
#[rustc_layout_scalar_valid_range_start(16)]
pub struct RestrictedAddress(&'static i16);
enum E {
A(RestrictedAddress),
B,
C,
}
// If the `nonnull` ever goes missing, you might have to tweak the
// scalar_valid_range on `RestrictedAddress` to get it back. You
// might even have to add a `rustc_layout_scalar_valid_range_end`.
// CHECK: @nonnull_and_nondereferenceable(ptr noundef nonnull %_x)
#[no_mangle]
pub fn nonnull_and_nondereferenceable(_x: E) {}

// CHECK: @raw_struct(ptr noundef %_1)
#[no_mangle]
Expand Down
Loading