Auto merge of rust-lang#122053 - erikdesjardins:alloca, r=nikic

Stop using LLVM struct types for alloca

The alloca type has no semantic meaning, only the size (and alignment, but we specify it explicitly) matter. Using `[N x i8]` is a more direct way to specify that we want `N` bytes, and avoids relying on LLVM's struct layout. It is likely that a future LLVM version will change to an untyped alloca representation.

Split out from rust-lang#121577.

r? `@ghost`

compiler/rustc_codegen_gcc/src/builder.rs

@@ -897,26 +897,14 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> {
         self.gcc_checked_binop(oop, typ, lhs, rhs)
     }
 
-    fn alloca(&mut self, ty: Type<'gcc>, align: Align) -> RValue<'gcc> {
-        // FIXME(antoyo): this check that we don't call get_aligned() a second time on a type.
-        // Ideally, we shouldn't need to do this check.
-        let aligned_type = if ty == self.cx.u128_type || ty == self.cx.i128_type {
-            ty
-        } else {
-            ty.get_aligned(align.bytes())
-        };
+    fn alloca(&mut self, size: Size, align: Align) -> RValue<'gcc> {
+        let ty = self.cx.type_array(self.cx.type_i8(), size.bytes()).get_aligned(align.bytes());
         // TODO(antoyo): It might be better to return a LValue, but fixing the rustc API is non-trivial.
         self.stack_var_count.set(self.stack_var_count.get() + 1);
-        self.current_func()
-            .new_local(
-                self.location,
-                aligned_type,
-                &format!("stack_var_{}", self.stack_var_count.get()),
-            )
-            .get_address(self.location)
+        self.current_func().new_local(None, ty, &format!("stack_var_{}", self.stack_var_count.get())).get_address(None)
     }
 
-    fn byte_array_alloca(&mut self, _len: RValue<'gcc>, _align: Align) -> RValue<'gcc> {
+    fn dynamic_alloca(&mut self, _len: RValue<'gcc>, _align: Align) -> RValue<'gcc> {
         unimplemented!();
     }
 

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs

@@ -530,7 +530,7 @@ impl<'gcc, 'tcx> ArgAbiExt<'gcc, 'tcx> for ArgAbi<'tcx, Ty<'tcx>> {
                 // We instead thus allocate some scratch space...
                 let scratch_size = cast.size(bx);
                 let scratch_align = cast.align(bx);
-                let llscratch = bx.alloca(cast.gcc_type(bx), scratch_align);
+                let llscratch = bx.alloca(scratch_size, scratch_align);
                 bx.lifetime_start(llscratch, scratch_size);
 
                 // ... where we first store the value...

compiler/rustc_codegen_gcc/src/intrinsic/simd.rs

@@ -18,7 +18,7 @@ use rustc_middle::span_bug;
 use rustc_middle::ty::layout::HasTyCtxt;
 use rustc_middle::ty::{self, Ty};
 use rustc_span::{sym, Span, Symbol};
-use rustc_target::abi::Align;
+use rustc_target::abi::{Align, Size};
 
 use crate::builder::Builder;
 #[cfg(not(feature = "master"))]
@@ -558,7 +558,7 @@ pub fn generic_simd_intrinsic<'a, 'gcc, 'tcx>(
                 let ze = bx.zext(result, bx.type_ix(expected_bytes * 8));
 
                 // Convert the integer to a byte array
-                let ptr = bx.alloca(bx.type_ix(expected_bytes * 8), Align::ONE);
+                let ptr = bx.alloca(Size::from_bytes(expected_bytes), Align::ONE);
                 bx.store(ze, ptr, Align::ONE);
                 let array_ty = bx.type_array(bx.type_i8(), expected_bytes);
                 let ptr = bx.pointercast(ptr, bx.cx.type_ptr_to(array_ty));

compiler/rustc_codegen_llvm/src/abi.rs

@@ -238,7 +238,7 @@ impl<'ll, 'tcx> ArgAbiExt<'ll, 'tcx> for ArgAbi<'tcx, Ty<'tcx>> {
                     // We instead thus allocate some scratch space...
                     let scratch_size = cast.size(bx);
                     let scratch_align = cast.align(bx);
-                    let llscratch = bx.alloca(cast.llvm_type(bx), scratch_align);
+                    let llscratch = bx.alloca(scratch_size, scratch_align);
                     bx.lifetime_start(llscratch, scratch_size);
 
                     // ... where we first store the value...

compiler/rustc_codegen_llvm/src/builder.rs

@@ -466,20 +466,21 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
         val
     }
 
-    fn alloca(&mut self, ty: &'ll Type, align: Align) -> &'ll Value {
+    fn alloca(&mut self, size: Size, align: Align) -> &'ll Value {
         let mut bx = Builder::with_cx(self.cx);
         bx.position_at_start(unsafe { llvm::LLVMGetFirstBasicBlock(self.llfn()) });
+        let ty = self.cx().type_array(self.cx().type_i8(), size.bytes());
         unsafe {
             let alloca = llvm::LLVMBuildAlloca(bx.llbuilder, ty, UNNAMED);
             llvm::LLVMSetAlignment(alloca, align.bytes() as c_uint);
             alloca
         }
     }
 
-    fn byte_array_alloca(&mut self, len: &'ll Value, align: Align) -> &'ll Value {
+    fn dynamic_alloca(&mut self, size: &'ll Value, align: Align) -> &'ll Value {
         unsafe {
             let alloca =
-                llvm::LLVMBuildArrayAlloca(self.llbuilder, self.cx().type_i8(), len, UNNAMED);
+                llvm::LLVMBuildArrayAlloca(self.llbuilder, self.cx().type_i8(), size, UNNAMED);
             llvm::LLVMSetAlignment(alloca, align.bytes() as c_uint);
             alloca
         }

compiler/rustc_codegen_llvm/src/intrinsic.rs

@@ -18,7 +18,7 @@ use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt, LayoutOf};
 use rustc_middle::ty::{self, GenericArgsRef, Ty};
 use rustc_middle::{bug, span_bug};
 use rustc_span::{sym, Span, Symbol};
-use rustc_target::abi::{self, Align, HasDataLayout, Primitive};
+use rustc_target::abi::{self, Align, HasDataLayout, Primitive, Size};
 use rustc_target::spec::{HasTargetSpec, PanicStrategy};
 
 use std::cmp::Ordering;
@@ -637,8 +637,9 @@ fn codegen_msvc_try<'ll>(
         //      }
         //
         // More information can be found in libstd's seh.rs implementation.
+        let ptr_size = bx.tcx().data_layout.pointer_size;
         let ptr_align = bx.tcx().data_layout.pointer_align.abi;
-        let slot = bx.alloca(bx.type_ptr(), ptr_align);
+        let slot = bx.alloca(ptr_size, ptr_align);
         let try_func_ty = bx.type_func(&[bx.type_ptr()], bx.type_void());
         bx.invoke(try_func_ty, None, None, try_func, &[data], normal, catchswitch, None);
 
@@ -908,15 +909,14 @@ fn codegen_emcc_try<'ll>(
 
         // We need to pass two values to catch_func (ptr and is_rust_panic), so
         // create an alloca and pass a pointer to that.
+        let ptr_size = bx.tcx().data_layout.pointer_size;
         let ptr_align = bx.tcx().data_layout.pointer_align.abi;
         let i8_align = bx.tcx().data_layout.i8_align.abi;
-        let catch_data_type = bx.type_struct(&[bx.type_ptr(), bx.type_bool()], false);
-        let catch_data = bx.alloca(catch_data_type, ptr_align);
-        let catch_data_0 =
-            bx.inbounds_gep(catch_data_type, catch_data, &[bx.const_usize(0), bx.const_usize(0)]);
-        bx.store(ptr, catch_data_0, ptr_align);
-        let catch_data_1 =
-            bx.inbounds_gep(catch_data_type, catch_data, &[bx.const_usize(0), bx.const_usize(1)]);
+        // Required in order for there to be no padding between the fields.
+        assert!(i8_align <= ptr_align);
+        let catch_data = bx.alloca(2 * ptr_size, ptr_align);
+        bx.store(ptr, catch_data, ptr_align);
+        let catch_data_1 = bx.inbounds_ptradd(catch_data, bx.const_usize(ptr_size.bytes()));
         bx.store(is_rust_panic, catch_data_1, i8_align);
 
         let catch_ty = bx.type_func(&[bx.type_ptr(), bx.type_ptr()], bx.type_void());
@@ -1362,7 +1362,7 @@ fn generic_simd_intrinsic<'ll, 'tcx>(
                 let ze = bx.zext(i_, bx.type_ix(expected_bytes * 8));
 
                 // Convert the integer to a byte array
-                let ptr = bx.alloca(bx.type_ix(expected_bytes * 8), Align::ONE);
+                let ptr = bx.alloca(Size::from_bytes(expected_bytes), Align::ONE);
                 bx.store(ze, ptr, Align::ONE);
                 let array_ty = bx.type_array(bx.type_i8(), expected_bytes);
                 return Ok(bx.load(array_ty, ptr, Align::ONE));

compiler/rustc_codegen_ssa/src/base.rs

@@ -506,7 +506,7 @@ fn get_argc_argv<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
         let ptr_size = bx.tcx().data_layout.pointer_size;
         let ptr_align = bx.tcx().data_layout.pointer_align.abi;
         let arg_argc = bx.const_int(cx.type_isize(), 2);
-        let arg_argv = bx.alloca(cx.type_array(cx.type_ptr(), 2), ptr_align);
+        let arg_argv = bx.alloca(2 * ptr_size, ptr_align);
         bx.store(param_handle, arg_argv, ptr_align);
         let arg_argv_el1 = bx.inbounds_ptradd(arg_argv, bx.const_usize(ptr_size.bytes()));
         bx.store(param_system_table, arg_argv_el1, ptr_align);

compiler/rustc_codegen_ssa/src/mir/operand.rs

@@ -324,7 +324,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
                 let llfield_ty = bx.cx().backend_type(field);
 
                 // Can't bitcast an aggregate, so round trip through memory.
-                let llptr = bx.alloca(llfield_ty, field.align.abi);
+                let llptr = bx.alloca(field.size, field.align.abi);
                 bx.store(*llval, llptr, field.align.abi);
                 *llval = bx.load(llfield_ty, llptr, field.align.abi);
             }
@@ -472,7 +472,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue<V> {
         let align_minus_1 = bx.sub(align, one);
         let size_extra = bx.add(size, align_minus_1);
         let min_align = Align::ONE;
-        let alloca = bx.byte_array_alloca(size_extra, min_align);
+        let alloca = bx.dynamic_alloca(size_extra, min_align);
         let address = bx.ptrtoint(alloca, bx.type_isize());
         let neg_address = bx.neg(address);
         let offset = bx.and(neg_address, align_minus_1);

compiler/rustc_codegen_ssa/src/mir/place.rs

@@ -57,7 +57,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
         align: Align,
     ) -> Self {
         assert!(layout.is_sized(), "tried to statically allocate unsized place");
-        let tmp = bx.alloca(bx.cx().backend_type(layout), align);
+        let tmp = bx.alloca(layout.size, align);
         Self::new_sized_aligned(tmp, layout, align)
     }
 

compiler/rustc_codegen_ssa/src/traits/builder.rs

@@ -141,8 +141,8 @@ pub trait BuilderMethods<'a, 'tcx>:
     }
     fn to_immediate_scalar(&mut self, val: Self::Value, scalar: Scalar) -> Self::Value;
 
-    fn alloca(&mut self, ty: Self::Type, align: Align) -> Self::Value;
-    fn byte_array_alloca(&mut self, len: Self::Value, align: Align) -> Self::Value;
+    fn alloca(&mut self, size: Size, align: Align) -> Self::Value;
+    fn dynamic_alloca(&mut self, size: Self::Value, align: Align) -> Self::Value;
 
     fn load(&mut self, ty: Self::Type, ptr: Self::Value, align: Align) -> Self::Value;
     fn volatile_load(&mut self, ty: Self::Type, ptr: Self::Value) -> Self::Value;

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

@@ -11,6 +11,11 @@
 //@ compile-flags: -C opt-level=2 -Z merge-functions=disabled
 //@ min-llvm-version: 17.0.2
 
+// NOTE: the heuristics for stack smash protection inappropriately rely on types in LLVM IR,
+// despite those types having no semantic meaning. This means that the `basic` and `strong`
+// settings do not behave in a coherent way. This is a known issue in LLVM.
+// See comments on https://github.com/rust-lang/rust/issues/114903.
+
 #![crate_type = "lib"]
 
 #![allow(incomplete_features)]
@@ -39,23 +44,9 @@ pub fn array_char(f: fn(*const char)) {
     f(&b as *const _);
     f(&c as *const _);
 
-    // Any type of local array variable leads to stack protection with the
-    // "strong" heuristic. The 'basic' heuristic only adds stack protection to
-    // functions with local array variables of a byte-sized type, however. Since
-    // 'char' is 4 bytes in Rust, this function is not protected by the 'basic'
-    // heuristic
-    //
-    // (This test *also* takes the address of the local stack variables. We
-    // cannot know that this isn't what triggers the `strong` heuristic.
-    // However, the test strategy of passing the address of a stack array to an
-    // external function is sufficient to trigger the `basic` heuristic (see
-    // test `array_u8_large()`). Since the `basic` heuristic only checks for the
-    // presence of stack-local array variables, we can be confident that this
-    // test also captures this part of the `strong` heuristic specification.)
-
     // all: __stack_chk_fail
     // strong: __stack_chk_fail
-    // basic-NOT: __stack_chk_fail
+    // basic: __stack_chk_fail
     // none-NOT: __stack_chk_fail
     // missing-NOT: __stack_chk_fail
 }
@@ -163,26 +154,11 @@ pub fn local_string_addr_taken(f: fn(&String)) {
     f(&x);
 
     // Taking the address of the local variable `x` leads to stack smash
-    // protection with the `strong` heuristic, but not with the `basic`
-    // heuristic. It does not matter that the reference is not mut.
-    //
-    // An interesting note is that a similar function in C++ *would* be
-    // protected by the `basic` heuristic, because `std::string` has a char
-    // array internally as a small object optimization:
-    // ```
-    // cat <<EOF | clang++ -O2 -fstack-protector -S -x c++ - -o - | grep stack_chk
-    // #include <string>
-    // void f(void (*g)(const std::string&)) {
-    //     std::string x;
-    //     g(x);
-    // }
-    // EOF
-    // ```
-    //
+    // protection. It does not matter that the reference is not mut.
 
     // all: __stack_chk_fail
     // strong: __stack_chk_fail
-    // basic-NOT: __stack_chk_fail
+    // basic: __stack_chk_fail
     // none-NOT: __stack_chk_fail
     // missing-NOT: __stack_chk_fail
 }
@@ -233,8 +209,8 @@ pub fn local_large_var_moved(f: fn(Gigastruct)) {
     // Even though the local variable conceptually doesn't have its address
     // taken, it's so large that the "move" is implemented with a reference to a
     // stack-local variable in the ABI. Consequently, this function *is*
-    // protected by the `strong` heuristic. This is also the case for
-    // rvalue-references in C++, regardless of struct size:
+    // protected. This is also the case for rvalue-references in C++,
+    // regardless of struct size:
     // ```
     // cat <<EOF | clang++ -O2 -fstack-protector-strong -S -x c++ - -o - | grep stack_chk
     // #include <cstdint>
@@ -248,7 +224,7 @@ pub fn local_large_var_moved(f: fn(Gigastruct)) {
 
     // all: __stack_chk_fail
     // strong: __stack_chk_fail
-    // basic-NOT: __stack_chk_fail
+    // basic: __stack_chk_fail
     // none-NOT: __stack_chk_fail
     // missing-NOT: __stack_chk_fail
 }
@@ -261,9 +237,9 @@ pub fn local_large_var_cloned(f: fn(Gigastruct)) {
     // A new instance of `Gigastruct` is passed to `f()`, without any apparent
     // connection to this stack frame. Still, since instances of `Gigastruct`
     // are sufficiently large, it is allocated in the caller stack frame and
-    // passed as a pointer. As such, this function is *also* protected by the
-    // `strong` heuristic, just like `local_large_var_moved`. This is also the
-    // case for pass-by-value of sufficiently large structs in C++:
+    // passed as a pointer. As such, this function is *also* protected, just
+    // like `local_large_var_moved`. This is also the case for pass-by-value
+    // of sufficiently large structs in C++:
     // ```
     // cat <<EOF | clang++ -O2 -fstack-protector-strong -S -x c++ - -o - | grep stack_chk
     // #include <cstdint>
@@ -275,10 +251,9 @@ pub fn local_large_var_cloned(f: fn(Gigastruct)) {
     // EOF
     // ```
 
-
     // all: __stack_chk_fail
     // strong: __stack_chk_fail
-    // basic-NOT: __stack_chk_fail
+    // basic: __stack_chk_fail
     // none-NOT: __stack_chk_fail
     // missing-NOT: __stack_chk_fail
 }

tests/codegen/align-byval-alignment-mismatch.rs

@@ -56,7 +56,7 @@ extern "C" {
 #[no_mangle]
 pub unsafe fn rust_to_c_increases_alignment(x: Align1) {
     // i686-linux: start:
-    // i686-linux-NEXT: [[ALLOCA:%[0-9a-z]+]] = alloca %Align1, align 4
+    // i686-linux-NEXT: [[ALLOCA:%[0-9a-z]+]] = alloca [48 x i8], align 4
     // i686-linux-NEXT: call void @llvm.memcpy.{{.+}}(ptr {{.*}}align 4 {{.*}}[[ALLOCA]], ptr {{.*}}align 1 {{.*}}%x
     // i686-linux-NEXT: call void @extern_c_align1({{.+}} [[ALLOCA]])
 
@@ -90,7 +90,7 @@ pub unsafe extern "C" fn c_to_rust_decreases_alignment(x: Align1) {
 #[no_mangle]
 pub unsafe extern "C" fn c_to_rust_increases_alignment(x: Align16) {
     // i686-linux: start:
-    // i686-linux-NEXT: [[ALLOCA:%[0-9a-z]+]] = alloca %Align16, align 16
+    // i686-linux-NEXT: [[ALLOCA:%[0-9a-z]+]] = alloca [48 x i8], align 16
     // i686-linux-NEXT: call void @llvm.memcpy.{{.+}}(ptr {{.*}}align 16 {{.*}}[[ALLOCA]], ptr {{.*}}align 4 {{.*}}%0
     // i686-linux-NEXT: call void @extern_rust_align16({{.+}} [[ALLOCA]])
 
@@ -116,7 +116,7 @@ pub unsafe extern "C" fn c_to_rust_ref_decreases_alignment(x: Align1) {
 #[no_mangle]
 pub unsafe extern "C" fn c_to_rust_ref_increases_alignment(x: Align16) {
     // i686-linux: start:
-    // i686-linux-NEXT: [[ALLOCA:%[0-9a-z]+]] = alloca %Align16, align 16
+    // i686-linux-NEXT: [[ALLOCA:%[0-9a-z]+]] = alloca [48 x i8], align 16
     // i686-linux-NEXT: call void @llvm.memcpy.{{.+}}(ptr {{.*}}align 16 {{.*}}[[ALLOCA]], ptr {{.*}}align 4 {{.*}}%0
     // i686-linux-NEXT: call void @extern_rust_ref_align16({{.+}} [[ALLOCA]])
 

tests/codegen/align-byval.rs

@@ -106,20 +106,20 @@ pub struct ForceAlign16 {
 pub unsafe fn call_na1(x: NaturalAlign1) {
     // CHECK: start:
 
-    // m68k: [[ALLOCA:%[a-z0-9+]]] = alloca %NaturalAlign1, align 1
+    // m68k: [[ALLOCA:%[a-z0-9+]]] = alloca [2 x i8], align 1
     // m68k: call void @natural_align_1({{.*}}byval([2 x i8]) align 1{{.*}} [[ALLOCA]])
 
-    // wasm: [[ALLOCA:%[a-z0-9+]]] = alloca %NaturalAlign1, align 1
+    // wasm: [[ALLOCA:%[a-z0-9+]]] = alloca [2 x i8], align 1
     // wasm: call void @natural_align_1({{.*}}byval([2 x i8]) align 1{{.*}} [[ALLOCA]])
 
     // x86_64-linux: call void @natural_align_1(i16
 
     // x86_64-windows: call void @natural_align_1(i16
 
-    // i686-linux: [[ALLOCA:%[a-z0-9+]]] = alloca %NaturalAlign1, align 4
+    // i686-linux: [[ALLOCA:%[a-z0-9+]]] = alloca [2 x i8], align 4
     // i686-linux: call void @natural_align_1({{.*}}byval([2 x i8]) align 4{{.*}} [[ALLOCA]])
 
-    // i686-windows: [[ALLOCA:%[a-z0-9+]]] = alloca %NaturalAlign1, align 4
+    // i686-windows: [[ALLOCA:%[a-z0-9+]]] = alloca [2 x i8], align 4
     // i686-windows: call void @natural_align_1({{.*}}byval([2 x i8]) align 4{{.*}} [[ALLOCA]])
     natural_align_1(x);
 }
@@ -134,10 +134,10 @@ pub unsafe fn call_na2(x: NaturalAlign2) {
     // x86_64-linux-NEXT: call void @natural_align_2
     // x86_64-windows-NEXT: call void @natural_align_2
 
-    // i686-linux: [[ALLOCA:%[0-9]+]] = alloca %NaturalAlign2, align 4
+    // i686-linux: [[ALLOCA:%[0-9]+]] = alloca [34 x i8], align 4
     // i686-linux: call void @natural_align_2({{.*}}byval([34 x i8]) align 4{{.*}} [[ALLOCA]])
 
-    // i686-windows: [[ALLOCA:%[0-9]+]] = alloca %NaturalAlign2, align 4
+    // i686-windows: [[ALLOCA:%[0-9]+]] = alloca [34 x i8], align 4
     // i686-windows: call void @natural_align_2({{.*}}byval([34 x i8]) align 4{{.*}} [[ALLOCA]])
     natural_align_2(x);
 }

tests/codegen/align-enum.rs

@@ -18,7 +18,7 @@ pub struct Nested64 {
 // CHECK-LABEL: @align64
 #[no_mangle]
 pub fn align64(a: u32) -> Align64 {
-// CHECK: %a64 = alloca %Align64, align 64
+// CHECK: %a64 = alloca [64 x i8], align 64
 // CHECK: call void @llvm.memcpy.{{.*}}(ptr align 64 %{{.*}}, ptr align 64 %{{.*}}, i{{[0-9]+}} 64, i1 false)
     let a64 = Align64::A(a);
     a64
@@ -27,7 +27,7 @@ pub fn align64(a: u32) -> Align64 {
 // CHECK-LABEL: @nested64
 #[no_mangle]
 pub fn nested64(a: u8, b: u32, c: u16) -> Nested64 {
-// CHECK: %n64 = alloca %Nested64, align 64
+// CHECK: %n64 = alloca [128 x i8], align 64
     let n64 = Nested64 { a, b: Align64::B(b), c };
     n64
 }