[PATCH 0/3] Support DW_AT_data_bit_offset

[PATCH 0/3] Support DW_AT_data_bit_offset

[Andreas Arnez]

This small series adds support for DW_AT_data_bit_offset, adds a test case
for it, and fixes a bug that limits proper testing of this feature.

Andreas Arnez (3):
  Fix PR12616 - gdb does not implement DW_AT_data_bit_offset
  Fix copy_bitwise()
  Optimize byte-aligned copies in copy_bitwise()

 gdb/dwarf2loc.c                            | 219 ++++++++++-------------------
 gdb/dwarf2read.c                           |   4 +
 gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 134 ++++++++++++++++++
 3 files changed, 209 insertions(+), 148 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/nonvar-access.exp

-- 
2.3.0

[PATCH 2/3] Fix copy_bitwise()

[Andreas Arnez]

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

[PATCH 1/3] Fix PR12616 - gdb does not implement DW_AT_data_bit_offset

[Andreas Arnez]

The DW_AT_data_bit_offset attribute was introduced by DWARF V4 and
allows specifying the offset of a data member within its containing
entity.  But although the new attribute was intended to replace
DW_AT_bit_offset for this purpose, GDB ignores it, and thus GCC still
emits DW_AT_bit_offset instead.  See also
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71669.

This change fixes GDB's lack of support for DW_AT_data_bit_offset and
adds an appropriate test case.

gdb/ChangeLog:

	PR gdb/12616
	* dwarf2read.c (dwarf2_add_field): Handle the DWARF V4 attribute
	DW_AT_data_bit_offset.

gdb/testsuite/ChangeLog:

	PR gdb/12616
	* gdb.dwarf2/nonvar-access.exp: New testcase.  Check that GDB
	respects the DW_AT_data_bit_offset attribute.
---
 gdb/dwarf2read.c                           |   4 ++
 gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 103 +++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+)
 create mode 100644 gdb/testsuite/gdb.dwarf2/nonvar-access.exp

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 1ad6b00..558159a 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -12584,6 +12584,10 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
 				 - bit_offset - FIELD_BITSIZE (*fp)));
 	    }
 	}
+      attr = dwarf2_attr (die, DW_AT_data_bit_offset, cu);
+      if (attr != NULL)
+	SET_FIELD_BITPOS (*fp, (FIELD_BITPOS (*fp)
+				+ dwarf2_get_attr_constant_value (attr, 0)));
 
       /* Get name of field.  */
       fieldname = dwarf2_name (die, cu);
diff --git a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
new file mode 100644
index 0000000..21532a6
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
@@ -0,0 +1,103 @@
+# Copyright 2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test accessing "non-variable" variables, i.e., variables which are
+# optimized to a constant DWARF location expression and/or
+# partially/fully optimized out.
+
+load_lib dwarf.exp
+
+if {![dwarf2_support]} { return 0 }
+
+standard_testfile main.c nonvar-access-dw.S
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+
+Dwarf::assemble $asm_file {
+    cu {} {
+	compile_unit {
+	    {DW_AT_name main.c}
+	} {
+	    declare_labels int_type_label short_type_label
+	    declare_labels struct_s_label
+
+	    int_type_label: base_type {
+		{name "int"}
+		{encoding @DW_ATE_signed}
+		{byte_size 4 DW_FORM_sdata}
+	    }
+
+	    struct_s_label: structure_type {
+		{name s}
+		{byte_size 4 DW_FORM_sdata}
+	    } {
+		member {
+		    {name a}
+		    {type :$int_type_label}
+		    {data_member_location 0 DW_FORM_udata}
+		    {bit_size 8 DW_FORM_udata}
+		}
+		member {
+		    {name b}
+		    {type :$int_type_label}
+		    {data_bit_offset 8 DW_FORM_udata}
+		    {bit_size 24 DW_FORM_udata}
+		}
+	    }
+
+	    DW_TAG_subprogram {
+		{name main}
+		{DW_AT_external 1 flag}
+		{low_pc [gdb_target_symbol main] DW_FORM_addr}
+		{high_pc [gdb_target_symbol main]+0x10000 DW_FORM_addr}
+	    } {
+		DW_TAG_variable {
+		    {name undef_int}
+		    {type :$int_type_label}
+		}
+		DW_TAG_variable {
+		    {name undef_s}
+		    {type :$struct_s_label}
+		}
+		DW_TAG_variable {
+		    {name def_s}
+		    {type :$struct_s_label}
+		    {location {
+			const1u 0
+			stack_value
+			bit_piece 8 0
+			const1s -1
+			stack_value
+			bit_piece 24 0
+		    } SPECIAL_expr}
+		}
+	    }
+	}
+    }
+}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "print def_s" " = \\{a = 0, b = -1\\}"
+gdb_test "print undef_int" " = <optimized out>"
+gdb_test "print undef_s.a" " = <optimized out>"
-- 
2.3.0

[PATCH 3/3] Optimize byte-aligned copies in copy_bitwise()

[Andreas Arnez]

The function copy_bitwise used for copying DWARF pieces can potentially
be invoked for large chunks of data.  For instance, consider a large
struct one of whose members is currently located in a register.  In this
case copy_bitwise would still copy the data bitwise in a loop, which is
much slower than necessary.

This change uses memcpy for the large part instead, if possible.

gdb/ChangeLog:

	* dwarf2loc.c (copy_bitwise): Use memcpy for the middle part, if
	it is byte-aligned.
---
 gdb/dwarf2loc.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 3a241a8..26f6bd8 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1547,11 +1547,30 @@ copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
     {
       size_t len = nbits / 8;
 
-      while (len--)
+      /* Use a faster method for byte-aligned copies.  */
+      if (avail == 0)
 	{
-	  buf |= *(bits_big_endian ? source-- : source++) << avail;
-	  *(bits_big_endian ? dest-- : dest++) = buf;
-	  buf >>= 8;
+	  if (bits_big_endian)
+	    {
+	      dest -= len;
+	      source -= len;
+	      memcpy (dest + 1, source + 1, len);
+	    }
+	  else
+	    {
+	      memcpy (dest, source, len);
+	      dest += len;
+	      source += len;
+	    }
+	}
+      else
+	{
+	  while (len--)
+	    {
+	      buf |= *(bits_big_endian ? source-- : source++) << avail;
+	      *(bits_big_endian ? dest-- : dest++) = buf;
+	      buf >>= 8;
+	    }
 	}
       nbits %= 8;
     }
-- 
2.3.0

Re: [PATCH 1/3] Fix PR12616 - gdb does not implement DW_AT_data_bit_offset

[Luis Machado]

On 11/14/2016 09:02 AM, Andreas Arnez wrote:
> The DW_AT_data_bit_offset attribute was introduced by DWARF V4 and
> allows specifying the offset of a data member within its containing
> entity.  But although the new attribute was intended to replace
> DW_AT_bit_offset for this purpose, GDB ignores it, and thus GCC still
> emits DW_AT_bit_offset instead.  See also
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71669.
>
> This change fixes GDB's lack of support for DW_AT_data_bit_offset and
> adds an appropriate test case.
>
> gdb/ChangeLog:
>
> 	PR gdb/12616
> 	* dwarf2read.c (dwarf2_add_field): Handle the DWARF V4 attribute
> 	DW_AT_data_bit_offset.
>
> gdb/testsuite/ChangeLog:
>
> 	PR gdb/12616
> 	* gdb.dwarf2/nonvar-access.exp: New testcase.  Check that GDB
> 	respects the DW_AT_data_bit_offset attribute.
> ---
>  gdb/dwarf2read.c                           |   4 ++
>  gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 103 +++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/nonvar-access.exp
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 1ad6b00..558159a 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -12584,6 +12584,10 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
>  				 - bit_offset - FIELD_BITSIZE (*fp)));
>  	    }
>  	}
> +      attr = dwarf2_attr (die, DW_AT_data_bit_offset, cu);
> +      if (attr != NULL)
> +	SET_FIELD_BITPOS (*fp, (FIELD_BITPOS (*fp)
> +				+ dwarf2_get_attr_constant_value (attr, 0)));
>
>        /* Get name of field.  */
>        fieldname = dwarf2_name (die, cu);
> diff --git a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
> new file mode 100644
> index 0000000..21532a6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
> @@ -0,0 +1,103 @@
> +# Copyright 2016 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test accessing "non-variable" variables, i.e., variables which are
> +# optimized to a constant DWARF location expression and/or
> +# partially/fully optimized out.
> +
> +load_lib dwarf.exp
> +
> +if {![dwarf2_support]} { return 0 }
> +
> +standard_testfile main.c nonvar-access-dw.S
> +
> +# Make some DWARF for the test.
> +set asm_file [standard_output_file $srcfile2]
> +
> +Dwarf::assemble $asm_file {
> +    cu {} {
> +	compile_unit {
> +	    {DW_AT_name main.c}
> +	} {
> +	    declare_labels int_type_label short_type_label
> +	    declare_labels struct_s_label
> +
> +	    int_type_label: base_type {
> +		{name "int"}
> +		{encoding @DW_ATE_signed}
> +		{byte_size 4 DW_FORM_sdata}
> +	    }
> +
> +	    struct_s_label: structure_type {
> +		{name s}
> +		{byte_size 4 DW_FORM_sdata}
> +	    } {
> +		member {
> +		    {name a}
> +		    {type :$int_type_label}
> +		    {data_member_location 0 DW_FORM_udata}
> +		    {bit_size 8 DW_FORM_udata}
> +		}
> +		member {
> +		    {name b}
> +		    {type :$int_type_label}
> +		    {data_bit_offset 8 DW_FORM_udata}
> +		    {bit_size 24 DW_FORM_udata}
> +		}
> +	    }
> +
> +	    DW_TAG_subprogram {
> +		{name main}
> +		{DW_AT_external 1 flag}
> +		{low_pc [gdb_target_symbol main] DW_FORM_addr}
> +		{high_pc [gdb_target_symbol main]+0x10000 DW_FORM_addr}
> +	    } {
> +		DW_TAG_variable {
> +		    {name undef_int}
> +		    {type :$int_type_label}
> +		}
> +		DW_TAG_variable {
> +		    {name undef_s}
> +		    {type :$struct_s_label}
> +		}
> +		DW_TAG_variable {
> +		    {name def_s}
> +		    {type :$struct_s_label}
> +		    {location {
> +			const1u 0
> +			stack_value
> +			bit_piece 8 0
> +			const1s -1
> +			stack_value
> +			bit_piece 24 0
> +		    } SPECIAL_expr}
> +		}
> +	    }
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} \
> +	  [list $srcfile $asm_file] {nodebug}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +gdb_test "print def_s" " = \\{a = 0, b = -1\\}"
> +gdb_test "print undef_int" " = <optimized out>"
> +gdb_test "print undef_s.a" " = <optimized out>"
>

Looks good to me.

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

[Luis Machado]

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.

Re: [PATCH 3/3] Optimize byte-aligned copies in copy_bitwise()

[Luis Machado]

On 11/14/2016 09:02 AM, Andreas Arnez wrote:
> The function copy_bitwise used for copying DWARF pieces can potentially
> be invoked for large chunks of data.  For instance, consider a large
> struct one of whose members is currently located in a register.  In this
> case copy_bitwise would still copy the data bitwise in a loop, which is
> much slower than necessary.
>
> This change uses memcpy for the large part instead, if possible.
>
> gdb/ChangeLog:
>
> 	* dwarf2loc.c (copy_bitwise): Use memcpy for the middle part, if
> 	it is byte-aligned.
> ---
>  gdb/dwarf2loc.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 3a241a8..26f6bd8 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1547,11 +1547,30 @@ copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
>      {
>        size_t len = nbits / 8;
>
> -      while (len--)
> +      /* Use a faster method for byte-aligned copies.  */
> +      if (avail == 0)
>  	{
> -	  buf |= *(bits_big_endian ? source-- : source++) << avail;
> -	  *(bits_big_endian ? dest-- : dest++) = buf;
> -	  buf >>= 8;
> +	  if (bits_big_endian)
> +	    {
> +	      dest -= len;
> +	      source -= len;
> +	      memcpy (dest + 1, source + 1, len);
> +	    }
> +	  else
> +	    {
> +	      memcpy (dest, source, len);
> +	      dest += len;
> +	      source += len;
> +	    }
> +	}
> +      else
> +	{
> +	  while (len--)
> +	    {
> +	      buf |= *(bits_big_endian ? source-- : source++) << avail;

Same as patch 2/3 about the construct.

> +	      *(bits_big_endian ? dest-- : dest++) = buf;
> +	      buf >>= 8;
> +	    }
>  	}
>        nbits %= 8;
>      }
>

Otherwise looks sane to me.

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

[Andreas Arnez]

On Mon, Nov 14 2016, Luis Machado wrote:

> On 11/14/2016 09:02 AM, Andreas Arnez wrote:

[...]

>> +      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.

No, copy_bitwise should work fine with source_offset and dest_offset set
to zero.  Where do you think it would crash?

[...]

>> +  /* 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.

Well, I've tried a few different ways and found this approach actually
the easiest to read, for my taste.  For instance, it makes the multiple
occurrences easy to recognize -- as you pointed out ;-)

Of course, if people feel that this post-decrement/increment pattern
really hurts readability, I can provide a more "stretched" form instead.

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

I wouldn't consider that necessary, but I'm open for other opinions.

[...]

> Otherwise it looks good to me.

Thanks!

--
Andreas

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

[Luis Machado]

On 11/14/2016 11:54 AM, Andreas Arnez wrote:
> On Mon, Nov 14 2016, Luis Machado wrote:
>
>> On 11/14/2016 09:02 AM, Andreas Arnez wrote:
>
> [...]
>
>>> +      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.
>
> No, copy_bitwise should work fine with source_offset and dest_offset set
> to zero.  Where do you think it would crash?
>

Well, obviously i wasn't fully awake. Nevermind this.

> [...]
>
>>> +  /* 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.
>
> Well, I've tried a few different ways and found this approach actually
> the easiest to read, for my taste.  For instance, it makes the multiple
> occurrences easy to recognize -- as you pointed out ;-)
>
> Of course, if people feel that this post-decrement/increment pattern
> really hurts readability, I can provide a more "stretched" form instead.
>

No strong opinions there. Just a suggestion.

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

[Andreas Arnez]

[-- Attachment #1: Type: text/plain, Size: 288 bytes --]

On Mon, Nov 14 2016, Andreas Arnez wrote:

[...]

> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 93aec1f..3a241a8 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c

[...]

For reference, since the patch itself might be a bit clumsy to read,
here's the resulting function:


[-- Attachment #2: copy_bitwise.c --]
[-- Type: text/plain, Size: 1870 bytes --]

/* 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
copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
	      const gdb_byte *source, ULONGEST source_offset,
	      ULONGEST nbits, int bits_big_endian)
{
  unsigned int buf, avail;

  if (nbits == 0)
    return;

  if (bits_big_endian)
    {
      /* 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
    {
      dest += dest_offset / 8;
      dest_offset %= 8;
      source += source_offset / 8;
      source_offset %= 8;
    }

  /* 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);

  /* NBITS: bits yet to be written; AVAIL: BUF's fill level.  */
  nbits += dest_offset;
  avail = dest_offset + 8 - source_offset;

  /* Flush 8 bits from BUF, if appropriate.  */
  if (nbits >= 8 && avail >= 8)
    {
      *(bits_big_endian ? dest-- : dest++) = buf;
      buf >>= 8;
      avail -= 8;
      nbits -= 8;
    }

  /* Copy the middle part.  */
  if (nbits >= 8)
    {
      size_t len = nbits / 8;

      while (len--)
	{
	  buf |= *(bits_big_endian ? source-- : source++) << avail;
	  *(bits_big_endian ? dest-- : dest++) = buf;
	  buf >>= 8;
	}
      nbits %= 8;
    }

  /* Write the last byte.  */
  if (nbits)
    {
      if (avail < nbits)
	buf |= *source << avail;

      buf &= (1 << nbits) - 1;
      *dest = (*dest & (~0 << nbits)) | buf;
    }
}

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

[Pedro Alves]

On 11/14/2016 03:02 PM, Andreas Arnez wrote:
> 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.

Looks like the sort of function that should be possible to
cover all sorts of inputs/outputs with unit tests.  Aligned, misaligned,
big/little endian, etc., that sort of thing.  That'd help
a lot with ensuring rewrites behave as intended.  Would you feel like
including some?

The unit testing framework is trivial to use, since it's baked inside
gdb.  All you need to do is write a function that throws some corner case
inputs at copy_bitwise, checking expected outputs and calling SELF_CHECK.
You register that function in the selftests frameworks with "register_self_test".
Then to trigger the tests, just do: gdb -ex "maintenance selftest".
testsuite/gdb.gdb/unittest.exp does that too.

Thanks,
Pedro Alves

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

[Andreas Arnez]

On Tue, Nov 15 2016, Pedro Alves wrote:

> Looks like the sort of function that should be possible to
> cover all sorts of inputs/outputs with unit tests.  Aligned, misaligned,
> big/little endian, etc., that sort of thing.  That'd help
> a lot with ensuring rewrites behave as intended.  Would you feel like
> including some?

Sure.  See the patch below, which I'd insert after the one that fixes
copy_bitwise.

With that added, is the series OK to apply?

--
Andreas

-- >8 --
Subject: [PATCH] Add unit test for copy_bitwise

This adds a unit test for the copy_bitwise function in dwarf2loc.c.
With the old (broken) version of copy_bitwise this test would generate
the following failure message:

(gdb) maintenance selftest
Self test failed: copy_bitwise 1101110111010110 != 1101110110010110 (0+9 -> 1)
Ran 3 unit tests, 1 failed

gdb/ChangeLog:

	* dwarf2loc.c (bits_to_str, check_copy_bitwise)
	(copy_bitwise_tests): New functions.
	(_initialize_dwarf2loc): Register the new function
	copy_bitwise_tests as a unit test.
	* selftest.c (run_self_tests): Improve the failure message's
	wording and formatting.
---
 gdb/dwarf2loc.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/selftest.c  |  3 +-
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index b238133..b0cab4b 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -38,6 +38,7 @@
 #include "dwarf2loc.h"
 #include "dwarf2-frame.h"
 #include "compile/compile.h"
+#include "selftest.h"
 #include <algorithm>
 #include <vector>
 
@@ -1567,6 +1568,86 @@ copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
     }
 }
 
+#if GDB_SELF_TEST
+
+/* Helper function for the unit test of copy_bitwise.  Convert NBITS bits
+   out of BITS, starting at OFFS, to the respective '0'/'1'-string.  MSB0
+   specifies whether to assume big endian bit numbering.  Store the
+   resulting (not null-terminated) string at STR.  */
+
+static void
+bits_to_str (char *str, const gdb_byte *bits, ULONGEST offs,
+	     ULONGEST nbits, int msb0)
+{
+  unsigned int j;
+  size_t i;
+
+  for (i = offs / 8, j = offs % 8; nbits; i++, j = 0)
+    {
+      unsigned int ch = bits[i];
+      for (; j < 8 && nbits; j++, nbits--)
+	*str++ = (ch & (msb0 ? (1 << (7 - j)) : (1 << j))) ? '1' : '0';
+    }
+}
+
+/* Check one invocation of copy_bitwise with the given parameters.  */
+
+static void
+check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset,
+		    const gdb_byte *source, unsigned int source_offset,
+		    unsigned int nbits, int msb0)
+{
+  size_t len = (dest_offset + nbits + 7) / 8 * 8;
+  char *a = (char *) alloca (len + 1);
+  char *b = (char *) alloca (len + 1);
+  gdb_byte *buf = (gdb_byte *) alloca (len / 8);
+
+  bits_to_str (a, dest, 0, len, msb0);
+  bits_to_str (a + dest_offset, source, source_offset, nbits, msb0);
+
+  memcpy (buf, dest, len / 8);
+  copy_bitwise (buf, dest_offset, source, source_offset, nbits, msb0);
+  bits_to_str (b, buf, 0, len, msb0);
+
+  a[len] = b[len] = '\0';
+  if (strcmp (a, b))
+    error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
+	   a, b, source_offset, nbits, dest_offset);
+}
+
+/* Unit test for copy_bitwise.  */
+
+static void
+copy_bitwise_tests (void)
+{
+  static const gdb_byte data1[] =
+    { 0xdd, 0x85, 0x51, 0x93, 0xb3, 0xc0, 0x62, 0xd4,
+      0xa6, 0x4b, 0x0a, 0xde, 0x36, 0x35, 0x1a, 0x66 };
+
+  static const gdb_byte data2[] =
+    { 0x47, 0x6a, 0x65, 0x61, 0xab, 0xed, 0xb6, 0xa9,
+      0xbf, 0x67, 0xb2, 0xeb, 0x86, 0x45, 0xfe, 0x09 };
+
+  enum { max_nbits = 24, max_offs = 8 };
+  unsigned int from_offs, nbits, to_offs;
+  int msb0;
+
+  for (msb0 = 0; msb0 < 2; msb0++)
+    {
+      for (from_offs = 0; from_offs <= max_offs; from_offs++)
+	{
+	  for (nbits = 1; nbits < max_nbits; nbits++)
+	    {
+	      for (to_offs = 0; to_offs <= max_offs; to_offs++)
+		check_copy_bitwise (data2, to_offs, data1, from_offs,
+				    nbits, msb0);
+	    }
+	}
+    }
+}
+
+#endif /* GDB_SELF_TEST */
+
 static void
 read_pieced_value (struct value *v)
 {
@@ -4527,4 +4608,8 @@ _initialize_dwarf2loc (void)
 			     NULL,
 			     show_entry_values_debug,
 			     &setdebuglist, &showdebuglist);
+
+#if GDB_SELF_TEST
+  register_self_test (copy_bitwise_tests);
+#endif
 }
diff --git a/gdb/selftest.c b/gdb/selftest.c
index 110b9e5..f771e57 100644
--- a/gdb/selftest.c
+++ b/gdb/selftest.c
@@ -50,8 +50,7 @@ run_self_tests (void)
       CATCH (ex, RETURN_MASK_ERROR)
 	{
 	  ++failed;
-	  exception_fprintf (gdb_stderr, ex,
-			     _("Self test threw exception"));
+	  exception_fprintf (gdb_stderr, ex, _("Self test failed: "));
 	}
       END_CATCH
     }
-- 
2.3.0

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

[Pedro Alves]

On 11/17/2016 07:36 PM, Andreas Arnez wrote:
> On Tue, Nov 15 2016, Pedro Alves wrote:
> 
>> Looks like the sort of function that should be possible to
>> cover all sorts of inputs/outputs with unit tests.  Aligned, misaligned,
>> big/little endian, etc., that sort of thing.  That'd help
>> a lot with ensuring rewrites behave as intended.  Would you feel like
>> including some?
> 
> Sure.  See the patch below, which I'd insert after the one that fixes
> copy_bitwise.

Thanks!

> With that added, is the series OK to apply?

I'm feeling dense and I can't make sense of the new test.  :-/

Can you add some more comments?  Is there some logic behind the
numbers of the data1/data2 arrays?  Some pattern between them?
E.g., it'd be nice to explain the logic between the steps
check_copy_bitwise is doing.

A couple other minor comments:


> +#if GDB_SELF_TEST
> +

Please add:

namespace selftests {

like utils-selftests.c does.


> 

> +  a[len] = b[len] = '\0';
> +  if (strcmp (a, b))

strcmp (...) != 0

> +    error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
> +	   a, b, source_offset, nbits, dest_offset);
> +}

> +
> +  enum { max_nbits = 24, max_offs = 8 };

IN C++11 we can express a compile-time integer directly with
constexpr, not need for enum hacks any longer:

 constexpr int max_nbits = 24;

Thanks,
Pedro Alves

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

[Andreas Arnez]

On Thu, Nov 17 2016, Pedro Alves wrote:

> I'm feeling dense and I can't make sense of the new test.  :-/
>
> Can you add some more comments?  Is there some logic behind the
> numbers of the data1/data2 arrays?  Some pattern between them?
> E.g., it'd be nice to explain the logic between the steps
> check_copy_bitwise is doing.

OK, commented the array definitions and the steps of check_copy_bitwise.
Also gave the variables a, b, and data1/2 more telling names.

> A couple other minor comments:
>
>
>> +#if GDB_SELF_TEST
>> +
>
> Please add:
>
> namespace selftests {
>
> like utils-selftests.c does.

Done.

>> +  a[len] = b[len] = '\0';
>> +  if (strcmp (a, b))
>
> strcmp (...) != 0

Done.

>> +    error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
>> +	   a, b, source_offset, nbits, dest_offset);
>> +}
>
>> +
>> +  enum { max_nbits = 24, max_offs = 8 };
>
> IN C++11 we can express a compile-time integer directly with
> constexpr, not need for enum hacks any longer:
>
>  constexpr int max_nbits = 24;

Done.

Updated patch below.  Does this help?

--
Andreas

-- >8 --

Subject: [PATCH v2] Add unit test for copy_bitwise

This adds a unit test for the copy_bitwise function in dwarf2loc.c.
With the old (broken) version of copy_bitwise this test would generate
the following failure message:

(gdb) maintenance selftest
Self test failed: copy_bitwise 1101110111010110 != 1101110110010110 (0+9 -> 1)
Ran 3 unit tests, 1 failed

gdb/ChangeLog:

	* dwarf2loc.c (bits_to_str, check_copy_bitwise)
	(copy_bitwise_tests): New functions.
	(_initialize_dwarf2loc): Register the new function
	copy_bitwise_tests as a unit test.
	* selftest.c (run_self_tests): Improve the failure message's
	wording and formatting.
---
 gdb/dwarf2loc.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/selftest.c  |   3 +-
 2 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index b238133..c7e0380 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -38,6 +38,7 @@
 #include "dwarf2loc.h"
 #include "dwarf2-frame.h"
 #include "compile/compile.h"
+#include "selftest.h"
 #include <algorithm>
 #include <vector>
 
@@ -1567,6 +1568,101 @@ copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
     }
 }
 
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+/* Helper function for the unit test of copy_bitwise.  Convert NBITS bits
+   out of BITS, starting at OFFS, to the respective '0'/'1'-string.  MSB0
+   specifies whether to assume big endian bit numbering.  Store the
+   resulting (not null-terminated) string at STR.  */
+
+static void
+bits_to_str (char *str, const gdb_byte *bits, ULONGEST offs,
+	     ULONGEST nbits, int msb0)
+{
+  unsigned int j;
+  size_t i;
+
+  for (i = offs / 8, j = offs % 8; nbits; i++, j = 0)
+    {
+      unsigned int ch = bits[i];
+      for (; j < 8 && nbits; j++, nbits--)
+	*str++ = (ch & (msb0 ? (1 << (7 - j)) : (1 << j))) ? '1' : '0';
+    }
+}
+
+/* Check one invocation of copy_bitwise with the given parameters.  */
+
+static void
+check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset,
+		    const gdb_byte *source, unsigned int source_offset,
+		    unsigned int nbits, int msb0)
+{
+  size_t len = (dest_offset + nbits + 7) / 8 * 8;
+  char *expected = (char *) alloca (len + 1);
+  char *actual = (char *) alloca (len + 1);
+  gdb_byte *buf = (gdb_byte *) alloca (len / 8);
+
+  /* Compose a '0'/'1'-string that represents the expected result of
+     copy_bitwise.  */
+  bits_to_str (expected, dest, 0, len, msb0);
+  bits_to_str (expected + dest_offset, source, source_offset, nbits, msb0);
+
+  /* Fill BUF with data from DEST, apply copy_bitwise, and convert the
+   result to a '0'/'1'-string.  */
+  memcpy (buf, dest, len / 8);
+  copy_bitwise (buf, dest_offset, source, source_offset, nbits, msb0);
+  bits_to_str (actual, buf, 0, len, msb0);
+
+  /* Compare the resulting strings.  */
+  expected[len] = actual[len] = '\0';
+  if (strcmp (expected, actual) != 0)
+    error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
+	   expected, actual, source_offset, nbits, dest_offset);
+}
+
+/* Unit test for copy_bitwise.  */
+
+static void
+copy_bitwise_tests (void)
+{
+  /* Some random data, to be used for source and target buffers.  */
+  static const gdb_byte source_data[] =
+    { 0xdd, 0x85, 0x51, 0x93, 0xb3, 0xc0, 0x62, 0xd4,
+      0xa6, 0x4b, 0x0a, 0xde, 0x36, 0x35, 0x1a, 0x66 };
+
+  static const gdb_byte dest_data[] =
+    { 0x47, 0x6a, 0x65, 0x61, 0xab, 0xed, 0xb6, 0xa9,
+      0xbf, 0x67, 0xb2, 0xeb, 0x86, 0x45, 0xfe, 0x09 };
+
+  constexpr int max_nbits = 24;
+  constexpr int max_offs = 8;
+  unsigned int from_offs, nbits, to_offs;
+  int msb0;
+
+  for (msb0 = 0; msb0 < 2; msb0++)
+    {
+      for (from_offs = 0; from_offs <= max_offs; from_offs++)
+	{
+	  for (nbits = 1; nbits < max_nbits; nbits++)
+	    {
+	      for (to_offs = 0; to_offs <= max_offs; to_offs++)
+		check_copy_bitwise (dest_data, to_offs,
+				    source_data, from_offs, nbits, msb0);
+	    }
+	}
+      /* Special cases: copy all, copy nothing.  */
+      check_copy_bitwise (dest_data, 0, source_data, 0,
+			  sizeof (source_data) * 8, msb0);
+      check_copy_bitwise (dest_data, 7, source_data, 9, 0, msb0);
+    }
+}
+
+} /* namespace selftests */
+
+#endif /* GDB_SELF_TEST */
+
 static void
 read_pieced_value (struct value *v)
 {
@@ -4527,4 +4623,8 @@ _initialize_dwarf2loc (void)
 			     NULL,
 			     show_entry_values_debug,
 			     &setdebuglist, &showdebuglist);
+
+#if GDB_SELF_TEST
+  register_self_test (selftests::copy_bitwise_tests);
+#endif
 }
diff --git a/gdb/selftest.c b/gdb/selftest.c
index 110b9e5..f771e57 100644
--- a/gdb/selftest.c
+++ b/gdb/selftest.c
@@ -50,8 +50,7 @@ run_self_tests (void)
       CATCH (ex, RETURN_MASK_ERROR)
 	{
 	  ++failed;
-	  exception_fprintf (gdb_stderr, ex,
-			     _("Self test threw exception"));
+	  exception_fprintf (gdb_stderr, ex, _("Self test failed: "));
 	}
       END_CATCH
     }
-- 
2.3.0

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

[Pedro Alves]

On 11/18/2016 03:05 PM, Andreas Arnez wrote:

> With that added, is the series OK to apply?

I read the whole series now, and it looks OK to me.

More below on the selftest though.

> On Thu, Nov 17 2016, Pedro Alves wrote:
> 
>> I'm feeling dense and I can't make sense of the new test.  :-/
>>
>> Can you add some more comments?  Is there some logic behind the
>> numbers of the data1/data2 arrays?  Some pattern between them?
>> E.g., it'd be nice to explain the logic between the steps
>> check_copy_bitwise is doing.
> 
> OK, commented the array definitions and the steps of check_copy_bitwise.
> Also gave the variables a, b, and data1/2 more telling names.

> Updated patch below.  Does this help?

Sorry for the delay on this.  It still wasn't immediately/obviously clear
to me what the test was doing.  Particularly, how is check_copy_bitwise
confirming the bits are being copied correctly.  I just needed to find
a bit of time to step through the code and figure it out.

So now I have a suggested patch, either on top of yours, or to
merge with yours, whatever you prefer.

> +static void
> +copy_bitwise_tests (void)
> +{
> +  /* Some random data, to be used for source and target buffers.  */
> +  static const gdb_byte source_data[] =
> +    { 0xdd, 0x85, 0x51, 0x93, 0xb3, 0xc0, 0x62, 0xd4,
> +      0xa6, 0x4b, 0x0a, 0xde, 0x36, 0x35, 0x1a, 0x66 };
> +
> +  static const gdb_byte dest_data[] =
> +    { 0x47, 0x6a, 0x65, 0x61, 0xab, 0xed, 0xb6, 0xa9,
> +      0xbf, 0x67, 0xb2, 0xeb, 0x86, 0x45, 0xfe, 0x09 };

I think that instead writing a all-zeros source on top of an all-zeros
destination and the converse would be clearer IMO.  Also, to cover
alternating bits we can include all 0xaa's and all 0x55's patterns.
I think that gives as much, if not more coverage while being more
"deterministic".

> +
> +  constexpr int max_nbits = 24;
> +  constexpr int max_offs = 8;
> +  unsigned int from_offs, nbits, to_offs;
> +  int msb0;
> +
> +  for (msb0 = 0; msb0 < 2; msb0++)
> +    {
> +      for (from_offs = 0; from_offs <= max_offs; from_offs++)

I think it's beneficial for readability to spell out "offset",
and to normalize from -> source, to -> dest, to match the other
variables and the parameter names of the callees.

And finally, getting back to check_copy_bitwise, I think
a comment like the one added by this patch makes it easier
to understand what's going on.

WDTY?

From e877a60c32aa520d6283c6be02030b22f95bc575 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 22 Nov 2016 20:16:54 +0000
Subject: [PATCH] copy_bitwise

---
 gdb/dwarf2loc.c | 96 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 69 insertions(+), 27 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index c7e0380..6eb7387 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1599,18 +1599,33 @@ check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset,
 		    const gdb_byte *source, unsigned int source_offset,
 		    unsigned int nbits, int msb0)
 {
-  size_t len = (dest_offset + nbits + 7) / 8 * 8;
+  size_t len = align_up (dest_offset + nbits, 8);
   char *expected = (char *) alloca (len + 1);
   char *actual = (char *) alloca (len + 1);
   gdb_byte *buf = (gdb_byte *) alloca (len / 8);
 
   /* Compose a '0'/'1'-string that represents the expected result of
-     copy_bitwise.  */
+     copy_bitwise below:
+      Bits from [0, DEST_OFFSET) are filled from DEST.
+      Bits from [DEST_OFFSET, DEST_OFFSET + NBITS) are filled from SOURCE.
+      Bits from [DEST_OFFSET + NBITS, LEN) are filled from DEST.
+
+     E.g., with:
+      dest_offset: 4
+      nbits:       2
+      len:         8
+      dest:        00000000
+      source:      11111111
+
+     We should end up with:
+      buf:         00001100
+                   DDDDSSDD (D=dest, S=source)
+  */
   bits_to_str (expected, dest, 0, len, msb0);
   bits_to_str (expected + dest_offset, source, source_offset, nbits, msb0);
 
   /* Fill BUF with data from DEST, apply copy_bitwise, and convert the
-   result to a '0'/'1'-string.  */
+     result to a '0'/'1'-string.  */
   memcpy (buf, dest, len / 8);
   copy_bitwise (buf, dest_offset, source, source_offset, nbits, msb0);
   bits_to_str (actual, buf, 0, len, msb0);
@@ -1627,35 +1642,62 @@ check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset,
 static void
 copy_bitwise_tests (void)
 {
-  /* Some random data, to be used for source and target buffers.  */
-  static const gdb_byte source_data[] =
-    { 0xdd, 0x85, 0x51, 0x93, 0xb3, 0xc0, 0x62, 0xd4,
-      0xa6, 0x4b, 0x0a, 0xde, 0x36, 0x35, 0x1a, 0x66 };
-
-  static const gdb_byte dest_data[] =
-    { 0x47, 0x6a, 0x65, 0x61, 0xab, 0xed, 0xb6, 0xa9,
-      0xbf, 0x67, 0xb2, 0xeb, 0x86, 0x45, 0xfe, 0x09 };
-
-  constexpr int max_nbits = 24;
-  constexpr int max_offs = 8;
-  unsigned int from_offs, nbits, to_offs;
-  int msb0;
-
-  for (msb0 = 0; msb0 < 2; msb0++)
+  /* Data to be used as both source and destination buffers.  */
+  static const gdb_byte data[4][16] = {
+    { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+      0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
+    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+    { 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
+      0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa },
+    { 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55,
+      0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55 }
+  };
+
+  constexpr size_t data_size = sizeof (data[0]);
+  constexpr unsigned max_nbits = 24;
+  constexpr unsigned max_offset = 8;
+
+  /* Try all combinations of:
+      writing ones|zeros|aa|55 on top of zeros|ones|aa|55
+       X big|little endian bits
+       X [0, MAX_OFFSET) source offset
+       X [0, MAX_OFFSET) destination offset
+       X [0, MAX_BITS) copy bit width
+  */
+  for (int source_index = 0; source_index < 4; source_index++)
     {
-      for (from_offs = 0; from_offs <= max_offs; from_offs++)
+      const gdb_byte *source_data = data[source_index];
+
+      for (int dest_index = 0; dest_index < 4; dest_index++)
 	{
-	  for (nbits = 1; nbits < max_nbits; nbits++)
+	  const gdb_byte *dest_data = data[dest_index];
+
+	  for (int msb0 = 0; msb0 < 2; msb0++)
 	    {
-	      for (to_offs = 0; to_offs <= max_offs; to_offs++)
-		check_copy_bitwise (dest_data, to_offs,
-				    source_data, from_offs, nbits, msb0);
+	      for (unsigned int source_offset = 0;
+		   source_offset <= max_offset;
+		   source_offset++)
+		{
+		  for (unsigned int nbits = 1; nbits < max_nbits; nbits++)
+		    {
+		      for (unsigned int dest_offset = 0;
+			   dest_offset <= max_offset;
+			   dest_offset++)
+			{
+			  check_copy_bitwise (dest_data, dest_offset,
+					      source_data, source_offset,
+					      nbits, msb0);
+			}
+		    }
+		}
+
+	      /* Special cases: copy all, copy nothing.  */
+	      check_copy_bitwise (dest_data, 0, source_data, 0,
+				  data_size * 8, msb0);
+	      check_copy_bitwise (dest_data, 7, source_data, 9, 0, msb0);
 	    }
 	}
-      /* Special cases: copy all, copy nothing.  */
-      check_copy_bitwise (dest_data, 0, source_data, 0,
-			  sizeof (source_data) * 8, msb0);
-      check_copy_bitwise (dest_data, 7, source_data, 9, 0, msb0);
     }
 }
 
-- 
2.5.5

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

[Andreas Arnez]

On Tue, Nov 22 2016, Pedro Alves wrote:

> I read the whole series now, and it looks OK to me.

Good, thanks for reviewing!  I'll push the series as soon as we've
settled on the self test.

> More below on the selftest though.

[...]

> So now I have a suggested patch, either on top of yours, or to
> merge with yours, whatever you prefer.
>
>> +static void
>> +copy_bitwise_tests (void)
>> +{
>> +  /* Some random data, to be used for source and target buffers.  */
>> +  static const gdb_byte source_data[] =
>> +    { 0xdd, 0x85, 0x51, 0x93, 0xb3, 0xc0, 0x62, 0xd4,
>> +      0xa6, 0x4b, 0x0a, 0xde, 0x36, 0x35, 0x1a, 0x66 };
>> +
>> +  static const gdb_byte dest_data[] =
>> +    { 0x47, 0x6a, 0x65, 0x61, 0xab, 0xed, 0xb6, 0xa9,
>> +      0xbf, 0x67, 0xb2, 0xeb, 0x86, 0x45, 0xfe, 0x09 };
>
> I think that instead writing a all-zeros source on top of an all-zeros
> destination and the converse would be clearer IMO.  Also, to cover
> alternating bits we can include all 0xaa's and all 0x55's patterns.
> I think that gives as much, if not more coverage while being more
> "deterministic".

Hm, I'm not quite convinced.  Fooling the test with a broken algorithm
is probably easier with regular test patterns than with pseudo-random
ones.  For instance, consider an implementation that does not advance
the byte position in the source buffer at all.

I do understand your concern about being "deterministic", though.  Thus
I've taken another shot, see the patch below.

> I think it's beneficial for readability to spell out "offset",
> and to normalize from -> source, to -> dest, to match the other
> variables and the parameter names of the callees.

Yeah, consistent naming is a good idea.

> And finally, getting back to check_copy_bitwise, I think
> a comment like the one added by this patch makes it easier
> to understand what's going on.

Sure.  Maybe I was too much into the material and thus considered it
obvious ;-)

Here's the adjusted patch.

-- >8 --
Subject: [PATCH v3] Add unit test for copy_bitwise

This adds a unit test for the copy_bitwise function in dwarf2loc.c.
With the old (broken) version of copy_bitwise this test would generate
the following failure message:

(gdb) maintenance selftest
Self test failed: copy_bitwise 11000000 != 10000000 (7+2 -> 0)

gdb/ChangeLog:
2016-11-24  Andreas Arnez <arnez@linux.vnet.ibm.com>
	    Pedro Alves <palves@redhat.com>

	* dwarf2loc.c (bits_to_str, check_copy_bitwise)
	(copy_bitwise_tests): New functions.
	(_initialize_dwarf2loc): Register the new function
	copy_bitwise_tests as a unit test.
	* selftest.c (run_self_tests): Improve the failure message's
	wording and formatting.
---
 gdb/dwarf2loc.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/selftest.c  |   3 +-
 2 files changed, 141 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index b238133..f7f13cf 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -38,6 +38,7 @@
 #include "dwarf2loc.h"
 #include "dwarf2-frame.h"
 #include "compile/compile.h"
+#include "selftest.h"
 #include <algorithm>
 #include <vector>
 
@@ -1567,6 +1568,141 @@ copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
     }
 }
 
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+/* Helper function for the unit test of copy_bitwise.  Convert NBITS bits
+   out of BITS, starting at OFFS, to the respective '0'/'1'-string.  MSB0
+   specifies whether to assume big endian bit numbering.  Store the
+   resulting (not null-terminated) string at STR.  */
+
+static void
+bits_to_str (char *str, const gdb_byte *bits, ULONGEST offs,
+	     ULONGEST nbits, int msb0)
+{
+  unsigned int j;
+  size_t i;
+
+  for (i = offs / 8, j = offs % 8; nbits; i++, j = 0)
+    {
+      unsigned int ch = bits[i];
+      for (; j < 8 && nbits; j++, nbits--)
+	*str++ = (ch & (msb0 ? (1 << (7 - j)) : (1 << j))) ? '1' : '0';
+    }
+}
+
+/* Check one invocation of copy_bitwise with the given parameters.  */
+
+static void
+check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset,
+		    const gdb_byte *source, unsigned int source_offset,
+		    unsigned int nbits, int msb0)
+{
+  size_t len = align_up (dest_offset + nbits, 8);
+  char *expected = (char *) alloca (len + 1);
+  char *actual = (char *) alloca (len + 1);
+  gdb_byte *buf = (gdb_byte *) alloca (len / 8);
+
+  /* Compose a '0'/'1'-string that represents the expected result of
+     copy_bitwise below:
+      Bits from [0, DEST_OFFSET) are filled from DEST.
+      Bits from [DEST_OFFSET, DEST_OFFSET + NBITS) are filled from SOURCE.
+      Bits from [DEST_OFFSET + NBITS, LEN) are filled from DEST.
+
+     E.g., with:
+      dest_offset: 4
+      nbits:       2
+      len:         8
+      dest:        00000000
+      source:      11111111
+
+     We should end up with:
+      buf:         00001100
+                   DDDDSSDD (D=dest, S=source)
+  */
+  bits_to_str (expected, dest, 0, len, msb0);
+  bits_to_str (expected + dest_offset, source, source_offset, nbits, msb0);
+
+  /* Fill BUF with data from DEST, apply copy_bitwise, and convert the
+     result to a '0'/'1'-string.  */
+  memcpy (buf, dest, len / 8);
+  copy_bitwise (buf, dest_offset, source, source_offset, nbits, msb0);
+  bits_to_str (actual, buf, 0, len, msb0);
+
+  /* Compare the resulting strings.  */
+  expected[len] = actual[len] = '\0';
+  if (strcmp (expected, actual) != 0)
+    error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
+	   expected, actual, source_offset, nbits, dest_offset);
+}
+
+/* Unit test for copy_bitwise.  */
+
+static void
+copy_bitwise_tests (void)
+{
+  /* Data to be used as both source and destination buffers.  The two
+     arrays below represent the lsb0- and msb0- encoded versions of the
+     following bit string, respectively:
+       00000000 00011111 11111111 01001000 10100101 11110010
+     This pattern is chosen such that it contains:
+     - constant 0- and 1- chunks of more than a full byte;
+     - 0/1- and 1/0 transitions on all bit positions within a byte;
+     - several sufficiently asymmetric bytes.
+  */
+  static const gdb_byte data_lsb0[] = {
+    0x00, 0xf8, 0xff, 0x12, 0xa5, 0x4f
+  };
+  static const gdb_byte data_msb0[] = {
+    0x00, 0x1f, 0xff, 0x48, 0xa5, 0xf2
+  };
+
+  constexpr size_t data_nbits = 8 * sizeof (data_lsb0);
+  constexpr unsigned max_nbits = 24;
+
+  /* Try all combinations of:
+      lsb0/msb0 bit order (using the respective data array)
+       X [0, MAX_NBITS] copy bit width
+       X feasible source offsets for the given copy bit width
+       X feasible destination offsets
+  */
+  for (int msb0 = 0; msb0 < 2; msb0++)
+    {
+      const gdb_byte *data = msb0 ? data_msb0 : data_lsb0;
+
+      for (unsigned int nbits = 1; nbits <= max_nbits; nbits++)
+	{
+	  const unsigned int max_offset = data_nbits - nbits;
+
+	  for (unsigned source_offset = 0;
+	       source_offset <= max_offset;
+	       source_offset++)
+	    {
+	      for (unsigned dest_offset = 0;
+		   dest_offset <= max_offset;
+		   dest_offset++)
+		{
+		  check_copy_bitwise (data + dest_offset / 8,
+				      dest_offset % 8,
+				      data + source_offset / 8,
+				      source_offset % 8,
+				      nbits, msb0);
+		}
+	    }
+	}
+
+      /* Special cases: copy all, copy nothing.  */
+      check_copy_bitwise (data_lsb0, 0, data_msb0, 0, data_nbits, msb0);
+      check_copy_bitwise (data_msb0, 0, data_lsb0, 0, data_nbits, msb0);
+      check_copy_bitwise (data, data_nbits - 7, data, 9, 0, msb0);
+    }
+}
+
+} /* namespace selftests */
+
+#endif /* GDB_SELF_TEST */
+
 static void
 read_pieced_value (struct value *v)
 {
@@ -4527,4 +4663,8 @@ _initialize_dwarf2loc (void)
 			     NULL,
 			     show_entry_values_debug,
 			     &setdebuglist, &showdebuglist);
+
+#if GDB_SELF_TEST
+  register_self_test (selftests::copy_bitwise_tests);
+#endif
 }
diff --git a/gdb/selftest.c b/gdb/selftest.c
index 110b9e5..f771e57 100644
--- a/gdb/selftest.c
+++ b/gdb/selftest.c
@@ -50,8 +50,7 @@ run_self_tests (void)
       CATCH (ex, RETURN_MASK_ERROR)
 	{
 	  ++failed;
-	  exception_fprintf (gdb_stderr, ex,
-			     _("Self test threw exception"));
+	  exception_fprintf (gdb_stderr, ex, _("Self test failed: "));
 	}
       END_CATCH
     }
-- 
2.3.0

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

[Pedro Alves]

On 11/24/2016 04:15 PM, Andreas Arnez wrote:

> Hm, I'm not quite convinced.  Fooling the test with a broken algorithm
> is probably easier with regular test patterns than with pseudo-random
> ones.  For instance, consider an implementation that does not advance
> the byte position in the source buffer at all.

Agreed.

> 
> I do understand your concern about being "deterministic", though.  Thus
> I've taken another shot, see the patch below.

Perfect!

Please push.

Thanks,
Pedro Alves

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

[Andreas Arnez]

On Thu, Nov 24 2016, Pedro Alves wrote:

> Perfect!
>
> Please push.

Thanks, done.

--
Andreas