From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68924 invoked by alias); 22 Nov 2016 23:18:21 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 68914 invoked by uid 89); 22 Nov 2016 23:18:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=spell, LEN, *source, normalize X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 22 Nov 2016 23:18:18 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2C0E781255; Tue, 22 Nov 2016 23:18:17 +0000 (UTC) Received: from [127.0.0.1] (ovpn03.gateway.prod.ext.phx2.redhat.com [10.5.9.3]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAMNIFUx023935; Tue, 22 Nov 2016 18:18:16 -0500 Subject: Re: [PATCH 2/3] Fix copy_bitwise() To: Andreas Arnez References: <1479135786-31150-1-git-send-email-arnez@linux.vnet.ibm.com> <1479135786-31150-3-git-send-email-arnez@linux.vnet.ibm.com> <6032efa2-828d-7423-4720-6925a9b4ea4b@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <22c53936-c8c5-fc1e-f50b-d208ef21587f@redhat.com> Date: Tue, 22 Nov 2016 23:18:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-11/txt/msg00654.txt.bz2 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 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