Rollup merge of #147608 - Zalathar:debuginfo, r=nnethercote

cg_llvm: Use `LLVMDIBuilderCreateGlobalVariableExpression`

- Part of #134001
- Follow-up to #146763

---

This PR dismantles the somewhat complicated `LLVMRustDIBuilderCreateStaticVariable` function, and replaces it with equivalent calls to `LLVMDIBuilderCreateGlobalVariableExpression` and `LLVMGlobalSetMetadata`.

A key difference is that the new code does not replicate the attempted downcast of `InitVal`. As far as I can tell, those downcasts were actually dead, because `llvm::ConstantInt` and `llvm::ConstantFP` are not subclasses of `llvm::GlobalVariable`. I tried replacing those code paths with fatal errors, and was unable to induce failure in any of the relevant test suites I ran.

I have also confirmed that if the calls to `create_static_variable` are commented out, debuginfo tests will fail, demonstrating some amount of relevant test coverage.

The new `DIBuilder` methods have been added via an extension trait, not as inherent methods, to avoid impeding #142897.

Author: GuillaumeGomez

compiler/rustc_codegen_llvm/src/debuginfo/di_builder.rs

@@ -0,0 +1,69 @@
+use libc::c_uint;
+use rustc_abi::Align;
+
+use crate::llvm::debuginfo::DIBuilder;
+use crate::llvm::{self, ToLlvmBool};
+
+/// Extension trait for defining safe wrappers and helper methods on
+/// `&DIBuilder<'ll>`, without requiring it to be defined in the same crate.
+pub(crate) trait DIBuilderExt<'ll> {
+    fn as_di_builder(&self) -> &DIBuilder<'ll>;
+
+    fn create_expression(&self, addr_ops: &[u64]) -> &'ll llvm::Metadata {
+        let this = self.as_di_builder();
+        unsafe { llvm::LLVMDIBuilderCreateExpression(this, addr_ops.as_ptr(), addr_ops.len()) }
+    }
+
+    fn create_static_variable(
+        &self,
+        scope: Option<&'ll llvm::Metadata>,
+        name: &str,
+        linkage_name: &str,
+        file: &'ll llvm::Metadata,
+        line_number: c_uint,
+        ty: &'ll llvm::Metadata,
+        is_local_to_unit: bool,
+        val: &'ll llvm::Value,
+        decl: Option<&'ll llvm::Metadata>,
+        align: Option<Align>,
+    ) -> &'ll llvm::Metadata {
+        let this = self.as_di_builder();
+        let align_in_bits = align.map_or(0, |align| align.bits() as u32);
+
+        // `LLVMDIBuilderCreateGlobalVariableExpression` would assert if we
+        // gave it a null `Expr` pointer, so give it an empty expression
+        // instead, which is what the C++ `createGlobalVariableExpression`
+        // method would do if given a null `DIExpression` pointer.
+        let expr = self.create_expression(&[]);
+
+        let global_var_expr = unsafe {
+            llvm::LLVMDIBuilderCreateGlobalVariableExpression(
+                this,
+                scope,
+                name.as_ptr(),
+                name.len(),
+                linkage_name.as_ptr(),
+                linkage_name.len(),
+                file,
+                line_number,
+                ty,
+                is_local_to_unit.to_llvm_bool(),
+                expr,
+                decl,
+                align_in_bits,
+            )
+        };
+
+        unsafe { llvm::LLVMGlobalSetMetadata(val, llvm::MD_dbg, global_var_expr) };
+
+        global_var_expr
+    }
+}
+
+impl<'ll> DIBuilderExt<'ll> for &DIBuilder<'ll> {
+    fn as_di_builder(&self) -> &DIBuilder<'ll> {
+        self
+    }
+
+    // All other methods have default bodies that rely on `as_di_builder`.
+}

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

@@ -34,9 +34,9 @@ use super::namespace::mangled_name_of_instance;
 use super::type_names::{compute_debuginfo_type_name, compute_debuginfo_vtable_name};
 use super::utils::{DIB, debug_context, get_namespace_for_item, is_node_local_to_unit};
 use crate::common::{AsCCharPtr, CodegenCx};
-use crate::debuginfo::dwarf_const;
 use crate::debuginfo::metadata::type_map::build_type_with_children;
 use crate::debuginfo::utils::{WidePtrKind, wide_pointer_kind};
+use crate::debuginfo::{DIBuilderExt, dwarf_const};
 use crate::llvm::debuginfo::{
     DIBasicType, DIBuilder, DICompositeType, DIDescriptor, DIFile, DIFlags, DILexicalBlock,
     DIScope, DIType, DebugEmissionKind, DebugNameTableKind,
@@ -1410,23 +1410,18 @@ pub(crate) fn build_global_var_di_node<'ll>(
 
     let global_align = cx.align_of(variable_type);
 
-    unsafe {
-        llvm::LLVMRustDIBuilderCreateStaticVariable(
-            DIB(cx),
-            Some(var_scope),
-            var_name.as_c_char_ptr(),
-            var_name.len(),
-            linkage_name.as_c_char_ptr(),
-            linkage_name.len(),
-            file_metadata,
-            line_number,
-            type_di_node,
-            is_local_to_unit,
-            global,
-            None,
-            global_align.bits() as u32,
-        );
-    }
+    DIB(cx).create_static_variable(
+        Some(var_scope),
+        var_name,
+        linkage_name,
+        file_metadata,
+        line_number,
+        type_di_node,
+        is_local_to_unit,
+        global, // (value)
+        None,   // (decl)
+        Some(global_align),
+    );
 }
 
 /// Generates LLVM debuginfo for a vtable.
@@ -1643,25 +1638,19 @@ pub(crate) fn create_vtable_di_node<'ll, 'tcx>(
     let vtable_name =
         compute_debuginfo_vtable_name(cx.tcx, ty, poly_trait_ref, VTableNameKind::GlobalVariable);
     let vtable_type_di_node = build_vtable_type_di_node(cx, ty, poly_trait_ref);
-    let linkage_name = "";
 
-    unsafe {
-        llvm::LLVMRustDIBuilderCreateStaticVariable(
-            DIB(cx),
-            NO_SCOPE_METADATA,
-            vtable_name.as_c_char_ptr(),
-            vtable_name.len(),
-            linkage_name.as_c_char_ptr(),
-            linkage_name.len(),
-            unknown_file_metadata(cx),
-            UNKNOWN_LINE_NUMBER,
-            vtable_type_di_node,
-            true,
-            vtable,
-            None,
-            0,
-        );
-    }
+    DIB(cx).create_static_variable(
+        NO_SCOPE_METADATA,
+        &vtable_name,
+        "", // (linkage_name)
+        unknown_file_metadata(cx),
+        UNKNOWN_LINE_NUMBER,
+        vtable_type_di_node,
+        true,   // (is_local_to_unit)
+        vtable, // (value)
+        None,   // (decl)
+        None::<Align>,
+    );
 }
 
 /// Creates an "extension" of an existing `DIScope` into another file.

compiler/rustc_codegen_llvm/src/debuginfo/mod.rs

@@ -28,6 +28,9 @@ use rustc_target::spec::DebuginfoKind;
 use smallvec::SmallVec;
 use tracing::debug;
 
+use self::create_scope_map::compute_mir_scopes;
+pub(crate) use self::di_builder::DIBuilderExt;
+pub(crate) use self::metadata::build_global_var_di_node;
 use self::metadata::{
     UNKNOWN_COLUMN_NUMBER, UNKNOWN_LINE_NUMBER, file_metadata, spanned_type_di_node, type_di_node,
 };
@@ -42,15 +45,13 @@ use crate::llvm::debuginfo::{
 use crate::llvm::{self, Value};
 
 mod create_scope_map;
+mod di_builder;
 mod dwarf_const;
 mod gdb;
 pub(crate) mod metadata;
 mod namespace;
 mod utils;
 
-use self::create_scope_map::compute_mir_scopes;
-pub(crate) use self::metadata::build_global_var_di_node;
-
 /// A context object for maintaining all state needed by the debuginfo module.
 pub(crate) struct CodegenUnitDebugContext<'ll, 'tcx> {
     llmod: &'ll llvm::Module,
@@ -182,9 +183,7 @@ impl<'ll> DebugInfoBuilderMethods for Builder<'_, 'll, '_> {
         }
 
         let di_builder = DIB(self.cx());
-        let addr_expr = unsafe {
-            llvm::LLVMDIBuilderCreateExpression(di_builder, addr_ops.as_ptr(), addr_ops.len())
-        };
+        let addr_expr = di_builder.create_expression(&addr_ops);
         unsafe {
             llvm::LLVMDIBuilderInsertDeclareRecordAtEnd(
                 di_builder,

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -22,9 +22,9 @@ use libc::{c_char, c_int, c_uchar, c_uint, c_ulonglong, c_void, size_t};
 
 use super::RustString;
 use super::debuginfo::{
-    DIArray, DIBuilder, DIDerivedType, DIDescriptor, DIEnumerator, DIFile, DIFlags,
-    DIGlobalVariableExpression, DILocation, DISPFlags, DIScope, DISubprogram,
-    DITemplateTypeParameter, DIType, DebugEmissionKind, DebugNameTableKind,
+    DIArray, DIBuilder, DIDerivedType, DIDescriptor, DIEnumerator, DIFile, DIFlags, DILocation,
+    DISPFlags, DIScope, DISubprogram, DITemplateTypeParameter, DIType, DebugEmissionKind,
+    DebugNameTableKind,
 };
 use crate::llvm::MetadataKindId;
 use crate::{TryFromU32, llvm};
@@ -781,7 +781,6 @@ pub(crate) mod debuginfo {
     pub(crate) type DIDerivedType = DIType;
     pub(crate) type DICompositeType = DIDerivedType;
     pub(crate) type DIVariable = DIDescriptor;
-    pub(crate) type DIGlobalVariableExpression = DIDescriptor;
     pub(crate) type DIArray = DIDescriptor;
     pub(crate) type DIEnumerator = DIDescriptor;
     pub(crate) type DITemplateTypeParameter = DIDescriptor;
@@ -1877,6 +1876,22 @@ unsafe extern "C" {
         Length: size_t,
     ) -> &'ll Metadata;
 
+    pub(crate) fn LLVMDIBuilderCreateGlobalVariableExpression<'ll>(
+        Builder: &DIBuilder<'ll>,
+        Scope: Option<&'ll Metadata>,
+        Name: *const c_uchar, // See "PTR_LEN_STR".
+        NameLen: size_t,
+        Linkage: *const c_uchar, // See "PTR_LEN_STR".
+        LinkLen: size_t,
+        File: &'ll Metadata,
+        LineNo: c_uint,
+        Ty: &'ll Metadata,
+        LocalToUnit: llvm::Bool,
+        Expr: &'ll Metadata,
+        Decl: Option<&'ll Metadata>,
+        AlignInBits: u32,
+    ) -> &'ll Metadata;
+
     pub(crate) fn LLVMDIBuilderInsertDeclareRecordAtEnd<'ll>(
         Builder: &DIBuilder<'ll>,
         Storage: &'ll Value,
@@ -2213,22 +2228,6 @@ unsafe extern "C" {
         Ty: &'a DIType,
     ) -> &'a DIType;
 
-    pub(crate) fn LLVMRustDIBuilderCreateStaticVariable<'a>(
-        Builder: &DIBuilder<'a>,
-        Context: Option<&'a DIScope>,
-        Name: *const c_char,
-        NameLen: size_t,
-        LinkageName: *const c_char,
-        LinkageNameLen: size_t,
-        File: &'a DIFile,
-        LineNo: c_uint,
-        Ty: &'a DIType,
-        isLocalToUnit: bool,
-        Val: &'a Value,
-        Decl: Option<&'a DIDescriptor>,
-        AlignInBits: u32,
-    ) -> &'a DIGlobalVariableExpression;
-
     pub(crate) fn LLVMRustDIBuilderCreateEnumerator<'a>(
         Builder: &DIBuilder<'a>,
         Name: *const c_char,

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

@@ -1038,37 +1038,6 @@ extern "C" LLVMMetadataRef LLVMRustDIBuilderCreateVariantMemberType(
       fromRust(Flags), unwrapDI<DIType>(Ty)));
 }
 
-extern "C" LLVMMetadataRef LLVMRustDIBuilderCreateStaticVariable(
-    LLVMDIBuilderRef Builder, LLVMMetadataRef Context, const char *Name,
-    size_t NameLen, const char *LinkageName, size_t LinkageNameLen,
-    LLVMMetadataRef File, unsigned LineNo, LLVMMetadataRef Ty,
-    bool IsLocalToUnit, LLVMValueRef V, LLVMMetadataRef Decl = nullptr,
-    uint32_t AlignInBits = 0) {
-  llvm::GlobalVariable *InitVal = cast<llvm::GlobalVariable>(unwrap(V));
-
-  llvm::DIExpression *InitExpr = nullptr;
-  if (llvm::ConstantInt *IntVal = llvm::dyn_cast<llvm::ConstantInt>(InitVal)) {
-    InitExpr = unwrap(Builder)->createConstantValueExpression(
-        IntVal->getValue().getSExtValue());
-  } else if (llvm::ConstantFP *FPVal =
-                 llvm::dyn_cast<llvm::ConstantFP>(InitVal)) {
-    InitExpr = unwrap(Builder)->createConstantValueExpression(
-        FPVal->getValueAPF().bitcastToAPInt().getZExtValue());
-  }
-
-  llvm::DIGlobalVariableExpression *VarExpr =
-      unwrap(Builder)->createGlobalVariableExpression(
-          unwrapDI<DIDescriptor>(Context), StringRef(Name, NameLen),
-          StringRef(LinkageName, LinkageNameLen), unwrapDI<DIFile>(File),
-          LineNo, unwrapDI<DIType>(Ty), IsLocalToUnit,
-          /* isDefined */ true, InitExpr, unwrapDIPtr<MDNode>(Decl),
-          /* templateParams */ nullptr, AlignInBits);
-
-  InitVal->setMetadata("dbg", VarExpr);
-
-  return wrap(VarExpr);
-}
-
 extern "C" LLVMMetadataRef
 LLVMRustDIBuilderCreateEnumerator(LLVMDIBuilderRef Builder, const char *Name,
                                   size_t NameLen, const uint64_t Value[2],

tests/debuginfo/basic-types-globals.rs

@@ -6,6 +6,38 @@
 //@ [lto] compile-flags:-C lto
 //@ [lto] no-prefer-dynamic
 
+// lldb-command:run
+// lldb-command:v B
+// lldb-check: ::B::[...] = false
+// lldb-command:v I
+// lldb-check: ::I::[...] = -1
+// lldb-command:v --format=d C
+// lldb-check: ::C::[...] = 97
+// lldb-command:v --format=d I8
+// lldb-check: ::I8::[...] = 68
+// lldb-command:v I16
+// lldb-check: ::I16::[...] = -16
+// lldb-command:v I32
+// lldb-check: ::I32::[...] = -32
+// lldb-command:v I64
+// lldb-check: ::I64::[...] = -64
+// lldb-command:v U
+// lldb-check: ::U::[...] = 1
+// lldb-command:v --format=d U8
+// lldb-check: ::U8::[...] = 100
+// lldb-command:v U16
+// lldb-check: ::U16::[...] = 16
+// lldb-command:v U32
+// lldb-check: ::U32::[...] = 32
+// lldb-command:v U64
+// lldb-check: ::U64::[...] = 64
+// lldb-command:v F16
+// lldb-check: ::F16::[...] = 1.5
+// lldb-command:v F32
+// lldb-check: ::F32::[...] = 2.5
+// lldb-command:v F64
+// lldb-check: ::F64::[...] = 3.5
+
 // gdb-command:run
 // gdb-command:print B
 // gdb-check:$1 = false