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

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

On Wednesday, December 13 2017, Pedro Alves wrote:

> On 12/13/2017 03:17 AM, Sergio Durigan Junior wrote:
>
>> +/* A struct with an union.  */
>> +
>> +struct poi
>> +{
>> +  int f1;
>> +
>> +  union qwe f2;
>> +
>> +  uint16_t f3;
>> +
>> +  struct pqr f4;
>> +};
>> +
>> +/* A struct with bitfields.  */
>> +
>> +struct tyu
>> +{
>> +  int a1 : 1;
>> +
>> +  int a2 : 3;
>> +
>> +  int a3 : 23;
>> +
>> +  char a4 : 2;
>> +
>> +  int64_t a5;
>> +
>> +  int a6 : 5;
>> +
>> +  int64_t a7 : 3;
>> +};
>
> I think that the testcase should also make sure to exercise the new
> offset computations in the case c_print_type_struct_field_offset's
> 'offset_bitpos' parameter is > 0.  Is it already covered?
> I assume we'll need a test like tyu (struct with bitfields with
> overlapping underlying objects), but that inherits some other
> base structure?

Ah, good point, I'll add this test.

>
>> +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp
>> @@ -0,0 +1,192 @@
>> +# This testcase is part of GDB, the GNU debugger.
>> +
>> +# Copyright 2017 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>
> Please add an intro comment describing what this testcase is about.

OK.

>> +standard_testfile .cc
>> +
>> +# Test only works on x86_64 LP64 targets.  That's how we guarantee
>> +# that the expected holes will be present in the struct.
>> +if { !([istarget "x86_64-*-*"] && [is_lp64_target]) } {
>> +    untested "test work only on x86_64 lp64"
>> +    return 0
>> +}
>
> I'm mildly worried about whether the bitfield handling is working
> correctly on big endian machines.  We may want to lift this
> x86-64-only restriction, by using e.g., alignas(N) or
> __attribute__((aligned(N)) to take care of most of the differences
> between architectures and end up with few per-arch code in
> the .exp.  But I'm fine with starting with only x86-64 if you
> confirm manually on e.g., a big endian PPC64 machine on the
> compile farm, and we can extend the testcase in that direction
> after this is merged.

OK, I'll confirm on PPC64BE.

>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
>> +	  { debug c++ optimize=-O0 }] } {
>> +    return -1
>> +}
>
> Weren't you going to remove that optimize thing? :-)

Yes, sorry.  Removed.

>> +# Test that the offset is properly reset when we are printing an union
>> +# and go inside two inner structs.
>> +# This also tests a struct inside a struct inside an union.
>
> "a union".  (two times here; there may be other places.)

Fixed.

>> +gdb_test "ptype /o union qwe" \
>> +    [multi_line \
>> +"/\\\* offset    |  size \\\*/" \
>> +"/\\\*                24 \\\*/    struct tuv {" \
>> +"/\\\*    0      |     4 \\\*/        int a1;" \
>> +"/\\\* XXX  4-byte hole  \\\*/" \
>> +"/\\\*    8      |     8 \\\*/        char \\\*a2;" \
>> +"/\\\*   16      |     4 \\\*/        int a3;" \
>> +"                           } /\\\* total size:   24 bytes \\\*/ fff1;" \
>> +"/\\\*                40 \\\*/    struct xyz {" \
>> +"/\\\*    0      |     4 \\\*/        int f1;" \
>> +"/\\\*    4      |     1 \\\*/        char f2;" \
>> +"/\\\* XXX  3-byte hole  \\\*/" \
>> +"/\\\*    8      |     8 \\\*/        void \\\*f3;" \
>> +"/\\\*   16      |    24 \\\*/        struct tuv {" \
>> +"/\\\*   16      |     4 \\\*/            int a1;" \
>> +"/\\\* XXX  4-byte hole  \\\*/" \
>> +"/\\\*   24      |     8 \\\*/            char \\\*a2;" \
>> +"/\\\*   32      |     4 \\\*/            int a3;" \
>> +"                               } /\\\* total size:   24 bytes \\\*/ f4;" \
>> +"                           } /\\\* total size:   40 bytes \\\*/ fff2;" \
>> +"} /\\\* total size:   40 bytes \\\*/"] \
>> +    "ptype offset union qwe"
>
> Did you try using {} instead of "" for these strings,
> avoiding all the escaping?

Yes, Simon also made the same comment.  I tried replacing by {} but it
didn't work at the first attempt and since I was hacking other stuff at
the moment I dind't bother tweaking it and just reverted to using "".
If it's something you really want, I can do.  Otherwise I'd prefer to
leave it like that.

>> @@ -499,6 +506,11 @@ whatis_exp (const char *exp, int show)
>>  	real_type = value_rtti_type (val, &full, &top, &using_enc);
>>      }
>>  
>> +  if (flags.print_offsets &&
>
> && goes on the next line.

Fixed.

>> +      (TYPE_CODE (type) == TYPE_CODE_STRUCT
>> +       || TYPE_CODE (type) == TYPE_CODE_UNION))
>> +    fprintf_filtered (gdb_stdout, "/* offset    |  size */\n");
>> +
>>    printf_filtered ("type = ");
>
> Thanks,
> Pedro Alves

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/