[lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results

[labath]

Summary

Dwarf says (Section 2.5.1.1. of DWARF v5) that these operations should
push "generic" (pointer-sized) values. This was not the case for
DW_OP_const operations (which were pushing values based on the size of
arguments), nor DW_OP_litN (which were always pushing 64-bit values).

The practical effect of this that were were unable to display the values
of variables if the size of the DW_OP_const opcode was smaller than the
value of the variable it was describing. This would happen because we
would store this (small) result into a buffer and then would not be able
to read sufficient data out of it (in Value::GetValueAsData). Gcc emits
debug info like this.

Other (more subtle) effects are also possible.

The same fix should be applied to DW_OP_const[us] (leb128 versions), but
I'm not doing that right now, because that would cause us to display
wrong (truncated) values of variables on 32-bit targets (pr48087).

[labath]

lldb/source/Expression/DWARFExpression.cpp

945

This is probably not conforming either -- dwarf says the generic type is of "unspecified signedness", but I think it means a single global value -- not choosing signedness on a per-operand basis.

I've kept that for now, since that was the original behavior, though I didn't notice any issues with this bit removed.

lldb/unittests/Expression/DWARFExpressionTest.cpp

191–192

Had to rewrite these, as they were relying on the fact that DW_OP_const operations were not implicitly converting to the generic type.

[dblaikie]

lldb/source/Expression/DWARFExpression.cpp

944

Local/non-escaping lambdas may be simpler written with default ref capture - is GetAddressByteSize() expensive or would it be fine to call it every time this lambda is called? (& capturing opcodes by ref)

945

Fair, would be good to mark it as at least suspect/uncertain with a comment or such. (or if there's some inverse that would assert if this ever did become relevant, that assertion would be great - but I guess there probably isn't anything we can assert to check that this doesn't matter in practice)

Re: non-conforming: I think the presence of constu and consts encodings that build "generic type"d values probably means the generic type itself may have no particular notion of signedness, but the values that populate it do and I guess intend the value to be interpreted contextually or to carry the sign information? I think it's probably good to carry the sign information from the operands, at least. Maybe with some truncation/sign extension. It's probably pretty close to right...

1275

Why the change to emplace_back? Generally I'd advocate for push_back where possible, since it can't invoke explicit conversions which can require more scrutiny (eg: a vector of unique_ptrs can have a unique_ptr rvalue push_back'd, but can't have a raw pointer push_back'd (a raw pointer would have to be emplace_back'd) - so when I see push_back I can assume it's a "safe" operation)

1298–1305

Seems fair

[labath]

• use push_back
• use default & capture
• use fix signedness (unsigned)

[labath]

Go back to using the signedness of the operand. Implementing the generic type
correctly would be a (much) larger undertaking, so stick to the existing
semantics as much as possible.

lldb/source/Expression/DWARFExpression.cpp

944

GetAddressByteSize is not expensive. A default capture would work just fine.

945

The DWARF standard (particularly version 4, which does not speak about signedness at all), could definitely clearer, but I think that if the committee wanted to convey the notion that a value carries the sign information with it, they would have phrased it differently. To me, the current weasel wording seems to be intended to handle the fact that different architectures have different notions about the signedness of pointers. It this world the exact DW_OP operation used does not affect the signedness of the result, but only the fact whether sign- or zero-extension takes place while converting to the "generic" type.

This is interpretation seems to match gcc behavior:

• gcc will happily use DW_OP_const1u to describe a (signed) int constant, even if that constant would fit into a DW_OP_const1s
• it will also use DW_OP_const1s -1 as a shorter form of DW_OP_const4u 0xffffffff when describing (unsigned)-1.

In fact, these findings (and the fact that clang does not use the fixed-size operands at all) have convinced me to just fix the signedness of the generic type.

1275

I don't know. I wanted to be fancy? Normally, I am also against the overuse of emplace_back...

[labath]

lldb/source/Expression/DWARFExpression.cpp

945

After more experimentation I realized this is a rabbit hole. The signedness of the "generic type" is defined on a per-operand basis, but per-operation:

If the type of the operands is the generic type, except as otherwise specified, the
arithmetic operations perform addressing arithmetic, that is, unsigned arithmetic
that is performed modulo one plus the largest representable address.

The only operation which explicitly uses signed arithmetic is division, but that's like one of the few operations where this distinction matters. Interestingly the modulo operation does _not_ use signed arithmetic.

Even more interestingly, GCC seems to get all of these nuances right -- it writes a smod b by a - (a sdiv b)*b, and uses multiplicative inverses or DW_OP_(GNU_)convert to produce unsigned division. I don't know what llvm does, as I have not been able to coax it to produce these kinds of expressions, but it seems it has a lot of work to do, both on producer and consumer sides.

For now, I'm just going back to the use-signedness-of-operand implementation, as that is what we've done before.

[JDevlieghere]

LGTM. It fixes the issue at hand and gets us closer to conforming to the standard.

lldb/source/Expression/DWARFExpression.cpp

948

Can't wait for C++20's templated lambdas ;-)

1275

TIL

[dblaikie]

Sounds good

lldb/include/lldb/Utility/Scalar.h

public:
   Scalar(double v) : m_type(e_float), m_float(v) {}
   Scalar(long double v) : m_type(e_float), m_float(double(v)) {
     bool ignore;
     m_float.convert(llvm::APFloat::x87DoubleExtended(),
                     llvm::APFloat::rmNearestTiesToEven, &ignore);
   }
   Scalar(llvm::APInt v)
       : m_type(e_int), m_integer(std::move(v), false), m_float(0.0f) {}
+  Scalar(llvm::APSInt v)
+      : m_type(e_int), m_integer(std::move(v)), m_float(0.0f) {}
 
   bool SignExtend(uint32_t bit_pos);
 
   bool ExtractBitfield(uint32_t bit_size, uint32_t bit_offset);
 
   bool SetBit(uint32_t bit);
 
   bool ClearBit(uint32_t bit);

lldb/source/Expression/DWARFExpression.cpp

bool DWARFExpression::Evaluate(
   Value tmp;
   uint32_t reg_num;
 
   /// Insertion point for evaluating multi-piece expression.
   uint64_t op_piece_offset = 0;
   Value pieces; // Used for DW_OP_piece
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+  // A generic type is "an integral type that has the size of an address and an
+  // unspecified signedness". For now, just use the signedness of the operand.
+  // TODO: Implement a real typed stack, and store the genericness of the value
+  // there.
+  auto to_generic = [&](auto v) {
+    bool is_signed = std::is_signed<decltype(v)>::value;
+    return Scalar(llvm::APSInt(
+        llvm::APInt(8 * opcodes.GetAddressByteSize(), v, is_signed),
+        !is_signed));
+  };
 
   while (opcodes.ValidOffset(offset)) {
     const lldb::offset_t op_offset = offset;
     const uint8_t op = opcodes.GetU8(&offset);
 
     if (log && log->GetVerbose()) {
       size_t count = stack.size();
       LLDB_LOGF(log, "Stack before operation has %" PRIu64 " values:",
while (opcodes.ValidOffset(offset)) {
     case DW_OP_xderef:
       if (error_ptr)
         error_ptr->SetErrorString("Unimplemented opcode: DW_OP_xderef.");
       return false;
 
     // All DW_OP_constXXX opcodes have a single operand as noted below:
     //
     // Opcode           Operand 1
-    // DW_OP_const1u    1-byte unsigned integer constant DW_OP_const1s
-    // 1-byte signed integer constant DW_OP_const2u    2-byte unsigned integer
-    // constant DW_OP_const2s    2-byte signed integer constant DW_OP_const4u
-    // 4-byte unsigned integer constant DW_OP_const4s    4-byte signed integer
-    // constant DW_OP_const8u    8-byte unsigned integer constant DW_OP_const8s
-    // 8-byte signed integer constant DW_OP_constu     unsigned LEB128 integer
-    // constant DW_OP_consts     signed LEB128 integer constant
+    // DW_OP_const1u    1-byte unsigned integer constant
+    // DW_OP_const1s    1-byte signed integer constant
+    // DW_OP_const2u    2-byte unsigned integer constant
+    // DW_OP_const2s    2-byte signed integer constant
+    // DW_OP_const4u    4-byte unsigned integer constant
+    // DW_OP_const4s    4-byte signed integer constant
+    // DW_OP_const8u    8-byte unsigned integer constant
+    // DW_OP_const8s    8-byte signed integer constant
+    // DW_OP_constu     unsigned LEB128 integer constant
+    // DW_OP_consts     signed LEB128 integer constant
     case DW_OP_const1u:
-      stack.push_back(Scalar((uint8_t)opcodes.GetU8(&offset)));
+      stack.push_back(to_generic(opcodes.GetU8(&offset)));
       break;
     case DW_OP_const1s:
-      stack.push_back(Scalar((int8_t)opcodes.GetU8(&offset)));
+      stack.push_back(to_generic((int8_t)opcodes.GetU8(&offset)));
       break;
     case DW_OP_const2u:
-      stack.push_back(Scalar((uint16_t)opcodes.GetU16(&offset)));
+      stack.push_back(to_generic(opcodes.GetU16(&offset)));
       break;
     case DW_OP_const2s:
-      stack.push_back(Scalar((int16_t)opcodes.GetU16(&offset)));
+      stack.push_back(to_generic((int16_t)opcodes.GetU16(&offset)));
       break;
     case DW_OP_const4u:
-      stack.push_back(Scalar((uint32_t)opcodes.GetU32(&offset)));
+      stack.push_back(to_generic(opcodes.GetU32(&offset)));
       break;
     case DW_OP_const4s:
-      stack.push_back(Scalar((int32_t)opcodes.GetU32(&offset)));
+      stack.push_back(to_generic((int32_t)opcodes.GetU32(&offset)));
       break;
     case DW_OP_const8u:
-      stack.push_back(Scalar((uint64_t)opcodes.GetU64(&offset)));
+      stack.push_back(to_generic(opcodes.GetU64(&offset)));
       break;
     case DW_OP_const8s:
-      stack.push_back(Scalar((int64_t)opcodes.GetU64(&offset)));
+      stack.push_back(to_generic((int64_t)opcodes.GetU64(&offset)));
       break;
+    // These should also use to_generic, but we can't do that due to a
+    // producer-side bug in llvm. See llvm.org/pr48087.
     case DW_OP_constu:
       stack.push_back(Scalar(opcodes.GetULEB128(&offset)));
       break;
     case DW_OP_consts:
       stack.push_back(Scalar(opcodes.GetSLEB128(&offset)));
       break;
 
     // OPCODE: DW_OP_dup
     // OPERANDS: none
     // DESCRIPTION: duplicates the value at the top of the stack
     case DW_OP_dup:
       if (stack.empty()) {
         if (error_ptr)
           error_ptr->SetErrorString("Expression stack empty for DW_OP_dup.");
while (opcodes.ValidOffset(offset)) {
     case DW_OP_lit24:
     case DW_OP_lit25:
     case DW_OP_lit26:
     case DW_OP_lit27:
     case DW_OP_lit28:
     case DW_OP_lit29:
     case DW_OP_lit30:
     case DW_OP_lit31:
-      stack.push_back(Scalar((uint64_t)(op - DW_OP_lit0)));
+      stack.push_back(to_generic(op - DW_OP_lit0));
       break;
 
     // OPCODE: DW_OP_regN
     // OPERANDS: none
     // DESCRIPTION: Push the value in register n on the top of the stack.
     case DW_OP_reg0:
     case DW_OP_reg1:
     case DW_OP_reg2:

lldb/unittests/Expression/DWARFExpressionTest.cpp

TEST(DWARFExpression, DW_OP_pick) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit1, DW_OP_lit0, DW_OP_pick, 0}),
                        llvm::HasValue(0));
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit1, DW_OP_lit0, DW_OP_pick, 1}),
                        llvm::HasValue(1));
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit1, DW_OP_lit0, DW_OP_pick, 2}),
                        llvm::Failed());
 }
 
+TEST(DWARFExpression, DW_OP_const) {
+  // Extend to address size.
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const1u, 0x88}), llvm::HasValue(0x88));
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const1s, 0x88}),
+                       llvm::HasValue(0xffffff88));
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x47, 0x88}),
+                       llvm::HasValue(0x8847));
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2s, 0x47, 0x88}),
+                       llvm::HasValue(0xffff8847));
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const4u, 0x44, 0x42, 0x47, 0x88}),
+                       llvm::HasValue(0x88474244));
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const4s, 0x44, 0x42, 0x47, 0x88}),
+                       llvm::HasValue(0x88474244));
+
+  // Truncate to address size.
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_const8u, 0x00, 0x11, 0x22, 0x33, 0x44, 0x42, 0x47, 0x88}),
+      llvm::HasValue(0x33221100));
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_const8s, 0x00, 0x11, 0x22, 0x33, 0x44, 0x42, 0x47, 0x88}),
+      llvm::HasValue(0x33221100));
+
+  // Don't truncate to address size for compatibility with clang (pr48087).
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_constu, 0x81, 0x82, 0x84, 0x88, 0x90, 0xa0, 0x40}),
+      llvm::HasValue(0x01010101010101));
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_consts, 0x81, 0x82, 0x84, 0x88, 0x90, 0xa0, 0x40}),
+      llvm::HasValue(0xffff010101010101));
+}
+
 TEST(DWARFExpression, DW_OP_convert) {
   /// Auxiliary debug info.
   const char *yamldata = R"(
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
)";
   // Constant is given as little-endian.
   bool is_signed = true;
   bool not_signed = false;
 
   //
   // Positive tests.
   //
 
-  // Truncate to default unspecified (pointer-sized) type.
-  EXPECT_THAT_EXPECTED(
-      t.Eval({DW_OP_const8u, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, //
-              DW_OP_convert, 0x00}),
-      llvm::HasValue(GetScalar(32, 0x44332211, not_signed)));
-  // Truncate to 32 bits.
-  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const8u, //
-                               0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88,//
+  // Leave as is.
+  EXPECT_THAT_EXPECTED(t.Eval({DW_OP_const4u, 0x11, 0x22, 0x33, 0x44, //
                                DW_OP_convert, offs_uint32_t}),
-                       llvm::HasValue(GetScalar(32, 0x44332211, not_signed)));
+                       llvm::HasValue(GetScalar(64, 0x44332211, not_signed)));
 
-  // Leave as is.
-  EXPECT_THAT_EXPECTED(
-      t.Eval({DW_OP_const8u, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, //
+  // 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, 0x8877665544332211, not_signed)));
+                       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}),
       llvm::HasValue(GetScalar(64, 0xffffffffffeeddcc, 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)));
+
+  // 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}),
+                       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)));
 
   // Also truncate to 8 bits.
   EXPECT_THAT_EXPECTED(
       t.Eval({DW_OP_const4s, 'A', 'B', 'C', 'D', DW_OP_convert, offs_schar}),