Re: RFC: implement DW_OP_bit_piece

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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