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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl 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;
----------------
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`.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:937
+  llvm::BitVector bit_pieces;
+  llvm::BitVector bit_mask;
 
----------------
This should perhaps be called undef_mask? And should have a comment what it is used for.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1267
     case DW_OP_const2u:
-      stack.push_back(Scalar((uint16_t)opcodes.GetU16(&offset)));
+      stack.push_back(Scalar((unsigned int)opcodes.GetU16(&offset)));
       break;
----------------
This is not portable. The size of int could be anything depending on the host. Why not always use APIInt?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2164
+          unsigned int curr_scalar = curr_piece.GetScalar().UInt();
+          curr_scalar = curr_scalar << curr_width;
+          bit_pieces.resize(curr_width + piece_byte_size * 8);
----------------
how do you know the `unsigned int` is large enough for this operation?


================
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:
----------------
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76449