From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 123641 invoked by alias); 12 Dec 2017 18:14:58 -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 123624 invoked by uid 89); 12 Dec 2017 18:14:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= 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, 12 Dec 2017 18:14:55 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C98B772D0F; Tue, 12 Dec 2017 18:14:53 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 480BB6C21F; Tue, 12 Dec 2017 18:14:39 +0000 (UTC) Subject: Re: [PATCH v3 2/2] Implement pahole-like 'ptype /o' option To: Sergio Durigan Junior , Simon Marchi References: <20171121160709.23248-1-sergiodj@redhat.com> <20171211195757.18044-1-sergiodj@redhat.com> <20171211195757.18044-3-sergiodj@redhat.com> <2b1ed21e-2d41-19d2-a0cf-3b4780cf9060@ericsson.com> <87lgi8c0g6.fsf@redhat.com> <87indczcw9.fsf@redhat.com> Cc: Simon Marchi , GDB Patches , Tom Tromey , Eli Zaretskii From: Pedro Alves Message-ID: Date: Tue, 12 Dec 2017 18:14: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: <87indczcw9.fsf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00281.txt.bz2 On 12/12/2017 06:19 AM, Sergio Durigan Junior wrote: > BTW, a little status update. > > Apparently the patch can't handle bitfields very well. I've found a few > cases where the bitfield handling gets confused, printing wrong > offsets/sizes/holes. Bitfields can be extremely complex to deal with > when it comes to offsets... > > I spent hours trying to improve the patch, managed to make some > progress, but there are still corner cases to fix. For example, the > patch doesn't deal well with this case: > > struct aa { > /* 0 | 1 */ char aa; > /* 1: 1 | 1 */ unsigned char a : 7; > /* 1:15 | 4 */ int b : 10; > } /* total size: 4 bytes */ > > In this case, the bitfield "b" would be combined with the previous > bitfield "a", like pahole reports: > > struct aa { > char aa; /* 0 1 */ > unsigned char a:7; /* 1: 1 1 */ > > /* Bitfield combined with previous fields */ > > int b:10; /* 0: 7 4 */ > } > > Confusing... I'm not sure why pahole reports b's offset to be 0. 0 seems right to me. The bitfield's type is int, with size 4, and it lives at byte offset 0: <2><53>: Abbrev Number: 5 (DW_TAG_member) <54> DW_AT_name : (indirect string, offset: 0x4ae): b <58> DW_AT_decl_file : 1 <59> DW_AT_decl_line : 7 <5a> DW_AT_type : <0x71> <5e> DW_AT_byte_size : 4 <5f> DW_AT_bit_size : 10 <60> DW_AT_bit_offset : 7 <61> DW_AT_data_member_location: 0 <<< Replacing the 0 with 1, like: int b:10; /* 1: 7 4 */ would look incorrect to me, because that'd make '(char*)&aa + 1 + 4' (address of containing object + byte offset + byte size) overshoot the size of the containing object. What is that number after ":" in bitfields supposed to mean in pahole's output (and I assume that that's what you're trying to emulate)? We're missing documentation for that. It seems like it's supposed to mean the number of bits left in the containing anonymous object (i.e., in the 4 bytes of the declared int)? Then "0:7" seems right, given: sizeof (int) * 8 - bitsof(aa.a) - bitsof(aa.a) - bits(aa.b) => sizeof (int) * 8 - 7 - 10 => 7 It took me a while to get to this conclusion (and writing a lot of text that I ended up deleting... :-P), because I originally assumed that this was meant to be the field's bit offset. > > Also, the patch doesn't understand cases like this: > > struct tyu > { > unsigned int a1 : 1; > unsigned int a2 : 3; > unsigned int a9 : 23; > /* PROBLEM HAPPENS HERE */ > unsigned char a0 : 2; > uint64_t a3; > unsigned int a4 : 5; > uint64_t a5 : 3; > }; > > In this case, we're switching types in the middle of the bitfield. The > bitfield "unsigned char a0" would be combined with the bitfields above > it, but the patch doesn't know that: > > struct tyu { > /* 0:31 | 4 */ unsigned int a1 : 1; > /* 0:28 | 4 */ unsigned int a2 : 3; > /* 0: 5 | 4 */ unsigned int a9 : 23; > /* 3: 6 | 1 */ unsigned char a0 : 2; > /* XXX 3-bit hole */ > /* XXX 4-byte hole */ > /* 8 | 8 */ uint64_t a3; > /* 16:27 | 4 */ unsigned int a4 : 5; > /* 16:56 | 8 */ uint64_t a5 : 3; > } /* total size: 24 bytes */ > > Whereas pahole reports: > > struct tyu { > unsigned int a1:1; /* 0:31 4 */ > unsigned int a2:3; /* 0:28 4 */ > unsigned int a9:23; /* 0: 5 4 */ > > /* XXX 253 bits hole, try to pack */ > /* Bitfield combined with next fields */ > > unsigned char a0:2; /* 3: 3 1 */ > > /* XXX 6 bits hole, try to pack */ > /* XXX 4 bytes hole, try to pack */ > > uint64_t a3; /* 8 8 */ > unsigned int a4:5; /* 16:27 4 */ > uint64_t a5:3; /* 16:56 8 */ > }; > > Hm, TBH pahole itself seems a bit confused here, saying there is a > 253-bit hole... A 253-bit hole does look odd. > > > Anyway, long story short, this is much more complex that I thought it > would be (TM). I am extremely tired right now and can't continue, but I > intend to resume work tomorrow morning. But I'd like to leave a few > options on the table: > > 1) Remove the patch from the 8.1 wishlist, which will unblock the > branching. > > 2) Remove the bitfield handling from the patch, leaving it > feature-incomplete, but working. > > 3) Push the patch with these known limitations, document them, mark them > as KFAIL in the testcase, and open a bug to fix them (I personally > wouldn't like this). > > 4) Wait more time until these issues are resolved. > Joel said that we'd re-evaluate Wednesday, so there's still some time. Thanks, Pedro Alves