This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Tom Tromey <tom at tromey dot com>, Eli Zaretskii <eliz at gnu dot org>, Simon Marchi <simon dot marchi at ericsson dot com>, Keith Seitz <keiths at redhat dot com>
- Date: Wed, 13 Dec 2017 12:41:07 -0500
- Subject: Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option
- Authentication-results: sourceware.org; auth=none
- References: <20171121160709.23248-1-sergiodj@redhat.com> <20171213031724.22721-1-sergiodj@redhat.com> <20171213031724.22721-3-sergiodj@redhat.com> <2c84dc10-a131-31bb-8db5-74c3b3d3a95e@redhat.com>
On Wednesday, December 13 2017, Pedro Alves wrote:
> [Sending user-interface suggestions separately from core
> functionality review.]
Thanks.
> 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?
I agree, and when I started working on this feature the first plan was
to print things on the right. I even had a patch that did that. But
the main problem was that it was hard to properly align the /* */
blocks. For example, "ptype /o" prints nested structures, which means
that the we may print things that are aligned at level + 8, level + 12,
or even further. This would make the /* */ blocks be aligned
differently, which IMHO makes the output much harder to read. pahole
(the tool) solves that by printing nested structures separately, but if
we were to do that we'd need a bit refactorization of the current code.
About the Python version of pahole, it also prints the /* */ blocks on
the left side, though in an uglier way IMHO:
(gdb) pahole struct wer
struct wer {
/* 0 24 */ struct tuv {
/* 0 4 */ int a1
/* XXX 32 bit hole, try to pack */
/* 8 8 */ char * a2
/* 16 4 */ int a3
} tuv
/* 24 24 */ struct tyu {
/* 0 0 */ int a1
/* 0 0 */ int a2
/* 0 2 */ int a3
/* 3 0 */ char a4
/* XXX 35 bit hole, try to pack */
/* 8 8 */ long a5
/* 16 0 */ int a6
/* 16 0 */ long a7
} a1
}
> 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 {
0> /* 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)
Yeah, this makes sense. I'll see about doing this.
> #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.).
I like the idea of putting them on a separate line; not sure how hard
that will be. I'll take a look at implemeting this.
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/