Re: MI: fix base members in references

Re: MI: fix base members in references

[Nick Roberts]

> > The patch below seems to fix it for me.  Its a diff on 1.63 _with_ your
> > yet to be committed changes.

> I would have preferred if instead of adding if, the code was modified to
> look at

>         value_type (var->value)

> as opposed to

>         var->type

I'm not sure that I follow your point.  The patch just gets the target type,
after dereferencing, in the case of a pointer.

> The latter is the type of the varobj expression as it is in source program.
> The former is the value we're actually showing. It makes sense to use
> value_type (var->value) for all presentation purposes.

The former appears to be a type also (not a value).

> > Since we have been the only two people directly contributing to for a
> > while now MI, and as it would stop the patches piling up, perhaps Vladimir
> > (if
> > interested) 

> I don't very well understand maintainership structure in gdb, but I would
> surely appreciate a permission to commit MI patches directly. 

Well it also carries the responsibility of reviewing other peoples patches :-)


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Vladimir Prus]

On Sunday 10 December 2006 00:22, you wrote:
> 
> > > The patch below seems to fix it for me.  Its a diff on 1.63 _with_ your
> > > yet to be committed changes.
> 
> > I would have preferred if instead of adding if, the code was modified to
> > look at
> 
> >         value_type (var->value)
> 
> > as opposed to
> 
> >         var->type
> 
> I'm not sure that I follow your point.  The patch just gets the target type,
> after dereferencing, in the case of a pointer.
> 
> > The latter is the type of the varobj expression as it is in source program.
> > The former is the value we're actually showing. It makes sense to use
> > value_type (var->value) for all presentation purposes.
> 
> The former appears to be a type also (not a value).

Slight typo: the former is the type of the value we're actually showing. So, you don't need
to take original type and try to arrived to the type that should be shown to the user, you
just use value_type (var->value), and don't need any further processing. One less thing
that can be broken in future.

- Volodya

Re: MI: fix base members in references

[Nick Roberts]

 > > > I would have preferred if instead of adding if, the code was modified to
 > > > look at
 > > 
 > > >         value_type (var->value)
 > > 
 > > > as opposed to
 > > 
 > > >         var->type
 > > 
 > > I'm not sure that I follow your point.  The patch just gets the target
 > > type, after dereferencing, in the case of a pointer.
 > > 
 > > > The latter is the type of the varobj expression as it is in source
 > > > program.  The former is the value we're actually showing. It makes sense
 > > > to use value_type (var->value) for all presentation purposes.
 > > 
 > > The former appears to be a type also (not a value).
 > 
 > Slight typo: the former is the type of the value we're actually showing. So,
 > you don't need to take original type and try to arrived to the type that
 > should be shown to the user, you just use value_type (var->value), and don't
 > need any further processing. One less thing that can be broken in future.

I'm still not sure that I follow.  My patch doesn't look at var->type directly.
Are you saying get_type should use value_type (var->value) instead of
var->type?  Or even all occurrances of var->type?  This would presumably be a
separate patch altogether.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Vladimir Prus]

On Sunday 10 December 2006 00:45, you wrote:
>  > > > I would have preferred if instead of adding if, the code was modified to
>  > > > look at
>  > > 
>  > > >         value_type (var->value)
>  > > 
>  > > > as opposed to
>  > > 
>  > > >         var->type
>  > > 
>  > > I'm not sure that I follow your point.  The patch just gets the target
>  > > type, after dereferencing, in the case of a pointer.
>  > > 
>  > > > The latter is the type of the varobj expression as it is in source
>  > > > program.  The former is the value we're actually showing. It makes sense
>  > > > to use value_type (var->value) for all presentation purposes.
>  > > 
>  > > The former appears to be a type also (not a value).
>  > 
>  > Slight typo: the former is the type of the value we're actually showing. So,
>  > you don't need to take original type and try to arrived to the type that
>  > should be shown to the user, you just use value_type (var->value), and don't
>  > need any further processing. One less thing that can be broken in future.
> 
> I'm still not sure that I follow.  My patch doesn't look at var->type directly.

Well, get_type does.

> Are you saying get_type should use value_type (var->value) instead of
> var->type?  

No. I'm saying that instead of using get_type and then additionally doing something
with the type, we should directly use value_type (var->value). If we want to know how
many children a value has, and you know the type of the value, it does not make
sense to take some other type, do some transformation on it, and the hope you'll
get the same type. You can just use the type that's right.

- Volodya

Re: MI: fix base members in references

[Nick Roberts]

Vladimir Prus writes:

> does the code you've added ever does anything?

It seems to handle variable objects of references to pointers correctly.
Have you tried it?

and:

 > > Are you saying get_type should use value_type (var->value) instead of
 > > var->type?
 > 
 > No. I'm saying that instead of using get_type and then additionally doing
 > something with the type, we should directly use value_type (var->value). If
 > we want to know how many children a value has, and you know the type of the
 > value, it does not make sense to take some other type, do some
 > transformation on it, and the hope you'll get the same type. You can just
 > use the type that's right.

I don't think so because it looks at target_type not type.  But these are
unfamiliar concepts to me, so if you think it can be done more simply, or if my
patch doesn't work, post a patch and I'll try it out.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Vladimir Prus]

On Sunday 10 December 2006 07:14, you wrote:
> Vladimir Prus writes:
> 
> > does the code you've added ever does anything?
> 
> It seems to handle variable objects of references to pointers correctly.
> Have you tried it?

No, I trust you when you say that the patch as the whole fixes the problem.
However, given the definition of get_type_deref I don't understand how it does
that. You patch has four hunks, and in two of the you check for pointer type 
and call get_target_type immediately after call to get_type_deref. It looks like it should
have no effect, so I'd like to understand what I'm missing.

- Volodya

Re: MI: fix base members in references

[Nick Roberts]

 > > It seems to handle variable objects of references to pointers correctly.
 > > Have you tried it?
 > 
 > No, I trust you when you say that the patch as the whole fixes the problem.
 > However, given the definition of get_type_deref I don't understand how it
 > does that. You patch has four hunks, and in two of the you check for pointer
 > type and call get_target_type immediately after call to get_type_deref. It
 > looks like it should have no effect, so I'd like to understand what I'm
 > missing.

Its not about trust but verification.  If you find my patch does what I think
it does then I'm probably right and you'll get to understand what you're
missing.  If you find it doesn't work then I'll have look more carefully to see
what I've got wrong.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Daniel Jacobowitz]

On Sun, Dec 10, 2006 at 05:14:28PM +1300, Nick Roberts wrote:
> Vladimir Prus writes:
> 
> > does the code you've added ever does anything?
> 
> It seems to handle variable objects of references to pointers correctly.
> Have you tried it?

I have not been able to follow this discussion, but there needs to be a
better level of understanding here.  Vlad's asking "why does this fix
the problem" and you're responding "it seems to fix the problem". 
Every time I see something like that, I assume the problem is only
being fixed by accident - I'm a big believer in understanding causes.

get_type_deref looks at the type, and if it is a pointer or reference,
it dereferences it.  I assume that this is because we want to show the
children of pointers to structs and references to structs.  Is that
right?

If you want to show the children of the struct given a reference to a
pointer to the struct, then it should handle that too.  Sounds like it
should check for a reference and _then_ for a pointer, instead of
checking at the same time.

There are probably some missing calls to check_typedef here, too.  I
don't think you can have a typedef to a reference in C++, but you can
definitely have a typedef to a pointer, so it would probably be
advisable to call check_typedef before checking for TYPE_CODE_PTR.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI: fix base members in references

[Nick Roberts]

 > > > does the code you've added ever does anything?
 > > 
 > > It seems to handle variable objects of references to pointers correctly.
 > > Have you tried it?
 > 
 > I have not been able to follow this discussion, but there needs to be a
 > better level of understanding here.  Vlad's asking "why does this fix
 > the problem" and you're responding "it seems to fix the problem". 
 > Every time I see something like that, I assume the problem is only
 > being fixed by accident - I'm a big believer in understanding causes.

Looking at code is a bit like peeling an onion: when you remove one layer
there's another layer beneath.  Unfortunately I have to work within my
limitations, both temporal and mental, and I was just trying to get Vladimir to
do a sanity check.  It's easier to show code doesn't work than prove it does
and I don't follow his point about using value_type (var->value) instead of
var->type.  Surely if he thinks that there is a simpler/better patch then
the onus is on him to provide it?

 > get_type_deref looks at the type, and if it is a pointer or reference,
 > it dereferences it.  I assume that this is because we want to show the
 > children of pointers to structs and references to structs.  Is that
 > right?

My patch was just for references to pointers.

 > If you want to show the children of the struct given a reference to a
 > pointer to the struct, then it should handle that too.  Sounds like it
 > should check for a reference and _then_ for a pointer, instead of
 > checking at the same time.

I'm not thinking specifically of structs...

 > There are probably some missing calls to check_typedef here, too.  I
 > don't think you can have a typedef to a reference in C++, but you can
 > definitely have a typedef to a pointer, so it would probably be
 > advisable to call check_typedef before checking for TYPE_CODE_PTR.

...or typedefs.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Daniel Jacobowitz]

On Mon, Dec 11, 2006 at 10:15:29AM +1300, Nick Roberts wrote:
> Looking at code is a bit like peeling an onion: when you remove one layer
> there's another layer beneath.  Unfortunately I have to work within my
> limitations, both temporal and mental, and I was just trying to get Vladimir to
> do a sanity check.  It's easier to show code doesn't work than prove it does
> and I don't follow his point about using value_type (var->value) instead of
> var->type.  Surely if he thinks that there is a simpler/better patch then
> the onus is on him to provide it?

If you can't explain why your change is correct, then by default, it is
not the correct fix.  We can't just throw bandages at the codebase;
that's how you get bigger onions, not better programs.

>  > get_type_deref looks at the type, and if it is a pointer or reference,
>  > it dereferences it.  I assume that this is because we want to show the
>  > children of pointers to structs and references to structs.  Is that
>  > right?
> 
> My patch was just for references to pointers.

Sorry, I don't know how else to explain what that function does; that's
the important thing here.  What does not happen that you want to make
happen, and why doesn't it happen?  The explanation in my previous
message _should_ answer those questions, if I've understood the
discussion right, but maybe it doesn't.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI: fix base members in references

[Nick Roberts]

 > If you can't explain why your change is correct, then by default, it is
 > not the correct fix.  We can't just throw bandages at the codebase;
 > that's how you get bigger onions, not better programs.

I'm not claiming it's correct, just a step in the right direction, but we could
go round in circles (or onion rings!) here.  Its not a RFA.

 > >  > get_type_deref looks at the type, and if it is a pointer or reference,
 > >  > it dereferences it.  I assume that this is because we want to show the
 > >  > children of pointers to structs and references to structs.  Is that
 > >  > right?
 > > 
 > > My patch was just for references to pointers.
 > 
 > Sorry, I don't know how else to explain what that function does;

I'm not querying your explanation, just saying that the patch *wasn't* about
showing the children of pointers to structs and references to structs.

 >                                                                  that's
 > the important thing here.  What does not happen that you want to make
 > happen, and why doesn't it happen?  The explanation in my previous
 > message _should_ answer those questions, if I've understood the
 > discussion right, but maybe it doesn't.

What doesn't work: references to pointers to structs/unions.

All my patch does is add an extra level of dereferencing to the relevant
cplus_* functions.  Presumably you could also have references to references to
pointers, pointers to references to pointers etc so maybe the change should
look something like:

      while (TYPE_CODE (type) == TYPE_CODE_PTR)
             || TYPE_CODE (type) == TYPE_CODE_REF)
	type = get_target_type (type);

but I suspect the business of fake children would make this awkward.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Vladimir Prus]

On Monday 11 December 2006 08:53, Nick Roberts wrote:

>  >                                                                  that's
>  > the important thing here.  What does not happen that you want to make
>  > happen, and why doesn't it happen?  The explanation in my previous
>  > message _should_ answer those questions, if I've understood the
>  > discussion right, but maybe it doesn't.
> 
> What doesn't work: references to pointers to structs/unions.
> 
> All my patch does is add an extra level of dereferencing to the relevant
> cplus_* functions.  Presumably you could also have references to references to

Fortnately, references to references do no exist.

> pointers, pointers to references to pointers etc 

Likewise, pointers to references do not exist either.

> so maybe the change should 
> look something like:
> 
>       while (TYPE_CODE (type) == TYPE_CODE_PTR)
>              || TYPE_CODE (type) == TYPE_CODE_REF)
> 	type = get_target_type (type);
> 
> but I suspect the business of fake children would make this awkward.

Then, if you have a pointer to pointer to pointer to struct, you'll collapse
the inner pointers, which might not be good idea.

I think the best short time fix might be to modify get_type_deref to first check
for reference, and after that check for pointer.

But what irks me most is that we have four functions that encapsulate knowledge of number of children
and how to get to them:

	*_number_of_children
	*_name_of_child
	*_value_of_child
	*_type_of_child

and all those functions are present for C and for C++. I'm thinking we can collapse the last
three functions into one, seriously cutting down on code duplication.

- Volodya

Re: MI: fix base members in references

[Nick Roberts]

 > > All my patch does is add an extra level of dereferencing to the relevant
 > > cplus_* functions.  Presumably you could also have references to
 > > references to
 > 
 > Fortnately, references to references do no exist.
 > 
 > > pointers, pointers to references to pointers etc 
 > 
 > Likewise, pointers to references do not exist either.

OK, thats good.  It keeps things simple.

 > > so maybe the change should 
 > > look something like:
 > > 
 > >       while (TYPE_CODE (type) == TYPE_CODE_PTR)
 > >              || TYPE_CODE (type) == TYPE_CODE_REF)
 > > 	type = get_target_type (type);
 > > 
 > > but I suspect the business of fake children would make this awkward.
 > 
 > Then, if you have a pointer to pointer to pointer to struct, you'll collapse
 > the inner pointers, which might not be good idea.

But as you point out, its not necessary anyway.

 > I think the best short time fix might be to modify get_type_deref to first
 > check for reference, and after that check for pointer.

That would do what my patch already does, but putting it all once in
get_type_deref would be cleaner, of course.

 > But what irks me most is that we have four functions that encapsulate
 > knowledge of number of children and how to get to them:
 > 
 > 	*_number_of_children
 > 	*_name_of_child
 > 	*_value_of_child
 > 	*_type_of_child
 > 
 > and all those functions are present for C and for C++. I'm thinking we can
 > collapse the last three functions into one, seriously cutting down on code
 > duplication.

Yes but this is a separate issue and IMO should be a separate patch.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

MI: fix base members in references

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]


A KDevelop user reported the following bug. If you have C++ reference variable 
that refers to a class type, and that class has bases, gdb is not able to 
show the values of any fields of bases. The trimmed down example is this:

	struct S { int i; int j; };
	struct S2 : S {};
	
	int foo(S2& s)
	{
		return s.i;
	}
	
	int main()
	{
		S2 s;
		s.i = 1;
		s.j = 2;
		return foo(s);
	}

If you are in 'foo' and try to create MI variable objects for s, and navigate 
it, the varobjs for 'i' and 'j' members of the base class will have no value.

The problem happens when creating varobj for the base object. MI sees that 
it's reference and tries to pass it via value_ind. The latter immediately 
removes top-level reference and rightly refuses to deference a structure.

MI should just do nothing about references -- the value_cast function used to
obtain base handles references just fine.

The attached patch fixed the problem, no regression. I'll write a testcase for 
it as soon as my previous references patch is reviewed -- I don't want to 
pile too many testcases in as-yet-uncommitted file.

OK?

If this patch is fine, can I also commit it to 6.6 branch? The bug in question 
is quite problematic for C++ code.

- Volodya




[-- Attachment #2: 01REFERENCES_AND_BASES.diff --]
[-- Type: text/x-diff, Size: 679 bytes --]

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.62
diff -u -p -r1.62 varobj.c
--- varobj.c	29 Nov 2006 06:41:13 -0000	1.62
+++ varobj.c	29 Nov 2006 12:51:53 -0000
@@ -2428,8 +2428,9 @@ cplus_value_of_child (struct varobj *par
 	    {
 	      struct value *temp = NULL;
 
+	      /* No special processing for references is needed --
+		 value_cast below handles references.  */
 	      if (TYPE_CODE (value_type (parent->value)) == TYPE_CODE_PTR
-		  || TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF)
 		{
 		  if (!gdb_value_ind (parent->value, &temp))
 		    return NULL;

Re: MI: fix base members in references

[Vladimir Prus]

Vladimir Prus wrote:

> 
> A KDevelop user reported the following bug. If you have C++ reference
> variable that refers to a class type, and that class has bases, gdb is not
> able to show the values of any fields of bases. The trimmed down example
> is this:
> 
> struct S { int i; int j; };
> struct S2 : S {};
> 
> int foo(S2& s)
> {
> return s.i;
> }
> 
> int main()
> {
> S2 s;
> s.i = 1;
> s.j = 2;
> return foo(s);
> }
> 
> If you are in 'foo' and try to create MI variable objects for s, and
> navigate it, the varobjs for 'i' and 'j' members of the base class will
> have no value.
> 
> The problem happens when creating varobj for the base object. MI sees that
> it's reference and tries to pass it via value_ind. The latter immediately
> removes top-level reference and rightly refuses to deference a structure.
> 
> MI should just do nothing about references -- the value_cast function used
> to obtain base handles references just fine.
> 
> The attached patch fixed the problem, no regression. I'll write a testcase
> for it as soon as my previous references patch is reviewed -- I don't want
> to pile too many testcases in as-yet-uncommitted file.
> 
> OK?
> 
> If this patch is fine, can I also commit it to 6.6 branch? The bug in
> question is quite problematic for C++ code.

*PING*? This is one-liner, and causes no regressions, so should be
non-controversial.

- Volodya

Re: MI: fix base members in references

[Jim Blandy]

Vladimir Prus <ghost@cs.msu.su> writes:
> *PING*? This is one-liner, and causes no regressions, so should be
> non-controversial.

(By the way --- it's handy to include a link to the post with the
patch, or ideally the patch itself.)

Here's the original patch:

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.62
diff -u -p -r1.62 varobj.c
--- varobj.c	29 Nov 2006 06:41:13 -0000	1.62
+++ varobj.c	29 Nov 2006 12:51:53 -0000
@@ -2428,8 +2428,9 @@ cplus_value_of_child (struct varobj *par
 	    {
 	      struct value *temp = NULL;
 
+	      /* No special processing for references is needed --
+		 value_cast below handles references.  */
 	      if (TYPE_CODE (value_type (parent->value)) == TYPE_CODE_PTR
-		  || TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF)
 		{
 		  if (!gdb_value_ind (parent->value, &temp))
 		    return NULL;

How should this behave if parent->value is a reference to a pointer?
Shouldn't it follow the ref, and then behave the same as when it's a
pointer?  If so, then the fix would be something like this instead
(not that I understand this code):

*** varobj.c	29 Nov 2006 15:47:40 -0800	1.62
--- varobj.c	05 Dec 2006 13:31:51 -0800	
***************
*** 2426,2441 ****
  	  /* Baseclass */
  	  if (parent->value != NULL)
  	    {
! 	      struct value *temp = NULL;
  
! 	      if (TYPE_CODE (value_type (parent->value)) == TYPE_CODE_PTR
! 		  || TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF)
  		{
! 		  if (!gdb_value_ind (parent->value, &temp))
  		    return NULL;
  		}
- 	      else
- 		temp = parent->value;
  
  	      if (temp != NULL)
  		{
--- 2426,2439 ----
  	  /* Baseclass */
  	  if (parent->value != NULL)
  	    {
! 	      struct value *temp = coerce_ref (parent->value);
  
! 	      if (TYPE_CODE (value_type (temp)) == TYPE_CODE_PTR
! 		  || TYPE_CODE (value_type (temp)) == TYPE_CODE_REF)
  		{
! 		  if (!gdb_value_ind (temp, &temp))
  		    return NULL;
  		}
  
  	      if (temp != NULL)
  		{

Re: MI: fix base members in references

[Nick Roberts]

> > The attached patch fixed the problem, no regression. I'll write a testcase
> > for it as soon as my previous references patch is reviewed -- I don't want
> > to > pile too many testcases in as-yet-uncommitted file.
> > 
> > OK?

> OK, with a changelog entry.

Actually Vladimir's other patch, MI/C++/references fixup
(http://sourceware.org/ml/gdb-patches/2006-11/msg00373.html), seems to fix
this.  As it also fixes the more general problem of updating references can
this not be applied instead?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Daniel Jacobowitz]

On Wed, Dec 06, 2006 at 09:54:34AM +1300, Nick Roberts wrote:
> 
> > > The attached patch fixed the problem, no regression. I'll write a testcase
> > > for it as soon as my previous references patch is reviewed -- I don't want
> > > to > pile too many testcases in as-yet-uncommitted file.
> > > 
> > > OK?
> 
> > OK, with a changelog entry.
> 
> Actually Vladimir's other patch, MI/C++/references fixup
> (http://sourceware.org/ml/gdb-patches/2006-11/msg00373.html), seems to fix
> this.  As it also fixes the more general problem of updating references can
> this not be applied instead?

They can both go in as far as I'm concerned; I think they're both
needed.  I hadn't got to that one yet.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI: fix base members in references

[Nick Roberts]

 > > Actually Vladimir's other patch, MI/C++/references fixup
 > > (http://sourceware.org/ml/gdb-patches/2006-11/msg00373.html), seems to fix
 > > this.  As it also fixes the more general problem of updating references can
 > > this not be applied instead?
 > 
 > They can both go in as far as I'm concerned; I think they're both
 > needed.  I hadn't got to that one yet.


The comment in varobj.c refers to "Baseclass" and I presume the original author
included the case

  TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF

for a specific reason.

If the patch (fix base members) is a reaction to the reported bug, it's seems
unnecessary with the other patch and therefore, I think it shouldn't be
applied.  If you can see more generally that it's needed then thats another
matter, and clearly it should be applied.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Daniel Jacobowitz]

On Wed, Dec 06, 2006 at 10:21:06AM +1300, Nick Roberts wrote:
> The comment in varobj.c refers to "Baseclass" and I presume the original author
> included the case
> 
>   TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF
> 
> for a specific reason.

Probably, but as Vlad's explanation is correct, either the original
author was wrong or the behavior of value_ind has changed.  Something
in this code should change, but see below.

On Tue, Dec 05, 2006 at 01:33:12PM -0800, Jim Blandy wrote:
> (By the way --- it's handy to include a link to the post with the
> patch, or ideally the patch itself.)

Is a threading mailer really so much to ask? :-)

> How should this behave if parent->value is a reference to a pointer?
> Shouldn't it follow the ref, and then behave the same as when it's a
> pointer?  If so, then the fix would be something like this instead
> (not that I understand this code):

If this is always the same values affected by Vlad's other patch which
calls coerce_ref when setting the value, then maybe we should just be
asserting there is no reference here after that patch.

> --- 2426,2439 ----
>   	  /* Baseclass */
>   	  if (parent->value != NULL)
>   	    {
> ! 	      struct value *temp = coerce_ref (parent->value);
>   
> ! 	      if (TYPE_CODE (value_type (temp)) == TYPE_CODE_PTR
> ! 		  || TYPE_CODE (value_type (temp)) == TYPE_CODE_REF)
>   		{
> ! 		  if (!gdb_value_ind (temp, &temp))
>   		    return NULL;

In any case, you can drop the TYPE_CODE_REF check if you do it this
way.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI: fix base members in references

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> On Wed, Dec 06, 2006 at 10:21:06AM +1300, Nick Roberts wrote:
>> The comment in varobj.c refers to "Baseclass" and I presume the original author
>> included the case
>> 
>>   TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF
>> 
>> for a specific reason.
>
> Probably, but as Vlad's explanation is correct, either the original
> author was wrong or the behavior of value_ind has changed.  Something
> in this code should change, but see below.
>
> On Tue, Dec 05, 2006 at 01:33:12PM -0800, Jim Blandy wrote:
>> (By the way --- it's handy to include a link to the post with the
>> patch, or ideally the patch itself.)
>
> Is a threading mailer really so much to ask? :-)

*blush*  (ding) Gnus is amazingly slow at finding parent messages.

>> How should this behave if parent->value is a reference to a pointer?
>> Shouldn't it follow the ref, and then behave the same as when it's a
>> pointer?  If so, then the fix would be something like this instead
>> (not that I understand this code):
>
> If this is always the same values affected by Vlad's other patch which
> calls coerce_ref when setting the value, then maybe we should just be
> asserting there is no reference here after that patch.
>
>> --- 2426,2439 ----
>>   	  /* Baseclass */
>>   	  if (parent->value != NULL)
>>   	    {
>> ! 	      struct value *temp = coerce_ref (parent->value);
>>   
>> ! 	      if (TYPE_CODE (value_type (temp)) == TYPE_CODE_PTR
>> ! 		  || TYPE_CODE (value_type (temp)) == TYPE_CODE_REF)
>>   		{
>> ! 		  if (!gdb_value_ind (temp, &temp))
>>   		    return NULL;
>
> In any case, you can drop the TYPE_CODE_REF check if you do it this
> way.

Oh, yes --- I meant to drop the TYPE_CODE_REF case.

Re: MI: fix base members in references

[Vladimir Prus]

On Wednesday 06 December 2006 00:46, Daniel Jacobowitz wrote:
> On Wed, Dec 06, 2006 at 10:21:06AM +1300, Nick Roberts wrote:
> > The comment in varobj.c refers to "Baseclass" and I presume the original author
> > included the case
> > 
> >   TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF
> > 
> > for a specific reason.
> 
> Probably, but as Vlad's explanation is correct, either the original
> author was wrong or the behavior of value_ind has changed.  Something
> in this code should change, but see below.
> 
> On Tue, Dec 05, 2006 at 01:33:12PM -0800, Jim Blandy wrote:
> > (By the way --- it's handy to include a link to the post with the
> > patch, or ideally the patch itself.)
> 
> Is a threading mailer really so much to ask? :-)
> 
> > How should this behave if parent->value is a reference to a pointer?
> > Shouldn't it follow the ref, and then behave the same as when it's a
> > pointer?  If so, then the fix would be something like this instead
> > (not that I understand this code):
> 
> If this is always the same values affected by Vlad's other patch which
> calls coerce_ref when setting the value, then maybe we should just be
> asserting there is no reference here after that patch.

That would be best.

However, I also want base/references problem to be fixed on branch, and the big
reference-changing patch might be too big for branch, while base/references patch
is small and strictly makes previously broken use case working, without any other
changes.

- Volodya

Re: MI: fix base members in references

[Nick Roberts]

 > > How should this behave if parent->value is a reference to a pointer?
 > > Shouldn't it follow the ref, and then behave the same as when it's a
 > > pointer?  If so, then the fix would be something like this instead
 > > (not that I understand this code):

More generally variable objects still don't seem to work (even with
Vladimir's yet to be committed patch) for references to pointers.

Assuming that the test program below makes sense, a variable object for n1
has a value which includes the address and reports the value has changed
(as it used to do for pure references)

-- 
Nick                                           http://www.inet.net.nz/~nickrob


int main ()
{
  int *n;
  int *&n1 = n;
  n = new int;
  *n = 1;
}

The initial program I tested with was a variation of Vladimir's


struct S { int i; int j; };

int main()
{
	S *s;
	S *&s1 = s;
	s = new S;
	s->i = 1;
	s->j = 2;
}

The variable object for s1 didn't behave correctly.

Re: MI: fix base members in references

[Nick Roberts]

Nick Roberts writes:
 > More generally variable objects still don't seem to work (even with
 > Vladimir's yet to be committed patch) for references to pointers.
 > 
 > Assuming that the test program below makes sense, a variable object for n1
 > has a value which includes the address and reports the value has changed
 > (as it used to do for pure references)

Sorry  thats actually rubbish (I was using an old GDB), but for

  struct S { int i; int j; };

  S *s;
  S *&s1 = s;

-var-create - * s
^done,name="var1",numchild="1",type="S *"
(gdb)
-var-create - * s1
^done,name="var2",numchild="0",type="S *&"

so the pointer is dereferenced by GDB but the reference to the pointer is
not.  I think that they should work in the same way but I'm not sure.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI: fix base members in references

[Vladimir Prus]

On Thursday 07 December 2006 07:57, Nick Roberts wrote:
> Nick Roberts writes:
>  > More generally variable objects still don't seem to work (even with
>  > Vladimir's yet to be committed patch) for references to pointers.
>  > 
>  > Assuming that the test program below makes sense, a variable object for n1
>  > has a value which includes the address and reports the value has changed
>  > (as it used to do for pure references)
> 
> Sorry  thats actually rubbish (I was using an old GDB), but for
> 
>   struct S { int i; int j; };
> 
>   S *s;
>   S *&s1 = s;
> 
> -var-create - * s
> ^done,name="var1",numchild="1",type="S *"
> (gdb)
> -var-create - * s1
> ^done,name="var2",numchild="0",type="S *&"
> 
> so the pointer is dereferenced by GDB but the reference to the pointer is
> not.  I think that they should work in the same way but I'm not sure.

Guess what happens? The code path that computes the number of children -- namely
c_number_of_children -- does not seem to handle references.

So even while the underlying value is passed via coerce_ref, the reported number of children
is always 0, so you  can't get any children. Ick!  

On the other hand, that's no something that I've broke -- I did not even touch c_number_of_children
or anything related. I guess we need to first commit the primary references patch and then
fix c_number_of_children. Not that I find references to pointers very common thing, but better
be correct.

- Volodya










> 
> 

Re: MI: fix base members in references

[Vladimir Prus]

Vladimir Prus wrote:

> On Thursday 07 December 2006 07:57, Nick Roberts wrote:
>> Nick Roberts writes:
>>  > More generally variable objects still don't seem to work (even with
>>  > Vladimir's yet to be committed patch) for references to pointers.
>>  > 
>>  > Assuming that the test program below makes sense, a variable object
>>  > for n1 has a value which includes the address and reports the value
>>  > has changed (as it used to do for pure references)
>> 
>> Sorry  thats actually rubbish (I was using an old GDB), but for
>> 
>>   struct S { int i; int j; };
>> 
>>   S *s;
>>   S *&s1 = s;
>> 
>> -var-create - * s
>> ^done,name="var1",numchild="1",type="S *"
>> (gdb)
>> -var-create - * s1
>> ^done,name="var2",numchild="0",type="S *&"
>> 
>> so the pointer is dereferenced by GDB but the reference to the pointer is
>> not.  I think that they should work in the same way but I'm not sure.
> 
> Guess what happens? The code path that computes the number of children --
> namely c_number_of_children -- does not seem to handle references.
> 
> So even while the underlying value is passed via coerce_ref, the reported
> number of children
> is always 0, so you  can't get any children. Ick!
> 
> On the other hand, that's no something that I've broke -- I did not even
> touch c_number_of_children or anything related. I guess we need to first
> commit the primary references patch and then fix c_number_of_children. Not
> that I find references to pointers very common thing, but better be
> correct.

To avoid potential confusion -- cplus_number_of_children does not handle
references. c_number_of_children is not expected to,
cplus_number_of_children might handle them, but does not.

- Volodya

Re: MI: fix base members in references

[Nick Roberts]

 >                                                   I guess we need to first
 > commit the primary references patch and then fix c_number_of_children. Not
 > that I find references to pointers very common thing, but better be correct.

The patch below seems to fix it for me.  Its a diff on 1.63 _with_ your
yet to be committed changes.

Since we have been the only two people directly contributing to for a while now
MI, and as it would stop the patches piling up, perhaps Vladimir (if
interested) and me could now be jointly made maintainers of MI.  For the extent
of changes that we could make without prior approval, I have in mind all the
files in the mi directory plus varobj.c.  That should insulate the rest of GDB
while exposing Insight slightly.  In any case we would be conservative in our
changes, and now might be a good time to start with the next release from the
main trunk being a long way off.

WDYT?

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** /home/nickrob/src6/gdb/varobj.c~	2006-12-07 10:54:18.000000000 +1300
--- /home/nickrob/src6/gdb/varobj.c	2006-12-07 23:11:34.000000000 +1300
*************** cplus_number_of_children (struct varobj 
*** 2179,2184 ****
--- 2179,2186 ----
    if (!CPLUS_FAKE_CHILD (var))
      {
        type = get_type_deref (var);
+       if (TYPE_CODE (type) == TYPE_CODE_PTR)
+ 	type = get_target_type (type);
  
        if (((TYPE_CODE (type)) == TYPE_CODE_STRUCT) ||
  	  ((TYPE_CODE (type)) == TYPE_CODE_UNION))
*************** cplus_number_of_children (struct varobj 
*** 2205,2210 ****
--- 2207,2214 ----
        int kids[3];
  
        type = get_type_deref (var->parent);
+       if (TYPE_CODE (type) == TYPE_CODE_PTR)
+ 	type = get_target_type (type);
  
        cplus_class_num_children (type, kids);
        if (strcmp (var->name, "public") == 0)
*************** cplus_name_of_child (struct varobj *pare
*** 2269,2274 ****
--- 2273,2281 ----
    else
      type = get_type_deref (parent);
  
+   if (TYPE_CODE (type) == TYPE_CODE_PTR)
+     type = get_target_type (type);
+ 
    name = NULL;
    switch (TYPE_CODE (type))
      {
*************** cplus_value_of_child (struct varobj *par
*** 2404,2411 ****
    else
      type = get_type_deref (parent);
  
!   value = NULL;
  
    if (((TYPE_CODE (type)) == TYPE_CODE_STRUCT) ||
        ((TYPE_CODE (type)) == TYPE_CODE_UNION))
      {
--- 2411,2420 ----
    else
      type = get_type_deref (parent);
  
!   if (TYPE_CODE (type) == TYPE_CODE_PTR)
!     type = get_target_type (type);
  
+   value = NULL;
    if (((TYPE_CODE (type)) == TYPE_CODE_STRUCT) ||
        ((TYPE_CODE (type)) == TYPE_CODE_UNION))
      {
*************** cplus_type_of_child (struct varobj *pare
*** 2481,2486 ****
--- 2490,2498 ----
    else
      t = get_type_deref (parent);
  
+   if (TYPE_CODE (t) == TYPE_CODE_PTR)
+     t = get_target_type (t);
+ 
    type = NULL;
    switch (TYPE_CODE (t))
      {

Re: MI: fix base members in references

[Vladimir Prus]

Nick Roberts wrote:

>  >                                                   I guess we need to
>  >                                                   first
>  > commit the primary references patch and then fix c_number_of_children.
>  > Not that I find references to pointers very common thing, but better be
>  > correct.
> 
> The patch below seems to fix it for me.  Its a diff on 1.63 _with_ your
> yet to be committed changes.

I would have preferred if instead of adding if, the code was modified to
look at

        value_type (var->value)

as opposed to

        var->type

The latter is the type of the varobj expression as it is in source program.
The former is the value we're actually showing. It makes sense to use
value_type (var->value) for all presentation purposes.

> 
> Since we have been the only two people directly contributing to for a
> while now MI, and as it would stop the patches piling up, perhaps Vladimir
> (if
> interested) 

I don't very well understand maintainership structure in gdb, but I would
surely appreciate a permission to commit MI patches directly. 

- Volodya

Re: MI: fix base members in references

[Vladimir Prus]

Nick Roberts wrote:

> >                                                   I guess we need to
> first > commit the primary references patch and then fix
> c_number_of_children. Not > that I find references to pointers very common
> thing, but better be correct.
> 
> The patch below seems to fix it for me.  Its a diff on 1.63 with your
> yet to be committed changes.

> *** /home/nickrob/src6/gdb/varobj.c~    2006-12-07 10:54:18.000000000
> +1300 --- /home/nickrob/src6/gdb/varobj.c     2006-12-07
> 23:11:34.000000000 +1300 *************** cplus_number_of_children (struct
> varobj *** 2179,2184 ****
> --- 2179,2186 ----
> if (!CPLUS_FAKE_CHILD (var))
> {
> type = get_type_deref (var);
> +       if (TYPE_CODE (type) == TYPE_CODE_PTR)
> +       type = get_target_type (type);


I must admit I have big troubles reading context diffs. Will it be a problem
for you to send diffs in unified format?

If I read the patch correctly, you add two lines of code? Given that
get_type_deref is defined like this:

static struct type *
get_type_deref (struct varobj *var)
{
  struct type *type;

  type = get_type (var);

  if (type != NULL && (TYPE_CODE (type) == TYPE_CODE_PTR
                       || TYPE_CODE (type) == TYPE_CODE_REF))
    type = get_target_type (type);

  return type;
}

does the code you've added ever does anything?

- Volodya

Re: MI: fix base members in references

[Daniel Jacobowitz]

On Wed, Nov 29, 2006 at 03:55:42PM +0300, Vladimir Prus wrote:
> The attached patch fixed the problem, no regression. I'll write a testcase for 
> it as soon as my previous references patch is reviewed -- I don't want to 
> pile too many testcases in as-yet-uncommitted file.
> 
> OK?

OK, with a changelog entry.

> If this patch is fine, can I also commit it to 6.6 branch? The bug in question 
> is quite problematic for C++ code.

Sounds fine to me, but check with Joel.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI: fix base members in references

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

Daniel Jacobowitz wrote:

> On Wed, Nov 29, 2006 at 03:55:42PM +0300, Vladimir Prus wrote:
>> The attached patch fixed the problem, no regression. I'll write a
>> testcase for it as soon as my previous references patch is reviewed -- I
>> don't want to pile too many testcases in as-yet-uncommitted file.
>> 
>> OK?
> 
> OK, with a changelog entry.

Thanks, attached is the version with changelog entry that I've committed.

>> If this patch is fine, can I also commit it to 6.6 branch? The bug in
>> question is quite problematic for C++ code.
> 
> Sounds fine to me, but check with Joel.

I'll check.

After (and if) this patch is merged to branch, and my other references patch
is checked in, I'll adjust the code to just assert, as discussed.

- Volodya


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: base_references_final.diff --]
[-- Type: text/x-diff; name="base_references_final.diff", Size: 1304 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.8008
diff -u -p -r1.8008 ChangeLog
--- ChangeLog	6 Dec 2006 06:51:48 -0000	1.8008
+++ ChangeLog	6 Dec 2006 09:01:09 -0000
@@ -1,3 +1,8 @@
+2006-12-06  Vladimir Prus  <vladimir@codesourcery.com>
+
+	* varobj.c (cplus_value_of_child): When accessing
+	base suboject, don't specially process references.
+	
 2006-12-05  Adam Nemet  <anemet@caviumnetworks.com>
 
 	* MAINTAINERS (Write After Approval): Add myself.
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.62
diff -u -p -r1.62 varobj.c
--- varobj.c	29 Nov 2006 06:41:13 -0000	1.62
+++ varobj.c	6 Dec 2006 09:01:10 -0000
@@ -2428,8 +2428,9 @@ cplus_value_of_child (struct varobj *par
 	    {
 	      struct value *temp = NULL;
 
-	      if (TYPE_CODE (value_type (parent->value)) == TYPE_CODE_PTR
-		  || TYPE_CODE (value_type (parent->value)) == TYPE_CODE_REF)
+	      /* No special processing for references is needed --
+		 value_cast below handles references.  */
+	      if (TYPE_CODE (value_type (parent->value)) == TYPE_CODE_PTR)
 		{
 		  if (!gdb_value_ind (parent->value, &temp))
 		    return NULL;

Re: MI: fix base members in references

[Joel Brobecker]

Volodya,

> >> If this patch is fine, can I also commit it to 6.6 branch? The bug in
> >> question is quite problematic for C++ code.
> > 
> > Sounds fine to me, but check with Joel.
> 
> I'll check.

This patch:

> 2006-12-06  Vladimir Prus  <vladimir@codesourcery.com>
> 
>        * varobj.c (cplus_value_of_child): When accessing
>        base suboject, don't specially process references.
       
is OK for the branch.

Do we have a testcase for that, btw?

-- 
Joel

Re: MI: fix base members in references

[Vladimir Prus]

Joel Brobecker wrote:

> Volodya,
> 
>> >> If this patch is fine, can I also commit it to 6.6 branch? The bug in
>> >> question is quite problematic for C++ code.
>> > 
>> > Sounds fine to me, but check with Joel.
>> 
>> I'll check.
> 
> This patch:
> 
>> 2006-12-06  Vladimir Prus  <vladimir@codesourcery.com>
>> 
>>        * varobj.c (cplus_value_of_child): When accessing
>>        base suboject, don't specially process references.
>        
> is OK for the branch.

Thanks!

> Do we have a testcase for that, btw?

We don't. As I've mentioned, we've short of MI/C++ testcases. My other
references patch adds MI/C++/varobj testcase, after committing that patch
I'll also add tests for base/references problem.

- Volodya