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

Describe lifetime of call argument temporaries passed indirectly #138489

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
46 changes: 33 additions & 13 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::cmp;

use rustc_abi::{BackendRepr, ExternAbi, HasDataLayout, Reg, WrappingRange};
use rustc_abi::{BackendRepr, ExternAbi, HasDataLayout, Reg, Size, WrappingRange};
use rustc_ast as ast;
use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece};
use rustc_data_structures::packed::Pu128;
@@ -158,7 +158,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
llargs: &[Bx::Value],
destination: Option<(ReturnDest<'tcx, Bx::Value>, mir::BasicBlock)>,
mut unwind: mir::UnwindAction,
copied_constant_arguments: &[PlaceRef<'tcx, <Bx as BackendTypes>::Value>],
lifetime_ends_after_call: &[(Bx::Value, Size)],
instance: Option<Instance<'tcx>>,
mergeable_succ: bool,
) -> MergingSucc {
@@ -245,8 +245,8 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
if let Some((ret_dest, target)) = destination {
bx.switch_to_block(fx.llbb(target));
fx.set_debug_loc(bx, self.terminator.source_info);
for tmp in copied_constant_arguments {
bx.lifetime_end(tmp.val.llval, tmp.layout.size);
for &(tmp, size) in lifetime_ends_after_call {
bx.lifetime_end(tmp, size);
}
fx.store_return(bx, ret_dest, &fn_abi.ret, invokeret);
}
@@ -259,8 +259,8 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
}

if let Some((ret_dest, target)) = destination {
for tmp in copied_constant_arguments {
bx.lifetime_end(tmp.val.llval, tmp.layout.size);
for &(tmp, size) in lifetime_ends_after_call {
bx.lifetime_end(tmp, size);
}
fx.store_return(bx, ret_dest, &fn_abi.ret, llret);
self.funclet_br(fx, bx, target, mergeable_succ)
@@ -1049,7 +1049,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
(args, None)
};

let mut copied_constant_arguments = vec![];
// Keeps track of temporary allocas whose liftime need to be ended after the call.
Copy link
Member

@workingjubilee workingjubilee Mar 24, 2025

Choose a reason for hiding this comment

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

typo, also slightly confusing wording to me, and often comments like this are written in the imperative, so perhaps this could be more like

Suggested change
// Keeps track of temporary allocas whose liftime need to be ended after the call.
// Keep track of temporary allocas with lifetimes that end after this call

Copy link
Member

Choose a reason for hiding this comment

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

The exact phrasing isn't too important tho' and I don't necessarily insist on the imperative, please feel free to revise as you see fit.

let mut lifetime_ends_after_call: Vec<(Bx::Value, Size)> = Vec::new();
Copy link
Member

@workingjubilee workingjubilee Mar 24, 2025

Choose a reason for hiding this comment

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

also comments should generally prefer to include "why": probably this?

Suggested change
let mut lifetime_ends_after_call: Vec<(Bx::Value, Size)> = Vec::new();
// because we want to later emit things like `@llvm.lifetime.end` for them.
let mut lifetime_ends_after_call: Vec<(Bx::Value, Size)> = Vec::new();

Comment on lines -1052 to +1053
Copy link
Member

@workingjubilee workingjubilee Mar 24, 2025

Choose a reason for hiding this comment

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

Also one thing we kinda lose here with this rename is that most of the entities in this list are arguments or owned by the effective arguments: that's why we accumulate them through codegen_argument calls, then actually handle emitting llvm.lifetime.end intrinsics and the like in do_call. Without this explicitly being called out in the identifier's name, you have to then go read another few hundred lines of code to extract that. We could probably spare a line of commentary here on that subject.

'make_args: for (i, arg) in first_args.iter().enumerate() {
let mut op = self.codegen_operand(bx, &arg.node);

@@ -1137,19 +1138,26 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bx.lifetime_start(tmp.val.llval, tmp.layout.size);
op.val.store(bx, tmp);
op.val = Ref(tmp.val);
copied_constant_arguments.push(tmp);
lifetime_ends_after_call.push((tmp.val.llval, tmp.layout.size));
}
_ => {}
}

self.codegen_argument(bx, op, &mut llargs, &fn_abi.args[i]);
self.codegen_argument(
bx,
op,
&mut llargs,
&fn_abi.args[i],
&mut lifetime_ends_after_call,
);
}
let num_untupled = untuple.map(|tup| {
self.codegen_arguments_untupled(
bx,
&tup.node,
&mut llargs,
&fn_abi.args[first_args.len()..],
&mut lifetime_ends_after_call,
)
});

@@ -1174,7 +1182,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
);

let last_arg = fn_abi.args.last().unwrap();
self.codegen_argument(bx, location, &mut llargs, last_arg);
self.codegen_argument(
bx,
location,
&mut llargs,
last_arg,
&mut lifetime_ends_after_call,
);
}

let fn_ptr = match (instance, llfn) {
@@ -1190,7 +1204,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
&llargs,
destination,
unwind,
&copied_constant_arguments,
&lifetime_ends_after_call,
instance,
mergeable_succ,
)
@@ -1475,6 +1489,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
op: OperandRef<'tcx, Bx::Value>,
llargs: &mut Vec<Bx::Value>,
arg: &ArgAbi<'tcx, Ty<'tcx>>,
lifetime_ends_after_call: &mut Vec<(Bx::Value, Size)>,
) {
match arg.mode {
PassMode::Ignore => return,
@@ -1513,7 +1528,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
None => arg.layout.align.abi,
};
let scratch = PlaceValue::alloca(bx, arg.layout.size, required_align);
bx.lifetime_start(scratch.llval, arg.layout.size);
op.val.store(bx, scratch.with_type(arg.layout));
lifetime_ends_after_call.push((scratch.llval, arg.layout.size));
(scratch.llval, scratch.align, true)
}
PassMode::Cast { .. } => {
@@ -1534,7 +1551,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// alignment requirements may be higher than the type's alignment, so copy
// to a higher-aligned alloca.
let scratch = PlaceValue::alloca(bx, arg.layout.size, required_align);
bx.lifetime_start(scratch.llval, arg.layout.size);
bx.typed_place_copy(scratch, op_place_val, op.layout);
lifetime_ends_after_call.push((scratch.llval, arg.layout.size));
(scratch.llval, scratch.align, true)
} else {
(op_place_val.llval, op_place_val.align, true)
@@ -1616,6 +1635,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
operand: &mir::Operand<'tcx>,
llargs: &mut Vec<Bx::Value>,
args: &[ArgAbi<'tcx, Ty<'tcx>>],
lifetime_ends_after_call: &mut Vec<(Bx::Value, Size)>,
) -> usize {
let tuple = self.codegen_operand(bx, operand);

@@ -1628,13 +1648,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
for i in 0..tuple.layout.fields.count() {
let field_ptr = tuple_ptr.project_field(bx, i);
let field = bx.load_operand(field_ptr);
self.codegen_argument(bx, field, llargs, &args[i]);
self.codegen_argument(bx, field, llargs, &args[i], lifetime_ends_after_call);
}
} else {
// If the tuple is immediate, the elements are as well.
for i in 0..tuple.layout.fields.count() {
let op = tuple.extract_field(self, bx, i);
self.codegen_argument(bx, op, llargs, &args[i]);
self.codegen_argument(bx, op, llargs, &args[i], lifetime_ends_after_call);
}
}
tuple.layout.fields.count()
2 changes: 2 additions & 0 deletions tests/codegen/align-byval-alignment-mismatch.rs
Original file line number Diff line number Diff line change
@@ -54,8 +54,10 @@ extern "C" {
pub unsafe fn rust_to_c_increases_alignment(x: Align1) {
// i686-linux: start:
// i686-linux-NEXT: [[ALLOCA:%[0-9a-z]+]] = alloca [48 x i8], align 4
// i686-linux-NEXT: call void @llvm.lifetime.start.p0(i64 48, ptr {{.*}}[[ALLOCA]])
// i686-linux-NEXT: call void @llvm.memcpy.{{.+}}(ptr {{.*}}align 4 {{.*}}[[ALLOCA]], ptr {{.*}}align 1 {{.*}}%x
// i686-linux-NEXT: call void @extern_c_align1({{.+}} [[ALLOCA]])
// i686-linux-NEXT: call void @llvm.lifetime.end.p0(i64 48, ptr {{.*}}[[ALLOCA]])

// x86_64-linux: start:
// x86_64-linux-NEXT: call void @extern_c_align1
68 changes: 68 additions & 0 deletions tests/codegen/call-tmps-lifetime.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Test that temporary allocas used for call arguments have their lifetimes described by
// intrinsics.
//
//@ add-core-stubs
//@ compile-flags: -Copt-level=1 -Cno-prepopulate-passes --crate-type=lib --target i686-unknown-linux-gnu
//@ needs-llvm-components: x86
#![feature(no_core)]
#![no_std]
#![no_core]
extern crate minicore;
use minicore::*;

// Const operand. Regression test for #98156.
//
// CHECK-LABEL: define void @const_indirect(
// CHECK-NEXT: start:
// CHECK-NEXT: [[B:%.*]] = alloca
// CHECK-NEXT: [[A:%.*]] = alloca
// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 4096, ptr [[A]])
// CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr align 4 [[A]], ptr align 4 {{.*}}, i32 4096, i1 false)
// CHECK-NEXT: call void %h(ptr {{.*}} [[A]])
// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 4096, ptr [[A]])
// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 4096, ptr [[B]])
// CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr align 4 [[B]], ptr align 4 {{.*}}, i32 4096, i1 false)
// CHECK-NEXT: call void %h(ptr {{.*}} [[B]])
// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 4096, ptr [[B]])
#[no_mangle]
pub fn const_indirect(h: extern "C" fn([u32; 1024])) {
const C: [u32; 1024] = [0; 1024];
h(C);
h(C);
}

#[repr(C)]
pub struct Str {
pub ptr: *const u8,
pub len: usize,
}

// Pair of immediates. Regression test for #132014.
//
// CHECK-LABEL: define void @immediate_indirect(ptr {{.*}}%s.0, i32 {{.*}}%s.1, ptr {{.*}}%g)
// CHECK-NEXT: start:
// CHECK-NEXT: [[A:%.*]] = alloca
// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 8, ptr [[A]])
// CHECK-NEXT: store ptr %s.0, ptr [[A]]
// CHECK-NEXT: [[B:%.]] = getelementptr inbounds i8, ptr [[A]], i32 4
// CHECK-NEXT: store i32 %s.1, ptr [[B]]
// CHECK-NEXT: call void %g(ptr {{.*}} [[A]])
// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 8, ptr [[A]])
#[no_mangle]
pub fn immediate_indirect(s: Str, g: extern "C" fn(Str)) {
g(s);
}

// Indirect argument with a higher alignment requirement than the type's.
//
// CHECK-LABEL: define void @align_indirect(ptr{{.*}} align 1{{.*}} %a, ptr{{.*}} %fun)
// CHECK-NEXT: start:
// CHECK-NEXT: [[A:%.*]] = alloca [1024 x i8], align 4
// CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 1024, ptr [[A]])
// CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr align 4 [[A]], ptr align 1 %a, i32 1024, i1 false)
// CHECK-NEXT: call void %fun(ptr {{.*}} [[A]])
// CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 1024, ptr [[A]])
#[no_mangle]
pub fn align_indirect(a: [u8; 1024], fun: extern "C" fn([u8; 1024])) {
fun(a);
}
27 changes: 0 additions & 27 deletions tests/codegen/issues/issue-98156-const-arg-temp-lifetime.rs

This file was deleted.

Loading
Oops, something went wrong.