Fix copy_bitwise()
authorAndreas Arnez <arnez@linux.vnet.ibm.com>
Thu, 24 Nov 2016 16:48:03 +0000 (17:48 +0100)
committerAndreas Arnez <arnez@linux.vnet.ibm.com>
Thu, 24 Nov 2016 16:48:03 +0000 (17:48 +0100)
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/ChangeLog
gdb/dwarf2loc.c
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.dwarf2/nonvar-access.exp

index 22f02cc..aa2fe41 100644 (file)
@@ -1,5 +1,12 @@
 2016-11-24  Andreas Arnez  <arnez@linux.vnet.ibm.com>
 
+       * dwarf2loc.c (extract_bits_primitive): Remove.
+       (extract_bits): Remove.
+       (copy_bitwise): Rewrite.  Fixes a possible corruption that may
+       occur for non-byte-aligned copies.
+
+2016-11-24  Andreas Arnez  <arnez@linux.vnet.ibm.com>
+
        PR gdb/12616
        * dwarf2read.c (dwarf2_add_field): Handle the DWARF V4 attribute
        DW_AT_data_bit_offset.
index 44dceda..b238133 100644 (file)
@@ -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;
     }
 }
 
index e9dc36f..8147091 100644 (file)
@@ -1,5 +1,10 @@
 2016-11-24  Andreas Arnez  <arnez@linux.vnet.ibm.com>
 
+       * gdb.dwarf2/nonvar-access.exp: Add a test for accessing
+       non-byte-aligned bit fields.
+
+2016-11-24  Andreas Arnez  <arnez@linux.vnet.ibm.com>
+
        PR gdb/12616
        * gdb.dwarf2/nonvar-access.exp: New testcase.  Check that GDB
        respects the DW_AT_data_bit_offset attribute.
index 21532a6..8910f6d 100644 (file)
@@ -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>"