-
Notifications
You must be signed in to change notification settings - Fork 12.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang][DebugInfo] Attach DW_AT_const_value to static data-member definitions if available #72730
base: main
Are you sure you want to change the base?
[clang][DebugInfo] Attach DW_AT_const_value to static data-member definitions if available #72730
Conversation
@llvm/pr-subscribers-clang @llvm/pr-subscribers-debuginfo Author: Michael Buch (Michael137) ChangesIn #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 This patch makes sure we also attach the Full diff: https://github.com/llvm/llvm-project/pull/72730.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 0b52d99ad07f164..e01c57baef19931 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -69,6 +69,19 @@ static uint32_t getDeclAlignIfRequired(const Decl *D, const ASTContext &Ctx) {
return D->hasAttr<AlignedAttr>() ? D->getMaxAlignment() : 0;
}
+APValue const *evaluateConstantInitializer(clang::VarDecl const *VD) {
+ assert(VD != nullptr);
+
+ VD = VD->getCanonicalDecl();
+ if (!VD)
+ return nullptr;
+
+ if (!VD->hasConstantInitialization() || !VD->hasInit())
+ return nullptr;
+
+ return VD->evaluateValue();
+}
+
CGDebugInfo::CGDebugInfo(CodeGenModule &CGM)
: CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
@@ -5503,11 +5516,17 @@ void CGDebugInfo::EmitGlobalVariable(llvm::GlobalVariable *Var,
}
AppendAddressSpaceXDeref(AddressSpace, Expr);
+ llvm::DIExpression *E = nullptr;
+ if (Expr.empty()) {
+ if (auto const *InitVal = evaluateConstantInitializer(D))
+ E = createConstantValueExpression(D, *InitVal);
+ } else
+ E = DBuilder.createExpression(Expr);
+
llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(D);
GVE = DBuilder.createGlobalVariableExpression(
DContext, DeclName, LinkageName, Unit, LineNo, getOrCreateType(T, Unit),
- Var->hasLocalLinkage(), true,
- Expr.empty() ? nullptr : DBuilder.createExpression(Expr),
+ Var->hasLocalLinkage(), true, E,
getOrCreateStaticDataMemberDeclarationOrNull(D), TemplateParameters,
Align, Annotations);
Var->addDebugInfo(GVE);
@@ -5596,14 +5615,11 @@ void CGDebugInfo::EmitGlobalVariable(const VarDecl *VD) {
if (VD->hasAttr<NoDebugAttr>())
return;
- if (!VD->hasInit())
- return;
-
const auto CacheIt = DeclCache.find(VD);
if (CacheIt != DeclCache.end())
return;
- auto const *InitVal = VD->evaluateValue();
+ auto const *InitVal = evaluateConstantInitializer(VD);
if (!InitVal)
return;
diff --git a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
index f2d4d9408a8297a..950ea9b302b290c 100644
--- a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
+++ b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
@@ -43,7 +43,7 @@ int main() {
// CHECK: @{{.*}}cexpr_struct_with_addr{{.*}} =
// CHECK-SAME !dbg ![[EMPTY_GLOBAL:[0-9]+]]
-// CHECK: !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression())
+// CHECK: !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 25, DW_OP_stack_value))
// CHECK: ![[INT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_int_with_addr", linkageName:
// CHECK-SAME: isLocal: false, isDefinition: true, declaration: ![[INT_DECL:[0-9]+]])
diff --git a/clang/test/CodeGenCXX/inline-dllexport-member.cpp b/clang/test/CodeGenCXX/inline-dllexport-member.cpp
index d6b004d66dc6cbd..6bc01599c466780 100644
--- a/clang/test/CodeGenCXX/inline-dllexport-member.cpp
+++ b/clang/test/CodeGenCXX/inline-dllexport-member.cpp
@@ -7,7 +7,7 @@ struct __declspec(dllexport) s {
static const unsigned int ui = 0;
};
-// CHECK: [[UI]] = !DIGlobalVariableExpression(var: [[UIV:.*]], expr: !DIExpression())
+// CHECK: [[UI]] = !DIGlobalVariableExpression(var: [[UIV:.*]], expr: !DIExpression(DW_OP_constu, 0, DW_OP_stack_value))
// CHECK: [[UIV]] = distinct !DIGlobalVariable(name: "ui", linkageName: "?ui@s@@2IB", scope: ![[SCOPE:[0-9]+]],
// CHECK: ![[SCOPE]] = distinct !DICompileUnit(
|
@llvm/pr-subscribers-clang-codegen Author: Michael Buch (Michael137) ChangesIn #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 This patch makes sure we also attach the Full diff: https://github.com/llvm/llvm-project/pull/72730.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 0b52d99ad07f164..e01c57baef19931 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -69,6 +69,19 @@ static uint32_t getDeclAlignIfRequired(const Decl *D, const ASTContext &Ctx) {
return D->hasAttr<AlignedAttr>() ? D->getMaxAlignment() : 0;
}
+APValue const *evaluateConstantInitializer(clang::VarDecl const *VD) {
+ assert(VD != nullptr);
+
+ VD = VD->getCanonicalDecl();
+ if (!VD)
+ return nullptr;
+
+ if (!VD->hasConstantInitialization() || !VD->hasInit())
+ return nullptr;
+
+ return VD->evaluateValue();
+}
+
CGDebugInfo::CGDebugInfo(CodeGenModule &CGM)
: CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
@@ -5503,11 +5516,17 @@ void CGDebugInfo::EmitGlobalVariable(llvm::GlobalVariable *Var,
}
AppendAddressSpaceXDeref(AddressSpace, Expr);
+ llvm::DIExpression *E = nullptr;
+ if (Expr.empty()) {
+ if (auto const *InitVal = evaluateConstantInitializer(D))
+ E = createConstantValueExpression(D, *InitVal);
+ } else
+ E = DBuilder.createExpression(Expr);
+
llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(D);
GVE = DBuilder.createGlobalVariableExpression(
DContext, DeclName, LinkageName, Unit, LineNo, getOrCreateType(T, Unit),
- Var->hasLocalLinkage(), true,
- Expr.empty() ? nullptr : DBuilder.createExpression(Expr),
+ Var->hasLocalLinkage(), true, E,
getOrCreateStaticDataMemberDeclarationOrNull(D), TemplateParameters,
Align, Annotations);
Var->addDebugInfo(GVE);
@@ -5596,14 +5615,11 @@ void CGDebugInfo::EmitGlobalVariable(const VarDecl *VD) {
if (VD->hasAttr<NoDebugAttr>())
return;
- if (!VD->hasInit())
- return;
-
const auto CacheIt = DeclCache.find(VD);
if (CacheIt != DeclCache.end())
return;
- auto const *InitVal = VD->evaluateValue();
+ auto const *InitVal = evaluateConstantInitializer(VD);
if (!InitVal)
return;
diff --git a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
index f2d4d9408a8297a..950ea9b302b290c 100644
--- a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
+++ b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
@@ -43,7 +43,7 @@ int main() {
// CHECK: @{{.*}}cexpr_struct_with_addr{{.*}} =
// CHECK-SAME !dbg ![[EMPTY_GLOBAL:[0-9]+]]
-// CHECK: !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression())
+// CHECK: !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 25, DW_OP_stack_value))
// CHECK: ![[INT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_int_with_addr", linkageName:
// CHECK-SAME: isLocal: false, isDefinition: true, declaration: ![[INT_DECL:[0-9]+]])
diff --git a/clang/test/CodeGenCXX/inline-dllexport-member.cpp b/clang/test/CodeGenCXX/inline-dllexport-member.cpp
index d6b004d66dc6cbd..6bc01599c466780 100644
--- a/clang/test/CodeGenCXX/inline-dllexport-member.cpp
+++ b/clang/test/CodeGenCXX/inline-dllexport-member.cpp
@@ -7,7 +7,7 @@ struct __declspec(dllexport) s {
static const unsigned int ui = 0;
};
-// CHECK: [[UI]] = !DIGlobalVariableExpression(var: [[UIV:.*]], expr: !DIExpression())
+// CHECK: [[UI]] = !DIGlobalVariableExpression(var: [[UIV:.*]], expr: !DIExpression(DW_OP_constu, 0, DW_OP_stack_value))
// CHECK: [[UIV]] = distinct !DIGlobalVariable(name: "ui", linkageName: "?ui@s@@2IB", scope: ![[SCOPE:[0-9]+]],
// CHECK: ![[SCOPE]] = distinct !DICompileUnit(
|
@ilovepi , should help with https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=136182 |
This looks great, thanks for keeping me in the loop. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, thanks!
I think we may want to add a test to check that the output DWARF has a const value in the expected place, since consumers are relying on it.
Any suggestions where to put such end-to-end test? This patch makes sure we encode the constant in a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch makes sure we encode the constant in a
DIExpression
on definitions, which is tested. And we have tests for ensuring thatDW_OP_constu
expressions get turned intoDW_AT_const_value
s (I know of at leastllvm/test/DebugInfo/Generic/dwarf5-debug-info-static-member.ll
)
Ah okay, then with that in mind, I think the test changes here are likely enough to merge. 🙂
This is more idiomatic
* `debug-info-static-member.cpp`: * We added the check for `const_b` as part of the patch series in `638a8393615e911b729d5662096f60ef49f1c65e`. The new check `isUsableAsConstantExpression` doesn't support constant inline floats (since they are neither constexpr nor integrals). This isn't a regression since before said patch series we wouldn't ever emit the definition for `const_b` anyway. Now we just don't do it for `const float`s. This is consistent with GCC's behaviour starting with C++11. * `debug-info-static-inline-member`: * This was just a bug which is now fixed. We shouldn't emit a `DW_AT_const_value` for a non-const static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, otherwise LGTM.
clang/lib/CodeGen/CGDebugInfo.cpp
Outdated
/// Given a VarDecl corresponding to either the definition or | ||
/// declaration of a C++ static data member, if it has a constant | ||
/// initializer and is evaluatable, return the evaluated value. | ||
/// Returns std::nullopt on failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of "on failure" say "otherwise." It's not really a failure condition.
This will need a slight tweak in DwarfCompileUnit because it currently doesn't attach a DW_AT_location to global variable DIEs that already have a DW_AT_const_value |
Hmm, I can't quite tell from the test case updates in the patch, at least at a glance: How does this get encoded at the IR level? Do we end up with two DIGlobalVariableExpressions? One with the constant value expresison, and one that references the actual global variable? Or somehow encode both in one? |
My understanding was that the DIExpression parameter to DIGlobalVariableExpression was empty for global variables with locations. So the patch just encodes the constant into that expression if it's otherwise empty. My plan was to make DwarfCompileUnit support global variables with locations and constants, which it currently assumes doesn't happen. Let me know if I missed something here |
I think in theory it can be non-empty (see what happens under a merge globals optimization... I'm not sure of an example of when this optimization fires, or what the debug info we emit looks like - in theory it should look like a global with a non-empty expression that describes offsetting the pointer forward to reach the global inside the merged global) & so then you'd have a hard time telling whether the expression is meant to be used in addition to the location, or as part of evaluating the location. We don't really have a mechanism for encoding a variable in two locations (or a constant and a location) at the same time, I think. We could invent a special opcode to use in the expression to communicate this, or define some special handling if there's two separate expressions providing a location (or a location and a constant in this case) for the same variable (say that they're both valid, and emit them both if we can). @adrian-prantl thoughts? |
llvm-project/llvm/lib/CodeGen/GlobalMerge.cpp Line 531 in abd85cd
|
I extracted the parts that are clean-ups/bugfixes into a new PR: #72974 |
Thanks for the pointers. Managed to trigger the optimisation you described as follows:
That will generate
Then run
That changes the IR into:
|
Just checked. With this patch, MergeGlobals would produce following IR:
I.e., two |
@Michael137 is this patch ready to land? We (I have took over the bug from @ilovepi ) have a few builders that are currently blocked by the behavior change introduced in PR #70639 . It would be great if this change can be landed timely. Thx. |
Would confuse |
Nope this isn't ready yet. We need to figure out how to make global variables with locations and constants work. I'm about to submit a separate PR that will unblock your builds |
Would only confuse |
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 aDW_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 aDW_AT_const_value
.This patch makes sure we also attach the
DW_AT_const_value
to definitions that have a location.Test changes
debug-info-static-member.cpp
:const_b
as part of thepatch series in
638a8393615e911b729d5662096f60ef49f1c65e
.The check for
isUsableAsConstantExpression
added in the current patchdoesn't support constant inline floats (since they are neither constexpr nor
integrals). This isn't a regression since before said patch series
we wouldn't ever emit the definition for
const_b
anyway. Nowwe just don't do it for
inline const float
s. This is consistent withGCC's behaviour starting with C++11.
debug-info-static-inline-member
:a
DW_AT_const_value
for a non-const static.