Re: RFC: implement DW_OP_bit_piece

Jan Kratochvil
2010-05-20T21:01:48Z

On Wed, 19 May 2010 01:27:33 +0200, Tom Tromey wrote:
> We do take care to optimize for the byte-aligned case.

FYI I am strongly against this decision. This creates (a) more complicated
code to maintain (b) less used and more dense to debug codepath (the bits one)
and (c) I doubt current hardware and current `gcc -O2' code will ever notice
a difference - GDB has much more serious performance issues (symbols reading,
lookup) than evaluation of any specific values.

> --- a/gdb/dwarf2expr.c
> +++ b/gdb/dwarf2expr.c
> +/* The lowest-level function to extract bits from a byte buffer.
> + SOURCE is the buffer. It is updated if we read to the end of a
> + byte.
> + SOURCE_OFFSET_BITS is the offset of the first bit to read. It is
> + updated to reflect the number of bits actually read.
> + NBITS is the number of bits we want to read. This function may
> + read fewer bits.
> + BITS_BIG_ENDIAN is taken directly from gdbarch.
> + RESULT is set to the extracted bits.
> + The function returns the number of bits actually read. */
> +
> +static int
> +extract_bits_primitive (const gdb_byte **source,
> + unsigned int *source_offset_bits,
> + int nbits, int bits_big_endian,
> + unsigned int *result)

IMHO either NBITS should be also updated by the number of bits read or SOURCE
and SOURCE_OFFSET_BITS should be updated togetierh with NBITS by the caller.
This is a bit mix of both styles. But this function is not used much so it
does not matter much.

> +{
> + unsigned int avail, mask, datum;
> +
> + gdb_assert (*source_offset_bits < 8);
> +
> + avail = 8 - *source_offset_bits;
> + if (avail > nbits)
> + avail = nbits;

NBITS is no longer used anywhere, AVAIL is used instead. Therefore this
function ignores the caller's wish of NBITS. I would see here more:
if (avail < nbits)
nbits = avail;
And use NBITS instead of AVAIL everywhere later...

> + mask = (1 << avail) - 1;
> +
> + datum = **source;
> + if (bits_big_endian)
> + datum >>= 8 - *source_offset_bits;

Assuming NBITS contains here the caller's wished read width (possible lowered
by the remaining bits available in current byte):

It should be rather:
datum >>= 8 - (*source_offset_bits + nbits);

source_offset_bits = 2
nbits = 3
mask = 7
b0 b1 b2 b3 b4 b5 b6 b7 = big endian bits numbering
128 64 32 16 8 4 2 1 = value represented by this bit
drop drop WANT WANT WANT drop drop drop

Your expression
datum >>= 6;
datum &= 7;
?? b0 b1
?? 128 64
zero drop drop
Suggested expression
datum >>= 3;
datum &= 7;
b2 b3 b4
32 16 8
WANT WANT WANT

> + else
> + datum >>= *source_offset_bits;
> + datum &= mask;

> + nbits -= avail;

NBITS is updated but its value is then never used anywhere later.

> + *source_offset_bits += avail;
> + if (*source_offset_bits >= 8)
> + {
> + *source_offset_bits -= 8;
> + ++*source;
> + }
> +
> + *result = datum;
> + return avail;
> +}
> +
> +/* Extract some bits from a source buffer and move forward in the
> + buffer.
> +
> + SOURCE is the source buffer. It is updated as bytes are read.
> + SOURCE_OFFSET_BITS is the offset into SOURCE. It is updated as
> + bits are read.
> + NBITS is the number of bits to read.
> + BITS_BIG_ENDIAN is taken directly from gdbarch.
> +
> + This function returns the bits that were read. */
> +
> +static unsigned int
> +extract_bits (const gdb_byte **source, unsigned int *source_offset_bits,
> + int nbits, int bits_big_endian)
> +{
> + unsigned int datum, bits_read;
> +
> + bits_read = extract_bits_primitive (source, source_offset_bits, nbits,
> + bits_big_endian, &datum);
> + if (bits_read < nbits)
> + {
> + unsigned int more;

Missing declarations empty line separator.

> + nbits -= bits_read;
> + if (bits_big_endian)
> + datum <<= nbits;
> + extract_bits_primitive (source, source_offset_bits, nbits,
> + bits_big_endian, &more);

> + datum |= more;

For (bits_big_endian == 0) MORE needs to be <<= NBITS.

extract_bits_primitive() always returns RESULT fit in the lowest-order bits.
Two |= without any << have to mangle both RESULTs each over the other.

> + }
> +
> + return datum;
> +}

If this function is passed NBITS >= 17 it does not assert-fail and it returns
only incomplete 9..16 bits of it. Should there be some
while (bits_read < nbits)
(and associated adjustments) instead?

And this function needs to distinguish both the bits endianity and the bytes
endianity. Out of big-byte-endian machines there are both big-bits-endian and
little-bits-endian ones:
http://en.wikipedia.org/wiki/Bit_numbering#Usage
OTOH there is currently no GDB target using set_gdbarch_bits_big_endian,
therefore all the GDB targets are little-bits-endian. This seems to be wrong.

http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.pdf
does not contain any Figures so I rather used:
http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.7.pdf
where on Figure 3-11 it seems that:
ppc64 big-byte-endian is big-bits-endian
ppc64 little-byte-endian is also big-bits-endian - this seems weird to me.

It is the same for ppc32:
http://refspecs.freestandards.org/elf/elfspec_ppc.pdf
Figure 3-15 and Figure 3-16

> +
> +/* Write some bits into a buffer and move forward in the buffer.
> +
> + DATUM is the bits to write. The low-order bits of DATUM are used.
> + DEST is the destination buffer. It is updated as bytes are
> + written.
> + DEST_OFFSET_BITS is the bit offset in DEST at which writing is
> + done.
> + NBITS is the number of valid bits in DATUM.
> + BITS_BIG_ENDIAN is taken directly from gdbarch. */
> +
> +static void
> +insert_bits (unsigned int datum,
> + gdb_byte *dest, unsigned int dest_offset_bits,
> + int nbits, int bits_big_endian)
> +{
> + unsigned int mask;
> +
> + gdb_assert (dest_offset_bits > 0 && dest_offset_bits < 8);

I would rather see for the ">" part:
gdb_assert (dest_offset_bits >= 0);
as for dest_offset_bits = 0, source_offset_bits != 0 satisfying
> + if (dest_offset_bits % 8 != 0 || source_offset_bits % 8 != 0)
it can IMO happen, and in general this function can support it.
Also if the bits and bytes variants of code would be merged we would need it.

and for the "<" part:
gdb_assert (dest_offset_bits + nbits <= 8);
as this function handles only write to single byte.

> +
> + mask = (1 << nbits) - 1;
> + if (bits_big_endian)
> + {
> + datum <<= 8 - dest_offset_bits;
> + mask <<= 8 - dest_offset_bits;

Again I would rather see:
datum <<= 8 - (dest_offset_bits + nbits);
mask <<= 8 - (dest_offset_bits + nbits);

For:
bits_big_endian = 1
dest_offset_bits = 1
nbits = 3
mask = 7
b0 b1 b2 b3 b4 b5 b6 b7 = big endian bits numbering
128 64 32 16 8 4 2 1 = value represented by this bit
0 0 0 0 0 1 1 1

former expression:
mask <<= 7
mask = 128
1 0 0 0 0 0 0 0

suggested expression:
mask <= 4
0 1 1 1 0 0 0 0

> + }
> + else
> + {
> + datum <<= dest_offset_bits;
> + mask <<= dest_offset_bits;
> + }
> +
> + gdb_assert ((datum & ~mask) == 0);
> +
> + *dest = (*dest & ~mask) | datum;
> +}

> +static void
> +copy_bitwise (gdb_byte *dest, unsigned int dest_offset_bits,
> + const gdb_byte *source, unsigned int source_offset_bits,
> + unsigned int bit_count,
> + int bits_big_endian)

Seems OK to me.

> read_pieced_value (struct value *v)
+
> @@ -402,11 +627,16 @@ write_pieced_value (struct value *to, struct value *from)

Not reviewed, the patches no longer apply to HEAD and also this part may or
may not be changed a lot depending on the discussions above.

Thanks,
Jan