[NFC] Allow fragment expressions in extractIfOffset #69006
Conversation
A fragment is part of the variable identity and should be ignored for the purposes of checking whether the expression contains an offset. This NFC patch is required for a subsequent change that addresses llvm#61981.
OCHyams
requested review from
jmorse,
adrian-prantl,
SLTozer,
scott-linder and
djtodoro
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-debuginfo Author: Orlando Cazalet-Hyams (OCHyams) ChangesA fragment is part of the variable identity and should be ignored for the purposes of checking whether the expression contains an offset. This NFC patch is required for a subsequent change that addresses #61981. Full diff: https://github.com/llvm/llvm-project/pull/69006.diff 2 Files Affected:
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index f7f36129ec8557c..2ab41e55db1caef 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -1673,18 +1673,21 @@ bool DIExpression::extractIfOffset(int64_t &Offset) const {
return false;
auto SingleLocElts = *SingleLocEltsOpt;
- if (SingleLocElts.size() == 0) {
+ unsigned FragmentOpsCount = isFragment() ? 3 : 0;
+
+ if (SingleLocElts.size() == FragmentOpsCount) {
Offset = 0;
return true;
}
- if (SingleLocElts.size() == 2 &&
+ if (SingleLocElts.size() == 2 + FragmentOpsCount &&
SingleLocElts[0] == dwarf::DW_OP_plus_uconst) {
Offset = SingleLocElts[1];
return true;
}
- if (SingleLocElts.size() == 3 && SingleLocElts[0] == dwarf::DW_OP_constu) {
+ if (SingleLocElts.size() == 3 + FragmentOpsCount &&
+ SingleLocElts[0] == dwarf::DW_OP_constu) {
if (SingleLocElts[2] == dwarf::DW_OP_plus) {
Offset = SingleLocElts[1];
return true;
diff --git a/llvm/unittests/IR/MetadataTest.cpp b/llvm/unittests/IR/MetadataTest.cpp
index 1570631287b2a78..e68d27b36054767 100644
--- a/llvm/unittests/IR/MetadataTest.cpp
+++ b/llvm/unittests/IR/MetadataTest.cpp
@@ -3511,6 +3511,57 @@ TEST_F(DIExpressionTest, isEqualExpression) {
#undef GET_EXPR
}
+TEST_F(DIExpressionTest, extractIfOffset) {
+#define GET_EXPR(...) DIExpression::get(Context, {__VA_ARGS__})
+#define GET_FRAG(...) \
+ DIExpression::get(Context, {__VA_ARGS__, dwarf::DW_OP_LLVM_fragment, 0, 32})
+#define EXPECT_OFFSET_IMPL(Offset, Success, Expr) \
+ do { \
+ int64_t ActualOffset; \
+ bool Result = Expr->extractIfOffset(ActualOffset); \
+ EXPECT_EQ(Success, Result); \
+ if (Success) { \
+ EXPECT_EQ(Offset, ActualOffset); \
+ } \
+ } while (0)
+#define EXPECT_OFFSET_EQ(Offset, Expr) EXPECT_OFFSET_IMPL(Offset, true, Expr)
+#define EXPECT_NO_OFFSET(Expr) EXPECT_OFFSET_IMPL(0, false, Expr)
+
+ // Check extractIfOffset behaves as expected with and without a fragment
+ // in the expression. extractIfOffset reads a constant offset out from the
+ // expression. It fails for all but exactly these with and without fragments:
+ // no expression
+ // DW_OP_plus_uconst, <u>
+ // DW_OP_constu, <u>, DW_OP_plus/minus
+ EXPECT_OFFSET_EQ(0, GET_EXPR());
+ EXPECT_OFFSET_EQ(0, GET_EXPR(dwarf::DW_OP_LLVM_fragment, 0, 32));
+
+ EXPECT_OFFSET_EQ(1, GET_EXPR(dwarf::DW_OP_plus_uconst, 1));
+ EXPECT_OFFSET_EQ(1, GET_FRAG(dwarf::DW_OP_plus_uconst, 1));
+
+ EXPECT_OFFSET_EQ(2, GET_EXPR(dwarf::DW_OP_constu, 2, dwarf::DW_OP_plus));
+ EXPECT_OFFSET_EQ(2, GET_FRAG(dwarf::DW_OP_constu, 2, dwarf::DW_OP_plus));
+
+ EXPECT_OFFSET_EQ(-3, GET_EXPR(dwarf::DW_OP_constu, 3, dwarf::DW_OP_minus));
+ EXPECT_OFFSET_EQ(-3, GET_FRAG(dwarf::DW_OP_constu, 3, dwarf::DW_OP_minus));
+
+ // Test some failure cases, e.g. unsupported operations and too many
+ // operations.
+ EXPECT_NO_OFFSET(GET_EXPR(dwarf::DW_OP_constu, 3, dwarf::DW_OP_mul));
+ EXPECT_NO_OFFSET(GET_FRAG(dwarf::DW_OP_constu, 3, dwarf::DW_OP_mul));
+
+ EXPECT_NO_OFFSET(
+ GET_EXPR(dwarf::DW_OP_constu, 3, dwarf::DW_OP_plus, dwarf::DW_OP_deref));
+ EXPECT_NO_OFFSET(
+ GET_FRAG(dwarf::DW_OP_constu, 3, dwarf::DW_OP_plus, dwarf::DW_OP_deref));
+
+#undef EXPECT_NO_OFFSET
+#undef EXPECT_OFFSET_EQ
+#undef EXPECT_OFFSET_IMPL
+#undef GET_FRAG
+#undef GET_EXPR
+}
+
TEST_F(DIExpressionTest, foldConstant) {
const ConstantInt *Int;
const ConstantInt *NewInt;
|
|
FWIW, if someone wants to hoist DW_OP_LLVM_fragment into two optional fields in DIExpression, I'd support that too. |
|
Thanks for the review.
+1, that would be nice. Just a note for anyone thinking of looking into it - I think @scott-linder is considering other improvements in this area so probably worth chatting before going ahead. |
It would require a larger rewrite, but conceptually it'd make more sense to move fragment info into |
Do we have an RFC for these ideas? I'd love to read/help. I often find these bits of logic in DIExpression quite difficult to work with. For example, besides what you describe with fragments, the idea that OP_entry_value has to be the first operand of an expr, and yet internally we have a vector of OPs instead of having a field indicating the entry value. Or anything that has special handling in |
|
For Scott's ideas (@slinder1 ?), they're summarised in this conference presentation last year https://www.youtube.com/watch?v=HEhigWu_0uE, and RFC here: https://discourse.llvm.org/t/rfc-heterogeneous-debug-info/66872 . I think the most attractive part is the idea of shifting the fragment information into metadata nodes, and then referring to those nodes from the debug intrinsics. So dbg.values would refer to a DIFragment (I think?) that then refers to a DILocalVariable. The problem of "what fragment does this thing refer to" is then largely not considered during optimisation passes. There are a bunch of other ideas for taking information out of DIExpressions, and I think there's a prototype in the AMD LLVM repo somewhere? |
A fragment is part of the variable identity and should be ignored for the purposes of checking whether the expression contains an offset.
This NFC patch is required for a subsequent change that addresses #61981.