diff mbox

[2/3] Fix copy_bitwise()

Message ID m3k2c0vnec.fsf@oc1027705133.ibm.com
State New
Headers show

Commit Message

Andreas Arnez Nov. 18, 2016, 3:05 p.m. UTC 
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 mbox

Patch

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
     }