Fix a buffer-size bug when the first DW_OP_piece is undefined

[aprantl]

Summary

Fix a buffer-size bug when the first DW_OP_piece is undefined and document the shortcomings of LLDB's partially defined DW_OP_piece  handling.

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

rdar://problem/46262998

[vsk]

LGTM, this seems like a clear improvement.

[vsk]

lldb/source/Expression/DWARFExpression.cpp

2077

FTR I'm not really sure this case is allowed by the dwarf standard. Under DW_OP_piece the standard references a "preceding simple location description" -- istm like an error if the stack is empty. My $0.0002 :)

[labath]

lldb/source/Expression/DWARFExpression.cpp

2077

I think this is fine -- an "empty location description" (section 2.6.1.1.4 of DWARF4) is a kind of a "simple location description" (sect. 2.6.1.1).

The part that worries me more is this: "Each simple location description that is a DWARF expression is evaluated independently of any others (as though on its own separate stack, if any).". I think that means we should reset/clear the stack after each DW_OP_piece, but we don't currently do that (we just pop the topmost value). However, I am not sure if this has any impact on valid DWARF expressions (though it can cause us to produce bogus values for invalid ones).

[aprantl]

lldb/source/Expression/DWARFExpression.cpp

2077

That's cute, so with our current implementation one could write a communicating DWARF expression like:

DW_OP_lit1 DW_OP_dup DW_OP_piece 1 DW_OP_piece 1

Is the second byte supposed to be 1 or undef?

;-)

I agree, we should probably clear the stack.

lldb/source/Expression/DWARFExpression.cpp

case DW_OP_piece: {
       if (piece_byte_size > 0) {
         Value curr_piece;
 
         if (stack.empty()) {
           // In a multi-piece expression, this means that the current piece is
           // not available. Fill with zeros for now by resizing the data and
           // appending it
           curr_piece.ResizeData(piece_byte_size);
+          // Note that "0" is not a correct value for the unknown bits.
+          // It would be better to also return a mask of valid bits together
+          // with the expression result, so the debugger can print missing
+          // members as "<optimized out>" or something.
           ::memset(curr_piece.GetBuffer().GetBytes(), 0, piece_byte_size);
           pieces.AppendDataToHostBuffer(curr_piece);
         } else {
           Status error;
           // Extract the current piece into "curr_piece"
           Value curr_piece_source_value(stack.back());
           stack.pop_back();
 
case DW_OP_piece: {
             }
 
             if (pieces.AppendDataToHostBuffer(curr_piece) == 0) {
               if (error_ptr)
                 error_ptr->SetErrorString("failed to append piece data");
               return false;
             }
           }
-          op_piece_offset += piece_byte_size;
         }
+        op_piece_offset += piece_byte_size;
       }
     } break;
 
     case DW_OP_bit_piece: // 0x9d ULEB128 bit size, ULEB128 bit offset (DWARF3);
       if (stack.size() < 1) {
         if (error_ptr)
           error_ptr->SetErrorString(
               "Expression stack needs at least 1 item for DW_OP_bit_piece.");

lldb/unittests/Expression/DWARFExpressionTest.cpp

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)));
+  EXPECT_THAT_EXPECTED(
+      Evaluate({DW_OP_piece, 1, DW_OP_const1u, 0xff, DW_OP_piece, 1}),
+      // Note that the "00" should really be "undef", but we can't
+      // represent that yet.
+      llvm::HasValue(GetScalar(16, 0xff00, true)));
 }