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

Pedro Alves
2016-11-22T23:18:21Z

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