From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104925 invoked by alias); 12 Dec 2017 18:40:38 -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 104913 invoked by uid 89); 12 Dec 2017 18:40:38 -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=aaaa 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:40:36 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 261EB76520; Tue, 12 Dec 2017 18:40:35 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E082C51C7D; Tue, 12 Dec 2017 18:40:34 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: Simon Marchi , Simon Marchi , GDB Patches , Tom Tromey , Eli Zaretskii Subject: Re: [PATCH v3 2/2] Implement pahole-like 'ptype /o' option 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> Date: Tue, 12 Dec 2017 18:40:00 -0000 In-Reply-To: (Pedro Alves's message of "Tue, 12 Dec 2017 18:14:38 +0000") Message-ID: <87lgi7x00t.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00282.txt.bz2 Thanks for looking into this. On Tuesday, December 12 2017, Pedro Alves wrote: > 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. OK. The GDB patch is printing "1:15" because the offset is calculated as: (bitpos + offset_bitpos) / TARGET_CHAR_BIT In this particular case, offset_bitpos is zero because we're not inside an inner struct, so we don't care about it. bitpos is TYPE_FIELD_BITPOS (type, field_idx), which is 15 in this case, because sizeof (aa.aa) + bitsof (aa.a) = 15. When we divide it by TARGET_CHAR_BIT, we get 1. It seems using TYPE_FIELD_BITPOS is not reliable when dealing with bitfields. So there is something I'm not accounting here. > 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. Yeah. I tried to find documentation on the output, but couldn't. Yesterday I was reading pahole's code. From what I understand, it is the number of bits left in the object. > 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 I guess you meant: sizeof (int) * 8 - bitsof (aa.a) - bitsof(aa.b) Right? But yeah, it makes sense when you consider that the first bitfield got "promoted" from "unsigned char" to "int". I haven't figured out a way to keep track of these type changes in order to calculate the right offsets, remaining space and holes. > 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. Yes. I haven't checked all the offsets too; there may be inconsistencies. >> >> >> 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. OK, I'll keep trying here. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/