diff mbox

[2/3] Fix copy_bitwise()

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

Commit Message

Andreas Arnez Nov. 17, 2016, 7:36 p.m. UTC
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(-)

Comments

Pedro Alves Nov. 17, 2016, 8:30 p.m. UTC | #1
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
diff mbox

Patch

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
     }