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

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

On Monday, December 11 2017, Simon Marchi wrote:

>>>> +/* Use 'print_spaces_filtered', but take into consideration the
>>>> +   type_print_options FLAGS in order to determine how many whitespaces
>>>> +   will be printed.  */
>>>> +
>>>> +static void
>>>> +print_spaces_filtered_with_print_options (int level, struct ui_file *stream,
>>>> +					const struct type_print_options *flags)
>>>
>>> Missing spaces here.
>> 
>> This is actually on purpose.  If I indent the line, it will have more
>> than 80 chars.  I believe this is a well known method for avoiding this
>> problem...?
>
> I am not aware of that.  In this case I would put the parameter list on the next,
> I'm not sure if it's 100% GNU-style approved, but nobody complained when I did it
> so far :)
>
> static void
> print_spaces_filtered_with_print_options
>   (int level, struct ui_file *stream, const struct type_print_options *flags);
>
> It helps with long function names.  In this case, I would probably just drop the
> "struct" to save a few chars, because C++.

Fair enough.  I use this trick for function prototypes, but not for the
definitions.

>>>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
>>>> +	  { debug c++ optimize=-O0 }] } {
>>>
>>> optimize=-O0 seems unnecessary to me, I've never seen it specified explicitly in a test.
>> 
>> There are very few tests that use it (2, currently).  I understand it's
>> not very common to explicitly specify -O0, but I put it there because I
>> decided to be on the safe side.  If the compiler performs any
>> optimization at all, it could mess with the layout of the structs.
>
> If GCC decided to optimize by default, so many things would break in the GDB testsuite.
> We would then probably make gdb_compile add -O0, so that we wouldn't need to do it in
> all tests.  The point is that IMO, tests should expect no optimization by default.

OK.

>>> I also noticed that the offset is not shown in front of the struct-in-union,
>>> as show above, but it is in the case of struct-in-struct:
>>>
>>>   /* offset    |  size */
>>>   type = struct my_struct_3 {
>>>   /*    0      |     8 */    struct my_struct_1 {
>>>   /*    0      |     4 */        int a;
>>>   /*    4      |     4 */        int b;
>>>                              } /* total size:    8 bytes */ s1;
>>>   /*    8      |     8 */    struct my_struct_2 {
>>>   /*    8      |     4 */        int c;
>>>   /*   12      |     4 */        int d;
>>>                              } /* total size:    8 bytes */ s2;
>>>   } /* total size:   16 bytes */
>>>
>>> Is this difference on purpose?
>> 
>> Yes; offsets are not shown for fields inside unions (not only structs,
>> but all types of fields), because it doesn't make much sense: they'd be
>> 0 every time.  This is also inspired from pahole's output.
>
> Not if that union is itself in a struct.  For example with this:
>
> struct hello
> {
>         int i;
>         union {
>                 struct {
>                         int x, y;
>                 } a;
>                 struct {
>                         int x, y;
>                 } b;
>         };
> };
>
> (gdb) ptype /o struct hello
> /* offset    |  size */
> type = struct hello {
> /*    0      |     4 */    int i;
> /*    4      |     8 */    union {
> /*                 8 */        struct {
> /*    4      |     4 */            int x;
> /*    8      |     4 */            int y;
>                                } /* total size:    8 bytes */ a;
> /*                 8 */        struct {
> /*    4      |     4 */            int x;
> /*    8      |     4 */            int y;
>                                } /* total size:    8 bytes */ b;
>                            } /* total size:    8 bytes */;
> } /* total size:   12 bytes */
>
>
> 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.

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/