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

[Pedro Alves <palves@redhat.com>]

[Sending user-interface suggestions separately from core
functionality review.]

On 12/13/2017 03:17 AM, Sergio Durigan Junior wrote:
>   (gdb) ptype /o stap_probe
>   /* offset    |  size */
>   struct stap_probe {
>   /*    0      |    40 */    struct probe {
>   /*    0      |     8 */        const probe_ops *pops;
>   /*    8      |     8 */        gdbarch *arch;
>   /*   16      |     8 */        const char *name;
>   /*   24      |     8 */        const char *provider;
>   /*   32      |     8 */        CORE_ADDR address;
> 			     } /* total size:   40 bytes */ p;
>   /*   40      |     8 */    CORE_ADDR sem_addr;
>   /*   48:31   |     4 */    unsigned int args_parsed : 1;
>   /* XXX  7-bit hole   */
>   /* XXX  7-byte hole  */
>   /*   56      |     8 */    union {
>   /*                 8 */        const char *text;
>   /*                 8 */        VEC_stap_probe_arg_s *vec;
> 			     } /* total size:    8 bytes */ args_u;
>   } /* total size:   64 bytes */

I've experimented with (different versions of) of this patch
against gdb a bit, and I have to say that I'm finding myself
a bit confused by the currently produced output.  It feels
like the output should be a bit more obvious and easier to
skim quickly.

For example, I was looking at gdb's minimal_symbol (what I had
used when I noticed the bitfield handling in earlier versions was
incorrect):

(top-gdb) ptype/tom minimal_symbol
/* offset    |  size */
type = struct minimal_symbol {
/*    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;
/*   16      |     8 */        union {
/*                 8 */            obstack *obstack;
/*                 8 */            const char *demangled_name;
                               } /* total size:    8 bytes */ language_specific;
/*   24:27   |     4 */        language language : 5;
/*   24:26   |     4 */        unsigned int ada_mangled : 1;
/* XXX  2-bit hole   */
/* XXX  1-byte hole  */
/*   26      |     2 */        short section;
                           } /* total size:   32 bytes */ mginfo;
/*   32      |     8 */    unsigned long size;
/*   40      |     8 */    const char *filename;
/*   48:28   |     4 */    minimal_symbol_type type : 4;
/*   48:27   |     4 */    unsigned int created_by_gdb : 1;
/*   48:26   |     4 */    unsigned int target_flag_1 : 1;
/*   48:25   |     4 */    unsigned int target_flag_2 : 1;
/*   48:24   |     4 */    unsigned int has_size : 1;
/* XXX  7-byte hole  */
/*   56      |     8 */    minimal_symbol *hash_next;
/*   64      |     8 */    minimal_symbol *demangled_hash_next;
} /* total size:   72 bytes */
(top-gdb) 

and I have the following observations:

#1 - left vs right

We print the extra offset/size info on the leftside of the
screen, shifting the whole type to the right.  If we were to put
the new info on the right, then a "ptype S" vs "ptype/o S" would
look mostly the same, except for the extra info that appears
on the right, and it'd map better to what you'd write in
actual code (who puts comments on the left side, right?)
pahole(1) prints the info on the right side.  Was there a
particular reason gdb's version prints it on the left hand
side?  Maybe the Python version did that too (did it?  I haven't
tried it) because it'd be difficult with the Python
implementation otherwise?  

But maybe it's just me.  On to the next point in
such case.

#2 - shift "type =" header to the right as well?

Currently, the leading "type =" and ending "}" lines
are aligned on the left, along with the offset/size info.
I think shifting those to the right as well might make the
output nicer.  The idea being that /o would split the
output in two more-clearly-separated columns, with the new info
panned hard on the left column, and old ptype info on
the right column.

I.e., currently we get:

~~~~~~~~~~~~~~~~~~~~~~~~~~~
(top-gdb) ptype/o minimal_symbol
/* offset    |  size */
type = struct minimal_symbol {
/*    0      |    32 */    struct general_symbol_info {
/*    0      |     8 */        const char *name;
...
} /* total size:   72 bytes */
~~~~~~~~~~~~~~~~~~~~~~~~~~~

And I'd propose instead:

~~~~~~~~~~~~~~~~~~~~~~~~~~~
(top-gdb) ptype/o minimal_symbol
/* offset    |  size */
                         type = struct minimal_symbol {
/*    0      |    32 */    struct general_symbol_info {
/*    0      |     8 */        const char *name;
...
                         } /* total size:   72 bytes */
~~~~~~~~~~~~~~~~~~~~~~~~~~~

Or even saving one line:

~~~~~~~~~~~~~~~~~~~~~~~~~~~
(top-gdb) ptype/o minimal_symbol
/* offset    |  size */  type = struct minimal_symbol {
/*    0      |    32 */    struct general_symbol_info {
/*    0      |     8 */        const char *name;
...
                         } /* total size:   72 bytes */
~~~~~~~~~~~~~~~~~~~~~~~~~~~


Or in full (compare to the version further above):

(top-gdb) ptype/o minimal_symbol
/* offset    |  size */  type = struct minimal_symbol {
/*    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;
/*   16      |     8 */        union {
/*                 8 */            obstack *obstack;
/*                 8 */            const char *demangled_name;
                               } /* total size:    8 bytes */ language_specific;
/*   24:27   |     4 */        language language : 5;
/*   24:26   |     4 */        unsigned int ada_mangled : 1;
/* XXX  2-bit hole   */
/* XXX  1-byte hole  */
/*   26      |     2 */        short section;
                           } /* total size:   32 bytes */ mginfo;
/*   32      |     8 */    unsigned long size;
/*   40      |     8 */    const char *filename;
/*   48:28   |     4 */    minimal_symbol_type type : 4;
/*   48:27   |     4 */    unsigned int created_by_gdb : 1;
/*   48:26   |     4 */    unsigned int target_flag_1 : 1;
/*   48:25   |     4 */    unsigned int target_flag_2 : 1;
/*   48:24   |     4 */    unsigned int has_size : 1;
/* XXX  7-byte hole  */
/*   56      |     8 */    minimal_symbol *hash_next;
/*   64      |     8 */    minimal_symbol *demangled_hash_next;
                         } /* total size:   72 bytes */
(top-gdb) 


#3 - I also find the position of those
"/* total size:    8 bytes */"  markers a bit hard
to grok/spot.

I'd suggest considering putting them on the left column
along with the rest of the offset/size info as well, like:

(top-gdb) ptype/tom minimal_symbol
/* offset    |  size */
type = struct minimal_symbol {
/*    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 */        } value;
/*   16      |     8 */        union {
/*                 8 */            obstack *obstack;
/*                 8 */            const char *demangled_name;
/* total size:     8 */        } language_specific;
/*   24:27   |     4 */        language language : 5;
/*   24:26   |     4 */        unsigned int ada_mangled : 1;
/* XXX  2-bit hole   */
/* XXX  1-byte hole  */
/*   26      |     2 */        short section;
/* total size:    32 */    } mginfo;
/*   32      |     8 */    unsigned long size;
/*   40      |     8 */    const char *filename;
/*   48:28   |     4 */    minimal_symbol_type type : 4;
/*   48:27   |     4 */    unsigned int created_by_gdb : 1;
/*   48:26   |     4 */    unsigned int target_flag_1 : 1;
/*   48:25   |     4 */    unsigned int target_flag_2 : 1;
/*   48:24   |     4 */    unsigned int has_size : 1;
/* XXX  7-byte hole  */
/*   56      |     8 */    minimal_symbol *hash_next;
/*   64      |     8 */    minimal_symbol *demangled_hash_next;
} /* total size:   72 bytes */
(top-gdb) 

Or if you don't like it, consider putting that 
"total size:" info on a separate line before the struct/union
is closed, like pahole does:

        /* size: 128, cachelines: 2, members: 4 */
        /* sum members: 124, holes: 1, sum holes: 4 */
};

That may be even better because it provides a place
to put extra info, like pahole does (cacheline info etc.).

Thanks,
Pedro Alves