Auto merge of #68665 - eddyb:debuginfo-early-create-var, r=nagisa

codegen: create DIVariables ahead of using them with llvm.dbg.declare.

Instead of having `rustc_codegen_llvm` bundle creation of a `DIVariable` and `llvm.dbg.declare` into a single operation, they are now two separate methods, and the `DIVariable` is created earlier, specifically when `mir::VarDebugInfo`s are being partitioned into locals.

While this isn't currently needed, it's a prerequisite for #48300, which adds fragmented debuginfo, where one `mir::VarDebugInfo` has multiple parts of itself mapped to different `mir::Place`s.
For debuggers to see one composite variable instead of several ones with the same name, we need to create a single `DIVariable` and share it between multiple `llvm.dbg.declare` calls, which are likely pointing to different MIR locals.
That makes the `per_local_var_debug_info` partitioning a good spot to do this in, as we can create *exactly* one `DIVariable` per `mir::VarDebugInfo`, and refer to it as many things as needed.

I'm opening this PR separately because I want to test its perf impact in isolation (see https://github.com/rust-lang/rust/pull/48300#issuecomment-580121438).

r? @nagisa or @oli-obk cc @michaelwoerister @nikomatsakis

src/librustc_codegen_llvm/builder.rs

@@ -57,6 +57,7 @@ impl BackendTypes for Builder<'_, 'll, 'tcx> {
     type Funclet = <CodegenCx<'ll, 'tcx> as BackendTypes>::Funclet;
 
     type DIScope = <CodegenCx<'ll, 'tcx> as BackendTypes>::DIScope;
+    type DIVariable = <CodegenCx<'ll, 'tcx> as BackendTypes>::DIVariable;
 }
 
 impl ty::layout::HasDataLayout for Builder<'_, '_, '_> {

src/librustc_codegen_llvm/common.rs

@@ -91,6 +91,7 @@ impl BackendTypes for CodegenCx<'ll, 'tcx> {
     type Funclet = Funclet<'ll>;
 
     type DIScope = &'ll llvm::debuginfo::DIScope;
+    type DIVariable = &'ll llvm::debuginfo::DIVariable;
 }
 
 impl CodegenCx<'ll, 'tcx> {

src/librustc_codegen_llvm/debuginfo/mod.rs

@@ -11,7 +11,7 @@ use self::utils::{create_DIArray, is_node_local_to_unit, span_start, DIB};
 
 use crate::llvm;
 use crate::llvm::debuginfo::{
-    DIArray, DIBuilder, DIFile, DIFlags, DILexicalBlock, DISPFlags, DIScope, DIType,
+    DIArray, DIBuilder, DIFile, DIFlags, DILexicalBlock, DISPFlags, DIScope, DIType, DIVariable,
 };
 use rustc::middle::codegen_fn_attrs::CodegenFnAttrFlags;
 use rustc::ty::subst::{GenericArgKind, SubstsRef};
@@ -143,33 +143,23 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) {
     };
 }
 
-impl DebugInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
+impl DebugInfoBuilderMethods for Builder<'a, 'll, 'tcx> {
-    fn declare_local(
+    // FIXME(eddyb) find a common convention for all of the debuginfo-related
+    // names (choose between `dbg`, `debug`, `debuginfo`, `debug_info` etc.).
+    fn dbg_var_addr(
         &mut self,
         dbg_context: &FunctionDebugContext<&'ll DIScope>,
-        variable_name: ast::Name,
+        dbg_var: &'ll DIVariable,
-        variable_type: Ty<'tcx>,
         scope_metadata: &'ll DIScope,
         variable_alloca: Self::Value,
         direct_offset: Size,
         indirect_offsets: &[Size],
-        variable_kind: VariableKind,
         span: Span,
     ) {
         assert!(!dbg_context.source_locations_enabled);
         let cx = self.cx();
 
-        let file = span_start(cx, span).file;
-        let file_metadata = file_metadata(cx, &file.name, dbg_context.defining_crate);
-
         let loc = span_start(cx, span);
-        let type_metadata = type_metadata(cx, variable_type, span);
-
-        let (argument_index, dwarf_tag) = match variable_kind {
-            ArgumentVariable(index) => (index as c_uint, DW_TAG_arg_variable),
-            LocalVariable => (0, DW_TAG_auto_variable),
-        };
-        let align = cx.align_of(variable_type);
 
         // Convert the direct and indirect offsets to address ops.
         let op_deref = || unsafe { llvm::LLVMRustDIBuilderCreateOpDeref() };
@@ -188,32 +178,19 @@ impl DebugInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
             }
         }
 
-        let name = SmallCStr::new(&variable_name.as_str());
+        // FIXME(eddyb) maybe this information could be extracted from `var`,
-        let metadata = unsafe {
+        // to avoid having to pass it down in both places?
-            llvm::LLVMRustDIBuilderCreateVariable(
-                DIB(cx),
-                dwarf_tag,
-                scope_metadata,
-                name.as_ptr(),
-                file_metadata,
-                loc.line as c_uint,
-                type_metadata,
-                cx.sess().opts.optimize != config::OptLevel::No,
-                DIFlags::FlagZero,
-                argument_index,
-                align.bytes() as u32,
-            )
-        };
         source_loc::set_debug_location(
             self,
             InternalDebugLocation::new(scope_metadata, loc.line, loc.col.to_usize()),
         );
         unsafe {
             let debug_loc = llvm::LLVMGetCurrentDebugLocation(self.llbuilder);
+            // FIXME(eddyb) replace `llvm.dbg.declare` with `llvm.dbg.addr`.
             let instr = llvm::LLVMRustDIBuilderInsertDeclareAtEnd(
                 DIB(cx),
                 variable_alloca,
-                metadata,
+                dbg_var,
                 addr_ops.as_ptr(),
                 addr_ops.len() as c_uint,
                 debug_loc,
@@ -313,7 +290,8 @@ impl DebugInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> {
         // Get the linkage_name, which is just the symbol name
         let linkage_name = mangled_name_of_instance(self, instance);
 
-        let scope_line = span_start(self, span).line;
+        // FIXME(eddyb) does this need to be separate from `loc.line` for some reason?
+        let scope_line = loc.line;
 
         let function_name = CString::new(name).unwrap();
         let linkage_name = SmallCStr::new(&linkage_name.name.as_str());
@@ -558,4 +536,44 @@ impl DebugInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> {
     fn debuginfo_finalize(&self) {
         finalize(self)
     }
+
+    // FIXME(eddyb) find a common convention for all of the debuginfo-related
+    // names (choose between `dbg`, `debug`, `debuginfo`, `debug_info` etc.).
+    fn create_dbg_var(
+        &self,
+        dbg_context: &FunctionDebugContext<&'ll DIScope>,
+        variable_name: ast::Name,
+        variable_type: Ty<'tcx>,
+        scope_metadata: &'ll DIScope,
+        variable_kind: VariableKind,
+        span: Span,
+    ) -> &'ll DIVariable {
+        let loc = span_start(self, span);
+        let file_metadata = file_metadata(self, &loc.file.name, dbg_context.defining_crate);
+
+        let type_metadata = type_metadata(self, variable_type, span);
+
+        let (argument_index, dwarf_tag) = match variable_kind {
+            ArgumentVariable(index) => (index as c_uint, DW_TAG_arg_variable),
+            LocalVariable => (0, DW_TAG_auto_variable),
+        };
+        let align = self.align_of(variable_type);
+
+        let name = SmallCStr::new(&variable_name.as_str());
+        unsafe {
+            llvm::LLVMRustDIBuilderCreateVariable(
+                DIB(self),
+                dwarf_tag,
+                scope_metadata,
+                name.as_ptr(),
+                file_metadata,
+                loc.line as c_uint,
+                type_metadata,
+                self.sess().opts.optimize != config::OptLevel::No,
+                DIFlags::FlagZero,
+                argument_index,
+                align.bytes() as u32,
+            )
+        }
+    }
 }

src/librustc_codegen_ssa/mir/debuginfo.rs

@@ -1,12 +1,12 @@
 use crate::traits::*;
 use rustc::mir;
 use rustc::session::config::DebugInfo;
+use rustc::ty;
 use rustc::ty::layout::{LayoutOf, Size};
-use rustc::ty::TyCtxt;
 use rustc_hir::def_id::CrateNum;
 use rustc_index::vec::IndexVec;
 
-use rustc_span::symbol::kw;
+use rustc_span::symbol::{kw, Symbol};
 use rustc_span::{BytePos, Span};
 
 use super::OperandValue;
@@ -24,6 +24,19 @@ pub enum VariableKind {
     LocalVariable,
 }
 
+/// Like `mir::VarDebugInfo`, but within a `mir::Local`.
+#[derive(Copy, Clone)]
+pub struct PerLocalVarDebugInfo<'tcx, D> {
+    pub name: Symbol,
+    pub source_info: mir::SourceInfo,
+
+    /// `DIVariable` returned by `create_dbg_var`.
+    pub dbg_var: Option<D>,
+
+    /// `.place.projection` from `mir::VarDebugInfo`.
+    pub projection: &'tcx ty::List<mir::PlaceElem<'tcx>>,
+}
+
 #[derive(Clone, Copy, Debug)]
 pub struct DebugScope<D> {
     pub scope_metadata: Option<D>,
@@ -103,6 +116,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
     // FIXME(eddyb) use `llvm.dbg.value` (which would work for operands),
     // not just `llvm.dbg.declare` (which requires `alloca`).
     pub fn debug_introduce_local(&self, bx: &mut Bx, local: mir::Local) {
+        let full_debug_info = bx.sess().opts.debuginfo == DebugInfo::Full;
+
         // FIXME(eddyb) maybe name the return place as `_0` or `return`?
         if local == mir::RETURN_PLACE {
             return;
@@ -112,35 +127,63 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             Some(per_local) => &per_local[local],
             None => return,
         };
-        let whole_local_var = vars.iter().copied().find(|var| var.place.projection.is_empty());
+        let whole_local_var = vars.iter().find(|var| var.projection.is_empty()).copied();
-        let has_proj = || vars.iter().any(|var| !var.place.projection.is_empty());
+        let has_proj = || vars.iter().any(|var| !var.projection.is_empty());
 
-        let (fallback_var, kind) = if self.mir.local_kind(local) == mir::LocalKind::Arg {
+        let fallback_var = if self.mir.local_kind(local) == mir::LocalKind::Arg {
             let arg_index = local.index() - 1;
 
             // Add debuginfo even to unnamed arguments.
             // FIXME(eddyb) is this really needed?
-            let var = if arg_index == 0 && has_proj() {
+            if arg_index == 0 && has_proj() {
                 // Hide closure environments from debuginfo.
                 // FIXME(eddyb) shouldn't `ArgumentVariable` indices
                 // be offset to account for the hidden environment?
                 None
+            } else if whole_local_var.is_some() {
+                // No need to make up anything, there is a `mir::VarDebugInfo`
+                // covering the whole local.
+                // FIXME(eddyb) take `whole_local_var.source_info.scope` into
+                // account, just in case it doesn't use `ArgumentVariable`
+                // (after #67586 gets fixed).
+                None
             } else {
-                Some(mir::VarDebugInfo {
+                let name = kw::Invalid;
-                    name: kw::Invalid,
+                let decl = &self.mir.local_decls[local];
-                    source_info: self.mir.local_decls[local].source_info,
+                let (scope, span) = if full_debug_info {
-                    place: local.into(),
+                    self.debug_loc(decl.source_info)
+                } else {
+                    (None, decl.source_info.span)
+                };
+                let dbg_var = scope.map(|scope| {
+                    // FIXME(eddyb) is this `+ 1` needed at all?
+                    let kind = VariableKind::ArgumentVariable(arg_index + 1);
+
+                    self.cx.create_dbg_var(
+                        self.debug_context.as_ref().unwrap(),
+                        name,
+                        self.monomorphize(&decl.ty),
+                        scope,
+                        kind,
+                        span,
+                    )
+                });
+
+                Some(PerLocalVarDebugInfo {
+                    name,
+                    source_info: decl.source_info,
+                    dbg_var,
+                    projection: ty::List::empty(),
                 })
-            };
+            }
-            (var, VariableKind::ArgumentVariable(arg_index + 1))
         } else {
-            (None, VariableKind::LocalVariable)
+            None
         };
 
         let local_ref = &self.locals[local];
 
         if !bx.sess().fewer_names() {
-            let name = match whole_local_var.or(fallback_var.as_ref()) {
+            let name = match whole_local_var.or(fallback_var) {
                 Some(var) if var.name != kw::Invalid => var.name.to_string(),
                 _ => format!("{:?}", local),
             };
@@ -163,7 +206,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             }
         }
 
-        if bx.sess().opts.debuginfo != DebugInfo::Full {
+        if !full_debug_info {
             return;
         }
 
@@ -178,11 +221,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             _ => return,
         };
 
-        let vars = vars.iter().copied().chain(if whole_local_var.is_none() {
+        let vars = vars.iter().copied().chain(fallback_var);
-            fallback_var.as_ref()
-        } else {
-            None
-        });
 
         for var in vars {
             let mut layout = base.layout;
@@ -190,10 +229,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             // FIXME(eddyb) use smallvec here.
             let mut indirect_offsets = vec![];
 
-            let kind =
+            for elem in &var.projection[..] {
-                if var.place.projection.is_empty() { kind } else { VariableKind::LocalVariable };
-
-            for elem in &var.place.projection[..] {
                 match *elem {
                     mir::ProjectionElem::Deref => {
                         indirect_offsets.push(Size::ZERO);
@@ -202,7 +238,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                                 .ty
                                 .builtin_deref(true)
                                 .unwrap_or_else(|| {
-                                    span_bug!(var.source_info.span, "cannot deref `{}`", layout.ty,)
+                                    span_bug!(var.source_info.span, "cannot deref `{}`", layout.ty)
                                 })
                                 .ty,
                         );
@@ -219,24 +255,24 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     _ => span_bug!(
                         var.source_info.span,
                         "unsupported var debuginfo place `{:?}`",
-                        var.place,
+                        mir::Place { local, projection: var.projection },
                     ),
                 }
             }
 
             let (scope, span) = self.debug_loc(var.source_info);
             if let Some(scope) = scope {
-                bx.declare_local(
+                if let Some(dbg_var) = var.dbg_var {
-                    debug_context,
+                    bx.dbg_var_addr(
-                    var.name,
+                        debug_context,
-                    layout.ty,
+                        dbg_var,
-                    scope,
+                        scope,
-                    base.llval,
+                        base.llval,
-                    direct_offset,
+                        direct_offset,
-                    &indirect_offsets,
+                        &indirect_offsets,
-                    kind,
+                        span,
-                    span,
+                    );
-                );
+                }
             }
         }
     }
@@ -248,20 +284,60 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             }
         }
     }
-}
 
-/// Partition all `VarDebuginfo` in `body`, by their base `Local`.
+    /// Partition all `VarDebugInfo` in `self.mir`, by their base `Local`.
-pub fn per_local_var_debug_info(
+    pub fn compute_per_local_var_debug_info(
-    tcx: TyCtxt<'tcx>,
+        &self,
-    body: &'a mir::Body<'tcx>,
+    ) -> Option<IndexVec<mir::Local, Vec<PerLocalVarDebugInfo<'tcx, Bx::DIVariable>>>> {
-) -> Option<IndexVec<mir::Local, Vec<&'a mir::VarDebugInfo<'tcx>>>> {
+        let full_debug_info = self.cx.sess().opts.debuginfo == DebugInfo::Full;
-    if tcx.sess.opts.debuginfo == DebugInfo::Full || !tcx.sess.fewer_names() {
+
-        let mut per_local = IndexVec::from_elem(vec![], &body.local_decls);
+        if !(full_debug_info || !self.cx.sess().fewer_names()) {
-        for var in &body.var_debug_info {
+            return None;
-            per_local[var.place.local].push(var);
+        }
+
+        let mut per_local = IndexVec::from_elem(vec![], &self.mir.local_decls);
+        for var in &self.mir.var_debug_info {
+            let (scope, span) = if full_debug_info {
+                self.debug_loc(var.source_info)
+            } else {
+                (None, var.source_info.span)
+            };
+            let dbg_var = scope.map(|scope| {
+                let place = var.place;
+                let var_ty = self.monomorphized_place_ty(place.as_ref());
+                let var_kind = if self.mir.local_kind(place.local) == mir::LocalKind::Arg
+                    && place.projection.is_empty()
+                {
+                    // FIXME(eddyb, #67586) take `var.source_info.scope` into
+                    // account to avoid using `ArgumentVariable` more than once
+                    // per argument local.
+
+                    let arg_index = place.local.index() - 1;
+
+                    // FIXME(eddyb) shouldn't `ArgumentVariable` indices be
+                    // offset in closures to account for the hidden environment?
+                    // Also, is this `+ 1` needed at all?
+                    VariableKind::ArgumentVariable(arg_index + 1)
+                } else {
+                    VariableKind::LocalVariable
+                };
+                self.cx.create_dbg_var(
+                    self.debug_context.as_ref().unwrap(),
+                    var.name,
+                    var_ty,
+                    scope,
+                    var_kind,
+                    span,
+                )
+            });
+
+            per_local[var.place.local].push(PerLocalVarDebugInfo {
+                name: var.name,
+                source_info: var.source_info,
+                dbg_var,
+                projection: var.place.projection,
+            });
         }
         Some(per_local)
-    } else {
-        None
     }
 }

src/librustc_codegen_ssa/mir/mod.rs

@@ -11,7 +11,7 @@ use rustc_index::bit_set::BitSet;
 use rustc_index::vec::IndexVec;
 
 use self::analyze::CleanupKind;
-use self::debuginfo::FunctionDebugContext;
+use self::debuginfo::{FunctionDebugContext, PerLocalVarDebugInfo};
 use self::place::PlaceRef;
 use rustc::mir::traversal;
 
@@ -74,9 +74,10 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {
     /// notably `expect`.
     locals: IndexVec<mir::Local, LocalRef<'tcx, Bx::Value>>,
 
-    /// All `VarDebuginfo` from the MIR body, partitioned by `Local`.
+    /// All `VarDebugInfo` from the MIR body, partitioned by `Local`.
-    /// This is `None` if no variable debuginfo/names are needed.
+    /// This is `None` if no var`#[non_exhaustive]`iable debuginfo/names are needed.
-    per_local_var_debug_info: Option<IndexVec<mir::Local, Vec<&'tcx mir::VarDebugInfo<'tcx>>>>,
+    per_local_var_debug_info:
+        Option<IndexVec<mir::Local, Vec<PerLocalVarDebugInfo<'tcx, Bx::DIVariable>>>>,
 
     /// Caller location propagated if this function has `#[track_caller]`.
     caller_location: Option<OperandRef<'tcx, Bx::Value>>,
@@ -178,10 +179,12 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
         funclets,
         locals: IndexVec::new(),
         debug_context,
-        per_local_var_debug_info: debuginfo::per_local_var_debug_info(cx.tcx(), mir_body),
+        per_local_var_debug_info: None,
         caller_location: None,
     };
 
+    fx.per_local_var_debug_info = fx.compute_per_local_var_debug_info();
+
     let memory_locals = analyze::non_ssa_locals(&fx);
 
     // Allocate variable and temp allocas

src/librustc_codegen_ssa/traits/backend.rs

@@ -21,7 +21,10 @@ pub trait BackendTypes {
     type Type: CodegenObject;
     type Funclet;
 
+    // FIXME(eddyb) find a common convention for all of the debuginfo-related
+    // names (choose between `Dbg`, `Debug`, `DebugInfo`, `DI` etc.).
     type DIScope: Copy;
+    type DIVariable: Copy;
 }
 
 pub trait Backend<'tcx>:

src/librustc_codegen_ssa/traits/builder.rs

@@ -28,7 +28,7 @@ pub enum OverflowOp {
 
 pub trait BuilderMethods<'a, 'tcx>:
     HasCodegen<'tcx>
-    + DebugInfoBuilderMethods<'tcx>
+    + DebugInfoBuilderMethods
     + ArgAbiMethods<'tcx>
     + AbiBuilderMethods<'tcx>
     + IntrinsicCallMethods<'tcx>

src/librustc_codegen_ssa/traits/debuginfo.rs

@@ -30,20 +30,32 @@ pub trait DebugInfoMethods<'tcx>: BackendTypes {
         defining_crate: CrateNum,
     ) -> Self::DIScope;
     fn debuginfo_finalize(&self);
-}
 
-pub trait DebugInfoBuilderMethods<'tcx>: BackendTypes {
+    // FIXME(eddyb) find a common convention for all of the debuginfo-related
-    fn declare_local(
+    // names (choose between `dbg`, `debug`, `debuginfo`, `debug_info` etc.).
-        &mut self,
+    fn create_dbg_var(
+        &self,
         dbg_context: &FunctionDebugContext<Self::DIScope>,
         variable_name: Name,
         variable_type: Ty<'tcx>,
         scope_metadata: Self::DIScope,
+        variable_kind: VariableKind,
+        span: Span,
+    ) -> Self::DIVariable;
+}
+
+pub trait DebugInfoBuilderMethods: BackendTypes {
+    // FIXME(eddyb) find a common convention for all of the debuginfo-related
+    // names (choose between `dbg`, `debug`, `debuginfo`, `debug_info` etc.).
+    fn dbg_var_addr(
+        &mut self,
+        dbg_context: &FunctionDebugContext<Self::DIScope>,
+        dbg_var: Self::DIVariable,
+        scope_metadata: Self::DIScope,
         variable_alloca: Self::Value,
         direct_offset: Size,
         // NB: each offset implies a deref (i.e. they're steps in a pointer chain).
         indirect_offsets: &[Size],
-        variable_kind: VariableKind,
         span: Span,
     );
     fn set_source_location(

src/librustc_codegen_ssa/traits/mod.rs

@@ -93,5 +93,6 @@ pub trait HasCodegen<'tcx>:
             Type = Self::Type,
             Funclet = Self::Funclet,
             DIScope = Self::DIScope,
+            DIVariable = Self::DIVariable,
         >;
 }