volatile: allow accesses to non-AM memory to trap

compiler/rustc_codegen_llvm/src/intrinsic.rs

@@ -171,6 +171,42 @@ fn call_simple_intrinsic<'ll, 'tcx>(
     ))
 }
 
+impl<'ll, 'tcx> Builder<'_, 'll, 'tcx> {
+    fn black_box(&mut self, result: PlaceRef<'tcx, &'ll Value>, span: Span) {
+        let result_val_span = [result.val.llval];
+        // We need to "use" the argument in some way LLVM can't introspect, and on
+        // targets that support it we can typically leverage inline assembly to do
+        // this. LLVM's interpretation of inline assembly is that it's, well, a black
+        // box. This isn't the greatest implementation since it probably deoptimizes
+        // more than we want, but it's so far good enough.
+        //
+        // For zero-sized types, the location pointed to by the result may be
+        // uninitialized. Do not "use" the result in this case; instead just clobber
+        // the memory.
+        let (constraint, inputs): (&str, &[_]) = if result.layout.is_zst() {
+            ("~{memory}", &[])
+        } else {
+            ("r,~{memory}", &result_val_span)
+        };
+        crate::asm::inline_asm_call(
+            self,
+            "",
+            constraint,
+            inputs,
+            self.type_void(),
+            &[],
+            true,
+            false,
+            llvm::AsmDialect::Att,
+            &[span],
+            false,
+            None,
+            None,
+        )
+        .unwrap_or_else(|| bug!("failed to generate inline asm call for `black_box`"));
+    }
+}
+
 impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
     fn codegen_intrinsic_call(
         &mut self,
@@ -356,9 +392,10 @@ impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
                 let ptr = args[0].immediate();
                 let abi_align = result_layout.align.abi;
                 let ptr_align = if name == sym::volatile_load { abi_align } else { Align::ONE };
+                let need_black_box = llvm_version < (23, 0, 0);
                 if result_layout.is_zst() {
                     return IntrinsicResult::Operand(OperandValue::ZeroSized);
-                } else if let BackendRepr::Scalar(scalar) = result_layout.backend_repr {
+                } else if let BackendRepr::Scalar(scalar) = result_layout.backend_repr && !need_black_box {
                     let load = self.volatile_load(self.type_from_scalar(scalar), ptr, ptr_align);
                     self.to_immediate_scalar(load, scalar)
                 } else {
@@ -376,6 +413,13 @@ impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
                     };
                     let llval = self.volatile_load(llty, ptr, ptr_align);
                     self.store(llval, temp.val.llval, abi_align);
+                    if need_black_box {
+                        // LLVM up until v22 considers volatile reads `willreturn` and hence can
+                        // move UB from further down up across this read. To prevent that, insert an
+                        // inline asm block that, as far as LLVM is concerned, might not terminate,
+                        // and hence should prevent such reordering.
+                        self.black_box(temp, span);
+                    }
                     return if result_place.is_none() {
                         IntrinsicResult::Operand(self.load_operand(temp).val)
                     } else {
@@ -605,41 +649,13 @@ impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
 
             sym::black_box => {
                 let result = PlaceRef {
+                    // This `unwrap` is justified by `intrinsic_call_expects_place_always` declaring
+                    // this intrinsic as always needing a return place.
                     val: result_place.unwrap(),
                     layout: result_layout,
                 };
                 args[0].val.store(self, result);
-                let result_val_span = [result.val.llval];
-                // We need to "use" the argument in some way LLVM can't introspect, and on
-                // targets that support it we can typically leverage inline assembly to do
-                // this. LLVM's interpretation of inline assembly is that it's, well, a black
-                // box. This isn't the greatest implementation since it probably deoptimizes
-                // more than we want, but it's so far good enough.
-                //
-                // For zero-sized types, the location pointed to by the result may be
-                // uninitialized. Do not "use" the result in this case; instead just clobber
-                // the memory.
-                let (constraint, inputs): (&str, &[_]) = if result.layout.is_zst() {
-                    ("~{memory}", &[])
-                } else {
-                    ("r,~{memory}", &result_val_span)
-                };
-                crate::asm::inline_asm_call(
-                    self,
-                    "",
-                    constraint,
-                    inputs,
-                    self.type_void(),
-                    &[],
-                    true,
-                    false,
-                    llvm::AsmDialect::Att,
-                    &[span],
-                    false,
-                    None,
-                    None,
-                )
-                .unwrap_or_else(|| bug!("failed to generate inline asm call for `black_box`"));
+                self.black_box(result, span);
 
                 // We have copied the value to `result` already.
                 return IntrinsicResult::WroteIntoPlace;

library/core/src/ptr/mod.rs

@@ -2078,10 +2078,10 @@ pub const unsafe fn write_unaligned<T>(dst: *mut T, src: T) {
 ///   semantics associated to their manipulation, and cannot be used as general purpose memory.
 ///   Here, any address value is possible, including 0 and [`usize::MAX`], so long as the semantics
 ///   of such a read are well-defined by the target hardware. The provenance of the pointer is
-///   irrelevant, and it can be created with [`without_provenance`]. The access must not trap. It
-///   can cause side-effects, but those must not affect Rust-allocated memory in any way. This
-///   access is still not considered [atomic], and as such it cannot be used for inter-thread
-///   synchronization.
+///   irrelevant, and it can be created with [`without_provenance`]. The access is allowed to trap,
+///   which must immediately abort the process. It can also cause other side-effects, but those
+///   must not affect Rust-allocated memory in any way. This access is still not considered
+///   [atomic], and as such it cannot be used for inter-thread synchronization.
 ///
 /// Note that volatile memory operations where T is a zero-sized type are noops and may be ignored.
 ///
@@ -2116,9 +2116,8 @@ pub const unsafe fn write_unaligned<T>(dst: *mut T, src: T) {
 /// Behavior is undefined if any of the following conditions are violated:
 ///
 /// * `src` must be either [valid] for reads, or `T` must be a ZST, or `src` must point to memory
-///   outside of all Rust allocations and reading from that memory must:
-///   - not trap, and
-///   - not cause any memory inside a Rust allocation to be modified.
+///   outside of all Rust allocations and reading from that memory must not cause any memory inside
+///   a Rust allocation to be modified.
 ///
 /// * `src` must be properly aligned.
 ///
@@ -2184,10 +2183,10 @@ pub const unsafe fn read_volatile<T>(src: *const T) -> T {
 ///   semantics associated to their manipulation, and cannot be used as general purpose memory.
 ///   Here, any address value is possible, including 0 and [`usize::MAX`], so long as the semantics
 ///   of such a write are well-defined by the target hardware. The provenance of the pointer is
-///   irrelevant, and it can be created with [`without_provenance`]. The access must not trap. It
-///   can cause side-effects, but those must not affect Rust-allocated memory in any way. This
-///   access is still not considered [atomic], and as such it cannot be used for inter-thread
-///   synchronization.
+///   irrelevant, and it can be created with [`without_provenance`]. The access is allowed to trap,
+///   which must immediately abort the process. It can also cause side-effects, but those must not
+///   affect Rust-allocated memory in any way. This access is still not considered [atomic], and as
+///   such it cannot be used for inter-thread synchronization.
 ///
 /// Note that volatile memory operations on zero-sized types (e.g., if a zero-sized type is passed
 /// to `write_volatile`) are noops and may be ignored.
@@ -2223,9 +2222,8 @@ pub const unsafe fn read_volatile<T>(src: *const T) -> T {
 /// Behavior is undefined if any of the following conditions are violated:
 ///
 /// * `dst` must be either [valid] for writes, or `T` must be a ZST, or `dst` must point to memory
-///   outside of all Rust allocations and writing to that memory must:
-///   - not trap, and
-///   - not cause any memory inside a Rust allocation to be modified.
+///   outside of all Rust allocations and writing to that memory must not cause any memory inside a
+///   Rust allocation to be modified.
 ///
 /// * `dst` must be properly aligned.
 ///

tests/assembly-llvm/stack-protector/stack-protector-heuristics-effect-2.rs

@@ -8,6 +8,7 @@
 //@ [strong] compile-flags: -Z stack-protector=strong
 //@ [none] compile-flags: -Z stack-protector=none
 //@ compile-flags: -C opt-level=2 -Z merge-functions=disabled
+//@ min-llvm-version: 23
 
 #![crate_type = "lib"]
 #![allow(internal_features)]

tests/codegen-llvm/intrinsics/volatile.rs

@@ -1,4 +1,5 @@
 //@ compile-flags: -C no-prepopulate-passes
+//@ min-llvm-version: 23
 
 #![crate_type = "lib"]
 #![feature(core_intrinsics)]