Skip to content
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

Emit @llvm.lifetime.end for moved arguments passed indirectly #98121

Closed
Closed
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
22 changes: 22 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,28 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}

fn codegen_return_terminator(&mut self, mut bx: Bx) {
// Mark storage dead for arguments passed indirectly.
for (arg_index, local) in self.mir.args_iter().enumerate() {
if Some(local) == self.mir.spread_arg {
// Bail out on spread args. This may result in reduced optimization in "rust-call" ABI functions.
// FIXME: handle spread args. This is subtle, see `mir::arg_local_refs`.
break;
}
if self.fn_abi.c_variadic && arg_index == self.fn_abi.args.len() {
// Ignore C variadic args (always the last argument, if present).
continue;
}
let abi = &self.fn_abi.args[arg_index];
if !abi.is_indirect() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we should special-case byval indirect here? Adding the lifetime marker isn't wrong in that case, just not necessary.

// Only indirect arguments need storage markers.
continue;
}
match self.locals[local] {
LocalRef::Place(place) => place.storage_dead(&mut bx),
LocalRef::UnsizedPlace(place) => place.storage_dead(&mut bx),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test for the unsized case?

_ => bug!("Unexpected non-place argument local: {:?}", local),
}
}
// Call `va_end` if this is the definition of a C-variadic function.
if self.fn_abi.c_variadic {
// The `VaList` "spoofed" argument is just after all the real arguments.
Expand Down
5 changes: 5 additions & 0 deletions src/test/codegen/array-equality.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ pub fn array_eq_value_still_passed_by_pointer(a: [u16; 9], b: [u16; 9]) -> bool
// CHECK-NEXT: start:
// CHECK: %[[CMP:.+]] = tail call i32 @{{bcmp|memcmp}}({{i8\*|ptr}} {{.*}} dereferenceable(18) %{{.+}}, {{i8\*|ptr}} {{.*}} dereferenceable(18) %{{.+}}, i64 18)
// CHECK-NEXT: %[[EQ:.+]] = icmp eq i32 %[[CMP]], 0
// CHECK-NEXT: call void @llvm.lifetime.end
// CHECK-NEXT: call void @llvm.lifetime.end
// CHECK-NEXT: ret i1 %[[EQ]]
a == b
}
Expand Down Expand Up @@ -58,6 +60,8 @@ pub fn array_eq_zero_mid(x: [u16; 8]) -> bool {
// CHECK-NEXT: start:
// CHECK: %[[LOAD:.+]] = load i128,
// CHECK-NEXT: %[[EQ:.+]] = icmp eq i128 %[[LOAD]], 0
// CHECK-NEXT: bitcast
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop bitcast here and use CHECK for the lifetime. Similar for other bitcast check lines. Otherwise these tests will fail on LLVM 15 with opaque pointers.

// CHECK-NEXT: call void @llvm.lifetime.end
// CHECK-NEXT: ret i1 %[[EQ]]
x == [0; 8]
}
Expand All @@ -69,6 +73,7 @@ pub fn array_eq_zero_long(x: [u16; 1234]) -> bool {
// CHECK-NOT: alloca
// CHECK: %[[CMP:.+]] = tail call i32 @{{bcmp|memcmp}}(
// CHECK-NEXT: %[[EQ:.+]] = icmp eq i32 %[[CMP]], 0
// CHECK-NEXT: call void @llvm.lifetime.end
// CHECK-NEXT: ret i1 %[[EQ]]
x == [0; 1234]
}
13 changes: 13 additions & 0 deletions src/test/codegen/issue-96497-array-indirect-writes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Check that LLVM can see that stores to a by-move array (passed indirectly) are dead.

// compile-flags: -O -Zmir-opt-level=0
// min-llvm-version: 14.0

#![crate_type="lib"]

// CHECK-LABEL: @array_dead_store
#[no_mangle]
pub fn array_dead_store(mut x: [u8; 1234]) {
// CHECK-NOT: store
x[10] = 42;
}
73 changes: 73 additions & 0 deletions src/test/codegen/lifetime-start-end-indirect-args.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// This test checks that moved arguments passed indirectly get lifetime markers.

// compile-flags: -O -C no-prepopulate-passes -Zmir-opt-level=0

#![crate_type = "lib"]

// CHECK-LABEL: @arg_indirect
#[no_mangle]
pub fn arg_indirect(a: [u8; 1234]) {
// Arguments passed indirectly should get lifetime markers.

// CHECK: [[A:%[0-9]+]] = bitcast{{.*}} %a
// CHECK: call void @llvm.lifetime.end{{.*}} [[A]])
}

// CHECK-LABEL: @arg_by_val
#[no_mangle]
pub fn arg_by_val(a: u8) {
// Arguments passed by value should not get lifetime markers.

// CHECK-NOT: call void @llvm.lifetime.end
}

// CHECK-LABEL: @arg_by_ref
#[no_mangle]
pub fn arg_by_ref(a: &[u8; 1234]) {
// Arguments passed by ref should not get lifetime markers.

// CHECK-NOT: call void @llvm.lifetime.end
}

// CHECK-LABEL: @with_other_args
#[no_mangle]
pub fn with_other_args(x: (), y: (), a: [u8; 1234], z: ()) {
// Lifetime markers should be applied to the correct argument,
// even in the presence of ignored ZST arguments.

// CHECK-NOT: call void @llvm.lifetime.end
// CHECK: [[A:%[0-9]+]] = bitcast{{.*}} %a
// CHECK: call void @llvm.lifetime.end{{.*}} [[A]])
// CHECK-NOT: call void @llvm.lifetime.end
}

// CHECK-LABEL: @forward_directly_to_ret
#[no_mangle]
pub fn forward_directly_to_ret(a: [u8; 1234]) -> [u8; 1234] {
// Ensure that lifetime markers appear after the argument is copied to the return place.
// (Since reading from `a` after `lifetime.end` would return poison.)

// CHECK: memcpy
// CHECK: [[A:%[0-9]+]] = bitcast{{.*}} %a
// CHECK: call void @llvm.lifetime.end{{.*}} [[A]])
// CHECK-NEXT: ret void
a
}

pub struct LargeWithField {
x: u8,
_rest: [u8; 1234],
}

// CHECK-LABEL: @read_from_arg
#[no_mangle]
pub fn read_from_arg(a: LargeWithField) -> u8 {
// Ensure that lifetime markers appear after reading from the argument.
// (Since reading from `a` after `lifetime.end` would return poison.)

// CHECK: [[LOAD:%[0-9]+]] = load i8
// CHECK: [[A:%[0-9]+]] = bitcast{{.*}} %a
// CHECK: call void @llvm.lifetime.end{{.*}} [[A]])
// CHECK-NEXT: ret i8 [[LOAD]]
a.x
}
10 changes: 10 additions & 0 deletions src/test/codegen/vec-in-place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ pub struct Bar {
pub fn vec_iterator_cast_primitive(vec: Vec<i8>) -> Vec<u8> {
// CHECK-NOT: loop
// CHECK-NOT: call
// CHECK: call void @llvm.lifetime.end
// CHECK-NEXT: ret
vec.into_iter().map(|e| e as u8).collect()
}

Expand All @@ -39,6 +41,8 @@ pub fn vec_iterator_cast_primitive(vec: Vec<i8>) -> Vec<u8> {
pub fn vec_iterator_cast_wrapper(vec: Vec<u8>) -> Vec<Wrapper<u8>> {
// CHECK-NOT: loop
// CHECK-NOT: call
// CHECK: call void @llvm.lifetime.end
// CHECK-NEXT: ret
vec.into_iter().map(|e| Wrapper(e)).collect()
}

Expand All @@ -47,6 +51,8 @@ pub fn vec_iterator_cast_wrapper(vec: Vec<u8>) -> Vec<Wrapper<u8>> {
pub fn vec_iterator_cast_unwrap(vec: Vec<Wrapper<u8>>) -> Vec<u8> {
// CHECK-NOT: loop
// CHECK-NOT: call
// CHECK: call void @llvm.lifetime.end
// CHECK-NEXT: ret
vec.into_iter().map(|e| e.0).collect()
}

Expand All @@ -55,6 +61,8 @@ pub fn vec_iterator_cast_unwrap(vec: Vec<Wrapper<u8>>) -> Vec<u8> {
pub fn vec_iterator_cast_aggregate(vec: Vec<[u64; 4]>) -> Vec<Foo> {
// CHECK-NOT: loop
// CHECK-NOT: call
// CHECK: call void @llvm.lifetime.end
// CHECK-NEXT: ret
vec.into_iter().map(|e| unsafe { std::mem::transmute(e) }).collect()
}

Expand All @@ -63,6 +71,8 @@ pub fn vec_iterator_cast_aggregate(vec: Vec<[u64; 4]>) -> Vec<Foo> {
pub fn vec_iterator_cast_deaggregate(vec: Vec<Bar>) -> Vec<[u64; 4]> {
// CHECK-NOT: loop
// CHECK-NOT: call
// CHECK: call void @llvm.lifetime.end
// CHECK-NEXT: ret

// Safety: For the purpose of this test we assume that Bar layout matches [u64; 4].
// This currently is not guaranteed for repr(Rust) types, but it happens to work here and
Expand Down