Variable objects: references formatting

Variable objects: references formatting

[Vladimir Prus]

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


Hi!
At the moment, when using variable objects to display a struct or a class,
the result of -data-evaluate-expression is "...". However, when displaying
a reference to a class, the result of -data-evaluate-expression is
{}-enclosed list of members and their values.

This disparity does not seem to be reasonable, the attached patch fixes it:

Changelog:

  2006-05-03 Vladimir Prus <ghost@cs.msu.su>
      varobj.c (c_value_of_variable): Ignore top-level references.

Patch attached.

Thanks,
Volodya

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

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.58
diff -u -r1.58 varobj.c
@@ -2055,8 +2219,14 @@
 {
   /* BOGUS: if val_print sees a struct/class, it will print out its
      children instead of "{...}" */
+  struct type* type = get_type (var);
+  /* Strip top-level references. */
+  while (TYPE_CODE (type) == TYPE_CODE_REF)
+    {
+      type = TYPE_TARGET_TYPE (type);
+    }
 
-  switch (TYPE_CODE (get_type (var)))
+  switch (TYPE_CODE (type))
     {
     case TYPE_CODE_STRUCT:
     case TYPE_CODE_UNION:

Re: Variable objects: references formatting

[Jim Blandy]

On 5/3/06, Vladimir Prus <ghost@cs.msu.su> wrote:
> At the moment, when using variable objects to display a struct or a class,
> the result of -data-evaluate-expression is "...". However, when displaying
> a reference to a class, the result of -data-evaluate-expression is
> {}-enclosed list of members and their values.
>
> This disparity does not seem to be reasonable, the attached patch fixes it:

I think you're right, and the patch looks good.  I'll apply it.

It seems we've committed some of your prior patches without getting a
copyright assignment; since they were small, this is fine.  But if you
plan to continue making contributions to GDB, it would be nice to get
the paperwork taken care of properly.  I'll send you mail off-line
about this.

Re: Variable objects: references formatting

[Jim Blandy]

On 5/3/06, Jim Blandy <jimb@red-bean.com> wrote:
> On 5/3/06, Vladimir Prus <ghost@cs.msu.su> wrote:
> > At the moment, when using variable objects to display a struct or a class,
> > the result of -data-evaluate-expression is "...". However, when displaying
> > a reference to a class, the result of -data-evaluate-expression is
> > {}-enclosed list of members and their values.
> >
> > This disparity does not seem to be reasonable, the attached patch fixes it:
>
> I think you're right, and the patch looks good.  I'll apply it.

Some further stuff I came across, for future reference:

- ChangeLog entries need to have two spaces between the date and the
name, and the name and the email address.  They need an asterisk
before the filename.  Check out the other ChangeLog entries for
examples.

- When declaring a pointer to a type, GNU coding style writes 'type
*ptr', not 'type* ptr'.

- In GDB, when traversing types, remember to call check_typedef to
avoid having your traversal stopped by typedef nodes.

Re: Variable objects: references formatting

[Vladimir Prus]

On Wednesday 03 May 2006 22:08, Jim Blandy wrote:
> On 5/3/06, Jim Blandy <jimb@red-bean.com> wrote:
> > On 5/3/06, Vladimir Prus <ghost@cs.msu.su> wrote:
> > > At the moment, when using variable objects to display a struct or a
> > > class, the result of -data-evaluate-expression is "...". However, when
> > > displaying a reference to a class, the result of
> > > -data-evaluate-expression is {}-enclosed list of members and their
> > > values.
> > >
> > > This disparity does not seem to be reasonable, the attached patch fixes
> > > it:
> >
> > I think you're right, and the patch looks good.  I'll apply it.
>
> Some further stuff I came across, for future reference:
>
> - ChangeLog entries need to have two spaces between the date and the
> name, and the name and the email address.  

Did you know about this rule, will use it in future.

> They need an asterisk 
> before the filename.  Check out the other ChangeLog entries for
> examples.

That's just a typo.

> - When declaring a pointer to a type, GNU coding style writes 'type
> *ptr', not 'type* ptr'.

Oh, sorry, C++ habits. Will try to avoid it.

> - In GDB, when traversing types, remember to call check_typedef to
> avoid having your traversal stopped by typedef nodes.

Thanks, noted. So, in this case I should have called check_typedef before 
checking if type is reference, right?

Would you like me to resend the patch adjusted per your comments, or you've 
already done those changes locally?

Thanks,
Volodya

Re: Variable objects: references formatting

[Jim Blandy]

On 5/3/06, Vladimir Prus <ghost@cs.msu.su> wrote:
> > - In GDB, when traversing types, remember to call check_typedef to
> > avoid having your traversal stopped by typedef nodes.
>
> Thanks, noted. So, in this case I should have called check_typedef before
> checking if type is reference, right?

Well, the get_type function is taking care of that one for you.  So
it's enough to check the new types you uncover with TYPE_TARGET_TYPE.

> Would you like me to resend the patch adjusted per your comments, or you've
> already done those changes locally?

I've already done them.  Here's the patch I actually committed (which
I should have posted when I did so):

gdb/ChangeLog:
2006-05-03  Vladimir Prus  <ghost@cs.msu.su>

       * varobj.c (c_value_of_variable): Ignore top-level references.
       (Committed by Jim Blandy.)

Index: gdb/varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.59
diff -u -p -r1.59 varobj.c
--- gdb/varobj.c	27 Mar 2006 00:15:22 -0000	1.59
+++ gdb/varobj.c	3 May 2006 18:07:43 -0000
@@ -2054,10 +2054,16 @@ c_variable_editable (struct varobj *var)
 static char *
 c_value_of_variable (struct varobj *var)
 {
-  /* BOGUS: if val_print sees a struct/class, it will print out its
-     children instead of "{...}" */
+  /* BOGUS: if val_print sees a struct/class, or a reference to one,
+     it will print out its children instead of "{...}".  So we need to
+     catch that case explicitly.  */
+  struct type *type = get_type (var);
+
+  /* Strip top-level references. */
+  while (TYPE_CODE (type) == TYPE_CODE_REF)
+    type = check_typedef (TYPE_TARGET_TYPE (type));

-  switch (TYPE_CODE (get_type (var)))
+  switch (TYPE_CODE (type))
     {
     case TYPE_CODE_STRUCT:
     case TYPE_CODE_UNION:

Re: Variable objects: references formatting

[Vladimir Prus]

On Thursday 04 May 2006 10:08, Jim Blandy wrote:
> On 5/3/06, Vladimir Prus <ghost@cs.msu.su> wrote:
> > > - In GDB, when traversing types, remember to call check_typedef to
> > > avoid having your traversal stopped by typedef nodes.
> >
> > Thanks, noted. So, in this case I should have called check_typedef before
> > checking if type is reference, right?
>
> Well, the get_type function is taking care of that one for you.  So
> it's enough to check the new types you uncover with TYPE_TARGET_TYPE.

Ok, I see.

>
> > Would you like me to resend the patch adjusted per your comments, or
> > you've already done those changes locally?
>
> I've already done them.  Here's the patch I actually committed (which
> I should have posted when I did so):

Thanks you!

- Volodya

Re: Variable objects: references formatting

[Nick Roberts]

> At the moment, when using variable objects to display a struct or a class,
> the result of -data-evaluate-expression is "...". However, when displaying
> a reference to a class, the result of -data-evaluate-expression is
> {}-enclosed list of members and their values.

> This disparity does not seem to be reasonable, the attached patch fixes it:

> Changelog:

>   2006-05-03 Vladimir Prus <ghost@cs.msu.su>
>       varobj.c (c_value_of_variable): Ignore top-level references.

> Patch attached.

> Thanks,
> Volodya

There are som many things about this patch that I don't understand:

> Index: varobj.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/varobj.c,v
> retrieving revision 1.58

Version 1.59 has been in the repository for over a month, so how come this
patch is against 1.58?

> diff -u -r1.58 varobj.c
> @@ -2055,8 +2219,14 @@

I'm not used to unified diffs, but as insertion appears to be done at the
same place why is it not something like:

@@ -2055,8 +2055,14 @@

>  {
>    /* BOGUS: if val_print sees a struct/class, it will print out its
>       children instead of "{...}" */
> +  struct type* type = get_type (var);
> +  /* Strip top-level references. */
> +  while (TYPE_CODE (type) == TYPE_CODE_REF)
> +    {
> +      type = TYPE_TARGET_TYPE (type);
> +    }
>  
> -  switch (TYPE_CODE (get_type (var)))
> +  switch (TYPE_CODE (type))
>      {
>      case TYPE_CODE_STRUCT:
>      case TYPE_CODE_UNION:


Most importantly, however, the preamble is about -data-evaluate-expression
but AFAICS this doesn't call c_value_of_variable.

I have tested the output of -data-evaluate-expression on pointers to typedeffed
structures and found that with the latter I get a {}-enclosed list of members
with gcc 3.2 and {...} with gcc 4.1.  More generally, I have found that gcc 4.1
treats typedefs differently, which leads to errors with variable objects.

So clearly I also don't understand how Jim can think that the patch looks
good and he'll apply it.


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

Re: Variable objects: references formatting

[Jim Blandy]

On 5/3/06, Nick Roberts <nickrob@snap.net.nz> wrote:
> There are som many things about this patch that I don't understand:

I may have made a mistake in reviewing, but please don't be
elliptical: just say what you think is wrong.

The patch is against an older version of varobj.c, and does seem to be
cut from a larger patch, but it applies, and given the surrounding
code in c_value_of_variable, and the behavior of c_val_print, the
general sense of the change seems correct.  Have you tried creating
varobjs for values that are references to structs and displaying them,
which I think is the case actually being addressed?

Re: Variable objects: references formatting

[Nick Roberts]

 > > There are som many things about this patch that I don't understand:
 > 
 > I may have made a mistake in reviewing, but please don't be
 > elliptical: just say what you think is wrong.

If I had understood what was wrong I would have said so.  As I didn't I
could only point out inconsistencies.

 > The patch is against an older version of varobj.c, and does seem to be
 > cut from a larger patch, but it applies, and given the surrounding
 > code in c_value_of_variable, and the behavior of c_val_print, the
 > general sense of the change seems correct.  Have you tried creating
 > varobjs for values that are references to structs and displaying them,
 > which I think is the case actually being addressed?

OK, if its a patch for -var-valuate-expression then perhaps the BOGUS
comment could be removed as it seems to address this issue.

I see a more serious error with references: they don't seem to disappear
from the changelist with -var-update:


-var-create - * q
^done,name="var1",numchild="1",type="tommy"
(gdb) 
-var-create - * rq
^done,name="var2",numchild="1",type="tommy &"
(gdb) 
-var-update * 
^done,changelist=[{name="var2",in_scope="true",type_changed="false"}]
(gdb) 
-var-update * 
^done,changelist=[{name="var2",in_scope="true",type_changed="false"}]
(gdb) 
-var-update * 
^done,changelist=[{name="var2",in_scope="true",type_changed="false"}]

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

Re: Variable objects: references formatting

[Jim Blandy]

On 5/3/06, Nick Roberts <nickrob@snap.net.nz> wrote:
>  > The patch is against an older version of varobj.c, and does seem to be
>  > cut from a larger patch, but it applies, and given the surrounding
>  > code in c_value_of_variable, and the behavior of c_val_print, the
>  > general sense of the change seems correct.  Have you tried creating
>  > varobjs for values that are references to structs and displaying them,
>  > which I think is the case actually being addressed?
>
> OK, if its a patch for -var-valuate-expression then perhaps the BOGUS
> comment could be removed as it seems to address this issue.

I read the "BOGUS" comment as referring to the fact that we have to
specially trap structs and unions, instead of simply passing
everything through common_val_print.  I don't think this patch affects
that "bogusness"; it just makes the workaround work better.

(Are there any cases where common_val_print (and thus c_val_print)
prints the "{...}" we need automatically for us?  Or is this code the
sole source of that behavior?)

Re: Variable objects: references formatting

[Nick Roberts]

 > > OK, if its a patch for -var-valuate-expression then perhaps the BOGUS
 > > comment could be removed as it seems to address this issue.
 > 
 > I read the "BOGUS" comment as referring to the fact that we have to
 > specially trap structs and unions, instead of simply passing
 > everything through common_val_print.  I don't think this patch affects
 > that "bogusness"; it just makes the workaround work better.

I was guessing.  I see that the check is for var->type, whereas perhaps
val_print prints out children if var->val->type is a struct/class.  But I don't
know the relationship between the two and you're the expert in these matters.

 > (Are there any cases where common_val_print (and thus c_val_print)
 > prints the "{...}" we need automatically for us?  Or is this code the
 > sole source of that behavior?)

I don't really know much about that end of things yet.  I've looked mainly
at the mi directory and variable objects.

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

Re: Variable objects: references formatting

[Jim Blandy]

Nick Roberts <nickrob@snap.net.nz> writes:
>  > > OK, if its a patch for -var-valuate-expression then perhaps the BOGUS
>  > > comment could be removed as it seems to address this issue.
>  > 
>  > I read the "BOGUS" comment as referring to the fact that we have to
>  > specially trap structs and unions, instead of simply passing
>  > everything through common_val_print.  I don't think this patch affects
>  > that "bogusness"; it just makes the workaround work better.
>
> I was guessing.  I see that the check is for var->type, whereas
> perhaps val_print prints out children if var->val->type is a
> struct/class.  But I don't know the relationship between the two and
> you're the expert in these matters.

Well, I don't feel very expert in MI; I appreciate having other eyes
look stuff over.

Re: Variable objects: references formatting

[Vladimir Prus]

On Thursday 04 May 2006 03:05, Nick Roberts wrote:

> There are som many things about this patch that I don't understand:
> > Index: varobj.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/varobj.c,v
> > retrieving revision 1.58
>
> Version 1.59 has been in the repository for over a month, so how come this
> patch is against 1.58?

I've at least 2 other changes to that file, and corresponding patches were 
neither applied nor rejected, AFAICT. I'd rather not update the file yet.

> > diff -u -r1.58 varobj.c
> > @@ -2055,8 +2219,14 @@
>
> I'm not used to unified diffs, but as insertion appears to be done at the
> same place why is it not something like:
>
> @@ -2055,8 +2055,14 @@

I'm sorry, I don't understand that question. This hunk was cut from a larger 
diff, maybe that explains something?

> >  {
> >    /* BOGUS: if val_print sees a struct/class, it will print out its
> >       children instead of "{...}" */
> > +  struct type* type = get_type (var);
> > +  /* Strip top-level references. */
> > +  while (TYPE_CODE (type) == TYPE_CODE_REF)
> > +    {
> > +      type = TYPE_TARGET_TYPE (type);
> > +    }
> >
> > -  switch (TYPE_CODE (get_type (var)))
> > +  switch (TYPE_CODE (type))
> >      {
> >      case TYPE_CODE_STRUCT:
> >      case TYPE_CODE_UNION:
>
> Most importantly, however, the preamble is about -data-evaluate-expression
> but AFAICS this doesn't call c_value_of_variable.

Sure it does. KDevelop uses -data-evaluate-expression to fetch values, and 
with this patch the value of "reference to structure" is rendered as "...", 
just like I'd want.

>
> I have tested the output of -data-evaluate-expression on pointers to
> typedeffed structures and found that with the latter I get a {}-enclosed
> list of members with gcc 3.2 and {...} with gcc 4.1.  More generally, I
> have found that gcc 4.1 treats typedefs differently, which leads to errors
> with variable objects.

How *pointers* to typedeffed structures are relevant to this patch? Now, maybe 
we need to call 'check_typedef' in one more place -- after stripping 
reference, to make sure typedefs to structures are also rendered as "...".

Is that what you're saying? And what errors do you see with gcc 4.1?

- Volodya

Re: Variable objects: references formatting

[Nick Roberts]

 > > Version 1.59 has been in the repository for over a month, so how come this
 > > patch is against 1.58?
 > 
 > I've at least 2 other changes to that file, and corresponding patches were 
 > neither applied nor rejected, AFAICT. I'd rather not update the file yet.

And I would think people on this mailing list would rather not work out
the patch relative to current CVS in order to apply.  I know it worked in
this case (after a shift) but it wouldn't in general.

 > > > diff -u -r1.58 varobj.c
 > > > @@ -2055,8 +2219,14 @@
 > >
 > > I'm not used to unified diffs, but as insertion appears to be done at the
 > > same place why is it not something like:
 > >
 > > @@ -2055,8 +2055,14 @@
 > 
 > I'm sorry, I don't understand that question. This hunk was cut from a larger 
 > diff, maybe that explains something?

Similarly, in general, the patch presumably won't apply properly.

 > > Most importantly, however, the preamble is about -data-evaluate-expression
 > > but AFAICS this doesn't call c_value_of_variable.
 > 
 > Sure it does. KDevelop uses -data-evaluate-expression to fetch values, and 
 > with this patch the value of "reference to structure" is rendered as "...", 
 > just like I'd want.

I could say "Oh know it doesn't!" but, since this is not a pantomime, could
you please give me a simple example of where it does call c_value_of_variable.
My loose reasoning is that the variable in "c_value_of_variable" refers to
variable object and -data-evaluate-expression doesn't use one.  What argument
do you give it?

 > > I have tested the output of -data-evaluate-expression on pointers to
 > > typedeffed structures and found that with the latter I get a {}-enclosed
 > > list of members with gcc 3.2 and {...} with gcc 4.1.  More generally, I
 > > have found that gcc 4.1 treats typedefs differently, which leads to errors
 > > with variable objects.
 > 
 > How *pointers* to typedeffed structures are relevant to this patch? Now, maybe 
 > we need to call 'check_typedef' in one more place -- after stripping 
 > reference, to make sure typedefs to structures are also rendered as "...".
 > 
 > Is that what you're saying? And what errors do you see with gcc 4.1?

No, I just didn't appreciate the difference between pointers and references in
GDB.  The discrepancy I "found" was due to me mistyping.  However I do see a
problem with gcc 4.1 and variable objects where GDB keeps telling me:

   Child of parent whose type does not allow children

when it didn't when my program was compiled with gcc 3.2.

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

Re: Variable objects: references formatting

[Vladimir Prus]

On Thursday 04 May 2006 10:22, Nick Roberts wrote:
>  > > Version 1.59 has been in the repository for over a month, so how come
>  > > this patch is against 1.58?
>  >
>  > I've at least 2 other changes to that file, and corresponding patches
>  > were neither applied nor rejected, AFAICT. I'd rather not update the
>  > file yet.
>
> And I would think people on this mailing list would rather not work out
> the patch relative to current CVS in order to apply.  I know it worked in
> this case (after a shift) but it wouldn't in general.

Well, if you say patches against the previous revision of a file cause 
problems, I'll try to send patches against most current version in future.

>  > > Most importantly, however, the preamble is about
>  > > -data-evaluate-expression but AFAICS this doesn't call
>  > > c_value_of_variable.
>  >
>  > Sure it does. KDevelop uses -data-evaluate-expression to fetch values,
>  > and with this patch the value of "reference to structure" is rendered as
>  > "...", just like I'd want.
>
> I could say "Oh know it doesn't!" but, since this is not a pantomime, could
> you please give me a simple example of where it does call
> c_value_of_variable. My loose reasoning is that the variable in
> "c_value_of_variable" refers to variable object and
> -data-evaluate-expression doesn't use one.  What argument do you give it?

It looks like small typo caused a lot of confusion. I meant 
-var-evaluate-expression, not -data-evaluate-expression. Sorry for confusing 
this.

>  > > I have tested the output of -data-evaluate-expression on pointers to
>  > > typedeffed structures and found that with the latter I get a
>  > > {}-enclosed list of members with gcc 3.2 and {...} with gcc 4.1.  More
>  > > generally, I have found that gcc 4.1 treats typedefs differently,
>  > > which leads to errors with variable objects.
>  >
>  > How *pointers* to typedeffed structures are relevant to this patch? Now,
>  > maybe we need to call 'check_typedef' in one more place -- after
>  > stripping reference, to make sure typedefs to structures are also
>  > rendered as "...".
>  >
>  > Is that what you're saying? And what errors do you see with gcc 4.1?
>
> No, I just didn't appreciate the difference between pointers and references
> in GDB.  The discrepancy I "found" was due to me mistyping.  However I do
> see a problem with gcc 4.1 and variable objects where GDB keeps telling me:
>
>    Child of parent whose type does not allow children
>
> when it didn't when my program was compiled with gcc 3.2.

Again, maybe you can provide specific case where this error is produced? If it 
affect real-world cases we'd  better fix it soon.

- Volodya

Re: Variable objects: references formatting

[Nick Roberts]

 > Well, if you say patches against the previous revision of a file cause 
 > problems, I'll try to send patches against most current version in future.

Thanks.

 ...
 > > No, I just didn't appreciate the difference between pointers and references
 > > in GDB.  The discrepancy I "found" was due to me mistyping.  However I do
 > > see a problem with gcc 4.1 and variable objects where GDB keeps telling me:
 > >
 > >    Child of parent whose type does not allow children
 > >
 > > when it didn't when my program was compiled with gcc 3.2.
 > 
 > Again, maybe you can provide specific case where this error is produced? If
 > it affect real-world cases we'd better fix it soon.

It happens when I debug Emacs but I can't provide a simple case yet.  
However, do you see the problem with references that I mentioned earlier
(that they don't seem to disappear from the changelist with -var-update)?
This seems to be the case for any variable object made from a reference.

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

Re: Variable objects: references formatting

[Vladimir Prus]

On Thursday 04 May 2006 11:20, Nick Roberts wrote:

>  > Again, maybe you can provide specific case where this error is produced?
>  > If it affect real-world cases we'd better fix it soon.
>
> It happens when I debug Emacs but I can't provide a simple case yet.
> However, do you see the problem with references that I mentioned earlier
> (that they don't seem to disappear from the changelist with -var-update)?
> This seems to be the case for any variable object made from a reference.

Yes, I see that for variable object created from reference, -var-update * 
always mentions that. Looking at this under debugger, it seems that the call 
to my_value_equal in varobj_update compares the value of *reference* to the 
value of new *referenced-to* object. This happens in my_value_equal 
(varobj.c):

  static int
  my_value_equal (struct value *val1, struct value *volatile val2, int
    *error2)
  {
    /* Make sure we also know the contents of VAL2.  */
    val2 = coerce_array (val2);

Before this call, val2 is the value of reference itself. After this call, it 
has value of referenced-to object. val1, on the other hand, is still the 
value of reference.

I'm not sure what's the point of that call is, and importantly, I'm not sure 
what would be the right behaviour. Variable object corresponding to reference 
can be reported as updated either when:

  (1) The referenced-to value changes
  (2) The value of reference changes itself
  (3) Both

I think that (1) is the right solution since in C++ reference can't change 
value, and change of reference value in gdb can be only when reference goes 
into scope or goes out of scope, so we're never interested in the value of 
reference itself. In fact, both Eclipse and KDevelop hide the reference value 
in its displays.

If (1) is what we need, there should be extra call to 'coerce_array' for 
'val1'. In fact, that call should probably be done when varobj is first 
created. Alas, quick attempt to do that results in segfault, and I'm out of 
time for today. Feel free to beat me to it ;-)


- Volodya