Page MenuHomePhabricator

[lldb/Dwarf] Change DW_OP_piece to use an llvm::BitVector (WIP)
Changes PlannedPublic

Authored by mib on Mar 19 2020, 8:04 PM.

Details

Summary

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 <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Mar 19 2020, 8:04 PM
aprantl added inline comments.Mar 19 2020, 9:38 PM
lldb/source/Expression/DWARFExpression.cpp
936

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.

937

This should perhaps be called undef_mask? And should have a comment what it is used for.

1267

This is not portable. The size of int could be anything depending on the host. Why not always use APIInt?

2164

how do you know the unsigned int is large enough for this operation?

lldb/source/Utility/Scalar.cpp
200

?

vsk added a reviewer: labath.Mar 19 2020, 9:47 PM

Excited to see this take shape :)

lldb/source/Expression/DWARFExpression.cpp
935

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

1261

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)

2505

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

lldb/source/Utility/Scalar.cpp
200

This is worth a comment, I'm not sure what this is for.

mib marked 3 inline comments as done.Mar 19 2020, 10:09 PM
mib added inline comments.
lldb/source/Expression/DWARFExpression.cpp
936

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 —‘

2505

Actually, I think this should be changed to std::ceil(bit_pieces.size() / 8.0) instead of putting an assert

lldb/source/Utility/Scalar.cpp
200

When the bitWidth is < 8, the division result is 0, but it should be 1 byte instead.

mib added a comment.Mar 19 2020, 10:16 PM

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.

mib marked an inline comment as done.Mar 19 2020, 10:21 PM
mib added inline comments.
lldb/source/Expression/DWARFExpression.cpp
2164

TBH, I just chose the same return type as GetScalar::UInt.
I’ll change it to GetScalar::UInt128 with an llvm::APInt

mib retitled this revision from [lldb/Dwarf] Change DW_OP_piece to use an llvm::BitVector to [lldb/Dwarf] Change DW_OP_piece to use an llvm::BitVector (WIP).Mar 19 2020, 10:32 PM

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

lldb/source/Expression/DWARFExpression.cpp
1261

+1 for doing this separately

1267

+1 for using APInt across the board. I'd be tempted to just delete the relevant Scalar constructors.

lldb/source/Utility/Scalar.cpp
200

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

mib planned changes to this revision.Mar 26 2020, 12:05 AM