[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 , @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()));
+  stack.push_back(Scalar((unsigned int)opcodes.GetU8()));
   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 $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()));
+  stack.push_back(Scalar((unsigned int)opcodes.GetU16()));
   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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 an inline comment as done.
mib added inline comments.



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);

aprantl wrote:
> how do you know the `unsigned int` is large enough for this operation?
TBH, I just chose the same return type as GetScalar::UInt.
I’ll change it to GetScalar::UInt128 with an llvm::APInt 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76449



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76449



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a reviewer: labath.
vsk added a comment.

Excited to see this take shape :)




Comment at: lldb/source/Expression/DWARFExpression.cpp:935
 
   /// Insertion point for evaluating multi-piece expression.
+  llvm::BitVector bit_pieces;

Stale comment here, would be nice to describe the purpose of the two bitvectors.



Comment at: lldb/source/Expression/DWARFExpression.cpp:1261
 case DW_OP_const1u:
-  stack.push_back(Scalar((uint8_t)opcodes.GetU8()));
+  stack.push_back(Scalar((unsigned int)opcodes.GetU8()));
   break;

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::value, int> = 0>` in front of each overload)



Comment at: lldb/source/Expression/DWARFExpression.cpp:2505
+  Value pieces;
+  pieces.AppendBytes(bit_pieces.getData().data(), bit_pieces.size() / 8);
   result = pieces;

Can we assert that this doesn't drop any bits (bit_pieces.size() % 8 == 0)?



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:

aprantl wrote:
> ?
This is worth a comment, I'm not sure what this is for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76449



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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()));
+  stack.push_back(Scalar((unsigned int)opcodes.GetU16()));
   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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: aprantl, vsk, friss.
mib added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch changes the implementation of DW_OP_piece to use
llvm::BitVector instead of the lldb_private::Value.

This allow to have more granularity which would be useful to implement
DW_OP_bit_piece but also when combined with a second BitVector for
masking the unknown bits/bytes, improves the DWARF Expression
evaluation Since it will show a future patch, the unknown bits/bytes as
optimised rather than showing - misleading - zeroes.

Additionally, this patch fixes a overloading resolution on the Scalar
constructor when using fixed-size interger types instead of fundamentale types.
It also fixes a bug when requesting a Scalar size when the value is less
than a byte.

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76449

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/source/Utility/Scalar.cpp

Index: lldb/source/Utility/Scalar.cpp
===
--- lldb/source/Utility/Scalar.cpp
+++ lldb/source/Utility/Scalar.cpp
@@ -197,7 +197,7 @@
   case e_uint256:
   case e_sint512:
   case e_uint512:
-return (m_integer.getBitWidth() / 8);
+return ceil(m_integer.getBitWidth() / 8.0);
   case e_float:
 return sizeof(float_t);
   case e_double:
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -933,8 +933,8 @@
   uint32_t reg_num;
 
   /// 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;
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
@@ -1258,34 +1258,34 @@
 // 8-byte signed integer constant DW_OP_constu unsigned LEB128 integer
 // constant DW_OP_consts signed LEB128 integer constant
 case DW_OP_const1u:
-  stack.push_back(Scalar((uint8_t)opcodes.GetU8()));
+  stack.push_back(Scalar((unsigned int)opcodes.GetU8()));
   break;
 case DW_OP_const1s:
-  stack.push_back(Scalar((int8_t)opcodes.GetU8()));
+  stack.push_back(Scalar((int)opcodes.GetU8()));
   break;
 case DW_OP_const2u:
-  stack.push_back(Scalar((uint16_t)opcodes.GetU16()));
+  stack.push_back(Scalar((unsigned int)opcodes.GetU16()));
   break;
 case DW_OP_const2s:
-  stack.push_back(Scalar((int16_t)opcodes.GetU16()));
+  stack.push_back(Scalar((int)opcodes.GetU16()));
   break;
 case DW_OP_const4u:
-  stack.push_back(Scalar((uint32_t)opcodes.GetU32()));
+  stack.push_back(Scalar((unsigned int)opcodes.GetU32()));
   break;
 case DW_OP_const4s:
-  stack.push_back(Scalar((int32_t)opcodes.GetU32()));
+  stack.push_back(Scalar((int)opcodes.GetU32()));
   break;
 case DW_OP_const8u:
-  stack.push_back(Scalar((uint64_t)opcodes.GetU64()));
+  stack.push_back(Scalar((unsigned long)opcodes.GetU64()));
   break;
 case DW_OP_const8s:
-  stack.push_back(Scalar((int64_t)opcodes.GetU64()));
+  stack.push_back(Scalar((long)opcodes.GetU64()));
   break;
 case DW_OP_constu:
-  stack.push_back(Scalar(opcodes.GetULEB128()));
+  stack.push_back(Scalar((unsigned long)opcodes.GetULEB128()));
   break;
 case DW_OP_consts:
-  stack.push_back(Scalar(opcodes.GetSLEB128()));
+  stack.push_back(Scalar((long)opcodes.GetSLEB128()));
   break;
 
 // OPCODE: DW_OP_dup
@@ -2064,27 +2064,19 @@
 
   if (piece_byte_size > 0) {
 Value curr_piece;
+
+const uint64_t curr_width = std::max(bit_mask.size(), bit_pieces.size());
 
 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 "" or something.
-  ::memset(curr_piece.GetBuffer().GetBytes(), 0, piece_byte_size);
-  pieces.AppendDataToHostBuffer(curr_piece);
+  bit_mask.resize(curr_width + piece_byte_size * 8);
+  bit_mask.set(curr_width, curr_width + piece_byte_size * 8);
 } else {
   Status error;
   // Extract the current piece into "curr_piece"
   Value curr_piece_source_value(stack.back());
   stack.pop_back();
 
-  const Value::ValueType curr_piece_source_value_type =
-