From 34246f89e268bd3741e51d21253951e3ffd53d74 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Thu, 8 Dec 2022 19:06:27 -0500 Subject: [PATCH 1/4] Add test case for ub in generated LLVM IR --- .../codegen/issue-105386-ub-in-debuginfo.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 src/test/codegen/issue-105386-ub-in-debuginfo.rs diff --git a/src/test/codegen/issue-105386-ub-in-debuginfo.rs b/src/test/codegen/issue-105386-ub-in-debuginfo.rs new file mode 100644 index 0000000000000..e30f373a793c6 --- /dev/null +++ b/src/test/codegen/issue-105386-ub-in-debuginfo.rs @@ -0,0 +1,22 @@ +// compile-flags: --crate-type=lib -O -Cdebuginfo=2 -Cno-prepopulate-passes +// min-llvm-version: 15.0 # this test uses opaque pointer notation +#![feature(stmt_expr_attributes)] + +pub struct S([usize; 8]); + +#[no_mangle] +pub fn outer_function(x: S, y: S) -> usize { + (#[inline(always)]|| { + let _z = x; + y.0[0] + })() +} + +// Check that we do not attempt to load from the spilled arg before it is assigned to +// when generating debuginfo. +// CHECK-LABEL: @outer_function +// CHECK: [[spill:%.*]] = alloca %"[closure@{{.*.rs}}:9:23: 9:25]" +// CHECK: [[ptr_tmp:%.*]] = getelementptr inbounds %"[closure@{{.*.rs}}:9:23: 9:25]", ptr [[spill]] +// CHECK: [[load:%.*]] = load ptr, ptr [[ptr_tmp]] +// CHECK: call void @llvm.lifetime.start{{.*}}({{.*}}, ptr [[spill]]) +// CHECK: call void @llvm.memcpy{{.*}}(ptr {{align .*}} [[spill]], ptr {{align .*}} %x From 525d0dd6e223357bc2520590cd1d9377445e0dc9 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Wed, 7 Dec 2022 12:51:07 -0500 Subject: [PATCH 2/4] Factor out debuginfo offset calculation --- .../rustc_codegen_ssa/src/mir/debuginfo.rs | 76 ++++++++++++------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs index 99283d3bb29f4..22e9eeea2551b 100644 --- a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs @@ -76,6 +76,53 @@ impl<'tcx, S: Copy, L: Copy> DebugScope { } } +struct DebugInfoOffset { + /// Offset from the `base` used to calculate the debuginfo offset. + direct_offset: Size, + /// Each offset in this vector indicates one level of indirection from the base or previous + /// indirect offset plus a dereference. + indirect_offsets: Vec, + /// The final location debuginfo should point to. + result: T, +} + +fn calculate_debuginfo_offset<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( + bx: &mut Bx, + local: mir::Local, + var: &PerLocalVarDebugInfo<'tcx, Bx::DIVariable>, + base: PlaceRef<'tcx, Bx::Value>, +) -> DebugInfoOffset> { + let mut direct_offset = Size::ZERO; + // FIXME(eddyb) use smallvec here. + let mut indirect_offsets = vec![]; + let mut place = base; + + for elem in &var.projection[..] { + match *elem { + mir::ProjectionElem::Deref => { + indirect_offsets.push(Size::ZERO); + place = bx.load_operand(place).deref(bx.cx()); + } + mir::ProjectionElem::Field(field, _) => { + let i = field.index(); + let offset = indirect_offsets.last_mut().unwrap_or(&mut direct_offset); + *offset += place.layout.fields.offset(i); + place = place.project_field(bx, i); + } + mir::ProjectionElem::Downcast(_, variant) => { + place = place.project_downcast(bx, variant); + } + _ => span_bug!( + var.source_info.span, + "unsupported var debuginfo place `{:?}`", + mir::Place { local, projection: var.projection }, + ), + } + } + + DebugInfoOffset { direct_offset, indirect_offsets, result: place } +} + impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { pub fn set_debug_loc(&self, bx: &mut Bx, source_info: mir::SourceInfo) { bx.set_span(source_info.span); @@ -262,33 +309,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let Some(dbg_var) = var.dbg_var else { continue }; let Some(dbg_loc) = self.dbg_loc(var.source_info) else { continue }; - let mut direct_offset = Size::ZERO; - // FIXME(eddyb) use smallvec here. - let mut indirect_offsets = vec![]; - let mut place = base; - - for elem in &var.projection[..] { - match *elem { - mir::ProjectionElem::Deref => { - indirect_offsets.push(Size::ZERO); - place = bx.load_operand(place).deref(bx.cx()); - } - mir::ProjectionElem::Field(field, _) => { - let i = field.index(); - let offset = indirect_offsets.last_mut().unwrap_or(&mut direct_offset); - *offset += place.layout.fields.offset(i); - place = place.project_field(bx, i); - } - mir::ProjectionElem::Downcast(_, variant) => { - place = place.project_downcast(bx, variant); - } - _ => span_bug!( - var.source_info.span, - "unsupported var debuginfo place `{:?}`", - mir::Place { local, projection: var.projection }, - ), - } - } + let DebugInfoOffset { direct_offset, indirect_offsets, result: place } = + calculate_debuginfo_offset(bx, local, &var, base); // When targeting MSVC, create extra allocas for arguments instead of pointing multiple // dbg_var_addr() calls into the same alloca with offsets. MSVC uses CodeView records From b33d1e26b220062e23bb202c6765663c157d971b Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Wed, 7 Dec 2022 13:47:51 -0500 Subject: [PATCH 3/4] Make `debuginfo_offset_calcuation` generic so we can resuse the logic This will allow us to separate the act of calculating the offsets from creating LLVM IR that performs the actions. --- .../rustc_codegen_ssa/src/mir/debuginfo.rs | 51 +++++++++++++++---- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs index 22e9eeea2551b..8935d42548f5f 100644 --- a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs @@ -3,12 +3,12 @@ use rustc_index::vec::IndexVec; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir; use rustc_middle::ty; +use rustc_middle::ty::layout::TyAndLayout; use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf}; use rustc_session::config::DebugInfo; use rustc_span::symbol::{kw, Symbol}; use rustc_span::{BytePos, Span}; -use rustc_target::abi::Abi; -use rustc_target::abi::Size; +use rustc_target::abi::{Abi, Size, VariantIdx}; use super::operand::{OperandRef, OperandValue}; use super::place::PlaceRef; @@ -76,6 +76,33 @@ impl<'tcx, S: Copy, L: Copy> DebugScope { } } +trait DebugInfoOffsetLocation<'tcx, Bx> { + fn deref(&self, bx: &mut Bx) -> Self; + fn layout(&self) -> TyAndLayout<'tcx>; + fn project_field(&self, bx: &mut Bx, field: mir::Field) -> Self; + fn downcast(&self, bx: &mut Bx, variant: VariantIdx) -> Self; +} + +impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> DebugInfoOffsetLocation<'tcx, Bx> + for PlaceRef<'tcx, Bx::Value> +{ + fn deref(&self, bx: &mut Bx) -> Self { + bx.load_operand(*self).deref(bx.cx()) + } + + fn layout(&self) -> TyAndLayout<'tcx> { + self.layout + } + + fn project_field(&self, bx: &mut Bx, field: mir::Field) -> Self { + PlaceRef::project_field(*self, bx, field.index()) + } + + fn downcast(&self, bx: &mut Bx, variant: VariantIdx) -> Self { + self.project_downcast(bx, variant) + } +} + struct DebugInfoOffset { /// Offset from the `base` used to calculate the debuginfo offset. direct_offset: Size, @@ -86,12 +113,17 @@ struct DebugInfoOffset { result: T, } -fn calculate_debuginfo_offset<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( +fn calculate_debuginfo_offset< + 'a, + 'tcx, + Bx: BuilderMethods<'a, 'tcx>, + L: DebugInfoOffsetLocation<'tcx, Bx>, +>( bx: &mut Bx, local: mir::Local, var: &PerLocalVarDebugInfo<'tcx, Bx::DIVariable>, - base: PlaceRef<'tcx, Bx::Value>, -) -> DebugInfoOffset> { + base: L, +) -> DebugInfoOffset { let mut direct_offset = Size::ZERO; // FIXME(eddyb) use smallvec here. let mut indirect_offsets = vec![]; @@ -101,16 +133,15 @@ fn calculate_debuginfo_offset<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( match *elem { mir::ProjectionElem::Deref => { indirect_offsets.push(Size::ZERO); - place = bx.load_operand(place).deref(bx.cx()); + place = place.deref(bx); } mir::ProjectionElem::Field(field, _) => { - let i = field.index(); let offset = indirect_offsets.last_mut().unwrap_or(&mut direct_offset); - *offset += place.layout.fields.offset(i); - place = place.project_field(bx, i); + *offset += place.layout().fields.offset(field.index()); + place = place.project_field(bx, field); } mir::ProjectionElem::Downcast(_, variant) => { - place = place.project_downcast(bx, variant); + place = place.downcast(bx, variant); } _ => span_bug!( var.source_info.span, From 7253057887b6de77e6847844311da517d2ada1eb Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Wed, 7 Dec 2022 13:48:34 -0500 Subject: [PATCH 4/4] Don't generate pointer loads to spills unless necessary In order for LLVM to correctly generate debuginfo for msvc, we sometimes need to spill arguments to the stack and perform some direct & indirect offsets into the value. Previously, this code always performed those actions, even when not required as LLVM would clean it up during optimization. However, when MIR inlining is enabled, this can cause problems as the operations occur prior to the spilled value being initialized. To solve this, we first calculate the necessary offsets using just the type which is side-effect free and does not alter the LLVM IR. Then, if we are in a situation which requires us to generate the LLVM IR (and this situation only occurs for arguments, not local variables) then we perform the same calculation again, this time generating the appropriate LLVM IR as we go. --- .../rustc_codegen_ssa/src/mir/debuginfo.rs | 29 +++++++++++++++++-- .../codegen/issue-105386-ub-in-debuginfo.rs | 4 +-- .../ui/debuginfo/issue-105386-debuginfo-ub.rs | 20 +++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/debuginfo/issue-105386-debuginfo-ub.rs diff --git a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs index 8935d42548f5f..b7982b633f57f 100644 --- a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs @@ -103,6 +103,28 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> DebugInfoOffsetLocation<'tcx, Bx> } } +impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> DebugInfoOffsetLocation<'tcx, Bx> + for TyAndLayout<'tcx> +{ + fn deref(&self, bx: &mut Bx) -> Self { + bx.cx().layout_of( + self.ty.builtin_deref(true).unwrap_or_else(|| bug!("cannot deref `{}`", self.ty)).ty, + ) + } + + fn layout(&self) -> TyAndLayout<'tcx> { + *self + } + + fn project_field(&self, bx: &mut Bx, field: mir::Field) -> Self { + self.field(bx.cx(), field.index()) + } + + fn downcast(&self, bx: &mut Bx, variant: VariantIdx) -> Self { + self.for_variant(bx.cx(), variant) + } +} + struct DebugInfoOffset { /// Offset from the `base` used to calculate the debuginfo offset. direct_offset: Size, @@ -340,8 +362,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let Some(dbg_var) = var.dbg_var else { continue }; let Some(dbg_loc) = self.dbg_loc(var.source_info) else { continue }; - let DebugInfoOffset { direct_offset, indirect_offsets, result: place } = - calculate_debuginfo_offset(bx, local, &var, base); + let DebugInfoOffset { direct_offset, indirect_offsets, result: _ } = + calculate_debuginfo_offset(bx, local, &var, base.layout); // When targeting MSVC, create extra allocas for arguments instead of pointing multiple // dbg_var_addr() calls into the same alloca with offsets. MSVC uses CodeView records @@ -359,6 +381,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { || !matches!(&indirect_offsets[..], [Size::ZERO] | [])); if should_create_individual_allocas { + let DebugInfoOffset { direct_offset: _, indirect_offsets: _, result: place } = + calculate_debuginfo_offset(bx, local, &var, base); + // Create a variable which will be a pointer to the actual value let ptr_ty = bx.tcx().mk_ty(ty::RawPtr(ty::TypeAndMut { mutbl: mir::Mutability::Mut, diff --git a/src/test/codegen/issue-105386-ub-in-debuginfo.rs b/src/test/codegen/issue-105386-ub-in-debuginfo.rs index e30f373a793c6..d54ac9e33bce2 100644 --- a/src/test/codegen/issue-105386-ub-in-debuginfo.rs +++ b/src/test/codegen/issue-105386-ub-in-debuginfo.rs @@ -16,7 +16,7 @@ pub fn outer_function(x: S, y: S) -> usize { // when generating debuginfo. // CHECK-LABEL: @outer_function // CHECK: [[spill:%.*]] = alloca %"[closure@{{.*.rs}}:9:23: 9:25]" -// CHECK: [[ptr_tmp:%.*]] = getelementptr inbounds %"[closure@{{.*.rs}}:9:23: 9:25]", ptr [[spill]] -// CHECK: [[load:%.*]] = load ptr, ptr [[ptr_tmp]] +// CHECK-NOT: [[ptr_tmp:%.*]] = getelementptr inbounds %"[closure@{{.*.rs}}:9:23: 9:25]", ptr [[spill]] +// CHECK-NOT: [[load:%.*]] = load ptr, ptr // CHECK: call void @llvm.lifetime.start{{.*}}({{.*}}, ptr [[spill]]) // CHECK: call void @llvm.memcpy{{.*}}(ptr {{align .*}} [[spill]], ptr {{align .*}} %x diff --git a/src/test/ui/debuginfo/issue-105386-debuginfo-ub.rs b/src/test/ui/debuginfo/issue-105386-debuginfo-ub.rs new file mode 100644 index 0000000000000..6c6eb5d4e86b7 --- /dev/null +++ b/src/test/ui/debuginfo/issue-105386-debuginfo-ub.rs @@ -0,0 +1,20 @@ +// run-pass +// compile-flags: --edition 2021 -Copt-level=3 -Cdebuginfo=2 -Zmir-opt-level=3 + +fn main() { + TranslatorI.visit_pre(); +} + +impl TranslatorI { + fn visit_pre(self) { + Some(()) + .map(|_| self.flags()) + .unwrap_or_else(|| self.flags()); + } +} + +struct TranslatorI; + +impl TranslatorI { + fn flags(&self) {} +}