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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

looking forward to seeing the final form of this

In D76449#1932293 <https://reviews.llvm.org/D76449#1932293>, @mib wrote:

> Note that this diff is more a quick draft than a “finished” patch, I’m not expecting to submit this until the DW_OP_bit_piece support is done.


Smaller patches are generally better. It would be great if you could split this into an "NFC" patch which just changes the underlying representation to BitVector and another patch which adds the validity mask. (It's fine if you wait with the first patch if you determine whether the validity mask approach is feasible, though honestly, I don't see any other way of achieving this.)



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1261
     case DW_OP_const1u:
-      stack.push_back(Scalar((uint8_t)opcodes.GetU8(&offset)));
+      stack.push_back(Scalar((unsigned int)opcodes.GetU8(&offset)));
       break;
----------------
vsk wrote:
> I don't really understand a bunch of these changes. Is the idea to prevent implicit conversion to a type with the wrong size/signedness? I think that should really be tackled separately. We can make `Scalar`'s constructors safe like this: https://godbolt.org/z/APUMA6
> 
> (basically stick `template<typename T, std::enable_if_t<std::is_same<T, $whateverType>::value, int> = 0>` in front of each overload)
+1 for doing this separately


================
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;
----------------
aprantl wrote:
> This is not portable. The size of int could be anything depending on the host. Why not always use APIInt?
+1 for using APInt across the board. I'd be tempted to just delete the relevant Scalar constructors.


================
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:
----------------
mib wrote:
> 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.
Normally that would be done via something like `(x+7)/8` and not by going floating point. However, that should be done as a separate patch (and a test).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76449