Re: [PATCH v2] Implement pahole-like 'ptype /o' option

[Pedro Alves <palves@redhat.com>]

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