[lldb] Produce generic results for DWARF relational operations#210122
Conversation
|
@llvm/pr-subscribers-lldb Author: firmiana (firmiana402) ChangesDWARF relational operations produce a generic result, whose width is the target address size. LLDB currently assigns the C++ The stricter binary operand checks added in #201288 exposed this in Apple's x86-64 Convert the results of TestingAdd a unit test that evaluates each relational opcode with an 8-byte address size and then consumes its result with Full diff: https://github.com/llvm/llvm-project/pull/210122.diff 2 Files Affected:
diff --git a/lldb/source/Expression/DWARFExpression.cpp b/lldb/source/Expression/DWARFExpression.cpp
index 5d5c03d81ef1f..299dbe97d6176 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -1724,7 +1724,8 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
return err;
tmp = stack.back();
stack.pop_back();
- stack.back().GetScalar() = stack.back().GetScalar() == tmp.GetScalar();
+ stack.back().GetScalar() =
+ to_generic(stack.back().GetScalar() == tmp.GetScalar());
break;
case DW_OP_ge:
@@ -1734,7 +1735,8 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
return err;
tmp = stack.back();
stack.pop_back();
- stack.back().GetScalar() = stack.back().GetScalar() >= tmp.GetScalar();
+ stack.back().GetScalar() =
+ to_generic(stack.back().GetScalar() >= tmp.GetScalar());
break;
case DW_OP_gt:
@@ -1744,7 +1746,8 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
return err;
tmp = stack.back();
stack.pop_back();
- stack.back().GetScalar() = stack.back().GetScalar() > tmp.GetScalar();
+ stack.back().GetScalar() =
+ to_generic(stack.back().GetScalar() > tmp.GetScalar());
break;
case DW_OP_le:
@@ -1754,7 +1757,8 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
return err;
tmp = stack.back();
stack.pop_back();
- stack.back().GetScalar() = stack.back().GetScalar() <= tmp.GetScalar();
+ stack.back().GetScalar() =
+ to_generic(stack.back().GetScalar() <= tmp.GetScalar());
break;
case DW_OP_lt:
@@ -1764,7 +1768,8 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
return err;
tmp = stack.back();
stack.pop_back();
- stack.back().GetScalar() = stack.back().GetScalar() < tmp.GetScalar();
+ stack.back().GetScalar() =
+ to_generic(stack.back().GetScalar() < tmp.GetScalar());
break;
case DW_OP_ne:
@@ -1774,7 +1779,8 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
return err;
tmp = stack.back();
stack.pop_back();
- stack.back().GetScalar() = stack.back().GetScalar() != tmp.GetScalar();
+ stack.back().GetScalar() =
+ to_generic(stack.back().GetScalar() != tmp.GetScalar());
break;
case DW_OP_lit0:
diff --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index 7dc707ac29a70..6df4553757de4 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -708,6 +708,39 @@ TEST(DWARFExpression, GenericBinaryOpsAllowDifferentSignedness) {
ExpectScalar(4));
}
+TEST(DWARFExpression, RelationalOpsProduceGenericResult) {
+ struct TestCase {
+ uint8_t opcode;
+ uint8_t lhs;
+ uint8_t rhs;
+ };
+ constexpr TestCase test_cases[] = {
+ {DW_OP_eq, 3, 3}, {DW_OP_ge, 4, 3}, {DW_OP_gt, 4, 3},
+ {DW_OP_le, 3, 4}, {DW_OP_lt, 3, 4}, {DW_OP_ne, 3, 4},
+ };
+
+ for (const TestCase &test : test_cases) {
+ const std::vector<uint8_t> expr = {
+ DW_OP_lit8,
+ static_cast<uint8_t>(DW_OP_lit0 + test.lhs),
+ static_cast<uint8_t>(DW_OP_lit0 + test.rhs),
+ test.opcode,
+ DW_OP_plus,
+ DW_OP_stack_value,
+ };
+ DataExtractor extractor(expr.data(), expr.size(), lldb::eByteOrderLittle,
+ /*addr_size=*/8);
+
+ EXPECT_THAT_EXPECTED(
+ DWARFExpression::Evaluate(
+ /*exe_ctx=*/nullptr, /*reg_ctx=*/nullptr, /*module_sp=*/{},
+ extractor, /*unit=*/nullptr, lldb::eRegisterKindLLDB,
+ /*initial_value_ptr=*/nullptr, /*object_address_ptr=*/nullptr),
+ ExpectScalar(64, 9, false))
+ << "opcode 0x" << llvm::utohexstr(test.opcode);
+ }
+}
+
TEST(DWARFExpression, DW_OP_stack_value) {
EXPECT_THAT_EXPECTED(Evaluate({DW_OP_stack_value}), llvm::Failed());
}
|
JDevlieghere
left a comment
JDevlieghere
left a comment
There was a problem hiding this comment.
LGTM, this should unblock the x86 bots.
|
The failing test is flaky and unrelated right? Can we merge this? |
It looks like the Linux failure is in functionalities/gdb_remote_client/TestGdbClientModuleLoad.py. I reran this test locally, and it passed. This test does not exercise the DWARF expression evaluator, so the failure appears unrelated to this patch. Could you please rerun the Linux check? |
DWARF relational operations produce a generic, address-sized result. Assigning their C++ bool results directly to Scalar instead created a 32-bit integer, causing later typed arithmetic to reject valid expressions on 64-bit targets, including Apple's x86-64 sigtramp CFI. Convert all relational results through the existing to_generic helper and add coverage that consumes each result in address-sized arithmetic.
7a1f18a to
211aea8
Compare
The tests have passed, and this is ready to merge. I don’t have write access yet, so could you please merge it for me? |
3 participants
DWARF relational operations produce a generic result, whose width is the target address size. LLDB currently assigns the C++
boolcomparison result directly toScalar; becauseScalarhas noboolconstructor, this creates a 32-bit integer even on 64-bit targets.The stricter binary operand checks added in #201288 exposed this in x86-64
_sigtrampCFI. A laterDW_OP_pluscombines a 64-bit generic address with the 32-bit relational result, so evaluation fails and signal-frame unwinding stops. The generic-operand relaxation in #209641 does not cover this case because the relational result is narrower than the address size.Convert the results of
DW_OP_eq,DW_OP_ge,DW_OP_gt,DW_OP_le,DW_OP_lt, andDW_OP_nethrough the existingto_generichelper.Testing
Add a unit test that evaluates each relational opcode with an 8-byte address size and then consumes its result with
DW_OP_plus.