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

[Pedro Alves <palves at redhat dot com>]

Hi Sergio,

I found a couple more nits to pick, but this is close.

This is OK with the issues below addressed.  Please address
them, and push this in.  Please post the end result for the
archives.

On 12/15/2017 01:12 AM, Sergio Durigan Junior wrote:

> @@ -867,15 +913,121 @@ output_access_specifier (struct ui_file *stream,
>        if (last_access != s_public)
>  	{
>  	  last_access = s_public;
> -	  fprintfi_filtered (level + 2, stream,
> -			     "public:\n");
> +	  print_spaces_filtered_with_print_options (level + 2, stream, flags);
> +	  fprintf_filtered (stream, "public:\n");
>  	}
>      }
>  
>    return last_access;
>  }
>  
> -/* Return true is an access label (i.e., "public:", "private:",
> +/* Print information about field at index FIELD_IDX of the union type
> +   TYPE.  Since union fields don't have the concept of offsets, we
> +   just print their sizes.
> +
> +   The output is strongly based on pahole(1).  */
> +
> +static void
> +c_print_type_union_field_offset (struct type *type, unsigned int field_idx,
> +				 struct ui_file *stream)
> +{
> +  struct type *ftype = check_typedef (TYPE_FIELD_TYPE (type, field_idx));
> +
> +  fprintf_filtered (stream, "/*              %4u */", TYPE_LENGTH (ftype));
> +}
> +
> +/* Print information about field at index FIELD_IDX of the struct type
> +   TYPE.
> +
> +   END_BITPOS is the one-past-the-end bit position of the previous
> +   field (where we expect this field to be if there is no hole).  At
> +   the end, ENDPOS is updated to the one-past-the-end bit position of
> +   the current field.

There's still a reference to ENDPOS above that I assume should
be END_BITPOS.  Now that I'm looking at this, do we still need
this parameter, given print_offset_data->end_bitpos exists?
What's the relationship between the two?  It's a bit confusing
to have two things for the same.  I think the comment could/should
clarify this.

> +
> +   OFFSET_BITPOS is the offset value we carry over when we are
> +   printing a struct that is inside another struct; this is useful so
> +   that the offset is constantly incremented (if we didn't carry it
> +   over, the offset would be reset to zero when printing the inner
> +   struct).

This parameter no longer exists (it's a field of print_offset_data now,
right?).  The comment should be adjusted, moved elsewhere, or deleted.

> +
> +   The output is strongly based on pahole(1).  */
> +
> +static void
> +c_print_type_struct_field_offset (struct type *type, unsigned int field_idx,
> +				  struct ui_file *stream,
> +				  unsigned int *end_bitpos,
> +				  struct print_offset_data *podata)
> +{


> @@ -1143,9 +1347,16 @@ c_type_print_base_struct_union (struct type *type, struct ui_file *stream,
>  		       && !is_full_physname_constructor  /* " " */
>  		       && !is_type_conversion_operator (type, i, j))
>  		{
> -		  c_print_type (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)),
> -				"", stream, -1, 0,
> -				&local_flags);
> +		  unsigned int old_po = local_flags.print_offsets;
> +
> +		  /* Temporarily disable print_offsets, because it
> +		     would mess with indentation.  */
> +		  local_flags.print_offsets = 0;
> +		  c_print_type_1 (TYPE_TARGET_TYPE (TYPE_FN_FIELD_TYPE (f, j)),
> +				  "", stream, -1, 0,
> +				  &local_flags, podata);
> +		  local_flags.print_offsets = old_po;

This pattern appears in several places.  Would it make sense to
add a c_print_type_no_offsets routine that handled the
print_offsets save/restore?

> +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp
> @@ -0,0 +1,317 @@
> +# 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/>.
> +
> +# This testcase exercises the "ptype /o" feature, which can be used to
> +# print the offsets and sizes of each field of a struct/union/class.
> +
> +standard_testfile .cc
> +
> +# Test only works on LP64 targets.  That's how we guarantee that the
> +# expected holes will be present in the struct.
> +if { ![is_lp64_target] } {
> +    untested "test work only on lp64 targets"
> +    return 0
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	  { debug c++ }] } {
> +    return -1
> +}
> +
> +# Test general offset printing, ctor/dtor printing, union, formatting.
> +gdb_test "ptype /o struct abc" \
> +    [multi_line \

...

> +{                         \}}] \
> +    "ptype offset struct abc"

I notice that you're replacing the "/o" in most test
names/messages.  Is there a reason for that?  Why
not just let the test name default to the command
invocation?  I.e., remove the 3rd argument in all
these calls to gdb_test.


> +
> +# Test "ptype /oTM".
> +gdb_test "ptype /oTM struct abc" \
> +    [multi_line \

..

> +    "ptype /oTM struct abc"

Here the "/" persisted, so I'm curious why the other cases
have "/o" replaced by "offset".

I'd just remove the explicit test messages.  It just seems
like potential for getting out of sync otherwise.

> +# Because dealing with bitfields and offsets is difficult, it can be
> +# tricky to confirm that the output of the this command is accurate.

typo: "of the this"

> @@ -499,6 +514,11 @@ whatis_exp (const char *exp, int show)
>  	real_type = value_rtti_type (val, &full, &top, &using_enc);
>      }
>  
> +  if (flags.print_offsets
> +      && (TYPE_CODE (type) == TYPE_CODE_STRUCT
> +	  || TYPE_CODE (type) == TYPE_CODE_UNION))
> +    fprintf_filtered (gdb_stdout, "/* offset    |  size */  ");

I noticed that "whatis" also prints this header:

 (top-gdb) whatis/o minimal_symbol
 /* offset    |  size */  type = minimal_symbol

I guess it shouldn't, for it's pointless.  You could either
use the "show" parameter here, or just ignore /o for whatis
where the options are parsed, further above.

And then please add a test covering that.  :-)

You'll have to use gdb_test_multiple with an anchor,
because 

   gdb_test "whatis/o minimal_symbol" \
      "type = minimal_symbol"

would match the bad output anyway.

So something like:

   set test "whatis /o minimal_symbol"
   gdb_test_multiple $test $test {
      -ex "^$test\r\ntype = minimal_symbol\r\n$gdb_prompt $" {
          pass $test
   }

As mentioned, OK with these issues addressed.  Please push and post.

Thanks,
Pedro Alves