[PATCH 2/3] Fix copy_bitwise()

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

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;
     }
 
-  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;
+  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>"
-- 
2.3.0