[python][patch] Pretty printers for anonymous types.

[python][patch] Pretty printers for anonymous types.

[Paul Pluzhnikov]

Greetings,

Consider this C source:

typedef struct {
  int x;
} Foo;

typedef Foo Bar;

int main()
{
  Foo f;
  Bar b;
  return f.x + b.x;
}

AFAICT, currently there is no way to establish a pretty printer for
'Foo', because get_type() in python/python.c answers with
"struct { ... }" for either 'f' or 'b'.

Attached is a proposed fix.

Ok to commit?

--
Paul Pluzhnikov

2009-01-16  Paul Pluzhnikov  <ppluzhnikov@google.com>

	    * python/python.c (get_type): Return innermost typdef
	    name for anonymous types.

diff --git a/gdb/python/python.c b/gdb/python/python.c
index bdb5319..f75a443 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -822,11 +822,24 @@ get_type (struct type *type)
   struct ui_file *stb;
   char *thetype;
   long length;
+  struct type *const orig_type = type;
 
   stb = mem_fileopen ();
   old_chain = make_cleanup_ui_file_delete (stb);
 
   CHECK_TYPEDEF (type);
+  if (TYPE_NAME (type) == NULL
+      && TYPE_CODE (orig_type) == TYPE_CODE_TYPEDEF)
+    {
+      /* We landed on unnamed type, something like
+	   typedef struct { ... } Foo;
+         in C. Try using the innermost typedef name instead.  */
+      type = orig_type;
+      while (TYPE_CODE (type) == TYPE_CODE_TYPEDEF
+	     && TYPE_TARGET_TYPE (type)
+	     && TYPE_NAME (TYPE_TARGET_TYPE (type)))
+	type = TYPE_TARGET_TYPE (type);
+    }
 
   type_print (type, "", stb, -1);
 

Re: [python][patch] Pretty printers for anonymous types.

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> AFAICT, currently there is no way to establish a pretty printer for
Paul> 'Foo', because get_type() in python/python.c answers with
Paul> "struct { ... }" for either 'f' or 'b'.

Paul> Ok to commit?

This needs a patch to the documentation -- the search process is
documented.  This is ok with that addition.

One issue I think you will hit is that gdb is fairly aggressive about
calling check_typedef.  For instance, value_cast strips typedefs from
the target type; so I suspect that "p (Foo *) x" will not do the right
thing even with your patch.  This is just something we'll have to
(eventually) fix.

thanks,
Tom

Re: [python][patch] Pretty printers for anonymous types.

[Paul Pluzhnikov]

On Fri, Jan 16, 2009 at 4:49 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>
> Paul> AFAICT, currently there is no way to establish a pretty printer for
> Paul> 'Foo', because get_type() in python/python.c answers with
> Paul> "struct { ... }" for either 'f' or 'b'.
>
> This needs a patch to the documentation -- the search process is
> documented.  This is ok with that addition.

Ok...

> One issue I think you will hit is that gdb is fairly aggressive about
> calling check_typedef.  For instance, value_cast strips typedefs from
> the target type; so I suspect that "p (Foo *) x" will not do the right
> thing even with your patch.  This is just something we'll have to
> (eventually) fix.

You are correct:

(gdb) p *(Foo *)&f
$1 = {x = 0}         # "custom" pretty-printer did not kick in.

There is a further complication: when the same source is compiled
in C++ mode, then the 'TYPE_NAME (type) == NULL' condition is
always false -- the name is set to "<anonymous struct>" by GCC 4.3.1.

I think that's somewhat unfortunate :-(

Yikes!

gcc-4.0.3:  DW_AT_name        : Foo
gcc-4.1.1:  DW_AT_name        : ._0
gcc-4.2.2:  DW_AT_name        : ._0
gcc-4.3.1:  DW_AT_name        : (indirect string, offset: 0x121):
<anonymous struct>

And we aren't even looking at the typedef anymore:

#3  0x00000000004abfa9 in print_formatted (val=0x1209230, size=0,
options=0x7fffffffdd80, stream=0xcb7410) at ../../gdb/printcmd.c:305
305         value_print (val, stream, options);
(top) p val.type.main_type[0]
$15 = {code = TYPE_CODE_STRUCT, flag_unsigned = 0, flag_nosign = 0,
       flag_stub = 0, flag_target_stub = 0, flag_static = 0,
flag_prototyped = 0,
       flag_incomplete = 0, flag_varargs = 0, flag_vector = 0,
       flag_stub_supported = 1, flag_nottext = 0, flag_fixed_instance = 0,
       nfields = 1, vptr_fieldno = -1, name = 0xb70b79 "<anonymous struct>",
       tag_name = 0xb70b79 "<anonymous struct>", objfile = 0xbbab50,
       target_type = 0x0, fields = 0xb10b08, vptr_basetype = 0x0,
       type_specific = {cplus_stuff = 0xa10040, floatformat = 0xa10040,
       calling_convention = 10551360}}

Given that C/C++ consistency is desirable, but dealing with
anonymous C++ structs will clearly take a lot more work, should I
still commit this (with doc update), or file a gdb-archer PR in bugzilla?

Thanks,
-- 
Paul Pluzhnikov

Re: [python][patch] Pretty printers for anonymous types.

[Jeffrey Yasskin]

Here's a thought for the interface; sorry if it's totally
unimplementable or you've already considered and rejected it.

You currently take a dict of regex->printer pairs, but you're using it
just as a set of pairs since you can't reverse-engineer the regex from
the type. What about, instead, if you let the first element of the
pair be any function from a gdb.Type->bool? Then, assuming that
gdb.Type(x_typedef)==gdb.Type(x), I could look up a type by any of its
typedef names and write a predicate to match it. It's straightforward
to write a generator for such predicates for regexes to maintain
compatibility:

def match_by_regex(regex_str):
  regex = re.compile(regex_str)
  def match(type):
    # Just guessing that str(type) is the right conversion.
    return regex.search(str(type)) is not None
  return match

At least at first glance, matching by any typedef's name seems like it
would be an easier interface than trying to guess the right string for
the type. And it'd be relatively easy to write similar
matcher-generators for templates or more complex type patterns.

Jeffrey

On Fri, Jan 16, 2009 at 6:16 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Fri, Jan 16, 2009 at 4:49 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>>
>> Paul> AFAICT, currently there is no way to establish a pretty printer for
>> Paul> 'Foo', because get_type() in python/python.c answers with
>> Paul> "struct { ... }" for either 'f' or 'b'.
>>
>> This needs a patch to the documentation -- the search process is
>> documented.  This is ok with that addition.
>
> Ok...
>
>> One issue I think you will hit is that gdb is fairly aggressive about
>> calling check_typedef.  For instance, value_cast strips typedefs from
>> the target type; so I suspect that "p (Foo *) x" will not do the right
>> thing even with your patch.  This is just something we'll have to
>> (eventually) fix.
>
> You are correct:
>
> (gdb) p *(Foo *)&f
> $1 = {x = 0}         # "custom" pretty-printer did not kick in.
>
> There is a further complication: when the same source is compiled
> in C++ mode, then the 'TYPE_NAME (type) == NULL' condition is
> always false -- the name is set to "<anonymous struct>" by GCC 4.3.1.
>
> I think that's somewhat unfortunate :-(
>
> Yikes!
>
> gcc-4.0.3:  DW_AT_name        : Foo
> gcc-4.1.1:  DW_AT_name        : ._0
> gcc-4.2.2:  DW_AT_name        : ._0
> gcc-4.3.1:  DW_AT_name        : (indirect string, offset: 0x121):
> <anonymous struct>
>
> And we aren't even looking at the typedef anymore:
>
> #3  0x00000000004abfa9 in print_formatted (val=0x1209230, size=0,
> options=0x7fffffffdd80, stream=0xcb7410) at ../../gdb/printcmd.c:305
> 305         value_print (val, stream, options);
> (top) p val.type.main_type[0]
> $15 = {code = TYPE_CODE_STRUCT, flag_unsigned = 0, flag_nosign = 0,
>       flag_stub = 0, flag_target_stub = 0, flag_static = 0,
> flag_prototyped = 0,
>       flag_incomplete = 0, flag_varargs = 0, flag_vector = 0,
>       flag_stub_supported = 1, flag_nottext = 0, flag_fixed_instance = 0,
>       nfields = 1, vptr_fieldno = -1, name = 0xb70b79 "<anonymous struct>",
>       tag_name = 0xb70b79 "<anonymous struct>", objfile = 0xbbab50,
>       target_type = 0x0, fields = 0xb10b08, vptr_basetype = 0x0,
>       type_specific = {cplus_stuff = 0xa10040, floatformat = 0xa10040,
>       calling_convention = 10551360}}
>
> Given that C/C++ consistency is desirable, but dealing with
> anonymous C++ structs will clearly take a lot more work, should I
> still commit this (with doc update), or file a gdb-archer PR in bugzilla?
>
> Thanks,
> --
> Paul Pluzhnikov
>

Re: [python][patch] Pretty printers for anonymous types.

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> There is a further complication: when the same source is compiled
Paul> in C++ mode, then the 'TYPE_NAME (type) == NULL' condition is
Paul> always false -- the name is set to "<anonymous struct>" by GCC 4.3.1.

Paul> I think that's somewhat unfortunate :-(

Yeah.  This seems like a gcc bug to me.

Perhaps a hack in the TYPE_NAME comparison is appropriate in this
instance.  Ugh.

Paul> Given that C/C++ consistency is desirable, but dealing with
Paul> anonymous C++ structs will clearly take a lot more work, should
Paul> I still commit this (with doc update), or file a gdb-archer PR
Paul> in bugzilla?

I think there are three separate questions:

1. How do we want the pretty-printer type lookup to work, in the
abstract?  I think your patch addresses this, and your approach seems
reasonable enough -- though maybe we should really just do what
Jeffrey suggested.  (I'll follow up there.)

2. How to handle what GCC emits.  Even if we get GCC fixed, 4.3 will
remain broken.  So it seems to me that we might as well work around
it.

3. How to deal with internal gdb oddities exposed by this approach.
Here I think an archer bug would be fine.

Tom

Re: [python][patch] Pretty printers for anonymous types.

[Tom Tromey]

>>>>> "Jeffrey" == Jeffrey Yasskin <jyasskin@google.com> writes:

Jeffrey> Here's a thought for the interface; sorry if it's totally
Jeffrey> unimplementable or you've already considered and rejected it.

I don't recall why I chose the approach I did.  Perhaps just because I
had to start somewhere, and it was simple.

Jeffrey> You currently take a dict of regex->printer pairs, but you're using it
Jeffrey> just as a set of pairs since you can't reverse-engineer the regex from
Jeffrey> the type. What about, instead, if you let the first element of the
Jeffrey> pair be any function from a gdb.Type->bool?

This seems reasonable to me.  It would also let us solve the
inheritance issue in a nice way.

I can't think of any drawbacks.  Anybody?

I've also been wondering whether we want to continue using a map, or
whether it would be better to have an explicit sequence of pairs.  A
sequence would permit control over the order in which predicates are
run.

Finally, right now we search maps of all the objfiles, in order, and
then the global pretty_printers map.  I've been wondering whether we
should search the type's objfile (if there is one) first.

I welcome your thoughts on any of this.  We're getting close to
submitting a bunch of this upstream, in preparation for GDB 7.0, so
now is a good time to deal with any lingering problems.

Jeffrey> Then, assuming that gdb.Type(x_typedef)==gdb.Type(x)

This is not the case -- but I don't think it matters.

It would not be hard to add a method to Type to strip the typdefs.
Then a comparison like that could be written easily:

   x_typedef.strip() == x.strip()

(Not sure about that name, it is just an example.)

Also, Type doesn't even have a meaningful comparison operator at the
moment.  Adding this has been on my to-do list for a while.  As I
recall, right now the glue code can construct more than one Type
object representing the same underlying struct type.

Jeffrey> It's straightforward to write a generator for such predicates
Jeffrey> for regexes to maintain compatibility:

FWIW -- until we push into upstream gdb (soon) or ship in Fedora (also
soon), I'm not super concerned about compatibility.

Even so we could also achieve it by just switching on the type of the
key in the map -- if it is a string, do what we do now, else invoke
the predicate.  I'm sort of hoping that we can cover up our inevitable
API mistakes this way, i.e., by using dynamic language features to
maintain backward compatibility.  This seems to work reasonably well
for Emacs.  The results aren't always pretty though.

Tom

Re: [python][patch] Pretty printers for anonymous types.

[Daniel Jacobowitz]

On Fri, Jan 16, 2009 at 08:16:52PM -0700, Tom Tromey wrote:
> I can't think of any drawbacks.  Anybody?

What Jeffrey suggests is what I had in mind years ago, so I like it
:-)  I do have an extension though... should it be gdb.Type -> printer
rather than (gdb.Type -> bool, printer)?  Then some of the ordering
problems could be solved by combining functions.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [python][patch] Pretty printers for anonymous types.

[Tom Tromey]

Daniel> What Jeffrey suggests is what I had in mind years ago, so I like it
Daniel> :-)  I do have an extension though... should it be gdb.Type -> printer
Daniel> rather than (gdb.Type -> bool, printer)?  Then some of the ordering
Daniel> problems could be solved by combining functions.

Yeah, we don't really need a map, just a sequence of type recognition
functions.  A function could return a printer, or None.

Tom

Re: [python][patch] Pretty printers for anonymous types.

[Paul Pluzhnikov]

On Fri, Jan 16, 2009 at 6:55 PM, Tom Tromey <tromey@redhat.com> wrote:

> I think there are three separate questions:
>
> 1. How do we want the pretty-printer type lookup to work, in the
> abstract?  I think your patch addresses this, and your approach seems
> reasonable enough -- though maybe we should really just do what
> Jeffrey suggested.  (I'll follow up there.)
>
> 2. How to handle what GCC emits.  Even if we get GCC fixed, 4.3 will
> remain broken.  So it seems to me that we might as well work around
> it.
>
> 3. How to deal with internal gdb oddities exposed by this approach.
> Here I think an archer bug would be fine.

So, given that my patch at the start of this thread fixes the
problem only for "plain C" anonymous types, should I commit it?

[It solves a real problem for Jeffrey, and anonymous types are much
less frequent in C++, so arguably this solves 66% of the problem :]

The C++ anonymous struct can't be solved without solving #3 (it
seems that the "wrong" (== fully resolved) type is attached to the
value at the dwarf reader level).

Thanks,
-- 
Paul Pluzhnikov