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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Sunday, November 26 2017, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> This commit implements the pahole-like '/o' option for 'ptype', which
> Sergio> prints the offsets and sizes of struct fields, reporting whenever
> Sergio> there is a hole found.
>
> Thanks for doing this!

Thanks for the insights :-).

> Sergio> The idea is to print offsets and sizes only for structs, so unions and
> Sergio> other types are mostly ignored.
>
> I tried out the patch, since I want to add support for this feature to
> the Rust language code.  I have two comments.
>
> First, I think it would be valuable to descend into embedded structures.
> This would make it simpler to review layout of the entire outer struct.
> That is, like this:
>
>     struct t {
>       int x;
>       int y;
>     };
>
>     struct s {
>       int x;
>       char c;
>       struct t t;
>     } s;
>
> The old pahole.py did do this, like
>
>     (gdb) pahole struct s
>                   struct s {
>      /*   0   4 */  int x
>      /*   4   1 */  char c
>       /* XXX 24 bit hole, try to pack */
>      /*   8   8 */  struct t {
>      /*   0   4 */    int x
>      /*   4   4 */    int y
>                     } t
>                   } 

I agree.  In order to test the patch, I've even hacked GDB and made
'ptype' print more levels instead of just 1.  However, due to the
idiosincrasies of the formatting function + the fact that this feature
would be better off as a second patch, I left it as TODO.

I think for 'ptype /o' the best would be to stick with 2 levels, because
otherwise the output gets too messed up.  But I've been thinking about
sending another patch to enable the multi-level ptype anyway.

> ... though the old code's output is kind of confusing, restarting the
> offset at 0 in the embedded struct.

The way my code is written, it also restarts the offset at 0.  Do you
think it'd be better to keep using the offset from the outter struct?

> Second, and similarly, descending into unions does seem to make sense
> sometimes (pahole.py didn't do this, but it seems like it should have).
>
> Continuing the above example, consider:
>
>     union u {
>       struct s s;
>       int i;
>     };
>
> Here ptype shows:
>
>     (gdb) ptype/o union u
>     type = union u {
>                                struct s s;
>                                int i;
>     }
>
> (The formatting looks weird without the layout info here...)
>
> I think one specific case where it is interesting to see the union's
> layout is when you want to know why a union field in a struct is as
> large as it is.  That is, what is the largest member of the union, in
> case you want to try to shrink it?

Good point.  I've implemented this on the patch.

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/