Rollup merge of #156281 - nikic:nofree, r=RalfJung

Emit nofree attribute

Treat the semantics of pointee.size as "dereferenceable-at-point" and always specify the size. Instead, use a separate NoFree attribute to determine whether dereferenceability extends to the whole function.

Then in the LLVM backend, only actually emit dereferenceable if nofree is also set, as dereferenceable currently implies nofree.

In addition, explicitly emit the nofree attribute, which will help when LLVM switches dereferenceable to be at-point.

Relevant recent LLVM PR: llvm/llvm-project#195658

Author: JonathanBrouwer

compiler/rustc_codegen_llvm/src/abi.rs

@@ -38,12 +38,16 @@ trait ArgAttributesExt {
 const ABI_AFFECTING_ATTRIBUTES: [(ArgAttribute, llvm::AttributeKind); 1] =
     [(ArgAttribute::InReg, llvm::AttributeKind::InReg)];
 
-const OPTIMIZATION_ATTRIBUTES: [(ArgAttribute, llvm::AttributeKind); 5] = [
+const OPTIMIZATION_ATTRIBUTES: [(ArgAttribute, llvm::AttributeKind); 6] = [
     (ArgAttribute::NoAlias, llvm::AttributeKind::NoAlias),
     (ArgAttribute::NonNull, llvm::AttributeKind::NonNull),
     (ArgAttribute::ReadOnly, llvm::AttributeKind::ReadOnly),
     (ArgAttribute::NoUndef, llvm::AttributeKind::NoUndef),
     (ArgAttribute::Writable, llvm::AttributeKind::Writable),
+    // Our internal NoFree attribute still allows deallocation of zero-size allocations. However,
+    // these don't render any bytes non-dereferenceable, so it's still fine to apply LLVM NoFree
+    // for them.
+    (ArgAttribute::NoFree, llvm::AttributeKind::NoFree),
 ];
 
 const CAPTURES_ATTRIBUTES: [(ArgAttribute, llvm::AttributeKind); 3] = [
@@ -75,7 +79,9 @@ fn get_attrs<'ll>(this: &ArgAttributes, cx: &CodegenCx<'ll, '_>) -> SmallVec<[&'
     // Only apply remaining attributes when optimizing
     if cx.sess().opts.optimize != config::OptLevel::No {
         let deref = this.pointee_size.bytes();
-        if deref != 0 {
+        // dereferenceable in LLVM currently implies nofree, so only emit dereferenceable if nofree
+        // is also set.
+        if deref != 0 && regular.contains(ArgAttribute::NoFree) {
             if regular.contains(ArgAttribute::NonNull) {
                 attrs.push(llvm::CreateDereferenceableAttr(cx.llcx, deref));
             } else {

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -318,6 +318,7 @@ pub(crate) enum AttributeKind {
     SanitizeRealtimeNonblocking = 47,
     SanitizeRealtimeBlocking = 48,
     Convergent = 49,
+    NoFree = 50,
 }
 
 /// LLVMIntPredicate

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

@@ -388,6 +388,7 @@ enum class LLVMRustAttributeKind {
   SanitizeRealtimeNonblocking = 47,
   SanitizeRealtimeBlocking = 48,
   Convergent = 49,
+  NoFree = 50,
 };
 
 static Attribute::AttrKind fromRust(LLVMRustAttributeKind Kind) {
@@ -486,6 +487,8 @@ static Attribute::AttrKind fromRust(LLVMRustAttributeKind Kind) {
     return Attribute::SanitizeRealtimeBlocking;
   case LLVMRustAttributeKind::Convergent:
     return Attribute::Convergent;
+  case LLVMRustAttributeKind::NoFree:
+    return Attribute::NoFree;
   }
   report_fatal_error("bad LLVMRustAttributeKind");
 }

compiler/rustc_middle/src/ty/layout.rs

@@ -1038,33 +1038,19 @@ where
             }
             ty::Ref(_, ty, mt) if offset.bytes() == 0 => {
                 tcx.layout_of(typing_env.as_query_input(ty)).ok().map(|layout| {
-                    let (size, kind);
-                    match mt {
+                    let kind = match mt {
                         hir::Mutability::Not => {
                             let frozen = optimize && ty.is_freeze(tcx, typing_env);
-
-                            // Non-frozen shared references are not necessarily dereferenceable for the entire duration of the function
-                            // (see <https://github.com/rust-lang/rust/pull/98017>)
-                            // (if we had "dereferenceable on entry", we could support this)
-                            size = if frozen { layout.size } else { Size::ZERO };
-
-                            kind = PointerKind::SharedRef { frozen };
+                            PointerKind::SharedRef { frozen }
                         }
                         hir::Mutability::Mut => {
                             let unpin = optimize
                                 && ty.is_unpin(tcx, typing_env)
                                 && ty.is_unsafe_unpin(tcx, typing_env);
-
-                            // Mutable references to potentially self-referential types are not
-                            // necessarily dereferenceable for the entire duration of the function
-                            // (see <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>)
-                            // (if we had "dereferenceable on entry", we could support this)
-                            size = if unpin { layout.size } else { Size::ZERO };
-
-                            kind = PointerKind::MutableRef { unpin };
+                            PointerKind::MutableRef { unpin }
                         }
                     };
-                    PointeeInfo { safe: Some(kind), size, align: layout.align.abi }
+                    PointeeInfo { safe: Some(kind), size: layout.size, align: layout.align.abi }
                 })
             }
 
@@ -1080,12 +1066,7 @@ where
                             && pointee.is_unsafe_unpin(tcx, typing_env),
                         global: this.ty.is_box_global(tcx),
                     }),
-
-                    // `Box` are not necessarily dereferenceable for the entire duration of the function as
-                    // they can be deallocated at any time.
-                    // (if we had "dereferenceable on entry", we could support this)
-                    size: Size::ZERO,
-
+                    size: layout.size,
                     align: layout.align.abi,
                 })
             }

compiler/rustc_target/src/callconv/mod.rs

@@ -123,6 +123,11 @@ mod attr_impl {
             const InReg    = 1 << 6;
             const NoUndef  = 1 << 7;
             const Writable = 1 << 8;
+            /// It is UB for this pointer or any pointer derived from it to be used for
+            /// deallocation (except for zero-sized deallocation) while the function is
+            /// executing. Only valid on arguments (including return values that are passed
+            /// indirectly as arguments).
+            const NoFree   = 1 << 9;
         }
     }
     rustc_data_structures::external_bitflags_debug! { ArgAttribute }
@@ -144,9 +149,8 @@ pub enum ArgExtension {
 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_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).
+    /// If the pointer is not null, the minimum dereferenceable size of the pointee, at the time of
+    /// function entry (for arguments) or function return (for return values).
     pub pointee_size: Size,
     /// The minimum alignment of the pointee, if any.
     pub pointee_align: Option<Align>,
@@ -414,7 +418,8 @@ impl<'a, Ty> ArgAbi<'a, Ty> {
             .set(ArgAttribute::NoAlias)
             .set(ArgAttribute::CapturesAddress)
             .set(ArgAttribute::NonNull)
-            .set(ArgAttribute::NoUndef);
+            .set(ArgAttribute::NoUndef)
+            .set(ArgAttribute::NoFree);
         attrs.pointee_size = layout.size;
         attrs.pointee_align = Some(layout.align.abi);
 

compiler/rustc_ty_utils/src/abi.rs

@@ -356,13 +356,7 @@ fn arg_attrs_for_rust_scalar<'tcx>(
                 Some(pointee.align.min(cx.tcx().sess.target.max_reliable_alignment()));
         }
 
-        // LLVM dereferenceable attribute has unclear semantics on the return type,
-        // they seem to be "dereferenceable until the end of the program", which is
-        // generally, not valid for references. See
-        // <https://rust-lang.zulipchat.com/#narrow/channel/136281-t-opsem/topic/LLVM.20dereferenceable.20on.20return.20type/with/563001493>
-        if !is_return {
-            attrs.pointee_size = pointee.size;
-        };
+        attrs.pointee_size = pointee.size;
 
         if let Some(kind) = pointee.safe {
             // The aliasing rules for `Box<T>` are still not decided, but currently we emit
@@ -407,6 +401,26 @@ fn arg_attrs_for_rust_scalar<'tcx>(
                 }
             }
 
+            // NoFree is not valid on return values. If it were, it would mean something like
+            // "will not be freed until the end of the program", which is generally not valid for
+            // references.
+            let no_free = !is_return
+                && match kind {
+                    // Non-frozen shared references are not necessarily dereferenceable for the
+                    // entire duration of the function
+                    // (see <https://github.com/rust-lang/rust/pull/98017>).
+                    PointerKind::SharedRef { frozen } => frozen,
+                    // Mutable references to potentially self-referential types are not necessarily
+                    // dereferenceable for the entire duration of the function
+                    // (see <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>).
+                    PointerKind::MutableRef { unpin } => unpin,
+                    // Box may be deallocated during execution of the function.
+                    PointerKind::Box { .. } => false,
+                };
+            if no_free {
+                attrs.set(ArgAttribute::NoFree);
+            }
+
             if matches!(kind, PointerKind::SharedRef { frozen: true }) && !is_return {
                 attrs.set(ArgAttribute::ReadOnly);
                 attrs.set(ArgAttribute::CapturesReadOnly);
@@ -423,11 +437,18 @@ fn fn_abi_sanity_check<'tcx>(
     fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
     spec_abi: ExternAbi,
 ) {
+    fn fn_arg_attrs_sanity_check(attrs: &ArgAttributes, is_ret: bool) {
+        if attrs.regular.contains(ArgAttribute::NoFree) {
+            assert!(!is_ret, "NoFree not valid on return values");
+        }
+    }
+
     fn fn_arg_sanity_check<'tcx>(
         cx: &LayoutCx<'tcx>,
         fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
         spec_abi: ExternAbi,
         arg: &ArgAbi<'tcx, Ty<'tcx>>,
+        is_ret: bool,
     ) {
         let tcx = cx.tcx();
 
@@ -452,7 +473,7 @@ fn fn_abi_sanity_check<'tcx>(
             PassMode::Ignore => {
                 assert!(arg.layout.is_zst());
             }
-            PassMode::Direct(_) => {
+            PassMode::Direct(attrs) => {
                 // Here the Rust type is used to determine the actual ABI, so we have to be very
                 // careful. Scalar/Vector is fine, since backends will generally use
                 // `layout.backend_repr` and ignore everything else. We should just reject
@@ -482,28 +503,33 @@ fn fn_abi_sanity_check<'tcx>(
                         );
                     }
                 }
+                fn_arg_attrs_sanity_check(attrs, is_ret);
             }
-            PassMode::Pair(_, _) => {
+            PassMode::Pair(attrs1, attrs2) => {
                 // Similar to `Direct`, we need to make sure that backends use `layout.backend_repr`
                 // and ignore the rest of the layout.
                 assert!(
                     matches!(arg.layout.backend_repr, BackendRepr::ScalarPair(..)),
                     "PassMode::Pair for type {}",
                     arg.layout.ty
                 );
+                fn_arg_attrs_sanity_check(attrs1, is_ret);
+                fn_arg_attrs_sanity_check(attrs2, is_ret);
             }
             PassMode::Cast { .. } => {
                 // `Cast` means "transmute to `CastType`"; that only makes sense for sized types.
                 assert!(arg.layout.is_sized());
             }
-            PassMode::Indirect { meta_attrs: None, .. } => {
+            PassMode::Indirect { meta_attrs: None, attrs, .. } => {
                 // No metadata, must be sized.
                 // Conceptually, unsized arguments must be copied around, which requires dynamically
                 // determining their size, which we cannot do without metadata. Consult
                 // t-opsem before removing this check.
                 assert!(arg.layout.is_sized());
+                // Indirect returns are arguments from an ABI perspective.
+                fn_arg_attrs_sanity_check(attrs, false);
             }
-            PassMode::Indirect { meta_attrs: Some(_), on_stack, .. } => {
+            PassMode::Indirect { meta_attrs: Some(meta_attrs), attrs, on_stack } => {
                 // With metadata. Must be unsized and not on the stack.
                 assert!(arg.layout.is_unsized() && !on_stack);
                 // Also, must not be `extern` type.
@@ -515,14 +541,17 @@ fn fn_abi_sanity_check<'tcx>(
                     // t-opsem before removing this check.
                     panic!("unsized arguments must not be `extern` types");
                 }
+                // Indirect returns are arguments from an ABI perspective.
+                fn_arg_attrs_sanity_check(attrs, false);
+                fn_arg_attrs_sanity_check(meta_attrs, false);
             }
         }
     }
 
     for arg in fn_abi.args.iter() {
-        fn_arg_sanity_check(cx, fn_abi, spec_abi, arg);
+        fn_arg_sanity_check(cx, fn_abi, spec_abi, arg, false);
     }
-    fn_arg_sanity_check(cx, fn_abi, spec_abi, &fn_abi.ret);
+    fn_arg_sanity_check(cx, fn_abi, spec_abi, &fn_abi.ret, true);
 }
 
 #[tracing::instrument(

tests/codegen-llvm/addr-of-mutate.rs

@@ -5,7 +5,7 @@
 // Test for the absence of `readonly` on the argument when it is mutated via `&raw const`.
 // See <https://github.com/rust-lang/rust/issues/111502>.
 
-// CHECK: i8 @foo(ptr{{( dead_on_return)?}} noalias noundef align 1{{( captures\(address\))?}}{{( dead_on_return)?}} dereferenceable(128) %x)
+// CHECK: i8 @foo(ptr{{( dead_on_return)?}} noalias nofree noundef align 1{{( captures\(address\))?}}{{( dead_on_return)?}} dereferenceable(128) %x)
 #[no_mangle]
 pub fn foo(x: [u8; 128]) -> u8 {
     let ptr = core::ptr::addr_of!(x).cast_mut();
@@ -15,7 +15,7 @@ pub fn foo(x: [u8; 128]) -> u8 {
     x[0]
 }
 
-// CHECK: i1 @second(ptr{{( dead_on_return)?}} noalias noundef align {{[0-9]+}}{{( captures\(address\))?}}{{( dead_on_return)?}} dereferenceable({{[0-9]+}}) %a_ptr_and_b)
+// CHECK: i1 @second(ptr{{( dead_on_return)?}} noalias nofree noundef align {{[0-9]+}}{{( captures\(address\))?}}{{( dead_on_return)?}} dereferenceable({{[0-9]+}}) %a_ptr_and_b)
 #[no_mangle]
 pub unsafe fn second(a_ptr_and_b: (*mut (i32, bool), (i64, bool))) -> bool {
     let b_bool_ptr = core::ptr::addr_of!(a_ptr_and_b.1.1).cast_mut();
@@ -24,7 +24,7 @@ pub unsafe fn second(a_ptr_and_b: (*mut (i32, bool), (i64, bool))) -> bool {
 }
 
 // If going through a deref (and there are no other mutating accesses), then `readonly` is fine.
-// CHECK: i1 @third(ptr{{( dead_on_return)?}} noalias noundef readonly align {{[0-9]+}}{{( captures\(none\))?}}{{( dead_on_return)?}} dereferenceable({{[0-9]+}}) %a_ptr_and_b)
+// CHECK: i1 @third(ptr{{( dead_on_return)?}} noalias nofree noundef readonly align {{[0-9]+}}{{( captures\(none\))?}}{{( dead_on_return)?}} dereferenceable({{[0-9]+}}) %a_ptr_and_b)
 #[no_mangle]
 pub unsafe fn third(a_ptr_and_b: (*mut (i32, bool), (i64, bool))) -> bool {
     let b_bool_ptr = core::ptr::addr_of!((*a_ptr_and_b.0).1).cast_mut();

tests/codegen-llvm/drop-in-place-noalias.rs

@@ -7,7 +7,7 @@
 
 use std::marker::PhantomPinned;
 
-// CHECK: define internal void @{{.*}}core{{.*}}ptr{{.*}}drop_in_place{{.*}}StructUnpin{{.*}}(ptr noalias noundef align 4 dereferenceable(12) %{{.+}})
+// CHECK: define internal void @{{.*}}core{{.*}}ptr{{.*}}drop_in_place{{.*}}StructUnpin{{.*}}(ptr noalias nofree noundef align 4 dereferenceable(12) %{{.+}})
 
 // CHECK: define internal void @{{.*}}core{{.*}}ptr{{.*}}drop_in_place{{.*}}StructNotUnpin{{.*}}(ptr noundef nonnull align 4 %{{.+}})