From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29791 invoked by alias); 10 Dec 2013 11:32:54 -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 29780 invoked by uid 89); 10 Dec 2013 11:32:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Dec 2013 11:32:53 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBABWji2014277 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 10 Dec 2013 06:32:45 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rBABWipU003926; Tue, 10 Dec 2013 06:32:44 -0500 Message-ID: <52A6FBDB.5040206@redhat.com> Date: Tue, 10 Dec 2013 11:32:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Andrew Burgess CC: gdb-patches@sourceware.org Subject: Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based. References: <529F489F.7070805@broadcom.com> <529F498F.7060909@broadcom.com> <52A0A760.8000509@redhat.com> <52A63067.8050702@broadcom.com> In-Reply-To: <52A63067.8050702@broadcom.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-12/txt/msg00367.txt.bz2 On 12/09/2013 09:04 PM, Andrew Burgess wrote: > On 05/12/2013 4:18 PM, Pedro Alves wrote: >> On 12/04/2013 03:26 PM, Andrew Burgess wrote: >> >>> >>> /* Compare the _available_ contents. */ >>> - if (memcmp (val1->contents + offset1, >>> - val2->contents + offset2, >>> - l1) != 0) >>> + if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT), >>> + val2->contents + (offset2 / TARGET_CHAR_BIT), >>> + (l1 / TARGET_CHAR_BIT)) != 0) >>> return 0; >> >> As memcmp compares bytes, isn't this potentially comparing bits >> at the beginning and end of the values' buffers, when it >> should not? That is, it looks like the >> 'offset1 % TARGET_CHAR_BIT != 0' and >> '(offset1 + l1) % TARGET_CHAR_BIT' cases should be considered here? >> > > Thanks for the review. You're right, this isn't doing the correct thing > here, I should have written something like this: > > > + if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT), > + val2->contents + (offset2 / TARGET_CHAR_BIT), > + ((l1 + TARGET_CHAR_BIT - 1)/ TARGET_CHAR_BIT)) != 0) > > That is round the start down to the nearest byte boundary, and round the > length up to the nearest byte boundary. > > I figured that as the value content buffer is always a round number of > bytes then there will always be memory backing the access, and as the > content buffer is zero initialized comparing the "unavailable" bits will > not cause the compare to fail. Yeah, given value_available_contents_eq's interface works with bytes, not bits, we know we'll always be comparing up to byte boundaries, so even though this iteration may compare more bits than the current iterated range covers, it's fine, we'd compare them the next iteration anyway. Something like: bit offset: 0 1 2 3 4 5 6 7 val1's contents: [ 1 U U U 2 2 2 2 ] val2's contents: [ 1 U U U 5 5 5 5 ] bits to compare: [ X X X X X X X X ] The first time, we'll memcmp bits bits 0-7 and return false, even though the first available range (bit 0), compares equal. This deserves a comment though. > Alternatively, I can update to only access the bits we'd like to compare. Hmm. That'd be necessary if value_available_contents_eq accepted bit offsets instead of byte offsets. E.g,. consider two values with these contents: bit offset: 0 1 2 3 4 5 6 7 val1's contents: [ 1 U U U 5 5 5 5 ] val2's contents: [ 0 U U U 5 5 5 5 ] bits to compare: [ X X X X X ] We'd have to make sure to not compare (or mask off before comparing) the bit at offset 0. I wonder if it wouldn't make it for more readable code (and less fragile, e.g., what if we want to stop zero initializing the contents buffer) if we handled this. That is, rename value_available_contents_eq to something like value_available_contents_bits_eq (and make it static), have it accept bit offsets, and take that case into consideration. Then, make value_available_contents_eq a wrapper that multiplies offsets/lengths by TARGET_CHAR_BIT. if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset, this_size, buffer, &optim, &unavail)) { /* Just so garbage doesn't ever shine through. */ memset (buffer, 0, this_size); > Which approach would would be best? I'm fine either way, but if we go the former route, then please add a comment mentioning the assumptions. > > Thanks for your advice, > Andrew > -- Pedro Alves