This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]