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(-)
@@ -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
}
@@ -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
}