Rollup merge of rust-lang#132745 - RalfJung:pointee-info-inside-enum, r=DianQK

pointee_info_at: fix logic for recursing into enums

Fixes rust-lang#131834

The logic in `pointee_info_at` was likely written at a time when the null pointer optimization was the *only* enum layout optimization -- and as `Variant::Multiple` kept getting expanded, nobody noticed that the logic is now unsound.

The job of this function is to figure out whether there is a dereferenceable-or-null and aligned pointer at a given offset inside a type. So when we recurse into a multi-variant enum, we better make sure that all the other enum variants must be null! This is the part that was forgotten, and this PR adds it.

The reason this didn't explode in many ways so far is that our references only have 1 niche value (null), so it's not possible on stable to have a multi-variant enum with a dereferenceable pointer and other enum variants that are not null. But with `rustc_layout_scalar_valid_range` attributes one can force such a layout, and if `@the8472's` work on alignment niches ever lands, that will make this possible on stable.

Author: matthiaskrgr

compiler/rustc_abi/src/lib.rs

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

compiler/rustc_middle/src/ty/layout.rs

@@ -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
+                        }
                     }
+                    Variants::Multiple { .. } => None,
                     _ => Some(this),
                 };
 

compiler/rustc_target/src/callconv/mod.rs

@@ -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>,
 }

tests/codegen/function-arguments.rs

@@ -1,5 +1,6 @@
 //@ compile-flags: -O -C no-prepopulate-passes
 #![crate_type = "lib"]
+#![feature(rustc_attrs)]
 #![feature(dyn_star)]
 #![feature(allocator_api)]
 
@@ -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]