From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 44267 invoked by alias); 12 Dec 2017 06:19:39 -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 44232 invoked by uid 89); 12 Dec 2017 06:19: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=PROBLEM 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 06:19:36 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8BADB85A02; Tue, 12 Dec 2017 06:19: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 468C1605FC; Tue, 12 Dec 2017 06:19:35 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: 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> Date: Tue, 12 Dec 2017 06:19:00 -0000 In-Reply-To: (Simon Marchi's message of "Mon, 11 Dec 2017 20:23:52 -0500") Message-ID: <87indczcw9.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/msg00271.txt.bz2 On Monday, December 11 2017, Simon Marchi wrote: > On 2017-12-11 18:24, Sergio Durigan Junior wrote: >>>> + >>>> + The output is strongly based on pahole(1). */ >>>> + >>>> +static void >>>> +c_print_type_struct_field_offset (struct type *type, unsigned int >>>> field_idx, >>>> + unsigned int *endpos, struct ui_file *stream, >>>> + unsigned int offset_bitpos) >>>> +{ >>>> + struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, >>>> field_idx)); >>>> + unsigned int bitpos = TYPE_FIELD_BITPOS (type, field_idx); >>>> + unsigned int fieldsize_byte = TYPE_LENGTH (ftype); >>>> + unsigned int fieldsize_bit; >>>> + >>>> + if (*endpos > 0 && *endpos < bitpos) >>> >>> Why do you check for *endpos > 0? Did you see a case where *endpos >>> is 0 >>> and bitpos > 0? That would mean that there's a "hole" before the >>> first field. >>> Would we want to show it as a hole anyway? >> >> Yeah, this situation happens when we have a virtual method in a class. >> Because of the vtable, the first field of the struct will start at >> offset 8 (for 64-bit architectures), and in this case *endpos will be 0 >> because we won't have updated it, leading to a confusing message >> about a >> 8-byte hole in the beginning of the struct: >> >> ... >> 50 /* offset | size */ >> 51 type = struct abc { >> 52 public: >> 53 /* XXX 8-byte hole */ >> 54 /* 8 | 8 */ void *field1; >> ... >> >> In order to suppress this first message, I check for *endpos > 0. >> >> I will add a comment to the code explaining this scenario. > > Ah ok that makes sense. Yeah, a comment would be nice. But now I'm > thinking that it would be nice if GDB showed the vtable. If I say the > first field at offset 8, it would probably take me some time to get > why, and would maybe think it's a bug. But if we showed a fake field, > such as: > > /* 0 | 8 */ /* vtable */ > > It would be immediately obvious. OK, I did that. 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. 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... 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. WDYT? 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/