[clang][DebugInfo] Emit global variable definitions for static data members with constant initializers #70639
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Summing the size of all *.o files in my local debug Clang build increased by |
llvmbot
added
clang
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang Author: Michael Buch (Michael137) ChangesWhen an LLDB user asks for the value of a static data member, LLDB starts by To make it more convenient for debuggers to get to the value of inline static data members, The implementation keeps track of newly created static data members. Then in The newly emitted Full diff: https://github.com/llvm/llvm-project/pull/70639.diff 5 Files Affected:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 0aaf678bf287c6e..8b85fb03c05f6e7 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1693,6 +1693,7 @@ CGDebugInfo::CreateRecordStaticField(const VarDecl *Var, llvm::DIType *RecordTy,
llvm::DIDerivedType *GV = DBuilder.createStaticMemberType(
RecordTy, VName, VUnit, LineNumber, VTy, Flags, C, Align);
StaticDataMemberCache[Var->getCanonicalDecl()].reset(GV);
+ StaticDataMemberDefinitionsToEmit.push_back(Var->getCanonicalDecl());
return GV;
}
@@ -5580,25 +5581,8 @@ void CGDebugInfo::EmitGlobalVariable(const ValueDecl *VD, const APValue &Init) {
auto &GV = DeclCache[VD];
if (GV)
return;
- llvm::DIExpression *InitExpr = nullptr;
- if (CGM.getContext().getTypeSize(VD->getType()) <= 64) {
- // FIXME: Add a representation for integer constants wider than 64 bits.
- if (Init.isInt()) {
- const llvm::APSInt &InitInt = Init.getInt();
- std::optional<uint64_t> InitIntOpt;
- if (InitInt.isUnsigned())
- InitIntOpt = InitInt.tryZExtValue();
- else if (auto tmp = InitInt.trySExtValue(); tmp.has_value())
- // Transform a signed optional to unsigned optional. When cpp 23 comes,
- // use std::optional::transform
- InitIntOpt = (uint64_t)tmp.value();
- if (InitIntOpt)
- InitExpr = DBuilder.createConstantValueExpression(InitIntOpt.value());
- } else if (Init.isFloat())
- InitExpr = DBuilder.createConstantValueExpression(
- Init.getFloat().bitcastToAPInt().getZExtValue());
- }
+ llvm::DIExpression *InitExpr = createConstantValueExpression(VD, Init);
llvm::MDTuple *TemplateParameters = nullptr;
if (isa<VarTemplateSpecializationDecl>(VD))
@@ -5613,6 +5597,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());
@@ -5883,6 +5900,18 @@ void CGDebugInfo::finalize() {
DBuilder.replaceTemporary(std::move(FwdDecl), cast<llvm::MDNode>(Repl));
}
+ for (auto const *VD : StaticDataMemberDefinitionsToEmit) {
+ assert (VD->isStaticDataMember());
+
+ if (auto It = DeclCache.find(VD); It != DeclCache.end())
+ continue;
+
+ if (!VD->hasInit())
+ continue;
+
+ EmitGlobalVariable(VD);
+ }
+
// We keep our own list of retained types, because we need to look
// up the final type in the type cache.
for (auto &RT : RetainedTypes)
@@ -5935,3 +5964,28 @@ llvm::DINode::DIFlags CGDebugInfo::getCallSiteRelatedAttrs() const {
return llvm::DINode::FlagAllCallsDescribed;
}
+
+llvm::DIExpression *
+CGDebugInfo::createConstantValueExpression(clang::ValueDecl const *VD,
+ const APValue &Val) {
+ llvm::DIExpression *ValExpr = nullptr;
+ if (CGM.getContext().getTypeSize(VD->getType()) <= 64) {
+ // FIXME: Add a representation for integer constants wider than 64 bits.
+ if (Val.isInt()) {
+ const llvm::APSInt &ValInt = Val.getInt();
+ std::optional<uint64_t> ValIntOpt;
+ if (ValInt.isUnsigned())
+ ValIntOpt = ValInt.tryZExtValue();
+ else if (auto tmp = ValInt.trySExtValue(); tmp.has_value())
+ // Transform a signed optional to unsigned optional. When cpp 23 comes,
+ // use std::optional::transform
+ ValIntOpt = (uint64_t)tmp.value();
+ if (ValIntOpt)
+ ValExpr = DBuilder.createConstantValueExpression(ValIntOpt.value());
+ } else if (Val.isFloat())
+ ValExpr = DBuilder.createConstantValueExpression(
+ Val.getFloat().bitcastToAPInt().getZExtValue());
+ }
+
+ return ValExpr;
+}
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index ae12485850ca775..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);
@@ -800,6 +806,11 @@ class CGDebugInfo {
llvm::MDTuple *&TemplateParameters,
llvm::DIScope *&VDContext);
+ /// Create a DIExpression representing the constant corresponding
+ /// to the specified 'Val'. Returns nullptr on failure.
+ llvm::DIExpression *createConstantValueExpression(const clang::ValueDecl *VD,
+ const APValue &Val);
+
/// Allocate a copy of \p A using the DebugInfoNames allocator
/// and return a reference to it. If multiple arguments are given the strings
/// are concatenated.
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-inline-member.cpp b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
new file mode 100644
index 000000000000000..917e101714846d7
--- /dev/null
+++ b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
@@ -0,0 +1,79 @@
+// RUN: %clangxx -target arm64-apple-macosx11.0.0 -g %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK %s
+
+enum class Enum : int {
+ VAL = -1
+};
+
+struct Empty {};
+
+constexpr auto func() { return 25; }
+
+struct Foo {
+ static constexpr int cexpr_int = func();
+ static constexpr int cexpr_int2 = func() + 1;
+ static constexpr float cexpr_float = 2.0 + 1.0;
+ static constexpr Enum cexpr_enum = Enum::VAL;
+ static constexpr Empty cexpr_empty{};
+
+ template<typename T>
+ static constexpr T cexpr_template{};
+};
+
+int main() {
+ Foo f;
+
+ // Force global variable definitions to be emitted.
+ (void)&Foo::cexpr_int;
+ (void)&Foo::cexpr_empty;
+
+ return Foo::cexpr_int + Foo::cexpr_float
+ + (int)Foo::cexpr_enum + Foo::cexpr_template<short>;
+}
+
+// CHECK: @{{.*}}cexpr_int{{.*}} =
+// CHECK-SAME: !dbg ![[INT_GLOBAL:[0-9]+]]
+
+// CHECK: @{{.*}}cexpr_empty{{.*}} =
+// CHECK-SAME !dbg ![[EMPTY_GLOBAL:[0-9]+]]
+
+// CHECK: !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression())
+// CHECK: ![[INT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_int", linkageName:
+// CHECK-SAME: isLocal: false, isDefinition: true, declaration: ![[INT_DECL:[0-9]+]])
+
+// CHECK: ![[INT_DECL]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int",
+// CHECK-SAME: flags: DIFlagStaticMember, extraData: i32 25)
+
+// CHECK: ![[INT_DECL2:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int2",
+// CHECK-SAME: flags: DIFlagStaticMember, extraData: i32 26)
+
+// CHECK: ![[FLOAT_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_float",
+// CHECK-SAME: flags: DIFlagStaticMember, extraData: float
+
+// CHECK: ![[ENUM_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_enum",
+// CHECK-SAME: flags: DIFlagStaticMember, extraData: i32 -1)
+
+// CHECK: ![[EMPTY_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_empty",
+// CHECK-SAME: flags: DIFlagStaticMember)
+
+// CHECK: ![[TEMPLATE_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_template",
+// CHECK-SAME: flags: DIFlagStaticMember, extraData: i16 0)
+
+// CHECK: !DIGlobalVariableExpression(var: ![[EMPTY_VAR:[0-9]+]], expr: !DIExpression())
+// CHECK: ![[EMPTY_VAR]] = distinct !DIGlobalVariable(name: "cexpr_empty", linkageName:
+// CHECK-SAME: isLocal: false, isDefinition: true, declaration: ![[EMPTY_DECL]])
+
+// CHECK: !DIGlobalVariableExpression(var: ![[INT_VAR2:[0-9]+]], expr: !DIExpression(DW_OP_constu, 26, DW_OP_stack_value))
+// CHECK: ![[INT_VAR2]] = distinct !DIGlobalVariable(name: "cexpr_int2", linkageName:
+// CHECK-SAME: isLocal: true, isDefinition: true, declaration: ![[INT_DECL2]])
+
+// CHECK: !DIGlobalVariableExpression(var: ![[FLOAT_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
+// CHECK: ![[FLOAT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_float", linkageName:
+// CHECK-SAME: isLocal: true, isDefinition: true, declaration: ![[FLOAT_DECL]])
+
+// CHECK: !DIGlobalVariableExpression(var: ![[ENUM_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
+// CHECK: ![[ENUM_VAR]] = distinct !DIGlobalVariable(name: "cexpr_enum", linkageName:
+// CHECK-SAME: isLocal: true, isDefinition: true, declaration: ![[ENUM_DECL]])
+
+// CHECK: !DIGlobalVariableExpression(var: ![[TEMPLATE_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 0, DW_OP_stack_value))
+// CHECK: ![[TEMPLATE_VAR]] = distinct !DIGlobalVariable(name: "cexpr_template", linkageName:
+// CHECK-SAME: isLocal: true, isDefinition: true, declaration: ![[TEMPLATE_DECL]], templateParams: ![[TEMPLATE_PARMS:[0-9]+]])
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(
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This could also help with the type merging issues discussed in #68721 (comment) |
Michael137
force-pushed
the
inline-statics-support/clang-emit-defs
branch
from
3e6ad34 to
9ccb456
Compare
|
Should not we remove constant initializer from the type definition if we have a DW_TAG_variable with a DW_AT_const_value now? |
dwblaikie
reviewed
Oct 30, 2023
| // CHECK: @{{.*}}cexpr_empty{{.*}} = | ||
| // CHECK-SAME !dbg ![[EMPTY_GLOBAL:[0-9]+]] | ||
|
|
||
| // CHECK: !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression()) |
There was a problem hiding this comment.
Why doesn't this have a value/expression? (but INT_VAR2 does)
There was a problem hiding this comment.
Because this one has a corresponding llvm::GlobalVariable which doesn't go down the code-path I added. We emit a DW_AT_location only for this. Should we be emitting both a location and constant in this case?
There was a problem hiding this comment.
Ah, that was the intent (the line 27 taking address-of)? Maybe give the variable a more descriptive name to make it clear this is testing the case where the variable has a definition emitted to the IR
Yep, +1 to this. This is where the type normalization benefit would come from - no longer having this const value on the member declaration, but only on the definition. |
That's a bit significant. Any chance you could get a per-section breakdown/growth on that? (& exact flags - is this with compressed debug info, for instance? Or not?) I use |
Makes sense. I'll create a separate PR for that, unless people are fine with doing that as part of this change |
If it's not too much code might make sense to do it as part of this change - so the size impacts are more clear, for instance. |
Probably the linkage names that contribute most?
This is an example command line invocation (with some paths redacted) for one of the object files: This is on Darwin. Haven't done anything specific to enable compressed debug-info, so pretty sure it's not enabled. |
Michael137
commented
Oct 31, 2023
|
|
||
| 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); |
There was a problem hiding this comment.
This parameter on createStaticMemberType is technically not needed anymore. So could think about removing it in a follow-up
Michael137
force-pushed
the
inline-statics-support/clang-emit-defs
branch
from
90dc818 to
28c5575
Compare
|
size report sounds generally OK - but there's a bunch of what look like unrelated or strange results in there, perhaps they weren't compared at exactly the same version? (like why did the apple_names size go down? How did the .text and .debuG_line size change at all? ) |
dwblaikie
reviewed
Oct 31, 2023
Comment on lines
70
to
71
| // CHECK-SAME: flags: DIFlagStaticMember | ||
| // CHECK-NOT: extraData: |
There was a problem hiding this comment.
Perhaps this test case could be extended to check the constatn values on the definition metadata? & /maybe/ we don't need a new test case? (or maybe we do, but perhaps they could be added to this file, rather than another one?)
There was a problem hiding this comment.
Merging the two makes sense. I tried that initially but the C++98 FileCheck was tedious to work around. But with the latest iteration of the patch that shouldn't be a problem
dwblaikie
reviewed
Oct 31, 2023
clang/lib/CodeGen/CGDebugInfo.cpp
Outdated
| for (auto const *VD : StaticDataMemberDefinitionsToEmit) { | ||
| assert(VD->isStaticDataMember()); | ||
|
|
||
| if (auto It = DeclCache.find(VD); It != DeclCache.end()) |
There was a problem hiding this comment.
DeclCache.contains might be simpler to use/read here?
clayborg
reviewed
Oct 31, 2023
There was a problem hiding this comment.
One question I have here: where will the DW_TAG_variable get emitted for these constexpr? For actual static member variables we emit a single DW_TAG_variable in the file that declares the global variable, but for constexpr we won't be able to do this will we? Will we end up emitting a new copy each time someone include the header file? In that case we might end up with many global variables being defined in a linked executable that includes this header file more than once?
dwblaikie
approved these changes
Oct 31, 2023
There was a problem hiding this comment.
Few minor issues, but looks good to me.
(you metnioned somewhere an lldb issue I think with this patch when the value is removed from the static member declaration inside the class? If that's a problem - probably hold off on committing this until lldb's been fixed so this doesn't regress things? And document the dependence clearly in the commit message)
Yea I was confused about that too. Might've mixed up commits of the control build. Did measurements again and here's what I got: Section breakdown is still similar |
Michael137
force-pushed
the
inline-statics-support/clang-emit-defs
branch
from
d1894b0 to
b5d9dd2
Compare
Michael137
added a commit
that referenced
this pull request
Nov 6, 2023
…ion if available (#71004) #70639 proposes moving the `DW_AT_const_value` on inline static members from the declaration DIE to the definition DIE. This patch makes sure the LLDB's expression evaluator can continue to support static initialisers even if the declaration doesn't have a `DW_AT_const_value` anymore. Previously the expression evaluator would find the constant for a VarDecl from its declaration `DW_TAG_member` DIE. In cases where the initialiser was specified out-of-class, LLDB could find it during symbol resolution. However, neither of those will work for constants, since we don't have a constant attribute on the declaration anymore and we don't have constants in the symbol table. **Testing** * If #70639 were to land without this patch then most of the `TestConstStaticIntegralMember.py` would start failing
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Nov 6, 2023
…c data members with constant initializers (llvm#70639)" This reverts commit 4909814. Following LLDB patch had to be reverted due to Linux test failures: ``` ef3feba ``` Since without that LLDB patch the LLDB tests would fail, revert this clang patch for now.
Michael137
added a commit
that referenced
this pull request
Nov 7, 2023
… data members with constant initializers (#70639)" 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.
Michael137
added a commit
that referenced
this pull request
Nov 7, 2023
…e defintion if available (#71004)" #70639 proposes moving the `DW_AT_const_value` on inline static members from the declaration DIE to the definition DIE. This patch makes sure the LLDB's expression evaluator can continue to support static initialisers even if the declaration doesn't have a `DW_AT_const_value` anymore. Previously the expression evaluator would find the constant for a VarDecl from its declaration `DW_TAG_member` DIE. In cases where the initialiser was specified out-of-class, LLDB could find it during symbol resolution. However, neither of those will work for constants, since we don't have a constant attribute on the declaration anymore and we don't have constants in the symbol table. **Testing** * If #70639 were to land without this patch then most of the `TestConstStaticIntegralMember.py` would start failing
|
This causes Clang to assert: See https://bugs.chromium.org/p/chromium/issues/detail?id=1500262#c1 for a reproducer. I'll revert to green. |
zmodem
added a commit
that referenced
this pull request
Nov 7, 2023
…r static data members with constant initializers (#70639)"" This casued asserts: llvm/lib/IR/Metadata.cpp:689: void llvm::MDNode::resolve(): Assertion `isUniqued() && "Expected this to be uniqued"' failed. See comments on the PR. This also reverts the dependent follow-up commits, see below. > 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<E2><80><99>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. This reverts commit 7c3707a. This reverts commit cab0a19. This reverts commit 317481b. This reverts commit 15fc809. This reverts commit 470de2b.
This was referenced Nov 8, 2023
Thanks for the repro. Reduced it to: when compiling with The issue was that |
Michael137
added a commit
that referenced
this pull request
Nov 13, 2023
…c data members with constant initializers" (#71780) 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 `DIType`s 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: * #71800
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…c data members with constant initializers" (llvm#71780) This patch relands llvm#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 `DIType`s 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: * llvm#71800
qihangkong
pushed a commit
to rvgpu/llvm
that referenced
this pull request
Apr 18, 2024
…674) This patch factors out the code to create a DIExpression from an APValue into a separate helper function. This will be useful in a follow-up patch where we re-use this logic elsewhere. Pre-requisite for llvm/llvm-project#70639
qihangkong
pushed a commit
to rvgpu/llvm
that referenced
this pull request
Apr 18, 2024
… members (#70641) Tests that LLDB can find inline static data members. Relies on the debug-info change in: llvm/llvm-project#70639
qihangkong
pushed a commit
to rvgpu/llvm
that referenced
this pull request
Apr 18, 2024
…ion if available (#71004) llvm/llvm-project#70639 proposes moving the `DW_AT_const_value` on inline static members from the declaration DIE to the definition DIE. This patch makes sure the LLDB's expression evaluator can continue to support static initialisers even if the declaration doesn't have a `DW_AT_const_value` anymore. Previously the expression evaluator would find the constant for a VarDecl from its declaration `DW_TAG_member` DIE. In cases where the initialiser was specified out-of-class, LLDB could find it during symbol resolution. However, neither of those will work for constants, since we don't have a constant attribute on the declaration anymore and we don't have constants in the symbol table. **Testing** * If llvm/llvm-project#70639 were to land without this patch then most of the `TestConstStaticIntegralMember.py` would start failing
qihangkong
pushed a commit
to rvgpu/llvm
that referenced
this pull request
Apr 18, 2024
…e defintion if available (#71004)" llvm/llvm-project#70639 proposes moving the `DW_AT_const_value` on inline static members from the declaration DIE to the definition DIE. This patch makes sure the LLDB's expression evaluator can continue to support static initialisers even if the declaration doesn't have a `DW_AT_const_value` anymore. Previously the expression evaluator would find the constant for a VarDecl from its declaration `DW_TAG_member` DIE. In cases where the initialiser was specified out-of-class, LLDB could find it during symbol resolution. However, neither of those will work for constants, since we don't have a constant attribute on the declaration anymore and we don't have constants in the symbol table. **Testing** * If llvm/llvm-project#70639 were to land without this patch then most of the `TestConstStaticIntegralMember.py` would start failing
qihangkong
pushed a commit
to rvgpu/llvm
that referenced
this pull request
Apr 18, 2024
…tic data members (#70641)" Tests that LLDB can find inline static data members. Relies on the debug-info change in: llvm/llvm-project#70639
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_memberthat usually holds the constant initializer.But we don't get a corresponding CU-level
DW_TAG_variablelike we do forout-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 globalDW_TAG_variablewith aDW_AT_const_valueforany of those declarations that didn't end up with a definition in the
DeclCache.The newly emitted
DW_TAG_variablewill look as follows: