[Lldb-commits] [lldb] [lldb] Evaluate DW_OP_abs and DW_OP_shra with signed operand semantics (PR #225772)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: firmiana (firmiana402)

<details>
<summary>Changes</summary>

`DW_OP_const*u` produces a generic value, but LLDB retains the constant's unsigned encoding in its internal `Scalar`. `DW_OP_abs` and `DW_OP_shra` then accidentally use that unsignedness, although both operations require signed interpretation ([DWARF v5](https://dwarfstd.org/doc/DWARF5.pdf), section 2.5.1.4). For example, on a 64-bit target, `DW_OP_abs` applied to `DW_OP_const8u 0xffffffffffffffff` yields all ones instead of 1, while `DW_OP_shra` by 63 yields 1 instead of all ones.

Interpret integer bits as signed for these two operations, then restore the original `Scalar` signedness so the result keeps the operand's type. Add 32- and 64-bit expression tests covering both cases.

Fixes #<!-- -->207497.


---
Full diff: https://github.com/llvm/llvm-project/pull/225772.diff


2 Files Affected:

- (modified) lldb/source/Expression/DWARFExpression.cpp (+21-6) 
- (modified) lldb/unittests/Expression/DWARFExpressionTest.cpp (+27-2) 


``````````diff
diff --git a/lldb/source/Expression/DWARFExpression.cpp b/lldb/source/Expression/DWARFExpression.cpp
index f884724766714..a04c498f22a9b 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -1604,12 +1604,20 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
       stack[last_idx - 2] = old_top;
     } break;
 
-    case DW_OP_abs:
-      if (!stack.back().GetScalar().AbsoluteValue()) {
+    case DW_OP_abs: {
+      Scalar &operand = stack.back().GetScalar();
+      const bool was_unsigned =
+          operand.GetType() == Scalar::e_int && !operand.IsSigned();
+      // DW_OP_abs interprets the bits as signed without changing the result
+      // type.
+      operand.MakeSigned();
+      if (!operand.AbsoluteValue()) {
         return llvm::createStringError(
             "failed to take the absolute value of the first stack item");
       }
-      break;
+      if (was_unsigned)
+        operand.MakeUnsigned();
+    } break;
 
     case DW_OP_and:
       if (llvm::Error err = CheckScalarOperandsHaveSameType(
@@ -1745,15 +1753,22 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
         return llvm::createStringError("DW_OP_shr failed");
       break;
 
-    case DW_OP_shra:
+    case DW_OP_shra: {
       if (llvm::Error err = CheckScalarOperandsHaveSameType(
               stack[stack.size() - 2].GetScalar(), stack.back().GetScalar(),
               opcode, address_size))
         return err;
       tmp = stack.back();
       stack.pop_back();
-      stack.back().GetScalar() >>= tmp.GetScalar();
-      break;
+      Scalar &operand = stack.back().GetScalar();
+      const bool was_unsigned =
+          operand.GetType() == Scalar::e_int && !operand.IsSigned();
+      // DW_OP_shra shifts signed bits without changing the result type.
+      operand.MakeSigned();
+      operand >>= tmp.GetScalar();
+      if (was_unsigned)
+        operand.MakeUnsigned();
+    } break;
 
     case DW_OP_xor:
       if (llvm::Error err = CheckScalarOperandsHaveSameType(
diff --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index 31e629967c6a2..f5a0e89ebbbd0 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -261,10 +261,11 @@ static llvm::Expected<Value> Evaluate(llvm::ArrayRef<uint8_t> expr,
                                       lldb::ModuleSP module_sp = {},
                                       DWARFExpression::Delegate *unit = nullptr,
                                       ExecutionContext *exe_ctx = nullptr,
-                                      RegisterContext *reg_ctx = nullptr) {
+                                      RegisterContext *reg_ctx = nullptr,
+                                      uint8_t address_size = 4) {
   DataExtractor extractor(
       expr.data(), expr.size(), lldb::eByteOrderLittle,
-      /*addr_size*/ exe_ctx ? exe_ctx->GetAddressByteSize() : 4);
+      /*addr_size*/ exe_ctx ? exe_ctx->GetAddressByteSize() : address_size);
 
   return DWARFExpression::Evaluate(exe_ctx, reg_ctx, module_sp, extractor, unit,
                                    lldb::eRegisterKindLLDB,
@@ -1089,6 +1090,18 @@ TEST(DWARFExpression, DW_OP_abs) {
       Evaluate({DW_OP_const1s, static_cast<uint8_t>(-5), DW_OP_abs}),
       ExpectScalar(5));
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit5, DW_OP_abs}), ExpectScalar(5));
+
+  // Generic values have unspecified signedness, but DW_OP_abs interprets its
+  // operand as signed.
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const4u, 0xff, 0xff, 0xff, 0xff,
+                                 DW_OP_abs, DW_OP_stack_value}),
+                       ExpectScalar(32, 1, false));
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_const8u, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+                DW_OP_abs, DW_OP_stack_value},
+               {}, nullptr, nullptr, nullptr,
+               /*address_size=*/8),
+      ExpectScalar(64, 1, false));
 }
 
 TEST(DWARFExpression, DW_OP_div_int_min_by_neg_one) {
@@ -1376,6 +1389,18 @@ TEST(DWARFExpression, DW_OP_shra) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const1s, static_cast<uint8_t>(-8),
                                  DW_OP_lit1, DW_OP_shra}),
                        ExpectScalar(static_cast<int32_t>(-4)));
+
+  // Generic values have unspecified signedness, but DW_OP_shra performs a
+  // signed arithmetic right shift.
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_const4u, 0xff, 0xff, 0xff, 0xff, DW_OP_const1u, 31,
+                DW_OP_shra, DW_OP_stack_value}),
+      ExpectScalar(32, 0xffffffff, false));
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_const8u, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+                DW_OP_const1u, 63, DW_OP_shra, DW_OP_stack_value},
+               {}, nullptr, nullptr, nullptr, /*address_size=*/8),
+      ExpectScalar(64, 0xffffffffffffffffULL, false));
 }
 
 TEST(DWARFExpression, DW_OP_shl_overflow_count) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/225772