Reland "[clang][DebugInfo] Emit global variable definitions for static data members with constant initializers"

[Michael137]

This patch relands #70639

It was reverted because under certain conditions we triggered an assertion
in DIBuilder. Specifically, in the original patch we called EmitGlobalVariable
at the end of CGDebugInfo::finalize, after all the temporary DITypes have
been uniqued. With limited debug-info such temporary nodes would be created
more frequently, leaving us with non-uniqued nodes by the time we got to
DIBuilder::finalize; this violated its pre-condition and caused
assertions to trigger.

To fix this, the latest iteration of the patch moves EmitGlobalVariable to the
beginning of CGDebugInfo::finalize. Now, when we create a temporary
DIType node as a result of emitting a variable definition, it will get uniqued
in time. A test-case was added for this scenario.

We also now don't emit a linkage name for non-locationed constants since
LLDB doesn't make use of it anyway.

Original commit message:
"""
When an LLDB user asks for the value of a static data member, LLDB starts
by searching the Names accelerator table for the corresponding variable
definition DIE. For static data members with out-of-class definitions that
works fine, because those get represented as global variables with a location
and making them eligible to be added to the Names table. However, in-class
definitions won’t get indexed because we usually don't emit global variables
for them. So in DWARF we end up with a single DW_TAG_member that
usually holds the constant initializer. But we don't get a corresponding
CU-level DW_TAG_variable like we do for out-of-class definitions.

To make it more convenient for debuggers to get to the value of inline static data
members, this patch makes sure we emit definitions for static variables with
constant initializers the same way we do for other static variables. This also aligns
Clang closer to GCC, which produces CU-level definitions for inline statics and also
emits these into .debug_pubnames.

The implementation keeps track of newly created static data members.
Then in CGDebugInfo::finalize, we emit a global DW_TAG_variable with a
DW_AT_const_value for any of those declarations that didn't end up with a
definition in the DeclCache.

The newly emitted DW_TAG_variable will look as follows:

0x0000007b:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("Foo")
                ...

0x0000008d:     DW_TAG_member
                  DW_AT_name    ("i")
                  DW_AT_type    (0x00000062 "const int")
                  DW_AT_external        (true)
                  DW_AT_declaration     (true)
                  DW_AT_const_value     (4)

Newly added
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv

0x0000009a:   DW_TAG_variable
                DW_AT_specification     (0x0000008d "i")
                DW_AT_const_value       (4)
                DW_AT_linkage_name      ("_ZN2t2IiE1iIfEE")

This patch also drops the DW_AT_const_value off of the declaration since we
now always have it on the definition. This ensures that the DWARFParallelLinker
can type-merge class with static members where we couldn't attach the constant
on the declaration in some CUs.
"""

Dependent changes:

• Reland "[lldb][DWARFASTParserClang] Fetch constant value from variable defintion if available" #71800

[llvmbot]

@llvm/pr-subscribers-lldb
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang

Author: Michael Buch (Michael137)

Changes

This patch relands #70639

It was reverted because under certain conditions we triggered an assertion
in DIBuilder. Specifically, in the original patch we called EmitGlobalVariable
at the end of CGDebugInfo::finalize, after all the temporary DITypes have
been uniqued. With limited debug-info such temporary nodes would be created
more frequently, leaving us with non-uniqued nodes by the time we got to
DIBuilder::finalize; this violated its pre-condition and caused
assertions to trigger.

To fix this, the latest iteration of the patch moves EmitGlobalVariable to the
beginning of CGDebugInfo::finalize. Now, when we create a temporary
DIType node as a result of emitting a variable definition, it will get uniqued
in time. A test-case was added for this scenario.

Original commit message:
"""
When an LLDB user asks for the value of a static data member, LLDB starts
by searching the Names accelerator table for the corresponding variable
definition DIE. For static data members with out-of-class definitions that
works fine, because those get represented as global variables with a location
and making them eligible to be added to the Names table. However, in-class
definitions won’t get indexed because we usually don't emit global variables
for them. So in DWARF we end up with a single DW_TAG_member that
usually holds the constant initializer. But we don't get a corresponding
CU-level DW_TAG_variable like we do for out-of-class definitions.

To make it more convenient for debuggers to get to the value of inline static data
members, this patch makes sure we emit definitions for static variables with
constant initializers the same way we do for other static variables. This also aligns
Clang closer to GCC, which produces CU-level definitions for inline statics and also
emits these into .debug_pubnames.

The implementation keeps track of newly created static data members.
Then in CGDebugInfo::finalize, we emit a global DW_TAG_variable with a
DW_AT_const_value for any of those declarations that didn't end up with a
definition in the DeclCache.

The newly emitted DW_TAG_variable will look as follows:

0x0000007b:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("Foo")
                ...

0x0000008d:     DW_TAG_member
                  DW_AT_name    ("i")
                  DW_AT_type    (0x00000062 "const int")
                  DW_AT_external        (true)
                  DW_AT_declaration     (true)
                  DW_AT_const_value     (4)

Newly added
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv

0x0000009a:   DW_TAG_variable
                DW_AT_specification     (0x0000008d "i")
                DW_AT_const_value       (4)
                DW_AT_linkage_name      ("_ZN2t2IiE1iIfEE")

This patch also drops the DW_AT_const_value off of the declaration since we
now always have it on the definition. This ensures that the DWARFParallelLinker
can type-merge class with static members where we couldn't attach the constant
on the declaration in some CUs.
"""

Dependent changes:

• [lldb][DWARFASTParserClang] Fetch constant value from variable defintion if available #71004
• [lldb][test] Add FindGlobalVariables tests for C++ inline static data members #70641

---

Full diff: https://github.com/llvm/llvm-project/pull/71780.diff

6 Files Affected:

• (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+47-11)
• (modified) clang/lib/CodeGen/CGDebugInfo.h (+6)
• (modified) clang/test/CodeGenCXX/debug-info-class.cpp (+9-4)
• (modified) clang/test/CodeGenCXX/debug-info-static-member.cpp (+32-20)
• (modified) lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py (+4-3)
• (modified) lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py (+5-4)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 84a166d3ac3659c..245f7516640d098 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1677,22 +1677,13 @@ CGDebugInfo::CreateRecordStaticField(const VarDecl *Var, llvm::DIType *RecordTy,
 
   unsigned LineNumber = getLineNumber(Var->getLocation());
   StringRef VName = Var->getName();
-  llvm::Constant *C = nullptr;
-  if (Var->getInit()) {
-    const APValue *Value = Var->evaluateValue();
-    if (Value) {
-      if (Value->isInt())
-        C = llvm::ConstantInt::get(CGM.getLLVMContext(), Value->getInt());
-      if (Value->isFloat())
-        C = llvm::ConstantFP::get(CGM.getLLVMContext(), Value->getFloat());
-    }
-  }
 
   llvm::DINode::DIFlags Flags = getAccessFlag(Var->getAccess(), RD);
   auto Align = getDeclAlignIfRequired(Var, CGM.getContext());
   llvm::DIDerivedType *GV = DBuilder.createStaticMemberType(
-      RecordTy, VName, VUnit, LineNumber, VTy, Flags, C, Align);
+      RecordTy, VName, VUnit, LineNumber, VTy, Flags, /* Val */ nullptr, Align);
   StaticDataMemberCache[Var->getCanonicalDecl()].reset(GV);
+  StaticDataMemberDefinitionsToEmit.push_back(Var->getCanonicalDecl());
   return GV;
 }
 
@@ -5596,6 +5587,39 @@ void CGDebugInfo::EmitGlobalVariable(const ValueDecl *VD, const APValue &Init) {
       TemplateParameters, Align));
 }
 
+void CGDebugInfo::EmitGlobalVariable(const VarDecl *VD) {
+  assert(VD->hasInit());
+  assert(CGM.getCodeGenOpts().hasReducedDebugInfo());
+  if (VD->hasAttr<NoDebugAttr>())
+    return;
+
+  auto &GV = DeclCache[VD];
+  if (GV)
+    return;
+
+  auto const *InitVal = VD->evaluateValue();
+  if (!InitVal)
+    return;
+
+  llvm::DIFile *Unit = nullptr;
+  llvm::DIScope *DContext = nullptr;
+  unsigned LineNo;
+  StringRef DeclName, LinkageName;
+  QualType T;
+  llvm::MDTuple *TemplateParameters = nullptr;
+  collectVarDeclProps(VD, Unit, LineNo, T, DeclName, LinkageName,
+                      TemplateParameters, DContext);
+
+  auto Align = getDeclAlignIfRequired(VD, CGM.getContext());
+  llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(VD);
+  llvm::DIExpression *InitExpr = createConstantValueExpression(VD, *InitVal);
+
+  GV.reset(DBuilder.createGlobalVariableExpression(
+      TheCU, DeclName, LinkageName, Unit, LineNo, getOrCreateType(T, Unit),
+      true, true, InitExpr, getOrCreateStaticDataMemberDeclarationOrNull(VD),
+      TemplateParameters, Align, Annotations));
+}
+
 void CGDebugInfo::EmitExternalVariable(llvm::GlobalVariable *Var,
                                        const VarDecl *D) {
   assert(CGM.getCodeGenOpts().hasReducedDebugInfo());
@@ -5800,6 +5824,18 @@ void CGDebugInfo::setDwoId(uint64_t Signature) {
 }
 
 void CGDebugInfo::finalize() {
+  for (auto const *VD : StaticDataMemberDefinitionsToEmit) {
+    assert(VD->isStaticDataMember());
+
+    if (DeclCache.contains(VD))
+      continue;
+
+    if (!VD->hasInit())
+      continue;
+
+    EmitGlobalVariable(VD);
+  }
+
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
   for (size_t i = 0; i != ObjCInterfaceCache.size(); ++i) {
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index 7b60e94555d0608..3e4c133b7f2b9f1 100644
--- a/clang/lib/CodeGen/CGDebugInfo.h
+++ b/clang/lib/CodeGen/CGDebugInfo.h
@@ -161,6 +161,9 @@ class CGDebugInfo {
   llvm::DenseMap<const Decl *, llvm::TypedTrackingMDRef<llvm::DIDerivedType>>
       StaticDataMemberCache;
 
+  /// Keeps track of static data members for which we should emit a definition.
+  std::vector<const VarDecl *> StaticDataMemberDefinitionsToEmit;
+
   using ParamDecl2StmtTy = llvm::DenseMap<const ParmVarDecl *, const Stmt *>;
   using Param2DILocTy =
       llvm::DenseMap<const ParmVarDecl *, llvm::DILocalVariable *>;
@@ -526,6 +529,9 @@ class CGDebugInfo {
   /// Emit a constant global variable's debug info.
   void EmitGlobalVariable(const ValueDecl *VD, const APValue &Init);
 
+  /// Emit debug-info for a variable with a constant initializer.
+  void EmitGlobalVariable(const VarDecl *VD);
+
   /// Emit information about an external variable.
   void EmitExternalVariable(llvm::GlobalVariable *GV, const VarDecl *Decl);
 
diff --git a/clang/test/CodeGenCXX/debug-info-class.cpp b/clang/test/CodeGenCXX/debug-info-class.cpp
index ef84b91b67821a9..a3111cd7c3640a0 100644
--- a/clang/test/CodeGenCXX/debug-info-class.cpp
+++ b/clang/test/CodeGenCXX/debug-info-class.cpp
@@ -116,11 +116,19 @@ int main(int argc, char **argv) {
 // CHECK-SAME:                             DIFlagFwdDecl
 // CHECK-NOT:                             identifier:
 // CHECK-SAME:                            ){{$}}
+
+// CHECK:      !DIGlobalVariableExpression(var: ![[HDR_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 52, DW_OP_stack_value))
+// CHECK:      ![[HDR_VAR]] = distinct !DIGlobalVariable(name: "HdrSize",
+// CHECK-SAME:                isLocal: true, isDefinition: true, declaration: ![[HDR_VAR_DECL:[0-9]+]])
+// CHECK:      ![[INT:[0-9]+]] = !DIBasicType(name: "int"
+// CHECK:      ![[HDR_VAR_DECL]] = !DIDerivedType(tag: DW_TAG_member, name: "HdrSize"
+
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "A"
+
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "I"
 // CHECK-NOT:              DIFlagFwdDecl
 // CHECK-SAME:             ){{$}}
 
-// CHECK: ![[INT:[0-9]+]] = !DIBasicType(name: "int"
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
 // CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "bar"
 // CHECK: !DICompositeType(tag: DW_TAG_union_type, name: "baz"
@@ -186,8 +194,5 @@ int main(int argc, char **argv) {
 // CHECK: [[G_INNER_I]] = !DIDerivedType(tag: DW_TAG_member, name: "j"
 // CHECK-SAME:                           baseType: ![[INT]]
 
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "A"
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "HdrSize"
-//
 // CHECK: ![[EXCEPTLOC]] = !DILocation(line: 100,
 // CHECK: ![[RETLOC]] = !DILocation(line: 99,
diff --git a/clang/test/CodeGenCXX/debug-info-static-member.cpp b/clang/test/CodeGenCXX/debug-info-static-member.cpp
index 260a3afdd6524f3..578995801c6400b 100644
--- a/clang/test/CodeGenCXX/debug-info-static-member.cpp
+++ b/clang/test/CodeGenCXX/debug-info-static-member.cpp
@@ -63,19 +63,19 @@ int C::a = 4;
 // CHECK-NOT:                                 offset:
 // CHECK-SAME:                                flags: DIFlagStaticMember)
 //
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "const_a"
-// CHECK-NOT:            size:
-// CHECK-NOT:            align:
-// CHECK-NOT:            offset:
-// CHECK-SAME:           flags: DIFlagStaticMember,
-// CHECK-SAME:           extraData: i1 true)
-
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "const_b"
-// CHECK-NOT:            size:
-// CHECK-NOT:            align:
-// CHECK-NOT:            offset:
-// CHECK-SAME:           flags: DIFlagProtected | DIFlagStaticMember,
-// CHECK-SAME:           extraData: float 0x{{.*}})
+// CHECK:    ![[CONST_A_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "const_a"
+// CHECK-NOT:                           size:
+// CHECK-NOT:                           align:
+// CHECK-NOT:                           offset:
+// CHECK-SAME:                          flags: DIFlagStaticMember
+// CHECK-NOT:                           extraData:
+
+// CHECK:     ![[CONST_B_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "const_b"
+// CHECK-NOT:                            size:
+// CHECK-NOT:                            align:
+// CHECK-NOT:                            offset:
+// CHECK-SAME:                           flags: DIFlagProtected | DIFlagStaticMember
+// CHECK-NOT:                            extraData:
 
 // CHECK: ![[DECL_C:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "c"
 // CHECK-NOT:                                 size:
@@ -83,12 +83,12 @@ int C::a = 4;
 // CHECK-NOT:                                 offset:
 // CHECK-SAME:                                flags: DIFlagPublic | DIFlagStaticMember)
 //
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "const_c"
-// CHECK-NOT:            size:
-// CHECK-NOT:            align:
-// CHECK-NOT:            offset:
-// CHECK-SAME:           flags: DIFlagPublic | DIFlagStaticMember,
-// CHECK-SAME:           extraData: i32 18)
+// CHECK:     ![[CONST_C_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "const_c"
+// CHECK-NOT:                            size:
+// CHECK-NOT:                            align:
+// CHECK-NOT:                            offset:
+// CHECK-SAME:                           flags: DIFlagPublic | DIFlagStaticMember
+// CHECK-NOT:                            extraData:
 //
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "x_a"
 // CHECK-SAME:           flags: DIFlagPublic | DIFlagStaticMember)
@@ -144,7 +144,7 @@ struct V {
 // const_va is not emitted for MS targets.
 // NOT-MS: !DIDerivedType(tag: DW_TAG_member, name: "const_va",
 // NOT-MS-SAME:           line: [[@LINE-5]]
-// NOT-MS-SAME:           extraData: i32 42
+// NOT-MS-NOT:            extraData:
 const int V::const_va;
 
 namespace x {
@@ -156,3 +156,15 @@ struct y {
 };
 int y::z;
 }
+
+// CHECK:      !DIGlobalVariableExpression(var: ![[CONST_A_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 1, DW_OP_stack_value))
+// CHECK:      ![[CONST_A_VAR]] = distinct !DIGlobalVariable(name: "const_a"
+// CHECK-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_A_DECL]])
+
+// CHECK:      !DIGlobalVariableExpression(var: ![[CONST_B_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
+// CHECK:      ![[CONST_B_VAR]] = distinct !DIGlobalVariable(name: "const_b"
+// CHECK-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_B_DECL]])
+
+// CHECK:      !DIGlobalVariableExpression(var: ![[CONST_C_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 18, DW_OP_stack_value))
+// CHECK:      ![[CONST_C_VAR]] = distinct !DIGlobalVariable(name: "const_c"
+// CHECK-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_C_DECL]])
diff --git a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
index 530191e8a37ba1b..78ea23ac8f70610 100644
--- a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -102,9 +102,10 @@ def test(self):
         # it does not crash.
         self.expect("image lookup -t A")
 
-    # dsymutil strips the debug info for classes that only have const static
-    # data members without a definition namespace scope.
-    @expectedFailureAll(debug_info=["dsym"])
+    # For debug-info produced by older versions of clang, dsymutil strips the
+    # debug info for classes that only have const static data members without
+    # definitions.
+    @expectedFailureAll(compiler=["clang"], compiler_version=["<", "18.0"])
     def test_class_with_only_const_static(self):
         self.build()
         lldbutil.run_to_source_breakpoint(
diff --git a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
index 11d9bf18db7a6e1..d2a21ad3cd1d44f 100644
--- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
+++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
@@ -87,10 +87,11 @@ def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
     def verify_variables(self, verify_dict, variables, varref_dict=None):
         for variable in variables:
             name = variable["name"]
-            self.assertIn(
-                name, verify_dict, 'variable "%s" in verify dictionary' % (name)
-            )
-            self.verify_values(verify_dict[name], variable, varref_dict)
+            if not name.startswith("std::"):
+                self.assertIn(
+                    name, verify_dict, 'variable "%s" in verify dictionary' % (name)
+                )
+                self.verify_values(verify_dict[name], variable, varref_dict)
 
     def darwin_dwarf_missing_obj(self, initCommands):
         self.build(debug_info="dwarf")

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[adrian-prantl]

Minor comments inside.

[adrian-prantl]

is it intentional that we cache nullptr here if the next condition fails?

[Michael137]

Good catch, I'll err on the side of caution and not do that

[adrian-prantl]

it's non-obvious why this skip exists, can you add a comment here?

[Michael137]

I'll update the EmitGlobalVariable docs to clarify that we need an initialiser, otherwise behaviour is undefined

[zmodem]

We're seeing crashes after this. Here is a reproducer: https://bugs.chromium.org/p/chromium/issues/detail?id=1502489#c1

Please consider reverting if it can't be fixed quickly (I had a look, but it seems there are some dependent changes on top).

[Michael137]

We're seeing crashes after this. Here is a reproducer: https://bugs.chromium.org/p/chromium/issues/detail?id=1502489#c1

Please consider reverting if it can't be fixed quickly (I had a look, but it seems there are some dependent changes on top).

Thanks for reporting, will check and revert if not obvious what's wrong

[Michael137]

We're seeing crashes after this. Here is a reproducer: https://bugs.chromium.org/p/chromium/issues/detail?id=1502489#c1
Please consider reverting if it can't be fixed quickly (I had a look, but it seems there are some dependent changes on top).

Thanks for reporting, will check and revert if not obvious what's wrong

Just based on the backtrace the only cause I see would be if we put a nullptr into StaticDataMemberDefinitionsToEmit. We unconditionally dereference it CGDebugInfo::finalize. In which case the fix is pretty straightforward. Building a local debug build to confirm this theory.

[Michael137]

Ok with ASAN I could trigger this quite consistently. Turns out we can't iterate over the StaticDataMemberDefinitionsToEmit using a for each because during EmitGlobalVariable we can end up adding more things to the vector, invalidating the iterators. Will push a fix for this shortly

[Michael137]

@zmodem Fix pushed in 14a84510f5d07b05b348188c7dfbe54076fa1485. Your reproducer works fine for me now. Let me know if you're still seeing issues.

[zmodem]

Thanks!

[dyung]

Hi @Michael137, we are seeing a failure in one of our internal tests that I bisected back to this change. Consider the following code:

struct X
{
    static const int constant = 1;
    int x;

    X() { x = constant; }
};
const int X::constant;

int main()
{
    X x;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    return 0;
}

Prior to your change, the compiler would generate the following DWARF for the constant value:

0x0000003a:     DW_TAG_member
                  DW_AT_name    ("constant")
                  DW_AT_type    (0x00000057 "const int")
                  DW_AT_decl_file       ("/home/dyung/sandbox/test.cpp")
                  DW_AT_decl_line       (3)
                  DW_AT_external        (true)
                  DW_AT_declaration     (true)
                  DW_AT_const_value     (1)

After your change, the DW_AT_const_value is gone from this DW_TAG_member group, but doesn't appear anywhere else in the DWARF output which seems to indicate that it was dropped completely which does not seem to be correct. Is this intended or am I missing something?

[Michael137]

Hi @Michael137, we are seeing a failure in one of our internal tests that I bisected back to this change. Consider the following code:

struct X
{
    static const int constant = 1;
    int x;

    X() { x = constant; }
};
const int X::constant;

int main()
{
    X x;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    return 0;
}

Prior to your change, the compiler would generate the following DWARF for the constant value:

0x0000003a:     DW_TAG_member
                  DW_AT_name    ("constant")
                  DW_AT_type    (0x00000057 "const int")
                  DW_AT_decl_file       ("/home/dyung/sandbox/test.cpp")
                  DW_AT_decl_line       (3)
                  DW_AT_external        (true)
                  DW_AT_declaration     (true)
                  DW_AT_const_value     (1)

After your change, the DW_AT_const_value is gone from this DW_TAG_member group, but doesn't appear anywhere else in the DWARF output which seems to indicate that it was dropped completely which does not seem to be correct. Is this intended or am I missing something?

Right, we stopped putting the DW_AT_const_value on the declaration. Instead we put either the location or the constant on the definition, depending on what's available. In your example you define the variable out-of-class which would generate a DW_TAG_variable with a location, so we don't emit the constant for it on the definition:

0x0000002e:   DW_TAG_variable
                DW_AT_specification     (0x0000004a "constant")
                DW_AT_location  (DW_OP_addr 0x90)
                DW_AT_linkage_name      ("_ZN1X8constantE")

What's the nature of the failure? Would you instead be able to read the value out of the definition?

CC @dwblaikie

[dyung]

Hi @Michael137, we are seeing a failure in one of our internal tests that I bisected back to this change. Consider the following code:

struct X
{
    static const int constant = 1;
    int x;

    X() { x = constant; }
};
const int X::constant;

int main()
{
    X x;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    x.x = X::constant;
    return 0;
}

Prior to your change, the compiler would generate the following DWARF for the constant value:

0x0000003a:     DW_TAG_member
                  DW_AT_name    ("constant")
                  DW_AT_type    (0x00000057 "const int")
                  DW_AT_decl_file       ("/home/dyung/sandbox/test.cpp")
                  DW_AT_decl_line       (3)
                  DW_AT_external        (true)
                  DW_AT_declaration     (true)
                  DW_AT_const_value     (1)

After your change, the DW_AT_const_value is gone from this DW_TAG_member group, but doesn't appear anywhere else in the DWARF output which seems to indicate that it was dropped completely which does not seem to be correct. Is this intended or am I missing something?

Right, we stopped putting the DW_AT_const_value on the declaration. Instead we put either the location or the constant on the definition, depending on what's available. In your example you define the variable out-of-class which would generate a DW_TAG_variable with a location, so we don't emit the constant for it on the definition:

0x0000002e:   DW_TAG_variable
                DW_AT_specification     (0x0000004a "constant")
                DW_AT_location  (DW_OP_addr 0x90)
                DW_AT_linkage_name      ("_ZN1X8constantE")

What's the nature of the failure? Would you instead be able to read the value out of the definition?

Our test is expecting to find a DW_AT_const_value somewhere in the dwarf output which it no longer seems to generate. From what I gather from your commit message, the compiler should now be emitting a new DW_TAG_variable section that it previously did not with the DW_AT_const_value attached? Or is my understanding incorrect?

[pogo59]

The member is const with an initializer in-class. How is the constant value not available for the definition?

[Michael137]

The member is const with an initializer in-class. How is the constant value not available for the definition?

Right, it is available, we just don't attach it if we have a location for it. Don't see why we couldn't put it on the definition if we have the constant on hand

[dyung]

The member is const with an initializer in-class. How is the constant value not available for the definition?

Right, it is available, we just don't attach it if we have a location for it. Don't see why we couldn't put it on the definition if we have the constant on hand

So I guess what you are saying in this case is that it is expected and the value is at the location indicated by the DW_AT_location value? As long as the value is still available I suppose that is fine then and the test just needs updating.

[pogo59]

I think it is a valuable bit of information for the debugger, to know the constant value without having to read it from memory. I think we should emit both the location and the value.

[Michael137]

So I guess what you are saying in this case is that it is expected and the value is at the location indicated by the DW_AT_location value? As long as the value is still available I suppose that is fine then and the test just needs updating.

Yup that's exactly right. I'll go ahead with @pogo59's suggestion then and make sure we also emit the constant on the definition when we can. If updating your test to accommodate for only having the DW_AT_location is feasible for now that'd be great since I'll only be able to get to it tomorrow

[ilovepi]

Hi, we're seeing some breakages, similar to the above in our debugger tests with this patch.

A failing bot can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-debug/b8764552260903625809/overview

You can find a fuller discussion in our bugtracker: https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=136182.

The problem in our test is that the DW_AT_const_value, no longer seems to be in the expected place, similar to @dyung's issue above. Is there an ETA on when your fix will land? If it won't be soon, would you mind reverting until you can address this issue?

[Michael137]

Hi, we're seeing some breakages, similar to the above in our debugger tests with this patch.

A failing bot can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-debug/b8764552260903625809/overview

You can find a fuller discussion in our bugtracker: https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=136182.

The problem in our test is that the DW_AT_const_value, no longer seems to be in the expected place, similar to @dyung's issue above. Is there an ETA on when your fix will land? If it won't be soon, would you mind reverting until you can address this issue?

Sorry for not updating the thread. I'm about to open a PR for said change

[ilovepi]

Fantastic!

[Michael137]

@ilovepi To clarify the upcoming change, we are planning to attach the DW_AT_const_value to the definition, not the declaration like we used to. In case that's what your unit-test is expecting

[ilovepi]

I'm unclear on the specifics of the check, but it's probably something we can adjust if that is the long-term solution.

CC @petrhosek Since he was interested in getting this resolved soon.

[Michael137]

I'm unclear on the specifics of the check, but it's probably something we can adjust if that is the long-term solution.

CC @petrhosek Since he was interested in getting this resolved soon.

There's a few things left to iron out with #72730, which I hope to resolve by Friday. If this is blocking you urgently then we could add back the constant to the declaration until I land #72730 (instead of reverting the entire patch).

[petrhosek]

Can we add back the constant to the declaration? It's been a week and the progress on #72730 seems to have stalled while our builders are still broken.

[Michael137]

Can we add back the constant to the declaration? It's been a week and the progress on #72730 seems to have stalled while our builders are still broken.

Yup will submit a PR today, apologies for the noise