[Lldb-commits] [PATCH] D76449: [lldb/Dwarf] Change DW_OP_piece to use an llvm::BitVector

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 3 inline comments as done.
mib added inline comments.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:936
   /// Insertion point for evaluating multi-piece expression.
-  uint64_t op_piece_offset = 0;
-  Value pieces; // Used for DW_OP_piece
+  llvm::BitVector bit_pieces;
+  llvm::BitVector bit_mask;
----------------
aprantl wrote:
> Is this only going to be used for DW_OP_bit_piece? Otherwise I would have expected a name like `top(_of_stack)` or `result`.
Yes, the same BitVector is used for both but I  separated the DW_OP_piece changes for this diff, I forgot to change the name —‘


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2505
+      Value pieces;
+      pieces.AppendBytes(bit_pieces.getData().data(), bit_pieces.size() / 8);
       result = pieces;
----------------
vsk wrote:
> Can we assert that this doesn't drop any bits (bit_pieces.size() % 8 == 0)?
Actually, I think this should be changed to `std::ceil(bit_pieces.size() / 8.0)` instead of putting an assert


================
Comment at: lldb/source/Utility/Scalar.cpp:200
   case e_uint512:
-    return (m_integer.getBitWidth() / 8);
+    return ceil(m_integer.getBitWidth() / 8.0);
   case e_float:
----------------
vsk wrote:
> aprantl wrote:
> > ?
> This is worth a comment, I'm not sure what this is for.
When the bitWidth is < 8, the division result is 0, but it should be 1 byte instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76449/new/

https://reviews.llvm.org/D76449