From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47587 invoked by alias); 11 Dec 2017 23:24:46 -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 47576 invoked by uid 89); 11 Dec 2017 23:24:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=BAYES_00,GIT_PATCH_1,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; Mon, 11 Dec 2017 23:24:43 +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 C541980468; Mon, 11 Dec 2017 23:24:42 +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 B7E7817B87; Mon, 11 Dec 2017 23:24:41 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: 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> Date: Mon, 11 Dec 2017 23:24:00 -0000 In-Reply-To: <2b1ed21e-2d41-19d2-a0cf-3b4780cf9060@ericsson.com> (Simon Marchi's message of "Mon, 11 Dec 2017 16:50:21 -0500") Message-ID: <87lgi8c0g6.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/msg00256.txt.bz2 On Monday, December 11 2017, Simon Marchi wrote: > Hi Sergio, > > LGTM, with some nits. > > On 2017-12-11 02:57 PM, Sergio Durigan Junior wrote: >> @@ -867,14 +890,86 @@ output_access_specifier (struct ui_file *stream, >> if (last_access != s_public) >> { >> last_access = s_public; >> - fprintfi_filtered (level + 2, stream, >> - "public:\n"); >> + print_spaces_filtered_with_print_options (level + 2, stream, flags); >> + fprintf_filtered (stream, "public:\n"); >> } >> } >> >> return last_access; >> } >> >> +/* Print information about the offset of TYPE inside its union. >> + FIELD_IDX represents the index of this TYPE inside the union. We >> + just print the type size, and nothing more. >> + >> + The output is strongly based on pahole(1). */ >> + >> +static void >> +c_print_type_union_field_offset (struct type *type, unsigned int field_idx, >> + struct ui_file *stream) >> +{ >> + struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx)); >> + >> + fprintf_filtered (stream, "/* %4u */", TYPE_LENGTH (ftype)); >> +} >> + >> +/* Print information about the offset of TYPE inside its struct. >> + FIELD_IDX represents the index of this TYPE inside the struct, and >> + ENDPOS is the end position of the previous type (this is how we >> + calculate whether there are holes in the struct). At the end, >> + ENDPOS is updated. > > The wording confuses me a bit. TYPE is not inside a struct; the struct > contains fields, not types. And TYPE is the struct type IIUC, not the > field type. So it should maybe be something like: > > Print information about field at index FIELD_IDX of the struct type TYPE. > ENDPOS is the one-past-the-end bit position of the previous field (where > we expect this field to be if there is no hole). At the end, ENDPOS is > updated to the one-past-the-end bit position of the current field. Thanks, I adopted your version. > What does OFFSET_BITPOS do? Ah, I forgot to include it in the comment. It is the offset value we carry over when we are printing an inner struct. This is needed because otherwise we'd print inner structs starting at position 0, which is something that Tom didn't like and suggested changing. >> + >> + 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. -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/