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

[Pedro Alves <palves@redhat.com>]

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?

> +++ 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.

> +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.

> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	  { debug c++ optimize=-O0 }] } {
> +    return -1
> +}

Weren't you going to remove that optimize thing? :-)

> +# 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.)

> +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?

> @@ -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.

> +      (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