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

[Pedro Alves <palves@redhat.com>]

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