[clang][DebugInfo] Revert to attaching DW_AT_const_value on static member declarations #73626

Michael137 · 2023-11-28T09:26:44Z

In #71780 we started emitting definitions for all static data-members with constant initialisers, even if they were constants (i.e., didn't have a location). We also dropped the DW_AT_const_value from the declaration to help resolve inconsistencies during type merging in the DWARFParallelLinker. However, for static data members that do have locations, we wouldn't emit a DW_AT_const_value on it, assuming that the consumer knows how to read the value using the location. This broke some consumers that really wanted to find a DW_AT_const_value. Ultimately we want to attach a DW_AT_const_value to definitions that have a location too. But to fix consumers broken by said change, this patch adds the constant back onto the declaration. This is what we used to do prior to #71780

…mber declarations In llvm#71780 we started emitting definitions for all static data-members with constant initialisers, even if they were constants (i.e., didn't have a location). We also dropped the DW_AT_const_value from the declaration to help resolve inconsistencies during type merging in the DWARFParallelLinker. However, for static data members that do have locations, we wouldn't emit a DW_AT_const_value on it, assuming that the consumer knows how to read the value using the location. This broke some consumers that really wanted to find a DW_AT_const_value. Ultimately we want to attach a DW_AT_const_value to definitions that have a location too. But to fix consumers broken by said change, this patch adds the constant back onto the declaration.

Michael137 · 2023-11-28T09:27:01Z

FYI @petrhosek

llvmbot · 2023-11-28T09:27:15Z

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

@llvm/pr-subscribers-debuginfo

Author: Michael Buch (Michael137)

Changes

In #71780 we started emitting definitions for all static data-members with constant initialisers, even if they were constants (i.e., didn't have a location). We also dropped the DW_AT_const_value from the declaration to help resolve inconsistencies during type merging in the DWARFParallelLinker. However, for static data members that do have locations, we wouldn't emit a DW_AT_const_value on it, assuming that the consumer knows how to read the value using the location. This broke some consumers that really wanted to find a DW_AT_const_value. Ultimately we want to attach a DW_AT_const_value to definitions that have a location too. But to fix consumers broken by said change, this patch adds the constant back onto the declaration.

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

3 Files Affected:

(modified) clang/lib/CodeGen/CGDebugInfo.cpp (+15-3)
(modified) clang/test/CodeGenCXX/debug-info-static-inline-member.cpp (+7-7)
(modified) clang/test/CodeGenCXX/debug-info-static-member.cpp (+4-4)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 0b52d99ad07f164..5d9d5d1792450c3 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1678,14 +1678,26 @@ CGDebugInfo::CreateRecordStaticField(const VarDecl *Var, llvm::DIType *RecordTy,
   unsigned LineNumber = getLineNumber(Var->getLocation());
   StringRef VName = Var->getName();
 
+  // FIXME: to avoid complications with type merging we should
+  // emit the constant on the definition instead of the declaration.
+  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 Tag = CGM.getCodeGenOpts().DwarfVersion >= 5
                  ? llvm::dwarf::DW_TAG_variable
                  : llvm::dwarf::DW_TAG_member;
   auto Align = getDeclAlignIfRequired(Var, CGM.getContext());
-  llvm::DIDerivedType *GV =
-      DBuilder.createStaticMemberType(RecordTy, VName, VUnit, LineNumber, VTy,
-                                      Flags, /* Val */ nullptr, Tag, Align);
+  llvm::DIDerivedType *GV = DBuilder.createStaticMemberType(
+      RecordTy, VName, VUnit, LineNumber, VTy, Flags, C, Tag, Align);
   StaticDataMemberCache[Var->getCanonicalDecl()].reset(GV);
   StaticDataMemberDefinitionsToEmit.push_back(Var->getCanonicalDecl());
   return GV;
diff --git a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
index f2d4d9408a8297a..d3b6a363c5bd8f2 100644
--- a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
+++ b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
@@ -49,19 +49,19 @@ int main() {
 
 // CHECK:      ![[INT_DECL]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int_with_addr",
 // CHECK-SAME:                 flags: DIFlagStaticMember
-// CHECK-NOT:                  extraData:
+// CHECK-SAME:                 extraData: i32 25
 
 // CHECK:      ![[INT_DECL2:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int2",
 // CHECK-SAME:                         flags: DIFlagStaticMember
-// CHECK-NOT:                          extraData:
+// CHECK-SAME:                         extraData: i32 26
 
 // CHECK:      ![[FLOAT_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_float",
 // CHECK-SAME:                          flags: DIFlagStaticMember
-// CHECK-NOT:                           extraData:
+// CHECK-SAME:                          extraData: float
 
 // CHECK:      ![[ENUM_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_enum",
 // CHECK-SAME:                         flags: DIFlagStaticMember
-// CHECK-NOT:                          extraData:
+// CHECK-SAME:                         extraData: i32 -1
 
 // CHECK:      ![[EMPTY_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_struct_with_addr",
 // CHECK-SAME:                          flags: DIFlagStaticMember
@@ -69,15 +69,15 @@ int main() {
 
 // CHECK:      ![[IENUM_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "inline_enum",
 // CHECK-SAME:                          flags: DIFlagStaticMember
-// CHECK-NOT:                           extraData:
+// CHECK-SAME:                          extraData: i32 -1
 
 // CHECK:      ![[EMPTY_TEMPLATED_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "empty_templated",
 // CHECK-SAME:                                    flags: DIFlagStaticMember
-// CHECK-NOT:                                     extraData:
+// CHECK-SAME:                                    extraData: i32 1
 
 // CHECK:      ![[TEMPLATE_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_template",
 // CHECK-SAME:                             flags: DIFlagStaticMember
-// CHECK-NOT:                              extraData:
+// CHECK-SAME:                             extraData: i32 1
 
 // CHECK:      !DIGlobalVariableExpression(var: ![[EMPTY_VAR:[0-9]+]], expr: !DIExpression())
 // CHECK:      ![[EMPTY_VAR]] = distinct !DIGlobalVariable(name: "cexpr_struct_with_addr", linkageName:
diff --git a/clang/test/CodeGenCXX/debug-info-static-member.cpp b/clang/test/CodeGenCXX/debug-info-static-member.cpp
index a2d25e98ed1cb62..096bfa0271e0e3d 100644
--- a/clang/test/CodeGenCXX/debug-info-static-member.cpp
+++ b/clang/test/CodeGenCXX/debug-info-static-member.cpp
@@ -74,7 +74,7 @@ int C::a = 4;
 // CHECK-NOT:                           align:
 // CHECK-NOT:                           offset:
 // CHECK-SAME:                          flags: DIFlagStaticMember
-// CHECK-NOT:                           extraData:
+// CHECK-SAME:                          extraData: i1 true
 
 // DWARF4:     ![[CONST_B_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "const_b"
 // DWARF5:     ![[CONST_B_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_variable, name: "const_b"
@@ -82,7 +82,7 @@ int C::a = 4;
 // CHECK-NOT:                            align:
 // CHECK-NOT:                            offset:
 // CHECK-SAME:                           flags: DIFlagProtected | DIFlagStaticMember
-// CHECK-NOT:                            extraData:
+// CHECK-SAME:                           extraData: float
 
 // DWARF4: ![[DECL_C:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "c"
 // DWARF5: ![[DECL_C:[0-9]+]] = !DIDerivedType(tag: DW_TAG_variable, name: "c"
@@ -97,7 +97,7 @@ int C::a = 4;
 // CHECK-NOT:                            align:
 // CHECK-NOT:                            offset:
 // CHECK-SAME:                           flags: DIFlagPublic | DIFlagStaticMember
-// CHECK-NOT:                            extraData:
+// CHECK-SAME:                           extraData: i32 18
 //
 // DWARF4: !DIDerivedType(tag: DW_TAG_member, name: "x_a"
 // DWARF5: !DIDerivedType(tag: DW_TAG_variable, name: "x_a"
@@ -154,7 +154,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-NOT:            extraData:
+// NOT-MS-SAME:           extraData: i32 42
 const int V::const_va;
 
 namespace x {

jryans

Thanks, seems like a fine temporary measure to me! 😄

Michael137 · 2023-11-28T11:25:08Z

I do wonder how feasible it would be for the downstream tests to be adjusted to look at the DW_AT_location..

pogo59 · 2023-11-28T14:20:34Z

I do wonder how feasible it would be for the downstream tests to be adjusted to look at the DW_AT_location..

As I mentioned on the other thread, the point is not to have to read the value from the process-under-debug. This is not efficient in a remote-debugging scenario.

Michael137 · 2023-11-28T15:40:13Z

I do wonder how feasible it would be for the downstream tests to be adjusted to look at the DW_AT_location..

As I mentioned on the other thread, the point is not to have to read the value from the process-under-debug. This is not efficient in a remote-debugging scenario.

Ah that's fair, missed your comment on the other thread

Michael137 · 2023-11-28T15:48:34Z

Looks like LLDB linux buildbot isn't happy, checking...

… of global variables In llvm#73626 we started attaching `DW_AT_const_value`s on a static data-member's declaration again. In DWARFv5, those static members are represented with a `DW_TAG_variable`. When LLDB builds the `ManualDWARFIndex`, it simply iterates over all DIEs in a CU and puts *any* `DW_TAG_variable` with a constant or location into the index. So when using the manual index, we can end up having 2 entries for a static data member in the index. This caused a test failure on Linux (where DWARFv5 is the default and the tests use the manual index). This patch loosens the restriction that we find exactly 1 variable.

Michael137 · 2023-11-28T22:43:56Z

Looks like LLDB linux buildbot isn't happy, checking...

Fix in #73707

… of global variables (#73707) In #73626 we started attaching `DW_AT_const_value`s on a static data-member's declaration again. In DWARFv5, those static members are represented with a `DW_TAG_variable`. When LLDB builds the `ManualDWARFIndex`, it simply iterates over all DIEs in a CU and puts *any* `DW_TAG_variable` with a constant or location into the index. So when using the manual index, we can end up having 2 entries for a static data member in the index, one for the declaration and one for the definition. This caused a test failure on Linux (where DWARFv5 is the default and the tests use the manual index). This patch loosens the restriction that we find exactly 1 variable.

Michael137 added 2 commits November 28, 2023 08:51

fixup! fix tests

64fcf57

Michael137 requested a review from dwblaikie November 28, 2023 09:26

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. debuginfo labels Nov 28, 2023

jryans approved these changes Nov 28, 2023

View reviewed changes

Michael137 merged commit 7f3ee3c into llvm:main Nov 28, 2023

Michael137 mentioned this pull request Nov 28, 2023

[lldb][test] TestConstStaticIntegralMember: relax assertion on number of global variables #73707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][DebugInfo] Revert to attaching DW_AT_const_value on static member declarations #73626

[clang][DebugInfo] Revert to attaching DW_AT_const_value on static member declarations #73626

Uh oh!

Michael137 commented Nov 28, 2023 •

edited

Loading

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

llvmbot commented Nov 28, 2023 •

edited

Loading

Uh oh!

jryans left a comment

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

pogo59 commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[clang][DebugInfo] Revert to attaching DW_AT_const_value on static member declarations #73626

[clang][DebugInfo] Revert to attaching DW_AT_const_value on static member declarations #73626

Uh oh!

Conversation

Michael137 commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

llvmbot commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jryans left a comment

Choose a reason for hiding this comment

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

pogo59 commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Michael137 commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Michael137 commented Nov 28, 2023 •

edited

Loading

llvmbot commented Nov 28, 2023 •

edited

Loading