Re: [RFA 0/2] Support ptype/o in Rust

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Saturday, June 23 2018, Tom Tromey wrote:

> This adds support for ptype/o to the Rust code.
>
> The first patch slightly refactors the existing ptype/o code.  The
> utility functions are now public methods on struct print_offset_data.

Thanks for the patches, Tom.

> The second patch changes the Rust language code.  I would self-approve
> this one but it required a change outside of Rust.  Perhaps this check
> ought to have been a flag on the language_defn.
>
> I noticed that ptype/o generates somewhat funny output:
>
>     /* offset    |  size */  type = union simple::Union {
>     /*                 1 */    f1: i8,
>     /*                 1 */    f2: u8,
>
> 			       /* total size (bytes):    1 */
> 			     }
>
> Here, I think it might be cleaner to put the "total size" information
> on the same line as the trailing "}" (and of course not indent it),
> like:
>
>     /* offset    |  size */  type = union simple::Union {
>     /*                 1 */    f1: i8,
>     /*                 1 */    f2: u8,
>     /* total size      1 */  }
>
> If you agree I can at least file a bug or maybe implement it.

Your version does look better (and IIRC, one of the early versions of
the patch printed "total size" at column 0), but I think it may be a bit
confusing when we're dealing with inner structures.  For example:

  $ cat 2.c
  struct a
  {
    int a1;
    char a2;
    int a3;
  };

  struct b
  {
    struct a b1;
    int b2;
    char b3;
  };

  struct c
  {
    struct a c1;
    struct b c2;
    char c3;
    int c4;
  };

  int main ()
  {
    struct c foo;

    return 0;
  }

We have:

  (gdb) ptype /o struct c
  /* offset    |  size */  type = struct c {
  /*    0      |    12 */    struct a {
  /*    0      |     4 */        int a1;
  /*    4      |     1 */        char a2;
  /* XXX  3-byte hole */
  /*    8      |     4 */        int a3;

                                 /* total size (bytes):   12 */
                             } c1;
  /*   12      |    20 */    struct b {
  /*   12      |    12 */        struct a {
  /*   12      |     4 */            int a1;
  /*   16      |     1 */            char a2;
  /* XXX  3-byte hole */
  /*   20      |     4 */            int a3;

                                     /* total size (bytes):   12 */
                                 } b1;
  /*   24      |     4 */        int b2;
  /*   28      |     1 */        char b3;
  /* XXX  3-byte padding */

                                 /* total size (bytes):   20 */
                             } c2;
  /*   32      |     1 */    char c3;
  /* XXX  3-byte hole */
  /*   36      |     4 */    int c4;

                             /* total size (bytes):   40 */
                           }

Even though it's a bit uglier than your solution, IMO it's easier to
understand and correlate the sizes with their respective structs.
Compare this to:

  (gdb) ptype /o struct c
  /* offset    |  size */  type = struct c {
  /*    0      |    12 */    struct a {
  /*    0      |     4 */        int a1;
  /*    4      |     1 */        char a2;
  /* XXX  3-byte hole */
  /*    8      |     4 */        int a3;
  /* total size (bytes):   12 */
                             } c1;
  /*   12      |    20 */    struct b {
  /*   12      |    12 */        struct a {
  /*   12      |     4 */            int a1;
  /*   16      |     1 */            char a2;
  /* XXX  3-byte hole */
  /*   20      |     4 */            int a3;
  /* total size (bytes):   12 */
                                 } b1;
  /*   24      |     4 */        int b2;
  /*   28      |     1 */        char b3;
  /* XXX  3-byte padding */
  /* total size (bytes):   20 */
                             } c2;
  /*   32      |     1 */    char c3;
  /* XXX  3-byte hole */
  /*   36      |     4 */    int c4;
  /* total size (bytes):   40 */
                           }

Of course, it's still possible to read the output and interpret it
correctly, but it demands a bit more effort, I think.

Maybe a solution would be to be a bit more verbose, like:

  /* total size of struct a (bytes):... */

> Additionally I noticed that in C, in most cases fields are indented 4
> spaces, but with ptype/o the outermost fields are only indented 2
> spaces (relative to the "type =" text).  I think this is probably
> unintended as well, but I thought I'd ask... ?

I don't really remember why I made this decision.  I guess it had to do
with the fact that using 4 whitespaces to indent would consume a lot of
unnecessary space, and since ptype/o prints more information than the
regular ptype, every whitespace counts.  I vaguely remember having this
thought, so that may be the reason, after all.  Or maybe it also had
something to do with increasing the readability?

Anyway, TBH I don't have a strong opinion here.  If you want to indent
the outermost fields by 4 spaces, I won't oppose.

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/