Re: [PATCH 2/3] Fix copy_bitwise()

[Luis Machado <lgustavo@codesourcery.com>]

On 11/14/2016 09:02 AM, Andreas Arnez wrote:
> When the user writes or reads a variable whose location is described
> with DWARF pieces (DW_OP_piece or DW_OP_bit_piece), GDB's helper
> function copy_bitwise is invoked for each piece.  The implementation of
> this function has a bug that may result in a corrupted copy, depending
> on alignment and bit size.  (Full-byte copies are not affected.)
>
> This rewrites copy_bitwise, replacing its algorithm by a fixed version,
> and adding an appropriate test case.  Without the fix the new test case
> fails, e.g.:
>
>   print def_t
>   $2 = {a = 0, b = 4177919}
>   (gdb) FAIL: gdb.dwarf2/nonvar-access.exp: print def_t
>
> Written in binary, the wrong result above looks like this:
>
>   01111111011111111111111
>
> Which means that two zero bits have sneaked into the copy of the
> original all-one bit pattern.  The test uses this simple all-one value
> in order to avoid another GDB bug that causes the DWARF piece of a
> DW_OP_stack_value to be taken from the wrong end on big-endian
> architectures.
>
> gdb/ChangeLog:
>
> 	* dwarf2loc.c (extract_bits_primitive): Remove.
> 	(extract_bits): Remove.
> 	(copy_bitwise): Rewrite.  Fixes a possible corruption that may
> 	occur for non-byte-aligned copies.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.dwarf2/nonvar-access.exp: Add a test for accessing
> 	non-byte-aligned bit fields.
> ---
>  gdb/dwarf2loc.c                            | 200 ++++++++---------------------
>  gdb/testsuite/gdb.dwarf2/nonvar-access.exp |  33 ++++-
>  2 files changed, 84 insertions(+), 149 deletions(-)
>
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 93aec1f..3a241a8 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1491,175 +1491,79 @@ allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
>    return 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.  It is updated to
> -   reflect the number of bits actually read.  This function may read
> -   fewer bits.
> -   BITS_BIG_ENDIAN is taken directly from gdbarch.
> -   This function returns the extracted bits.  */
> -
> -static unsigned int
> -extract_bits_primitive (const gdb_byte **source,
> -			unsigned int *source_offset_bits,
> -			int *nbits, int bits_big_endian)
> -{
> -  unsigned int avail, mask, datum;
> -
> -  gdb_assert (*source_offset_bits < 8);
> -
> -  avail = 8 - *source_offset_bits;
> -  if (avail > *nbits)
> -    avail = *nbits;
> -
> -  mask = (1 << avail) - 1;
> -  datum = **source;
> -  if (bits_big_endian)
> -    datum >>= 8 - (*source_offset_bits + *nbits);
> -  else
> -    datum >>= *source_offset_bits;
> -  datum &= mask;
> -
> -  *nbits -= avail;
> -  *source_offset_bits += avail;
> -  if (*source_offset_bits >= 8)
> -    {
> -      *source_offset_bits -= 8;
> -      ++*source;
> -    }
> -
> -  return datum;
> -}
> -
> -/* 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;
> -
> -  gdb_assert (nbits > 0 && nbits <= 8);
> -
> -  datum = extract_bits_primitive (source, source_offset_bits, &nbits,
> -				  bits_big_endian);
> -  if (nbits > 0)
> -    {
> -      unsigned int more;
> -
> -      more = extract_bits_primitive (source, source_offset_bits, &nbits,
> -				     bits_big_endian);
> -      if (bits_big_endian)
> -	datum <<= nbits;
> -      else
> -	more <<= nbits;
> -      datum |= more;
> -    }
> -
> -  return datum;
> -}
> -
> -/* 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.  */
> +/* Copy NBITS bits from SOURCE to DEST starting at the given bit
> +   offsets.  Use the bit order as specified by BITS_BIG_ENDIAN.
> +   Source and destination buffers must not overlap.  */
>
>  static void
> -insert_bits (unsigned int datum,
> -	     gdb_byte *dest, unsigned int dest_offset_bits,
> -	     int nbits, int bits_big_endian)
> +copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
> +	      const gdb_byte *source, ULONGEST source_offset,
> +	      ULONGEST nbits, int bits_big_endian)
>  {
> -  unsigned int mask;
> +  unsigned int buf, avail;
>
> -  gdb_assert (dest_offset_bits + nbits <= 8);
> +  if (nbits == 0)
> +    return;
>
> -  mask = (1 << nbits) - 1;
>    if (bits_big_endian)
>      {
> -      datum <<= 8 - (dest_offset_bits + nbits);
> -      mask <<= 8 - (dest_offset_bits + nbits);
> +      /* Start from the end, then work backwards.  */
> +      dest_offset += nbits - 1;
> +      dest += dest_offset / 8;
> +      dest_offset = 7 - dest_offset % 8;
> +      source_offset += nbits - 1;
> +      source += source_offset / 8;
> +      source_offset = 7 - source_offset % 8;
>      }
>    else
>      {
> -      datum <<= dest_offset_bits;
> -      mask <<= dest_offset_bits;
> +      dest += dest_offset / 8;
> +      dest_offset %= 8;
> +      source += source_offset / 8;
> +      source_offset %= 8;

Are you sure you will always have non-zero source_offset and dest_offset 
when explicitly dividing them by 8? If i were to feed (or GDB, in some 
erroneous state) invalid data to the function, this would likely crash?

There are other cases of explicit / operations.

>      }
>
> -  gdb_assert ((datum & ~mask) == 0);
> -
> -  *dest = (*dest & ~mask) | datum;
> -}
> -
> -/* Copy bits from a source to a destination.
> -
> -   DEST is where the bits should be written.
> -   DEST_OFFSET_BITS is the bit offset into DEST.
> -   SOURCE is the source of bits.
> -   SOURCE_OFFSET_BITS is the bit offset into SOURCE.
> -   BIT_COUNT is the number of bits to copy.
> -   BITS_BIG_ENDIAN is taken directly from gdbarch.  */
> -
> -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)
> -{
> -  unsigned int dest_avail;
> -  int datum;
> +  /* Fill BUF with DEST_OFFSET bits from the destination and 8 -
> +     SOURCE_OFFSET bits from the source.  */
> +  buf = *(bits_big_endian ? source-- : source++) >> source_offset;

Maybe it's just me, but having constructs like the above don't help much 
performance-wise and make the code slightly less readable. Should we 
expand this further? There are multiple occurrences of this.

Also, should we harden the method to prevent dereferencing NULL source 
or dest?

> +  buf <<= dest_offset;
> +  buf |= *dest & ((1 << dest_offset) - 1);
>
> -  /* Reduce everything to byte-size pieces.  */
> -  dest += dest_offset_bits / 8;
> -  dest_offset_bits %= 8;
> -  source += source_offset_bits / 8;
> -  source_offset_bits %= 8;
> +  /* NBITS: bits yet to be written; AVAIL: BUF's fill level.  */
> +  nbits += dest_offset;
> +  avail = dest_offset + 8 - source_offset;
>
> -  dest_avail = 8 - dest_offset_bits % 8;
> -
> -  /* See if we can fill the first destination byte.  */
> -  if (dest_avail < bit_count)
> +  /* Flush 8 bits from BUF, if appropriate.  */
> +  if (nbits >= 8 && avail >= 8)
>      {
> -      datum = extract_bits (&source, &source_offset_bits, dest_avail,
> -			    bits_big_endian);
> -      insert_bits (datum, dest, dest_offset_bits, dest_avail, bits_big_endian);
> -      ++dest;
> -      dest_offset_bits = 0;
> -      bit_count -= dest_avail;
> +      *(bits_big_endian ? dest-- : dest++) = buf;
> +      buf >>= 8;
> +      avail -= 8;
> +      nbits -= 8;
>      }
>
> -  /* Now, either DEST_OFFSET_BITS is byte-aligned, or we have fewer
> -     than 8 bits remaining.  */
> -  gdb_assert (dest_offset_bits % 8 == 0 || bit_count < 8);
> -  for (; bit_count >= 8; bit_count -= 8)
> +  /* Copy the middle part.  */
> +  if (nbits >= 8)
>      {
> -      datum = extract_bits (&source, &source_offset_bits, 8, bits_big_endian);
> -      *dest++ = (gdb_byte) datum;
> +      size_t len = nbits / 8;
> +
> +      while (len--)
> +	{
> +	  buf |= *(bits_big_endian ? source-- : source++) << avail;
> +	  *(bits_big_endian ? dest-- : dest++) = buf;
> +	  buf >>= 8;
> +	}
> +      nbits %= 8;
>      }
>
> -  /* Finally, we may have a few leftover bits.  */
> -  gdb_assert (bit_count <= 8 - dest_offset_bits % 8);
> -  if (bit_count > 0)
> +  /* Write the last byte.  */
> +  if (nbits)
>      {
> -      datum = extract_bits (&source, &source_offset_bits, bit_count,
> -			    bits_big_endian);
> -      insert_bits (datum, dest, dest_offset_bits, bit_count, bits_big_endian);
> +      if (avail < nbits)
> +	buf |= *source << avail;
> +
> +      buf &= (1 << nbits) - 1;
> +      *dest = (*dest & (~0 << nbits)) | buf;
>      }
>  }
>
> diff --git a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
> index 21532a6..8910f6d 100644
> --- a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
> +++ b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
> @@ -32,7 +32,7 @@ Dwarf::assemble $asm_file {
>  	    {DW_AT_name main.c}
>  	} {
>  	    declare_labels int_type_label short_type_label
> -	    declare_labels struct_s_label
> +	    declare_labels struct_s_label struct_t_label
>
>  	    int_type_label: base_type {
>  		{name "int"}
> @@ -58,6 +58,24 @@ Dwarf::assemble $asm_file {
>  		}
>  	    }
>
> +	    struct_t_label: structure_type {
> +		{name t}
> +		{byte_size 4 DW_FORM_sdata}
> +	    } {
> +		member {
> +		    {name a}
> +		    {type :$int_type_label}
> +		    {data_member_location 0 DW_FORM_udata}
> +		    {bit_size 9 DW_FORM_udata}
> +		}
> +		member {
> +		    {name b}
> +		    {type :$int_type_label}
> +		    {data_bit_offset 9 DW_FORM_udata}
> +		    {bit_size 23 DW_FORM_udata}
> +		}
> +	    }
> +
>  	    DW_TAG_subprogram {
>  		{name main}
>  		{DW_AT_external 1 flag}
> @@ -84,6 +102,18 @@ Dwarf::assemble $asm_file {
>  			bit_piece 24 0
>  		    } SPECIAL_expr}
>  		}
> +		DW_TAG_variable {
> +		    {name def_t}
> +		    {type :$struct_t_label}
> +		    {location {
> +			const1u 0
> +			stack_value
> +			bit_piece 9 0
> +			const1s -1
> +			stack_value
> +			bit_piece 23 0
> +		    } SPECIAL_expr}
> +		}
>  	    }
>  	}
>      }
> @@ -99,5 +129,6 @@ if ![runto_main] {
>  }
>
>  gdb_test "print def_s" " = \\{a = 0, b = -1\\}"
> +gdb_test "print def_t" " = \\{a = 0, b = -1\\}"
>  gdb_test "print undef_int" " = <optimized out>"
>  gdb_test "print undef_s.a" " = <optimized out>"
>

Otherwise it looks good to me.