This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Tom Tromey <tom at tromey dot com>, Eli Zaretskii <eliz at gnu dot org>, Simon Marchi <simon dot marchi at ericsson dot com>, Keith Seitz <keiths at redhat dot com>
- Date: Wed, 13 Dec 2017 12:13:10 -0500
- Subject: Re: [PATCH v5 2/2] Implement pahole-like 'ptype /o' option
- Authentication-results: sourceware.org; auth=none
- References: <20171121160709.23248-1-sergiodj@redhat.com> <20171213031724.22721-1-sergiodj@redhat.com> <20171213031724.22721-3-sergiodj@redhat.com> <614d15fc-3793-8a55-b7cc-67570e8c46d3@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/