From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15318 invoked by alias); 20 May 2010 21:01:48 -0000 Received: (qmail 14897 invoked by uid 22791); 20 May 2010 21:01:46 -0000 X-SWARE-Spam-Status: No, hits=-5.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 20 May 2010 21:01:41 +0000 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o4KL1dLA013189 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 20 May 2010 17:01:39 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx04.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o4KL1bpe022683 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 20 May 2010 17:01:39 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o4KL1a0Q007975; Thu, 20 May 2010 23:01:36 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o4KL1aLt007974; Thu, 20 May 2010 23:01:36 +0200 Date: Thu, 20 May 2010 21:07:00 -0000 From: Jan Kratochvil To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: RFC: implement DW_OP_bit_piece Message-ID: <20100520210135.GA31238@host0.dyn.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-08-17) X-IsSubscribed: yes 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 X-SW-Source: 2010-05/txt/msg00446.txt.bz2 On Wed, 19 May 2010 01:27:33 +0200, Tom Tromey wrote: > We do take care to optimize for the byte-aligned case. FYI I am strongly against this decision. This creates (a) more complicated code to maintain (b) less used and more dense to debug codepath (the bits one) and (c) I doubt current hardware and current `gcc -O2' code will ever notice a difference - GDB has much more serious performance issues (symbols reading, lookup) than evaluation of any specific values. > --- a/gdb/dwarf2expr.c > +++ b/gdb/dwarf2expr.c > +/* The lowest-level function to extract bits from a byte buffer. > + SOURCE is the buffer. It is updated if we read to the end of a > + byte. > + SOURCE_OFFSET_BITS is the offset of the first bit to read. It is > + updated to reflect the number of bits actually read. > + NBITS is the number of bits we want to read. This function may > + read fewer bits. > + BITS_BIG_ENDIAN is taken directly from gdbarch. > + RESULT is set to the extracted bits. > + The function returns the number of bits actually read. */ > + > +static int > +extract_bits_primitive (const gdb_byte **source, > + unsigned int *source_offset_bits, > + int nbits, int bits_big_endian, > + unsigned int *result) IMHO either NBITS should be also updated by the number of bits read or SOURCE and SOURCE_OFFSET_BITS should be updated togetierh with NBITS by the caller. This is a bit mix of both styles. But this function is not used much so it does not matter much. > +{ > + unsigned int avail, mask, datum; > + > + gdb_assert (*source_offset_bits < 8); > + > + avail = 8 - *source_offset_bits; > + if (avail > nbits) > + avail = nbits; NBITS is no longer used anywhere, AVAIL is used instead. Therefore this function ignores the caller's wish of NBITS. I would see here more: if (avail < nbits) nbits = avail; And use NBITS instead of AVAIL everywhere later... > + mask = (1 << avail) - 1; > + > + datum = **source; > + if (bits_big_endian) > + datum >>= 8 - *source_offset_bits; Assuming NBITS contains here the caller's wished read width (possible lowered by the remaining bits available in current byte): It should be rather: datum >>= 8 - (*source_offset_bits + nbits); source_offset_bits = 2 nbits = 3 mask = 7 b0 b1 b2 b3 b4 b5 b6 b7 = big endian bits numbering 128 64 32 16 8 4 2 1 = value represented by this bit drop drop WANT WANT WANT drop drop drop Your expression datum >>= 6; datum &= 7; ?? b0 b1 ?? 128 64 zero drop drop Suggested expression datum >>= 3; datum &= 7; b2 b3 b4 32 16 8 WANT WANT WANT > + else > + datum >>= *source_offset_bits; > + datum &= mask; > + nbits -= avail; NBITS is updated but its value is then never used anywhere later. > + *source_offset_bits += avail; > + if (*source_offset_bits >= 8) > + { > + *source_offset_bits -= 8; > + ++*source; > + } > + > + *result = datum; > + return avail; > +} > + > +/* Extract some bits from a source buffer and move forward in the > + buffer. > + > + SOURCE is the source buffer. It is updated as bytes are read. > + SOURCE_OFFSET_BITS is the offset into SOURCE. It is updated as > + bits are read. > + NBITS is the number of bits to read. > + BITS_BIG_ENDIAN is taken directly from gdbarch. > + > + This function returns the bits that were read. */ > + > +static unsigned int > +extract_bits (const gdb_byte **source, unsigned int *source_offset_bits, > + int nbits, int bits_big_endian) > +{ > + unsigned int datum, bits_read; > + > + bits_read = extract_bits_primitive (source, source_offset_bits, nbits, > + bits_big_endian, &datum); > + if (bits_read < nbits) > + { > + unsigned int more; Missing declarations empty line separator. > + nbits -= bits_read; > + if (bits_big_endian) > + datum <<= nbits; > + extract_bits_primitive (source, source_offset_bits, nbits, > + bits_big_endian, &more); > + datum |= more; For (bits_big_endian == 0) MORE needs to be <<= NBITS. extract_bits_primitive() always returns RESULT fit in the lowest-order bits. Two |= without any << have to mangle both RESULTs each over the other. > + } > + > + return datum; > +} If this function is passed NBITS >= 17 it does not assert-fail and it returns only incomplete 9..16 bits of it. Should there be some while (bits_read < nbits) (and associated adjustments) instead? And this function needs to distinguish both the bits endianity and the bytes endianity. Out of big-byte-endian machines there are both big-bits-endian and little-bits-endian ones: http://en.wikipedia.org/wiki/Bit_numbering#Usage OTOH there is currently no GDB target using set_gdbarch_bits_big_endian, therefore all the GDB targets are little-bits-endian. This seems to be wrong. http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.pdf does not contain any Figures so I rather used: http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.7.pdf where on Figure 3-11 it seems that: ppc64 big-byte-endian is big-bits-endian ppc64 little-byte-endian is also big-bits-endian - this seems weird to me. It is the same for ppc32: http://refspecs.freestandards.org/elf/elfspec_ppc.pdf Figure 3-15 and Figure 3-16 > + > +/* Write some bits into a buffer and move forward in the buffer. > + > + DATUM is the bits to write. The low-order bits of DATUM are used. > + DEST is the destination buffer. It is updated as bytes are > + written. > + DEST_OFFSET_BITS is the bit offset in DEST at which writing is > + done. > + NBITS is the number of valid bits in DATUM. > + BITS_BIG_ENDIAN is taken directly from gdbarch. */ > + > +static void > +insert_bits (unsigned int datum, > + gdb_byte *dest, unsigned int dest_offset_bits, > + int nbits, int bits_big_endian) > +{ > + unsigned int mask; > + > + gdb_assert (dest_offset_bits > 0 && dest_offset_bits < 8); I would rather see for the ">" part: gdb_assert (dest_offset_bits >= 0); as for dest_offset_bits = 0, source_offset_bits != 0 satisfying > + if (dest_offset_bits % 8 != 0 || source_offset_bits % 8 != 0) it can IMO happen, and in general this function can support it. Also if the bits and bytes variants of code would be merged we would need it. and for the "<" part: gdb_assert (dest_offset_bits + nbits <= 8); as this function handles only write to single byte. > + > + mask = (1 << nbits) - 1; > + if (bits_big_endian) > + { > + datum <<= 8 - dest_offset_bits; > + mask <<= 8 - dest_offset_bits; Again I would rather see: datum <<= 8 - (dest_offset_bits + nbits); mask <<= 8 - (dest_offset_bits + nbits); For: bits_big_endian = 1 dest_offset_bits = 1 nbits = 3 mask = 7 b0 b1 b2 b3 b4 b5 b6 b7 = big endian bits numbering 128 64 32 16 8 4 2 1 = value represented by this bit 0 0 0 0 0 1 1 1 former expression: mask <<= 7 mask = 128 1 0 0 0 0 0 0 0 suggested expression: mask <= 4 0 1 1 1 0 0 0 0 > + } > + else > + { > + datum <<= dest_offset_bits; > + mask <<= dest_offset_bits; > + } > + > + gdb_assert ((datum & ~mask) == 0); > + > + *dest = (*dest & ~mask) | datum; > +} > +static void > +copy_bitwise (gdb_byte *dest, unsigned int dest_offset_bits, > + const gdb_byte *source, unsigned int source_offset_bits, > + unsigned int bit_count, > + int bits_big_endian) Seems OK to me. > read_pieced_value (struct value *v) + > @@ -402,11 +627,16 @@ write_pieced_value (struct value *to, struct value *from) Not reviewed, the patches no longer apply to HEAD and also this part may or may not be changed a lot depending on the discussions above. Thanks, Jan