[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2c6710a5e100: Teach DWARFExpression about DWARF 4+ Location
Descriptions (authored by aprantl).
Herald added a project: LLDB.
Changed prior to commit:
https://reviews.llvm.org/D98996?vs=332493&id=332717#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98996/new/
https://reviews.llvm.org/D98996
Files:
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -213,42 +213,45 @@
//
// Leave as is.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
- DW_OP_convert, offs_uint32_t}),
- llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+ DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
// Zero-extend to 64 bits.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
- DW_OP_convert, offs_uint64_t}),
- llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+ DW_OP_convert, offs_uint64_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
// Sign-extend to 64 bits.
EXPECT_THAT_EXPECTED(
t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
- DW_OP_convert, offs_sint64_t}),
+ DW_OP_convert, offs_sint64_t, DW_OP_stack_value}),
llvm::HasValue(GetScalar(64, 0xffeeddcc, is_signed)));
// Sign-extend, then truncate.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
- DW_OP_convert, offs_sint64_t, //
- DW_OP_convert, offs_uint32_t}),
- llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
+ DW_OP_convert, offs_sint64_t, //
+ DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
// Truncate to default unspecified (pointer-sized) type.
EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
DW_OP_convert, offs_sint64_t, //
- DW_OP_convert, 0x00}),
+ DW_OP_convert, 0x00, DW_OP_stack_value}),
llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
// Truncate to 8 bits.
- EXPECT_THAT_EXPECTED(
- t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_uchar}),
- llvm::HasValue(GetScalar(8, 'A', not_signed)));
+ EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+ offs_uchar, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(8, 'A', not_signed)));
// Also truncate to 8 bits.
- EXPECT_THAT_EXPECTED(
- t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_schar}),
- llvm::HasValue(GetScalar(8, 'A', is_signed)));
+ EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+ offs_schar, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(8, 'A', is_signed)));
//
// Errors.
@@ -354,4 +357,13 @@
// Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4, DW_OP_deref}, {}, {}, &exe_ctx),
llvm::HasValue(Scalar(LLDB_INVALID_ADDRESS)));
+ // Memory location: *0x4.
+ // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+ EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4}, {}, {}, &exe_ctx),
+ llvm::HasValue(Scalar(4)));
+ // Implicit location: *0x4.
+ // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+ EXPECT_THAT_EXPECTED(
+ Evaluate({DW_OP_lit4, DW_OP_deref, DW_OP_stack_value}, {}, {}, &exe_ctx),
+ llvm::HasValue(GetScalar(32, 0x07060504, false)));
}
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -904,6 +904,52 @@
object_address_ptr, result, error_ptr);
}
+namespace {
+/// The location descripti
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
JDevlieghere accepted this revision. JDevlieghere added a comment. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98996/new/ https://reviews.llvm.org/D98996 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
aprantl updated this revision to Diff 332493.
aprantl added a comment.
Updated the patch to compute the classification on the fly and take composition
operators into account.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98996/new/
https://reviews.llvm.org/D98996
Files:
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -213,42 +213,45 @@
//
// Leave as is.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
- DW_OP_convert, offs_uint32_t}),
- llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+ DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
// Zero-extend to 64 bits.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
- DW_OP_convert, offs_uint64_t}),
- llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+ DW_OP_convert, offs_uint64_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
// Sign-extend to 64 bits.
EXPECT_THAT_EXPECTED(
t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
- DW_OP_convert, offs_sint64_t}),
+ DW_OP_convert, offs_sint64_t, DW_OP_stack_value}),
llvm::HasValue(GetScalar(64, 0xffeeddcc, is_signed)));
// Sign-extend, then truncate.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
- DW_OP_convert, offs_sint64_t, //
- DW_OP_convert, offs_uint32_t}),
- llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
+ DW_OP_convert, offs_sint64_t, //
+ DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
// Truncate to default unspecified (pointer-sized) type.
EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
DW_OP_convert, offs_sint64_t, //
- DW_OP_convert, 0x00}),
+ DW_OP_convert, 0x00, DW_OP_stack_value}),
llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
// Truncate to 8 bits.
- EXPECT_THAT_EXPECTED(
- t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_uchar}),
- llvm::HasValue(GetScalar(8, 'A', not_signed)));
+ EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+ offs_uchar, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(8, 'A', not_signed)));
// Also truncate to 8 bits.
- EXPECT_THAT_EXPECTED(
- t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_schar}),
- llvm::HasValue(GetScalar(8, 'A', is_signed)));
+ EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+ offs_schar, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(8, 'A', is_signed)));
//
// Errors.
@@ -354,4 +357,13 @@
// Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4, DW_OP_deref}, {}, {}, &exe_ctx),
llvm::HasValue(Scalar(LLDB_INVALID_ADDRESS)));
+ // Memory location: *0x4.
+ // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+ EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4}, {}, {}, &exe_ctx),
+ llvm::HasValue(Scalar(4)));
+ // Implicit location: *0x4.
+ // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+ EXPECT_THAT_EXPECTED(
+ Evaluate({DW_OP_lit4, DW_OP_deref, DW_OP_stack_value}, {}, {}, &exe_ctx),
+ llvm::HasValue(GetScalar(32, 0x07060504, false)));
}
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -904,6 +904,43 @@
object_address_ptr, result, error_ptr);
}
+namespace {
+enum LocationDescriptionKind { Empty, Memory, Register, Implicit /*, Composite*/ };
+/// Adjust value's ValueType according to the kind of location description.
+void UpdateValueTypeFromLocationDescription(Log *log, const DWARFUnit *dwarf_cu,
+
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
aprantl added a comment.
@dblaikie wrote:
> (idle question: Is this code remotely related to any code in LLVM's
> libDebugInfoDWARF? Does this patch take us closer to or further away from
> unifying them? If it takes us further away, any chance of designing it in a
> direction that points towards each other rather than away from?)
There's definitely an opportunity to refactor the main loop of Evaluate() to
use llvm::DWARFExpression::iterator. I would say The impact of this patch
towards that goal is neutral. The iterator (and all of libDebugInfo) doesn't
care about the semantics of DWARF expressions.
Comment at: lldb/source/Expression/DWARFExpression.cpp:2615
if (stack.empty()) {
// Nothing on the stack, check if we created a piece value from DW_OP_piece
@labath wrote:
> Are you sure that this logic is correct in presence of DW_OP(_bit)_piece? If
> I follow this right, then the final value type will be determined by the last
> operand. That sounds like it could be right for regular dwarf expressions,
> but I'm not sure about those with pieces. The classification function will
> mark those as "memory", and that doesn't sound right...
DW_OP_pieces are handled in this block.
For your immediate question, this means that this patch is safe. However, it
also highlights that this patch isn't fixing the same bug in a composite
location description. I'll address this in my next revision of the patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98996/new/
https://reviews.llvm.org/D98996
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
JDevlieghere added inline comments.
Comment at: lldb/source/Expression/DWARFExpression.cpp:992
+case DW_OP_regx:
+ dwarf4_location_description_kind = Register;
+ break;
Could you return the kind and do `LocationDescriptionKind
dwarf4_location_description_kind = classifyLocationDescription`? That way you
can make it a static instead of a lambda in a function that's already a
hundreds of lines long.
Comment at: lldb/source/Expression/DWARFExpression.cpp:1007
const uint8_t op = opcodes.GetU8(&offset);
+classifyLocationDescription(op);
That also makes it more explicit that this is updating the
`dwarf4_location_description_kind`
Comment at: lldb/source/Expression/DWARFExpression.cpp:2636
+switch (dwarf4_location_description_kind) {
+case Empty: // FIXME: This would be suspicious. Add some error handling.
+ LLDB_LOGF(log, log_msg, "Empty");
lldb_assert?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98996/new/
https://reviews.llvm.org/D98996
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
labath added a comment.
Are you sure that this logic is correct in presence of DW_OP(_bit)_piece? If I
follow this right, then the final value type will be determined by the last
operand. That sounds like it could be right for regular dwarf expressions, but
I'm not sure about those with pieces. The classification function will mark
those as "memory", and that doesn't sound right...
Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:360-361
llvm::HasValue(Scalar(LLDB_INVALID_ADDRESS)));
+ // Memory location: *(*0x4).
+ // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+ EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4}, {}, {}, &exe_ctx),
This comment doesn't seem right.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98996/new/
https://reviews.llvm.org/D98996
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
jasonmolenda accepted this revision. jasonmolenda added a comment. This revision is now accepted and ready to land. This fits with my reading of the DWARF5 doc; LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98996/new/ https://reviews.llvm.org/D98996 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
dblaikie added a comment. (idle question: Is this code remotely related to any code in LLVM's libDebugInfoDWARF? Does this patch take us closer to or further away from unifying them? If it takes us further away, any chance of designing it in a direction that points towards each other rather than away from?) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98996/new/ https://reviews.llvm.org/D98996 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions
aprantl created this revision.
aprantl added reviewers: jasonmolenda, JDevlieghere, labath.
aprantl requested review of this revision.
DWARFExpression implements the DWARF2 expression model that left ambiguity on
whether the result of an expression was a value or an address. This patch
implements the DWARF location description model introduces in DWARF 4 and sets
the result Value's kind accordingly, if the expression comes from a DWARF v4+
compile unit. The nomenclature is taken from DWARF 5, chapter 2.6 "Location
Descriptions".
https://reviews.llvm.org/D98996
Files:
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -213,42 +213,45 @@
//
// Leave as is.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
- DW_OP_convert, offs_uint32_t}),
- llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+ DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
// Zero-extend to 64 bits.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
- DW_OP_convert, offs_uint64_t}),
- llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
+ DW_OP_convert, offs_uint64_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
// Sign-extend to 64 bits.
EXPECT_THAT_EXPECTED(
t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
- DW_OP_convert, offs_sint64_t}),
+ DW_OP_convert, offs_sint64_t, DW_OP_stack_value}),
llvm::HasValue(GetScalar(64, 0xffeeddcc, is_signed)));
// Sign-extend, then truncate.
- EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
- DW_OP_convert, offs_sint64_t, //
- DW_OP_convert, offs_uint32_t}),
- llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
+ EXPECT_THAT_EXPECTED(
+ t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
+ DW_OP_convert, offs_sint64_t, //
+ DW_OP_convert, offs_uint32_t, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
// Truncate to default unspecified (pointer-sized) type.
EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 0xcc, 0xdd, 0xee, 0xff, //
DW_OP_convert, offs_sint64_t, //
- DW_OP_convert, 0x00}),
+ DW_OP_convert, 0x00, DW_OP_stack_value}),
llvm::HasValue(GetScalar(32, 0xffeeddcc, not_signed)));
// Truncate to 8 bits.
- EXPECT_THAT_EXPECTED(
- t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_uchar}),
- llvm::HasValue(GetScalar(8, 'A', not_signed)));
+ EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+ offs_uchar, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(8, 'A', not_signed)));
// Also truncate to 8 bits.
- EXPECT_THAT_EXPECTED(
- t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_schar}),
- llvm::HasValue(GetScalar(8, 'A', is_signed)));
+ EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert,
+ offs_schar, DW_OP_stack_value}),
+ llvm::HasValue(GetScalar(8, 'A', is_signed)));
//
// Errors.
@@ -354,4 +357,13 @@
// Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4, DW_OP_deref}, {}, {}, &exe_ctx),
llvm::HasValue(Scalar(LLDB_INVALID_ADDRESS)));
+ // Memory location: *(*0x4).
+ // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+ EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit4}, {}, {}, &exe_ctx),
+ llvm::HasValue(Scalar(4)));
+ // Memory location: *(*0x4).
+ // Evaluate returns LLDB_INVALID_ADDRESS for all load addresses.
+ EXPECT_THAT_EXPECTED(
+ Evaluate({DW_OP_lit4, DW_OP_deref, DW_OP_stack_value}, {}, {}, &exe_ctx),
+ llvm::HasValue(GetScalar(32, 0x07060504, false)));
}
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -952,9 +952,59 @@
!is_signed));
};
+ enum L
