clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land.
In D73148#1832762 <https://reviews.llvm.org/D73148#1832762>, @vsk wrote: > In D73148#1832704 <https://reviews.llvm.org/D73148#1832704>, @clayborg wrote: > > > Would it be better to just ensure that the buffer for DW_OP_piece is at > > least the size of the type instead? To do this right we would really need > > to track which bytes in the buffer are actually valid. This patch assumes > > we will always get the first N bytes of a value. Is it possible to describe > > bytes 4:7 of a 16 bit value where bits 0:3 and 8:15 are not valid? In this > > case, with this patch, we would think that bytes 0:3 are valid when they > > are just not specified, 4:7 would be valid, and 8:15 would not display > > because our value doesn't contain the value. > > > It's not necessary for DW_OP_piece to produce a type-sized result buffer, as > the description of an object must start at byte offset 0, and we represent > the undefined bits within the Value either with 0 or by leaving them out. > E.g. `Evaluate({DW_OP_piece, 1, DW_OP_const1u, 0xff, DW_OP_piece, 1, > DW_OP_piece, 1})` produces the 24-bit scalar `0x00ff00`, where the 0's > correspond to the undefined low bits and the undefined high bits are left > out. It /is/ a bug that we use 0 for undefined bits instead of `u` (or > something), but we don't need a type-sized buffer to fix that (a bitset would > suffice). Finding a way to improve the representation of undefined bits is > something I'm interested in, but it seems like a separate problem, and it's > more urgent for me to fix the memory smasher. > > I'm also not sure that making DW_OP_piece produce a type-sized result would > be a sufficient fix, as there (theoretically) may be clients of > `Value::ResizeData()` other than the DW_OP_piece logic. I just count one, > though (`ExpressionVariable::GetValueBytes()`), and I'm not sure whether it > can resize the buffer to something other than the underlying type size. Thanks for the clarification. This will work for now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73148/new/ https://reviews.llvm.org/D73148 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
