From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93861 invoked by alias); 11 Dec 2017 23:46:03 -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 93812 invoked by uid 89); 11 Dec 2017 23:46:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=Simons, Fair, worthless 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:46:01 +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 C586DC00B90C; Mon, 11 Dec 2017 23:46:00 +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 A95BA600D3; Mon, 11 Dec 2017 23:45:57 +0000 (UTC) Subject: Re: [PATCH v2] Implement pahole-like 'ptype /o' option To: Sergio Durigan Junior References: <20171121160709.23248-1-sergiodj@redhat.com> <20171128212137.15655-1-sergiodj@redhat.com> <87o9n5drbn.fsf@redhat.com> <99286acb-ce9f-42f0-41c3-ef10e03171ff@ericsson.com> <87tvwxc6t9.fsf@redhat.com> <964bae42-6e60-4e9d-048c-ef570c1d3a5b@redhat.com> <87fu8gdgl8.fsf@redhat.com> Cc: Simon Marchi , GDB Patches , Tom Tromey , Eli Zaretskii From: Pedro Alves Message-ID: <0d314798-6e0d-f7a9-ace8-3cb43bda9947@redhat.com> Date: Mon, 11 Dec 2017 23:46: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: <87fu8gdgl8.fsf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00260.txt.bz2 On 12/11/2017 10:50 PM, Sergio Durigan Junior wrote: >>> Fair enough. I use this trick for function prototypes, but not for the >>> definitions. >> >> Simon's format is what I've been using for a long while too. > > Well, I could post a few examples of the format I chose, but I'm pretty > sure this would be worthless. As I said, I will change my code. > By stating the format that I've been using too, my intention is to show that Simon's format isn't his own unique snowflake either. If there are many examples of the format you chose, I've not seem many. Maybe it depends on area of code one is touching, or IOW, on the preferences of who wrote specific areas of the codebase. IMO, the right way to go about this is to decide on a format, document it in the wiki and then we can point everyone at it with a URL. (ISTR that GCC's coding conventions documents yet another way to break the line, and in that case, not even GCC follows it.) IMO, the format you've chosen isn't ideal because it requires manual right-alignment for every line, while breaking before the parens makes emacs's TAB automatically align the following lines. The latter also gives a lot more space for the params again; it's sort of a "well, I have to do something, so might as well start way back from the left again). >>>> But I don't mind it, it just stuck out as a little inconsistency. >>> >>> I don't see the inconsistency. >>> >>> If a field is inside a struct, it has its offset *and* size printed. No >>> matter if the field is an int, another struct, or an union. >>> >>> If a field is inside an union, it has only its size printed. >>> >>> In the case above, it makes sense to have the offsets printed for the >>> fields inside the two structs (inside the union), because there might be >>> holes to report (well, one can argue that it doesn't matter if there are >>> holes or not in this case, because if the other struct is bigger then >>> the union size will stay the same). However, it doesn't make sense to >>> print the offsets for the two structs themselves, because they are >>> members of the union. >>> >>> I hope it makes more sense now. >> >> But why do we need the special case? Does it help anything? >> So far, it seems it only added confusion. > > What do you mean by "special case"? Special case: "If a field is inside a union, it has only its size printed; otherwise print the offset and size." No special case: "Always print the offset and size." > > This is what pahole does, and as I've said a few times, the output of > 'ptype /o' has been based on pahole's output. That's abundantly clear, but I don't see why we need to follow "pahole"'s output religiously... > I don't consider this a > special case; I consider it to be the natural thing to do, because > offsets don't make much sense in unions. Of course they do. You can do 'offsetof(foo_union, foo_field)' just fine, for example. Saying that the offsets happen to be the same is not the same as saying that the offsets don't exist. I asked: >> Does it help anything? >> So far, it seems it only added confusion. I think a much better rationale for omitting the offsets would be: Not printing the offsets in the case of union members helps by increasing signal/noise ratio. Why? With patch as is: /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; /* 8 | 8 */ union { /* 8 */ LONGEST ivalue; /* 8 */ const block *block; /* 8 */ const gdb_byte *bytes; /* 8 */ CORE_ADDR address; /* 8 */ const common_block *common_block; /* 8 */ symbol *chain; } /* total size: 8 bytes */ value; vs always-print-offset: /* 0 | 32 */ struct general_symbol_info { /* 0 | 8 */ const char *name; /* 8 | 8 */ union { /* 8 8 */ LONGEST ivalue; /* 8 8 */ const block *block; /* 8 8 */ const gdb_byte *bytes; /* 8 8 */ CORE_ADDR address; /* 8 8 */ const common_block *common_block; /* 8 8 */ symbol *chain; } /* total size: 8 bytes */ value; Note that it can be argued that the version that does _not_ print the offsets includes _more_ information, or less noise, because with that version it's much easier to not get distracted with the offsets of the union fields, which can do nothing about. So from that angle, I see value in not printing the offsets of union members. (Also note that the assertion that offsets of union members would be 0 every time is inaccurate. When printing a union that is itself a field of a struct, and the union is at offset > 0 in the containing struct, the offset to print would be offset > 0.) > >> The option is "/o" for "print offsets". Why not print offsets always? > > I hope I explained it above. > >> BTW, shouldn't the documentation in the manual include an example >> of GDB's output? > > I can include an example, OK. Thanks, Pedro Alves