Python API - nested pretty printers MI implications

Python API - nested pretty printers MI implications

[Andrew Oakley]

I've got a patch to allow the Python pretty printer children iterators
to return more pretty printers so they can create "phony groups" a bit
like this:

class outer_pretty_printer:
	class inner_pretty_printer:
		def __init__(self, ...):
			...

		def to_string(self):
			...

		def children(self):
			...

	def __init__(self, val):
		...

	def to_string(self):
		...

	def children(self):
		yield ("normal variable", "some value")
		yield ("phony group 1", inner_pretty_printer(...))
		yield ("phony group 2", inner_pretty_printer(...))

This seems to work well but I'm not quite sure how to handle the for MI.

As far as I can tell I need to create "fake" varobj structures with no
underlying value or type (because I don't have one).  This appears to
already happen and is tested for with CPLUS_FAKE_CHILD, but I'm not
quite sure what that is for.  Does this seem like a reasonable thing to
do (after searching around for CPLUS_FAKE_CHILD)?

My other worry is varobj invalidation and update.  Am I correct in
thinking that updates can only happen from a "root" varobj?  If so it
should be possible to reconstruct the "fake" varobj structures in
varobj_update (I don't think it will be straightforward though).

Am I correct in assuming you would want MI to work before accepting
patches?

-- 
Andrew Oakley

Re: Python API - nested pretty printers MI implications

[Daniel Jacobowitz]

On Sun, Aug 14, 2011 at 12:10 PM, Andrew Oakley
<andrew@ado.is-a-geek.net> wrote:
> I've got a patch to allow the Python pretty printer children iterators
> to return more pretty printers so they can create "phony groups" a bit
> like this:

Yay!

> As far as I can tell I need to create "fake" varobj structures with no
> underlying value or type (because I don't have one).  This appears to
> already happen and is tested for with CPLUS_FAKE_CHILD, but I'm not
> quite sure what that is for.  Does this seem like a reasonable thing to
> do (after searching around for CPLUS_FAKE_CHILD)?

It does seem like a reasonable idea, but watch out for
CPLUS_FAKE_CHILD; it may not be a good model.  It's used right now for
public/protected/private groups, and no one seems to want it there -
it might go away at some point.

> My other worry is varobj invalidation and update.  Am I correct in
> thinking that updates can only happen from a "root" varobj?  If so it
> should be possible to reconstruct the "fake" varobj structures in
> varobj_update (I don't think it will be straightforward though).

My memory on this is a bit spotty.  I think it may be possible to
update non-root varobjs, but it's the common case to update the root.
That might have been an out-of-tree patch though...

> Am I correct in assuming you would want MI to work before accepting
> patches?

Even if it's not a requirement, I strongly encourage it.

-- 
Thanks,
Daniel

Re: Python API - nested pretty printers MI implications

[André Pönitz]

On Monday 15 August 2011 00:18:30 ext Daniel Jacobowitz wrote:
> On Sun, Aug 14, 2011 at 12:10 PM, Andrew Oakley
> <andrew@ado.is-a-geek.net> wrote:
> > I've got a patch to allow the Python pretty printer children iterators
> > to return more pretty printers so they can create "phony groups" a bit
> > like this:
> 
> Yay!

That would be really nice to have indeed. 

> > As far as I can tell I need to create "fake" varobj structures with no
> > underlying value or type (because I don't have one).  This appears to
> > already happen and is tested for with CPLUS_FAKE_CHILD, but I'm not
> > quite sure what that is for.  Does this seem like a reasonable thing to
> > do (after searching around for CPLUS_FAKE_CHILD)?
> 
> It does seem like a reasonable idea, but watch out for
> CPLUS_FAKE_CHILD; it may not be a good model.  It's used right now for
> public/protected/private groups, and no one seems to want it there -
> it might go away at some point.
> 
> > My other worry is varobj invalidation and update.  Am I correct in
> > thinking that updates can only happen from a "root" varobj?  If so it
> > should be possible to reconstruct the "fake" varobj structures in
> > varobj_update (I don't think it will be straightforward though).
> 
> My memory on this is a bit spotty.  I think it may be possible to
> update non-root varobjs, but it's the common case to update the root.
> That might have been an out-of-tree patch though...

I am not sure how partial updates on MI varobjs with phony levels 
would work at all. 

Imagine a data structure containing a  char m[1000000][10], and a 
"phony level pretty printer" that displays all the m[i] with m[i][0] == 'A'.
Suppose initially that would be the items m[1] and m[10000], so we 
get a display like 

   m --
      m[1]  "A...."
      m[1000]   "A...."

Now the user steps over m[5000][0] = 'A'. Assuming there is no dummy
varobj for every _potential_ child, what mechanism would trigger the 
varobj's update to produce the display

   m --
      m[1]  "A...."
      m[5000]   "A...."
      m[10000]   "A...."     

eventually?

[While the example might look contrived, it is a situation that naturally
arises when using "phony levels", in various ways.]

The only working "solution" for this kind of problem I am aware of is to 
"manually" force a full re-evaluation of all top level items, i.e. not to
rely on varobj's update mechanism at all.

> > Am I correct in assuming you would want MI to work before accepting
> > patches?
> 
> Even if it's not a requirement, I strongly encourage it.

From a gdb/python (and previously varobj...;-)) user's point of view it
would be nice if caring for MI's varobj machinery (I really only mean 
the varobjs, not MI in general) would not impact progress on the "pure"
python side much.

Andre'

Re: Python API - nested pretty printers MI implications

[Pedro Alves]

On Sunday 14 August 2011 17:10:23, Andrew Oakley wrote:
>         def children(self):
>                 yield ("normal variable", "some value")
>                 yield ("phony group 1", inner_pretty_printer(...))
>                 yield ("phony group 2", inner_pretty_printer(...))
> 
> This seems to work well but I'm not quite sure how to handle the for MI.
> 
> As far as I can tell I need to create "fake" varobj structures with no
> underlying value or type (because I don't have one). 

I'm not very familiar with the pretty printing stuff, but,
doesn't something like this work nowadays?

class phony_value:
       ...
       /* extend Value */

class phony_value_pretty_printer:
        def __init__(self, ...):
                ...

def new_phony(outer, whatnot):
       /* return new phony value for outer */
       ...

class outer_pretty_printer:
        def __init__(self, val):
                ...

        def to_string(self):
                ...

        def children(self):
                yield ("normal variable", "some value")
                yield ("phony group 1", new_phony(1))
                yield ("phony group 2", new_phony(2))


That is, create a specialized Value class and install the pretty
printer for that _value_.

-- 
Pedro Alves

Re: Python API - nested pretty printers MI implications

[Pedro Alves]

On Monday 15 August 2011 13:36:50, André Pönitz wrote:

> I am not sure how partial updates on MI varobjs with phony levels 
> would work at all. 
> 
> Imagine a data structure containing a  char m[1000000][10], and a 
> "phony level pretty printer" that displays all the m[i] with m[i][0] == 'A'.
> Suppose initially that would be the items m[1] and m[10000], so we 
> get a display like 
> 
>    m --
>       m[1]  "A...."
>       m[1000]   "A...."
> 
> Now the user steps over m[5000][0] = 'A'. Assuming there is no dummy
> varobj for every _potential_ child, what mechanism would trigger the 
> varobj's update to produce the display
> 
>    m --
>       m[1]  "A...."
>       m[5000]   "A...."
>       m[10000]   "A...."     
> 
> eventually?

AFAIK, frontends do:

-var-update 2 *

and that should yield (but doesn't):

^done,changelist=[{name="var1.m",value="",in_scope="true",type_changed="false",new_num_children="1",has_more="0",new_children=[{name="var1.m.5000",exp="5000",numchild="1",value="A....",type="foo"}]}]
(gdb)

Note new_num_children.  This should trigger the frontend re-fetching
the children of var1.m.

I think the issue here is that dynamic varobj's code doesn't
handle new children appearing before existing ones.  I got a 
patch to address that though, needed for supporting varobj's
that hide "<unavailable>" children.

-- 
Pedro Alves

Re: Python API - nested pretty printers MI implications

[Andrew Oakley]

On 15/08/11 13:57, Pedro Alves wrote:
> On Sunday 14 August 2011 17:10:23, Andrew Oakley wrote:
>>         def children(self):
>>                 yield ("normal variable", "some value")
>>                 yield ("phony group 1", inner_pretty_printer(...))
>>                 yield ("phony group 2", inner_pretty_printer(...))
>>
>> This seems to work well but I'm not quite sure how to handle the for MI.
>>
>> As far as I can tell I need to create "fake" varobj structures with no
>> underlying value or type (because I don't have one). 
> 
> I'm not very familiar with the pretty printing stuff, but,
> doesn't something like this work nowadays?

[snip]

> That is, create a specialized Value class and install the pretty
> printer for that _value_.

This didn't come up when I asked about this previously.

I assume the idea is to create a gdb.Value (with some data it doesn't
really matter what) and then detect that it is that particular gdb.Value
when the pretty printers list is searched?  Perhaps you could do
something like this:

def fake_value_printer(val):
	if hasattr(val, "prettyprinter"):
		return val.prettyprinter
	else:
		return None

gdb.pretty_printers.insert(0, fake_value_printer)

Then you could just return any old gdb.Value and as long as it had a
prettyprinter attribute then that would be called instead of the
"normal" version.

Is this what you were thinking of?

That's quite a nice trick but I'm not sure its a good long-term
solution.  It relies on the same python gdb.Value being passed back to
the pretty printer selection function and probably causes exactly the
same problems for the MI.

Back to my initial question I guess for MI this is also creating a
"dummy" varobj with some type and value chosen by the python script.  Do
you know if this works in practice with MI frontends?

-- 
Andrew Oakley

Re: Python API - nested pretty printers MI implications

[Pedro Alves]

On Monday 15 August 2011 15:06:10, Andrew Oakley wrote:
> On 15/08/11 13:57, Pedro Alves wrote:
> > On Sunday 14 August 2011 17:10:23, Andrew Oakley wrote:
> >>         def children(self):
> >>                 yield ("normal variable", "some value")
> >>                 yield ("phony group 1", inner_pretty_printer(...))
> >>                 yield ("phony group 2", inner_pretty_printer(...))
> >>
> >> This seems to work well but I'm not quite sure how to handle the for MI.
> >>
> >> As far as I can tell I need to create "fake" varobj structures with no
> >> underlying value or type (because I don't have one). 
> > 
> > I'm not very familiar with the pretty printing stuff, but,
> > doesn't something like this work nowadays?
> 
> [snip]
> 
> > That is, create a specialized Value class and install the pretty
> > printer for that _value_.
> 
> This didn't come up when I asked about this previously.
> 
> I assume the idea is to create a gdb.Value (with some data it doesn't
> really matter what) and then detect that it is that particular gdb.Value
> when the pretty printers list is searched?  Perhaps you could do
> something like this:
> 
> def fake_value_printer(val):
> 	if hasattr(val, "prettyprinter"):
> 		return val.prettyprinter
> 	else:
> 		return None
> 
> gdb.pretty_printers.insert(0, fake_value_printer)
> 
> Then you could just return any old gdb.Value and as long as it had a
> prettyprinter attribute then that would be called instead of the
> "normal" version.
> 
> Is this what you were thinking of?

I was actually thinking more like:

gdb.pretty_printers.insert(0, fake_value_printer)

def fake_value_printer(val):
   isinstance(o, MyFakeValue)
		return FakeValuePrinter(val, or whatever args needed)
 	else:
 		return None

instead of duck typing, but yes, that sounds similar.

> That's quite a nice trick but I'm not sure its a good long-term
> solution.  It relies on the same python gdb.Value being passed back to
> the pretty printer selection function 

I don't understand.

> and probably causes exactly the same problems for the MI.

There'd be no NULL values this way.  Wasn't that the problem?

> Back to my initial question I guess for MI this is also creating a
> "dummy" varobj with some type and value chosen by the python script.  Do
> you know if this works in practice with MI frontends?

I'm not sure what you mean.  You'd always need a "dummy" varobj
for each of the "fake values", wouldn't you?  (I'm not sure you've
seen my reply to André though).

-- 
Pedro Alves

Re: Python API - nested pretty printers MI implications

[André Pönitz]

On Monday 15 August 2011 15:25:48 ext Pedro Alves wrote:
> On Monday 15 August 2011 13:36:50, André Pönitz wrote:
> 
> > I am not sure how partial updates on MI varobjs with phony levels 
> > would work at all. 
> > 
> > Imagine a data structure containing a  char m[1000000][10], and a 
> > "phony level pretty printer" that displays all the m[i] with m[i][0] == 'A'.
> > Suppose initially that would be the items m[1] and m[10000], so we 
> > get a display like 
> > 
> >    m --
> >       m[1]  "A...."
> >       m[1000]   "A...."
> > 
> > Now the user steps over m[5000][0] = 'A'. Assuming there is no dummy
> > varobj for every _potential_ child, what mechanism would trigger the 
> > varobj's update to produce the display
> > 
> >    m --
> >       m[1]  "A...."
> >       m[5000]   "A...."
> >       m[10000]   "A...."     
> > 
> > eventually?
> 
> AFAIK, frontends do:
> 
> -var-update 2 *
> 
> and that should yield (but doesn't):
> 
> ^done,changelist=[{name="var1.m",value="",in_scope="true",type_changed="false",new_num_children="1",has_more="0",new_children=[{name="var1.m.5000",exp="5000",numchild="1",value="A....",type="foo"}]}]
> (gdb)
> 
> Note new_num_children.  This should trigger the frontend re-fetching
> the children of var1.m.
> 
> I think the issue here is that dynamic varobj's code doesn't
> handle new children appearing before existing ones.  I got a 
> patch to address that though, needed for supporting varobj's
> that hide "<unavailable>" children.

Just to confirm I understood correctly: Assuming everything would work as 
planned, a good strategy for frontends is to call '-var-update ... *'. Then
gdb would walk the whole varobj hierarchy, running pretty printers as 
appropriate, and produce a "diff" against the last reported state which is
output as a changelist, announcing potential new children to the FE, 
which in turn could ask for that in another roundtrip.

If so, isn't this very similar to the "fat script" approach, where a python 
command (fed with a list of "names" of the expanded items) does all
the tree walking by itself? That would put everything into "user space",
let the pretty printers output additional data that's not of "general"
(i.e. for all FEs) interest, and would make implementation of pretty 
printers with multiple phony levels straightforward?

Andre'

Re: Python API - nested pretty printers MI implications

[Pedro Alves]

On Monday 15 August 2011 15:33:45, André Pönitz wrote:
> On Monday 15 August 2011 15:25:48 ext Pedro Alves wrote:
> > On Monday 15 August 2011 13:36:50, André Pönitz wrote:
> > 
> > > I am not sure how partial updates on MI varobjs with phony levels 
> > > would work at all. 
> > > 
> > > Imagine a data structure containing a  char m[1000000][10], and a 
> > > "phony level pretty printer" that displays all the m[i] with m[i][0] == 'A'.
> > > Suppose initially that would be the items m[1] and m[10000], so we 
> > > get a display like 
> > > 
> > >    m --
> > >       m[1]  "A...."
> > >       m[1000]   "A...."
> > > 
> > > Now the user steps over m[5000][0] = 'A'. Assuming there is no dummy
> > > varobj for every _potential_ child, what mechanism would trigger the 
> > > varobj's update to produce the display
> > > 
> > >    m --
> > >       m[1]  "A...."
> > >       m[5000]   "A...."
> > >       m[10000]   "A...."     
> > > 
> > > eventually?
> > 
> > AFAIK, frontends do:
> > 
> > -var-update 2 *
> > 
> > and that should yield (but doesn't):
> > 
> > ^done,changelist=[{name="var1.m",value="",in_scope="true",type_changed="false",new_num_children="1",has_more="0",new_children=[{name="var1.m.5000",exp="5000",numchild="1",value="A....",type="foo"}]}]
> > (gdb)
> > 
> > Note new_num_children.  This should trigger the frontend re-fetching
> > the children of var1.m.
> > 
> > I think the issue here is that dynamic varobj's code doesn't
> > handle new children appearing before existing ones.  I got a 
> > patch to address that though, needed for supporting varobj's
> > that hide "<unavailable>" children.
> 
> Just to confirm I understood correctly: Assuming everything would work as 
> planned, a good strategy for frontends is to call '-var-update ... *'. Then
> gdb would walk the whole varobj hierarchy, running pretty printers as 
> appropriate, and produce a "diff" against the last reported state which is
> output as a changelist, announcing potential new children to the FE, 
> which in turn could ask for that in another roundtrip.

Yes.  I believe things could be extended to avoid extra roundtrips.

> If so, isn't this very similar to the "fat script" approach, where a python 
> command (fed with a list of "names" of the expanded items) does all
> the tree walking by itself? That would put everything into "user space",
> let the pretty printers output additional data that's not of "general"
> (i.e. for all FEs) interest, and would make implementation of pretty 
> printers with multiple phony levels straightforward?

Probably.  But doesn't that mean library writters would get to write
pretty printers for each FE out there?

-- 
Pedro Alves

Re: Python API - nested pretty printers MI implications

[André Pönitz]

On Monday 15 August 2011 16:48:55 ext Pedro Alves wrote:
> > [...]
> > Just to confirm I understood correctly: Assuming everything would work as 
> > planned, a good strategy for frontends is to call '-var-update ... *'. Then
> > gdb would walk the whole varobj hierarchy, running pretty printers as 
> > appropriate, and produce a "diff" against the last reported state which is
> > output as a changelist, announcing potential new children to the FE, 
> > which in turn could ask for that in another roundtrip.
> 
> Yes.  I believe things could be extended to avoid extra roundtrips.
> 
> > If so, isn't this very similar to the "fat script" approach, where a python 
> > command (fed with a list of "names" of the expanded items) does all
> > the tree walking by itself? That would put everything into "user space",
> > let the pretty printers output additional data that's not of "general"
> > (i.e. for all FEs) interest, and would make implementation of pretty 
> > printers with multiple phony levels straightforward?
> 
> Probably.  But doesn't that mean library writters would get to write
> pretty printers for each FE out there?

If the library writer wants to "support" every fancy feature of every FE,
then, perhaps, yes.

However, without that flexibility we are in "one size fits all" mode, where
gdb hardcodes which kind of data is deemed useful for every FE, and the
FE has to jump through hoops (like extra roundtrips to retrieve "missing"
 information, or ignore unneeded data) to get hold of what it (the FE) 
actually wants to display. [That's not just anticipation, I had that kind
of situations in the past when a varobj based "full display" of a QObject
based class resulted in 50 or more roundtrips, basically rendering the
whole approach non-viable]

Anyway, in practice I would expect most implementors of "library pretty 
printers" to just provide the "usual" data (i.e. most likely what the current
varobjs display looks like), and the FE to override a handful of them with
an FE-specific "enriched" version - if at all. A schism on an individual 
pretty printer level is certainly much easier to handle (especially if the
maintainance burden for "fancy stuff" is solely on the individual FE side)
than a schism on the fundamental "varobj vs fat script" level.

Andre'

Re: Python API - nested pretty printers MI implications

[Andrew Oakley]

On Mon, 15 Aug 2011 15:30:08 +0100
Pedro Alves <pedro@codesourcery.com> wrote:
> On Monday 15 August 2011 15:06:10, Andrew Oakley wrote:
> > I assume the idea is to create a gdb.Value (with some data it
> > doesn't really matter what) and then detect that it is that
> > particular gdb.Value when the pretty printers list is searched?
> > Perhaps you could do something like this:
> > 
> > def fake_value_printer(val):
> > 	if hasattr(val, "prettyprinter"):
> > 		return val.prettyprinter
> > 	else:
> > 		return None
> > 
> > gdb.pretty_printers.insert(0, fake_value_printer)
> > 
> > Then you could just return any old gdb.Value and as long as it had a
> > prettyprinter attribute then that would be called instead of the
> > "normal" version.
> > 
> > Is this what you were thinking of?
> 
> I was actually thinking more like:
> 
> gdb.pretty_printers.insert(0, fake_value_printer)
> 
> def fake_value_printer(val):
>    isinstance(o, MyFakeValue)
> 		return FakeValuePrinter(val, or whatever args needed)
>  	else:
>  		return None
> 
> instead of duck typing, but yes, that sounds similar.
> 
> > That's quite a nice trick but I'm not sure its a good long-term
> > solution.  It relies on the same python gdb.Value being passed back
> > to the pretty printer selection function 
> 
> I don't understand.

Imagine for a minute that a "struct value" didn't have a reference to a
gdb.Value.  Instead a gdb.Value is created every time we want to pass
a value to python.  The result of this is that the pretty-printer could
return one gdb.Value and the pretty printer selection function would
get a completely different gdb.Value that represented the same thing
(breaking any code that worked like the examples above).  

Given that GDB is quite happy giving you different gdb.Value objects
for exactly the same thing it doesn't seem unreasonable to expect it to
happen with pretty printers too (and the documentation doesn't say that
it can't happen).

As a random example of GDB returning different gdb.Values for the same 
thing:

> $ gdb --quiet `which cat`
> Reading symbols from /bin/cat...Reading symbols from /usr/lib64/debug/bin/cat.debug...done. 
> done.
> (gdb) start
> Temporary breakpoint 1 at 0x401d7b: file cat.c, line 502.
> Starting program: /bin/cat 
> 
> Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe068) at cat.c:5
> 502     cat.c: No such file or directory.
>         in cat.c
> (gdb) python print gdb.selected_frame().read_var('argc') is gdb.selected_frame().read_var('argc')
> False
> (gdb) 

Given some changes to the MI I can envisage this actually happening in
reality, Daniel Jacobowitz was talking about allowing non-root object
updates which might lead to this kind of behaviour.  

I hope this makes more sense now because I don't think I can explain it
any better :(.

> > and probably causes exactly the same problems for the MI.
> 
> There'd be no NULL values this way.  Wasn't that the problem?

Kind of.  Unfortunately this could well confuse front ends.  They see
something that looks like a real value, it even has a type they can
"helpfully" display.  That's not good because this isn't a real value so
we shouldn't make the FE (and by extension varobj) think that it is.

> > Back to my initial question I guess for MI this is also creating a
> > "dummy" varobj with some type and value chosen by the python
> > script.  Do you know if this works in practice with MI frontends?
> 
> I'm not sure what you mean.  You'd always need a "dummy" varobj
> for each of the "fake values", wouldn't you?  (I'm not sure you've
> seen my reply to André though).

Again, it's a question of whether the varobj knows it is fake or not.
I'd prefer it if the varobj did know (even if this isn't exposed over
MI for now).

-- 
Andrew Oakley

Re: Python API - nested pretty printers MI implications

[Andrew Oakley]

On Mon, 15 Aug 2011 17:36:30 +0200
André Pönitz <andre.poenitz@nokia.com> wrote:
> > > If so, isn't this very similar to the "fat script" approach,
> > > where a python command (fed with a list of "names" of the
> > > expanded items) does all the tree walking by itself? That would
> > > put everything into "user space", let the pretty printers output
> > > additional data that's not of "general" (i.e. for all FEs)
> > > interest, and would make implementation of pretty printers with
> > > multiple phony levels straightforward?
> > 
> > Probably.  But doesn't that mean library writters would get to write
> > pretty printers for each FE out there?
> 
> If the library writer wants to "support" every fancy feature of every
> FE, then, perhaps, yes.
> 
> Anyway, in practice I would expect most implementors of "library
> pretty printers" to just provide the "usual" data (i.e. most likely
> what the current varobjs display looks like), and the FE to override
> a handful of them with an FE-specific "enriched" version - if at all.
> A schism on an individual pretty printer level is certainly much
> easier to handle (especially if the maintainance burden for "fancy
> stuff" is solely on the individual FE side) than a schism on the
> fundamental "varobj vs fat script" level.

OK, so I guess the consensus here is that we should expose the tree
produced by python scripts to MI, including any phony children.  

On Sun, 14 Aug 2011 18:18:30 -0400
Daniel Jacobowitz <drow@false.org> wrote:
> It does seem like a reasonable idea, but watch out for
> CPLUS_FAKE_CHILD; it may not be a good model.  It's used right now for
> public/protected/private groups, and no one seems to want it there -
> it might go away at some point.

I still don't have a better idea for this, so it is probably what I'll
have to go with.  I'll rename it as it won't just be a C++ thing any
more.

If anybody has a way to avoid this then I'm like to hear about it, but I
certainly think the varobj code needs some way to deal with fake
objects that are untyped and have no underlying value.  

On Sun, 14 Aug 2011 18:18:30 -0400
Daniel Jacobowitz <drow@false.org> wrote:
> On Sun, Aug 14, 2011 at 12:10 PM, Andrew Oakley
> <andrew@ado.is-a-geek.net> wrote:
> > My other worry is varobj invalidation and update.  Am I correct in
> > thinking that updates can only happen from a "root" varobj?  If so
> > it should be possible to reconstruct the "fake" varobj structures in
> > varobj_update (I don't think it will be straightforward though).
> 
> My memory on this is a bit spotty.  I think it may be possible to
> update non-root varobjs, but it's the common case to update the root.
> That might have been an out-of-tree patch though...

Nobody else seems to indicate that this is possible and I can't find
any way to do it.  I think it's probably safe to assume you can't do
this for now.  If we want to be able to update non-root objects in the
front end will probably have to know that the object is phony and
therefore it can't update it directly (or it could try to update it
and get some kind of error).  


The general impression I have got is that MI is somewhat lacking in
"advanced" features and people have just hacked away to get front ends
to do what they want. On this basis it seems sensible to get this change
in and do my best not to break any existing code.  That way when
somebody does decide to rework some of the MI code they will not
overlook the idea of having phony objects (however they are
implemented).  

Unless anyone has any other suggestions or objections I guess I'll
probably start work on this "soon".

-- 
Andrew Oakley

[PATCH] Allow nested python pretty printers.

[andrew]

From: Andrew Oakley <andrew@ado.is-a-geek.net>

I don't think this is quite ready to commit - MI support needs to be added and
documentation needs to be updated.  I thought it was sensible to post it now
though so I can deal with any feedback and so that others don't find the need
to implement the same thing again.

Will resend to gdb-patches when I think it's ready.

By allowing the children iterator of pretty printers to return more
pretty printers multi levels of display can be created.

Most of the changes here are simple refactoring to make the desired APIs
avaliable:
- try_convert_value_from_python trys to convert a python PyObject to a
  gdb struct value like convert_value_from_python but does not raise an
  error if the PyObject type was not valid.
- run_pretty_printer performs all of the steps to print a value given a
  pretty printer rather having this split between
  apply_val_pretty_printer and print_children.

---
 gdb/python/py-prettyprint.c                 |   69 +++++++++++++++++----------
 gdb/python/py-value.c                       |   50 ++++++++++++-------
 gdb/python/python-internal.h                |    1 +
 gdb/testsuite/gdb.python/py-prettyprint.c   |    5 ++
 gdb/testsuite/gdb.python/py-prettyprint.exp |    2 +
 gdb/testsuite/gdb.python/py-prettyprint.py  |   23 +++++++++
 6 files changed, 107 insertions(+), 43 deletions(-)

diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index b3660de..df71fdf 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -458,20 +458,31 @@ push_dummy_python_frame (void)
   return (PyObject *) frame;
 }
 
-/* Helper for apply_val_pretty_printer that formats children of the
-   printer, if any exist.  If is_py_none is true, then nothing has
-   been printed by to_string, and format output accordingly. */
+/* Helper for apply_val_pretty_printer that runs a pretty printer,
+   including formattin of the children if any exist.  */
 static void
-print_children (PyObject *printer, const char *hint,
-		struct ui_file *stream, int recurse,
-		const struct value_print_options *options,
-		const struct language_defn *language,
-		int is_py_none)
+run_pretty_printer (PyObject *printer, struct ui_file *stream, int recurse,
+		    const struct value_print_options *options,
+		    const struct language_defn *language,
+		    struct gdbarch *gdbarch)
 {
-  int is_map, is_array, done_flag, pretty;
+  int is_py_none, is_map, is_array, done_flag, pretty;
   unsigned int i;
   PyObject *children, *iter, *frame;
   struct cleanup *cleanups;
+  enum string_repr_result print_result;
+
+  /* If we are printing a map, we want some special formatting.  */
+  char * hint = gdbpy_get_display_hint (printer);
+  cleanups = make_cleanup(free_current_contents, &hint);
+
+  /* Print the section */
+  print_result = print_string_repr (printer, hint, stream, recurse,
+				    options, language, gdbarch);
+  if (print_result == string_repr_error)
+    return;
+
+  is_py_none = print_result == string_repr_none;
 
   if (! PyObject_HasAttr (printer, gdbpy_children_cst))
     return;
@@ -489,7 +500,7 @@ print_children (PyObject *printer, const char *hint,
       return;
     }
 
-  cleanups = make_cleanup_py_decref (children);
+  make_cleanup_py_decref (children);
 
   iter = PyObject_GetIter (children);
   if (!iter)
@@ -634,15 +645,33 @@ print_children (PyObject *printer, const char *hint,
 	}
       else
 	{
-	  struct value *value = convert_value_from_python (py_v);
+	  struct value *value;
 
-	  if (value == NULL)
+	  if (try_convert_value_from_python (py_v, &value))
+	    {
+	      if (value == NULL)
+		{
+		  gdbpy_print_stack ();
+		  error (_("Error while executing Python code."));
+		}
+	      else
+		common_val_print (value, stream, recurse + 1, options,
+				  language);
+	    }
+	  else if (PyObject_HasAttr (py_v, gdbpy_to_string_cst))
+	    {
+	      /* have another pretty printer to run */
+	      run_pretty_printer (py_v, stream, recurse + 1, options, language,
+	      			  gdbarch);
+	    }
+	  else
 	    {
+	      PyErr_Format (PyExc_TypeError,
+			    _("Could not convert Python object: %s."),
+			    PyString_AsString (PyObject_Str (py_v)));
 	      gdbpy_print_stack ();
 	      error (_("Error while executing Python code."));
 	    }
-	  else
-	    common_val_print (value, stream, recurse + 1, options, language);
 	}
 
       if (is_map && i % 2 == 0)
@@ -722,20 +751,10 @@ apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr,
   if (! printer || printer == Py_None)
     goto done;
 
-  /* If we are printing a map, we want some special formatting.  */
-  hint = gdbpy_get_display_hint (printer);
-  make_cleanup (free_current_contents, &hint);
-
-  /* Print the section */
-  print_result = print_string_repr (printer, hint, stream, recurse,
-				    options, language, gdbarch);
-  if (print_result != string_repr_error)
-    print_children (printer, hint, stream, recurse, options, language,
-		    print_result == string_repr_none);
+  run_pretty_printer (printer, stream, recurse, options, language, gdbarch);
 
   result = 1;
 
-
  done:
   if (PyErr_Occurred ())
     print_stack_unless_memory_error (stream);
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index fc5053a..dff3be6 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -1074,17 +1074,19 @@ value_object_to_value (PyObject *self)
 }
 
 /* Try to convert a Python value to a gdb value.  If the value cannot
-   be converted, set a Python exception and return NULL.  Returns a
-   reference to a new value on the all_values chain.  */
-
-struct value *
-convert_value_from_python (PyObject *obj)
+   be converted because it was of the incorrect type return 0.  If the value
+   was of the correct type set value to a reference to a new value on the
+   all_values chain and returns 1.  If a Python exception occurs attempting to
+   convert the value then value is set to NULL and 1 is returned */
+int
+try_convert_value_from_python (PyObject *obj, struct value **value)
 {
-  struct value *value = NULL; /* -Wall */
+  int typeok = 1;
   struct cleanup *old;
   volatile struct gdb_exception except;
   int cmp;
 
+  *value = NULL;
   gdb_assert (obj != NULL);
 
   TRY_CATCH (except, RETURN_MASK_ALL)
@@ -1093,14 +1095,14 @@ convert_value_from_python (PyObject *obj)
 	{
 	  cmp = PyObject_IsTrue (obj);
 	  if (cmp >= 0)
-	    value = value_from_longest (builtin_type_pybool, cmp);
+	    *value = value_from_longest (builtin_type_pybool, cmp);
 	}
       else if (PyInt_Check (obj))
 	{
 	  long l = PyInt_AsLong (obj);
 
 	  if (! PyErr_Occurred ())
-	    value = value_from_longest (builtin_type_pyint, l);
+	    *value = value_from_longest (builtin_type_pyint, l);
 	}
       else if (PyLong_Check (obj))
 	{
@@ -1124,7 +1126,7 @@ convert_value_from_python (PyObject *obj)
 
 		      ul = PyLong_AsUnsignedLongLong (obj);
 		      if (! PyErr_Occurred ())
-			value = value_from_ulongest (builtin_type_upylong, ul);
+			*value = value_from_ulongest (builtin_type_upylong, ul);
 		    }
 		  else
 		    /* There's nothing we can do.  */
@@ -1134,14 +1136,14 @@ convert_value_from_python (PyObject *obj)
 		}
 	    }
 	  else
-	    value = value_from_longest (builtin_type_pylong, l);
+	    *value = value_from_longest (builtin_type_pylong, l);
 	}
       else if (PyFloat_Check (obj))
 	{
 	  double d = PyFloat_AsDouble (obj);
 
 	  if (! PyErr_Occurred ())
-	    value = value_from_double (builtin_type_pyfloat, d);
+	    *value = value_from_double (builtin_type_pyfloat, d);
 	}
       else if (gdbpy_is_string (obj))
 	{
@@ -1151,32 +1153,44 @@ convert_value_from_python (PyObject *obj)
 	  if (s != NULL)
 	    {
 	      old = make_cleanup (xfree, s);
-	      value = value_cstring (s, strlen (s), builtin_type_pychar);
+	      *value = value_cstring (s, strlen (s), builtin_type_pychar);
 	      do_cleanups (old);
 	    }
 	}
       else if (PyObject_TypeCheck (obj, &value_object_type))
-	value = value_copy (((value_object *) obj)->value);
+	*value = value_copy (((value_object *) obj)->value);
       else if (gdbpy_is_lazy_string (obj))
 	{
 	  PyObject *result;
 
 	  result = PyObject_CallMethodObjArgs (obj, gdbpy_value_cst,  NULL);
-	  value = value_copy (((value_object *) result)->value);
+	  *value = value_copy (((value_object *) result)->value);
 	}
       else
-	PyErr_Format (PyExc_TypeError,
-		      _("Could not convert Python object: %s."),
-		      PyString_AsString (PyObject_Str (obj)));
+	typeok = 0;
     }
   if (except.reason < 0)
     {
       PyErr_Format (except.reason == RETURN_QUIT
 		    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
 		    "%s", except.message);
-      return NULL;
+      *value = NULL;
     }
 
+  return typeok;
+}
+
+/* Try to convert a Python value to a gdb value.  If the value cannot
+   be converted, set a Python exception and return NULL.  Returns a
+   reference to a new value on the all_values chain.  */
+struct value *
+convert_value_from_python (PyObject *obj)
+{
+  struct value * value;
+  if (!try_convert_value_from_python(obj, &value))
+    PyErr_Format (PyExc_TypeError,
+		  _("Could not convert Python object: %s."),
+		  PyString_AsString (PyObject_Str (obj)));
   return value;
 }
 
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 996b23b..1cfaf75 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -180,6 +180,7 @@ PyObject *inferior_to_inferior_object (struct inferior *inferior);
 struct block *block_object_to_block (PyObject *obj);
 struct symbol *symbol_object_to_symbol (PyObject *obj);
 struct value *value_object_to_value (PyObject *self);
+int try_convert_value_from_python (PyObject *obj, struct value **value);
 struct value *convert_value_from_python (PyObject *obj);
 struct type *type_object_to_type (PyObject *obj);
 struct symtab *symtab_object_to_symtab (PyObject *obj);
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.c b/gdb/testsuite/gdb.python/py-prettyprint.c
index 30f649c..10ab025 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.c
+++ b/gdb/testsuite/gdb.python/py-prettyprint.c
@@ -48,6 +48,10 @@ struct hint_error {
   int x;
 };
 
+struct nested_printer {
+  int x;
+};
+
 #ifdef __cplusplus
 struct S : public s {
   int zs;
@@ -236,6 +240,7 @@ main ()
   struct ns ns, ns2;
   struct lazystring estring, estring2;
   struct hint_error hint_error;
+  struct nested_printer nested_printer;
 
   nstype.elements = narray;
   nstype.len = 0;
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.exp b/gdb/testsuite/gdb.python/py-prettyprint.exp
index b0e7d62..f362cfd 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.exp
+++ b/gdb/testsuite/gdb.python/py-prettyprint.exp
@@ -102,6 +102,8 @@ proc run_lang_tests {exefile lang} {
 
     gdb_test "print c" " = container \"container\" with 2 elements = {$nl *.0. = 23,$nl *.1. = 72$nl}"
 
+    gdb_test "print nested_printer" " = nested_printer_val = {$nl *child1 = child_val_1,$nl *child2 = child_val_2$nl}"
+
     gdb_test "print nstype" " = {$nl *.0. = 7,$nl *.1. = 42$nl}"
 
     gdb_test_no_output "set print pretty off"
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.py b/gdb/testsuite/gdb.python/py-prettyprint.py
index 8bff3c0..be85682 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.py
+++ b/gdb/testsuite/gdb.python/py-prettyprint.py
@@ -187,6 +187,26 @@ class pp_outer:
         yield 's', self.val['s']
         yield 'x', self.val['x']
 
+class pp_nested_printer:
+    "Return a pretty printer as a child"
+
+    def __init__(self, val):
+        self.val = val
+
+    def to_string(self):
+        return 'nested_printer_val'
+
+    def children(self):
+    	class pp_child:
+		def __init__(self, val):
+			self.val = val
+
+		def to_string(self):
+			return self.val
+
+        yield 'child1', pp_child('child_val_1')
+        yield 'child2', pp_child('child_val_2')
+
 class MemoryErrorString:
     "Raise an error"
 
@@ -274,6 +294,9 @@ def register_pretty_printers ():
     pretty_printers_dict[re.compile ('^struct hint_error$')]  = pp_hint_error
     pretty_printers_dict[re.compile ('^hint_error$')]  = pp_hint_error
 
+    pretty_printers_dict[re.compile ('^struct nested_printer$')]  = pp_nested_printer
+    pretty_printers_dict[re.compile ('^nested_printer$')]  = pp_nested_printer
+
     pretty_printers_dict[re.compile ('^memory_error$')]  = MemoryErrorString
 
 pretty_printers_dict = {}
-- 
1.7.3.4

Re: [PATCH] Allow nested python pretty printers.

[Phil Muldoon]

andrew@ado.is-a-geek.net writes:

> From: Andrew Oakley <andrew@ado.is-a-geek.net>
>
> I don't think this is quite ready to commit - MI support needs to be added and
> documentation needs to be updated.  I thought it was sensible to post it now
> though so I can deal with any feedback and so that others don't find the need
> to implement the same thing again.

Understood.  If you want comments on a work-in-progress, (I believe) you
should submit it to gdb-patches anyway, with a [RFC] tag.


> By allowing the children iterator of pretty printers to return more
> pretty printers multi levels of display can be created.

I think this idea is ok, but I do not think renaming functions ad-hoc is
ok.

> - try_convert_value_from_python trys to convert a python PyObject to a
>   gdb struct value like convert_value_from_python but does not raise an
>   error if the PyObject type was not valid.

I think if you want to do this, you should have
try_convert_value_from_python wrap convert_value_from_python and deal
with the exception internally, instead of renaming the function.

> - run_pretty_printer performs all of the steps to print a value given a

The name change seems gratuitous.

> -/* Helper for apply_val_pretty_printer that formats children of the
> -   printer, if any exist.  If is_py_none is true, then nothing has
> -   been printed by to_string, and format output accordingly. */
> +/* Helper for apply_val_pretty_printer that runs a pretty printer,
> +   including formattin of the children if any exist.  */

Typo formattin.

>  static void
> -print_children (PyObject *printer, const char *hint,
> -		struct ui_file *stream, int recurse,
> -		const struct value_print_options *options,
> -		const struct language_defn *language,
> -		int is_py_none)
> +run_pretty_printer (PyObject *printer, struct ui_file *stream, int recurse,
> +		    const struct value_print_options *options,
> +		    const struct language_defn *language,
> +		    struct gdbarch *gdbarch)

I'm not sure I agree with name change.  I don't want to bike-shed this
but "print_children" is a legitimate atomic operation that should remain
as its own function.  I would suggest architecting another function to
call the relevant pieces if we detect nested printers.

>    if (!iter)
> @@ -634,15 +645,33 @@ print_children (PyObject *printer, const char *hint,
>  	}
>        else
>  	{
> -	  struct value *value = convert_value_from_python (py_v);
> +	  struct value *value;
>  
> -	  if (value == NULL)
> +	  if (try_convert_value_from_python (py_v, &value))
> +	    {
> +	      if (value == NULL)
> +		{
> +		  gdbpy_print_stack ();
> +		  error (_("Error while executing Python code."));
> +		}

I'm not sure what his change  gives us?  You still raise an exception,
if value = NULL? Which is what the old code did.

> +	      else
> +		common_val_print (value, stream, recurse + 1, options,
> +				  language);
> +	    }
> +	  else if (PyObject_HasAttr (py_v, gdbpy_to_string_cst))
> +	    {
> +	      /* have another pretty printer to run */
> +	      run_pretty_printer (py_v, stream, recurse + 1, options, language,
> +	      			  gdbarch);
> +	    }

So, in this case, if try_convert_value_to_python returns false, one
assumes there is a nested printer?  Can printers nest other printer
ad infinitum? What happens if the nested printer returns another nested
printer? It's not clear here what the implications are.


>  /* Try to convert a Python value to a gdb value.  If the value cannot
> -   be converted, set a Python exception and return NULL.  Returns a
> -   reference to a new value on the all_values chain.  */
> -
> -struct value *
> -convert_value_from_python (PyObject *obj)
> +   be converted because it was of the incorrect type return 0.  If the value
> +   was of the correct type set value to a reference to a new value on the
> +   all_values chain and returns 1.  If a Python exception occurs attempting to
> +   convert the value then value is set to NULL and 1 is returned */
> +int
> +try_convert_value_from_python (PyObject *obj, struct value **value)
>  {
> -  struct value *value = NULL; /* -Wall */
> +  int typeok = 1;

As I noted, I really don't like this change.  I think you should create
a new function that wraps this one and discards the exception rather
than altering this.  Essentially the opposite of what you have done.


> +/* Try to convert a Python value to a gdb value.  If the value cannot
> +   be converted, set a Python exception and return NULL.  Returns a
> +   reference to a new value on the all_values chain.  */
> +struct value *
> +convert_value_from_python (PyObject *obj)
> +{
> +  struct value * value;
> +  if (!try_convert_value_from_python(obj, &value))
> +    PyErr_Format (PyExc_TypeError,
> +		  _("Could not convert Python object: %s."),
> +		  PyString_AsString (PyObject_Str (obj)));
>    return value;


So this would be try_convert_value_from_python  that just discarded the
exception.  But there are deeper problems.  In this case, if there
really is an error converting a value, in your code you replace it with
a generic exception.  I think the old code is wrong in this too.  I
think if there is an exception, we should not mask it.  I guess we could
mask particular types given your case.

Cheers,

Phil

Re: Python API - nested pretty printers MI implications

[Pedro Alves]

On Tuesday 16 August 2011 23:11:51, Andrew Oakley wrote:
> 
> On Mon, 15 Aug 2011 15:30:08 +0100
> Pedro Alves <pedro@codesourcery.com> wrote:
> > On Monday 15 August 2011 15:06:10, Andrew Oakley wrote:
> > > I assume the idea is to create a gdb.Value (with some data it
> > > doesn't really matter what) and then detect that it is that
> > > particular gdb.Value when the pretty printers list is searched?
> > > Perhaps you could do something like this:
> > > 
> > > def fake_value_printer(val):
> > > 	if hasattr(val, "prettyprinter"):
> > > 		return val.prettyprinter
> > > 	else:
> > > 		return None
> > > 
> > > gdb.pretty_printers.insert(0, fake_value_printer)
> > > 
> > > Then you could just return any old gdb.Value and as long as it had a
> > > prettyprinter attribute then that would be called instead of the
> > > "normal" version.
> > > 
> > > Is this what you were thinking of?
> > 
> > I was actually thinking more like:
> > 
> > gdb.pretty_printers.insert(0, fake_value_printer)
> > 
> > def fake_value_printer(val):
> >    isinstance(o, MyFakeValue)
> > 		return FakeValuePrinter(val, or whatever args needed)
> >  	else:
> >  		return None
> > 
> > instead of duck typing, but yes, that sounds similar.
> > 
> > > That's quite a nice trick but I'm not sure its a good long-term
> > > solution.  It relies on the same python gdb.Value being passed back
> > > to the pretty printer selection function 
> > 
> > I don't understand.
> 
> Imagine for a minute that a "struct value" didn't have a reference to a
> gdb.Value.  Instead a gdb.Value is created every time we want to pass
> a value to python.  The result of this is that the pretty-printer could
> return one gdb.Value and the pretty printer selection function would
> get a completely different gdb.Value that represented the same thing
> (breaking any code that worked like the examples above).  

I understand now, thanks.  Actually, it looks like that is already
happening, as when gdb always takes a copy of the struct value
under the gdb.Value internally, and then wraps it in a _new_ gdb.Value
before passing it to the python pretty printer lookup functions (in
the pretty_printers array).  :-(  IMO this is a bug, and the internal
conversions should be short-circuited to garantee the same gdb.Value
is passed ...  

(I remembered <http://sourceware.org/ml/gdb/2010-09/msg00125.html>, 
which ended up in gdb.Value being extendable, but I see that Tom had
identified the internal copy problem at the time too.)

> Given that GDB is quite happy giving you different gdb.Value objects
> for exactly the same thing it doesn't seem unreasonable to expect it to
> happen with pretty printers too (and the documentation doesn't say that
> it can't happen).
> 
> As a random example of GDB returning different gdb.Values for the same 
> thing:
> 
> > $ gdb --quiet `which cat`
> > Reading symbols from /bin/cat...Reading symbols from /usr/lib64/debug/bin/cat.debug...done. 
> > done.
> > (gdb) start
> > Temporary breakpoint 1 at 0x401d7b: file cat.c, line 502.
> > Starting program: /bin/cat 
> > 
> > Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe068) at cat.c:5
> > 502     cat.c: No such file or directory.
> >         in cat.c
> > (gdb) python print gdb.selected_frame().read_var('argc') is gdb.selected_frame().read_var('argc')
> > False
> > (gdb) 

That's really an unrelated example, IMO.

> Given some changes to the MI I can envisage this actually happening in
> reality, Daniel Jacobowitz was talking about allowing non-root object
> updates which might lead to this kind of behaviour.  

Yes, we should keep those working.

> I hope this makes more sense now because I don't think I can explain it
> any better :(.

Yes, thanks.

> > > and probably causes exactly the same problems for the MI.
> > 
> > There'd be no NULL values this way.  Wasn't that the problem?
> 
> Kind of.  Unfortunately this could well confuse front ends.  They see
> something that looks like a real value, it even has a type they can
> "helpfully" display.  That's not good because this isn't a real value so
> we shouldn't make the FE (and by extension varobj) think that it is.

Not sure that's a real problem.  We could maybe just make it type void.

-- 
Pedro Alves

Re: [PATCH] Allow nested python pretty printers.

[Andrew Oakley]

On 17/08/11 10:56, Phil Muldoon wrote:
> andrew@ado.is-a-geek.net writes:
> 
>> From: Andrew Oakley <andrew@ado.is-a-geek.net>
>>
>> I don't think this is quite ready to commit - MI support needs to be added and
>> documentation needs to be updated.  I thought it was sensible to post it now
>> though so I can deal with any feedback and so that others don't find the need
>> to implement the same thing again.
> 
> Understood.  If you want comments on a work-in-progress, (I believe) you
> should submit it to gdb-patches anyway, with a [RFC] tag.

Fair enough, I'll do that in future.

>> By allowing the children iterator of pretty printers to return more
>> pretty printers multi levels of display can be created.
> 
> I think this idea is ok, but I do not think renaming functions ad-hoc is
> ok.
> 
>> - try_convert_value_from_python trys to convert a python PyObject to a
>>   gdb struct value like convert_value_from_python but does not raise an
>>   error if the PyObject type was not valid.
> 
> I think if you want to do this, you should have
> try_convert_value_from_python wrap convert_value_from_python and deal
> with the exception internally, instead of renaming the function.

I don't really regard this as a rename - there is still a function with
the same name, signature and behaviour as the old function.  I'll come
back to this later on.

>> - run_pretty_printer performs all of the steps to print a value given a
> 
> The name change seems gratuitous.
> 
>> -/* Helper for apply_val_pretty_printer that formats children of the
>> -   printer, if any exist.  If is_py_none is true, then nothing has
>> -   been printed by to_string, and format output accordingly. */
>> +/* Helper for apply_val_pretty_printer that runs a pretty printer,
>> +   including formattin of the children if any exist.  */
> 
> Typo formattin.
> 
>>  static void
>> -print_children (PyObject *printer, const char *hint,
>> -		struct ui_file *stream, int recurse,
>> -		const struct value_print_options *options,
>> -		const struct language_defn *language,
>> -		int is_py_none)
>> +run_pretty_printer (PyObject *printer, struct ui_file *stream, int recurse,
>> +		    const struct value_print_options *options,
>> +		    const struct language_defn *language,
>> +		    struct gdbarch *gdbarch)
> 
> I'm not sure I agree with name change.  I don't want to bike-shed this
> but "print_children" is a legitimate atomic operation that should remain
> as its own function.  I would suggest architecting another function to
> call the relevant pieces if we detect nested printers.

Thats fine by me, I'll do that.  Because print_children was a static
function that was only used in one place I didn't think it would matter
(the rename happened because it now does more than just print the children).

>>    if (!iter)
>> @@ -634,15 +645,33 @@ print_children (PyObject *printer, const char *hint,
>>  	}
>>        else
>>  	{
>> -	  struct value *value = convert_value_from_python (py_v);
>> +	  struct value *value;
>>  
>> -	  if (value == NULL)
>> +	  if (try_convert_value_from_python (py_v, &value))
>> +	    {
>> +	      if (value == NULL)
>> +		{
>> +		  gdbpy_print_stack ();
>> +		  error (_("Error while executing Python code."));
>> +		}
> 
> I'm not sure what his change  gives us?  You still raise an exception,
> if value = NULL? Which is what the old code did.

Yes, if the type was OK for converting to a struct value then the
behaviour is unchanged.  The real difference is that we now know when
the type was not OK (so we can try doing something else).

> 
>> +	      else
>> +		common_val_print (value, stream, recurse + 1, options,
>> +				  language);
>> +	    }
>> +	  else if (PyObject_HasAttr (py_v, gdbpy_to_string_cst))
>> +	    {
>> +	      /* have another pretty printer to run */
>> +	      run_pretty_printer (py_v, stream, recurse + 1, options, language,
>> +	      			  gdbarch);
>> +	    }
> 
> So, in this case, if try_convert_value_to_python returns false, one
> assumes there is a nested printer?  Can printers nest other printer
> ad infinitum? What happens if the nested printer returns another nested
> printer? It's not clear here what the implications are.

You are correct.  If the value could not be handled any other way and it
has a to_string member then we assume it is a pretty printer.  This has
been discussed previously on the list in the thread "Python API - pretty
printing complex types".  This should become more clear when I do the
documentation update.

With regards infinite loops of pretty printers you are correct that we
will get stuck.  This isn't a regression - it has always been possible
to construct cases like this.  Perhaps I need to stick a QUIT somewhere
so you can interrupt it though.

>>  /* Try to convert a Python value to a gdb value.  If the value cannot
>> -   be converted, set a Python exception and return NULL.  Returns a
>> -   reference to a new value on the all_values chain.  */
>> -
>> -struct value *
>> -convert_value_from_python (PyObject *obj)
>> +   be converted because it was of the incorrect type return 0.  If the value
>> +   was of the correct type set value to a reference to a new value on the
>> +   all_values chain and returns 1.  If a Python exception occurs attempting to
>> +   convert the value then value is set to NULL and 1 is returned */
>> +int
>> +try_convert_value_from_python (PyObject *obj, struct value **value)
>>  {
>> -  struct value *value = NULL; /* -Wall */
>> +  int typeok = 1;
> 
> As I noted, I really don't like this change.  I think you should create
> a new function that wraps this one and discards the exception rather
> than altering this.  Essentially the opposite of what you have done.

There main reason why I wanted to not throw the exception at all rather
than throwing it and catching it again is so I can tell why the error
occured - was the value of the wrong type (so we didn't even attempt to
do a conversion) or did the conversion itself fail?  I also didn't
really want to have to work out what the nasty macros + setjmp/longjmp
code was doing in GDB.

To be honest what I really wanted to be able to do is say "is this
python value convertible to a gdb.Value" and then only actually do the
conversion if it was.  The problem with that is we would get duplication
of code between the type checking function and the function that
actually did the conversion.

Is it possible to check what exception has been raised in GDB?  Do you
still think this would be better the other way round?

>> +/* Try to convert a Python value to a gdb value.  If the value cannot
>> +   be converted, set a Python exception and return NULL.  Returns a
>> +   reference to a new value on the all_values chain.  */
>> +struct value *
>> +convert_value_from_python (PyObject *obj)
>> +{
>> +  struct value * value;
>> +  if (!try_convert_value_from_python(obj, &value))
>> +    PyErr_Format (PyExc_TypeError,
>> +		  _("Could not convert Python object: %s."),
>> +		  PyString_AsString (PyObject_Str (obj)));
>>    return value;
> 
> So this would be try_convert_value_from_python  that just discarded the
> exception.  But there are deeper problems.  In this case, if there
> really is an error converting a value, in your code you replace it with
> a generic exception.  I think the old code is wrong in this too.  I
> think if there is an exception, we should not mask it.  I guess we could
> mask particular types given your case.

try_convert_value_from_python didn't discard an exception - it didn't
even try to convert the value if it returns 0.

There are essentially two failure modes for
try_convert_value_from_python - if it returns 0 then the given value was
not of the correct type and it made no attempt to convert it, if it
returns 1 but value is set to NULL then it was of the correct type but
the conversion failed (and a python exception has been raised).  The
comment above try_convert_value_from_python explains this.

The gdb (rather than python) error state does indeed seem to be reset to
be a generic error even though we could have started with a gdb error
and turned it into a python KeyboardInterrupt or RuntimeError.  I think
the code probably should be more careful about this, but again it isn't
a regression in behaviour.

Thanks for reviewing my patch, apologies if I sound like I'm just trying
to be argumentative in places (thats not the intention).


-- 
Andrew Oakley

Re: Python API - nested pretty printers MI implications

[Andrew Oakley]

On Wed, 17 Aug 2011 13:49:10 +0100
Pedro Alves <pedro@codesourcery.com> wrote:

> On Tuesday 16 August 2011 23:11:51, Andrew Oakley wrote:
> > 
> > On Mon, 15 Aug 2011 15:30:08 +0100
> > Pedro Alves <pedro@codesourcery.com> wrote:
> > > On Monday 15 August 2011 15:06:10, Andrew Oakley wrote:
> > > > I assume the idea is to create a gdb.Value (with some data it
> > > > doesn't really matter what) and then detect that it is that
> > > > particular gdb.Value when the pretty printers list is searched?
> > > > Perhaps you could do something like this:
> > > > 
> > > > def fake_value_printer(val):
> > > > 	if hasattr(val, "prettyprinter"):
> > > > 		return val.prettyprinter
> > > > 	else:
> > > > 		return None
> > > > 
> > > > gdb.pretty_printers.insert(0, fake_value_printer)
> > > > 
> > > > Then you could just return any old gdb.Value and as long as it
> > > > had a prettyprinter attribute then that would be called instead
> > > > of the "normal" version.
> > > > 
> > > > Is this what you were thinking of?
> > > 
> > > I was actually thinking more like:
> > > 
> > > gdb.pretty_printers.insert(0, fake_value_printer)
> > > 
> > > def fake_value_printer(val):
> > >    isinstance(o, MyFakeValue)
> > > 		return FakeValuePrinter(val, or whatever args
> > > needed) else:
> > >  		return None
> > > 
> > > instead of duck typing, but yes, that sounds similar.
> > > 
> > > > That's quite a nice trick but I'm not sure its a good long-term
> > > > solution.  It relies on the same python gdb.Value being passed
> > > > back to the pretty printer selection function 
> > > 
> > > I don't understand.
> > 
> > Imagine for a minute that a "struct value" didn't have a reference
> > to a gdb.Value.  Instead a gdb.Value is created every time we want
> > to pass a value to python.  The result of this is that the
> > pretty-printer could return one gdb.Value and the pretty printer
> > selection function would get a completely different gdb.Value that
> > represented the same thing (breaking any code that worked like the
> > examples above).  
> 
> I understand now, thanks.  Actually, it looks like that is already
> happening, as when gdb always takes a copy of the struct value
> under the gdb.Value internally, and then wraps it in a _new_ gdb.Value
> before passing it to the python pretty printer lookup functions (in
> the pretty_printers array).  :-(  IMO this is a bug, and the internal
> conversions should be short-circuited to garantee the same gdb.Value
> is passed ...  
> 
> (I remembered <http://sourceware.org/ml/gdb/2010-09/msg00125.html>, 
> which ended up in gdb.Value being extendable, but I see that Tom had
> identified the internal copy problem at the time too.)
> 
> > Given that GDB is quite happy giving you different gdb.Value objects
> > for exactly the same thing it doesn't seem unreasonable to expect
> > it to happen with pretty printers too (and the documentation
> > doesn't say that it can't happen).

Now I have this information would it be better to attempt to fix the
internal copy "problem" rather than continuing with the current changes
that try to allow a pretty printer to be returned directly?

Obviously documentation should be updated to indicate that you can rely
on this behaviour (once any problems have been fixed) but a lot of
scary things might go away.

> > > There'd be no NULL values this way.  Wasn't that the problem?
> > 
> > Kind of.  Unfortunately this could well confuse front ends.  They
> > see something that looks like a real value, it even has a type they
> > can "helpfully" display.  That's not good because this isn't a real
> > value so we shouldn't make the FE (and by extension varobj) think
> > that it is.
> 
> Not sure that's a real problem.  We could maybe just make it type
> void.

If I was wanting to write my own FE I would certainly want to know.  On
the other hand I have no intention of doing that so I guess it depends
on what others think.

Perhaps we could simply have a convention that fake values have a
"void" type (assuming that can be done) so the FE can detect them if it
wants to but it shouldn't break anything.  I think should satisfy
everybody.

-- 
Andrew Oakley