Add testing for DW_OP_piece and fix a bug with small Scalar values.

[aprantl]

Summary

By switching to Scalars that are backed by explicitly-sized APInts we can avoid a bug that increases the buffer reserved for a small piece to the next-largest host integer type.

This manifests as "DW_OP_piece for offset foo but top of stack is of size bar".

[davide]

Looks great! Thanks Adrian. My understanding is that DW_OP_piece is still incomplete, right?

[aprantl]

My understanding is that DW_OP_piece is still incomplete, right?

DW_OP_piece should work (with the documented caveat about the undefined values) fine, what definitely doesn't yet work is the combination of DW_OP_piece and DW_OP_bit_piece.

[vsk]

LGTM. In the future it'd be nice to maybe split out piece/bit_piece handling into their own self-contained functions, and maybe share more code between them. But landing this narrow fix now sounds good.

lldb/source/Expression/DWARFExpression.cpp

case DW_OP_piece: {
                                        : "host",
                   addr);
             }
             return false;
 
           case Value::eValueTypeScalar: {
             uint32_t bit_size = piece_byte_size * 8;
             uint32_t bit_offset = 0;
-            if (!curr_piece_source_value.GetScalar().ExtractBitfield(
+            Scalar &scalar = curr_piece_source_value.GetScalar();
+            if (!scalar.ExtractBitfield(
                     bit_size, bit_offset)) {
               if (error_ptr)
                 error_ptr->SetErrorStringWithFormat(
                     "unable to extract %" PRIu64 " bytes from a %" PRIu64
                     " byte scalar value.",
                     piece_byte_size,
                     (uint64_t)curr_piece_source_value.GetScalar()
                         .GetByteSize());
               return false;
             }
-            curr_piece = curr_piece_source_value;
+            // Create curr_piece with bit_size. By default Scalar
+            // grows to the nearest host integer type.
+            llvm::APInt fail_value(1, 0, false);
+            llvm::APInt ap_int = scalar.UInt128(fail_value);
+            assert(ap_int.getBitWidth() >= bit_size);
+            llvm::ArrayRef<uint64_t> buf{ap_int.getRawData(),
+                                         ap_int.getNumWords()};
+            curr_piece.GetScalar() = Scalar(llvm::APInt(bit_size, buf));
           } break;
 
           case Value::eValueTypeVector: {
             if (curr_piece_source_value.GetVector().length >= piece_byte_size)
               curr_piece_source_value.GetVector().length = piece_byte_size;
             else {
               if (error_ptr)
                 error_ptr->SetErrorStringWithFormat(
                     "unable to extract %" PRIu64 " bytes from a %" PRIu64
                     " byte vector value.",
                     piece_byte_size,
                     (uint64_t)curr_piece_source_value.GetVector().length);
               return false;
             }
           } break;
           }
 
           // Check if this is the first piece?
           if (op_piece_offset == 0) {
             // This is the first piece, we should push it back onto the stack
             // so subsequent pieces will be able to access this piece and add
-            // to it
+            // to it.
             if (pieces.AppendDataToHostBuffer(curr_piece) == 0) {
               if (error_ptr)
                 error_ptr->SetErrorString("failed to append piece data");
               return false;
             }
           } else {
             // If this is the second or later piece there should be a value on
-            // the stack
+            // the stack.
             if (pieces.GetBuffer().GetByteSize() != op_piece_offset) {
               if (error_ptr)
                 error_ptr->SetErrorStringWithFormat(
                     "DW_OP_piece for offset %" PRIu64
                     " but top of stack is of size %" PRIu64,
                     op_piece_offset, pieces.GetBuffer().GetByteSize());
               return false;
             }

lldb/unittests/Expression/DWARFExpressionTest.cpp

static llvm::Expected<Scalar> Evaluate(llvm::ArrayRef<uint8_t> expr,
   Status status;
   if (!DWARFExpression::Evaluate(
           /*exe_ctx*/ nullptr, /*reg_ctx*/ nullptr, module_sp, extractor, unit,
           lldb::eRegisterKindLLDB,
           /*initial_value_ptr*/ nullptr,
           /*object_address_ptr*/ nullptr, result, &status))
     return status.ToError();
 
+  switch (result.GetValueType()) {
+  case Value::eValueTypeScalar:
     return result.GetScalar();
+  case Value::eValueTypeHostAddress: {
+    // Convert small buffers to scalars to simplify the tests.
+    DataBufferHeap &buf = result.GetBuffer();
+    if (buf.GetByteSize() <= 8) {
+      uint64_t val = 0;
+      memcpy(&val, buf.GetBytes(), buf.GetByteSize());
+      return Scalar(llvm::APInt(buf.GetByteSize()*8, val, false));
+    }
+  }
+    LLVM_FALLTHROUGH;
+  default:
+    return status.ToError();
+  }
 }
 
 /// A mock module holding an object file parsed from YAML.
 class YAMLModule : public lldb_private::Module {
 public:
   YAMLModule(ArchSpec &arch) : Module(FileSpec("test"), arch) {}
   void SetObjectFile(lldb::ObjectFileSP obj_file) { m_objfile_sp = obj_file; }
   ObjectFile *GetObjectFile() override { return m_objfile_sp.get(); }
EXPECT_THAT_ERROR(
       t.Eval({DW_OP_const1s, 'X', DW_OP_convert, 0x01}).takeError(),
       llvm::Failed());
 
   // Unsupported.
   EXPECT_THAT_ERROR(
       t.Eval({DW_OP_const1s, 'X', DW_OP_convert, 0x1d}).takeError(),
       llvm::Failed());
 }
+
+TEST(DWARFExpression, DW_OP_piece) {
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
+                                 DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
+                       llvm::HasValue(GetScalar(32, 0x44332211, true)));
+}