Re: Variable objects: references formatting

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

[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

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.

[PATCH] -var-update [was Re: Variable objects: references formatting]

[Nick Roberts]

 > > 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)
 > ...

 >   ...Alas, quick attempt to do that results in segfault, and I'm out of 
 > time for today. Feel free to beat me to it ;-)

I think this patch works.  My reasoning is one of symmetry: whatever is done to
val2 should also be done to val1, and that you probably don't want to change
the contents of val1 (hence val3).  I don't know exactly what coerce_array
does, apart from convert the type from TYPE_CODE_REF to TYPE_CODE_INT or
TYPE_CODE_FLOAT or whatever, so the comment might not be quite right.

AFAICS it introduces no new fails into the testsuite.  Then again there
are no tests for references, so I guess a new one would be appropriate


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



2006-05-09  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (my_value_equal): Make it work for references.


*** varobj.c	04 May 2006 22:11:38 +1200	1.60
--- varobj.c	09 May 2006 00:08:55 +1200	
***************
*** 1460,1465 ****
--- 1460,1466 ----
  my_value_equal (struct value *val1, struct value *volatile val2, int *error2)
  {
    volatile struct gdb_exception except;
+   struct value *val3;
  
    /* As a special case, if both are null, we say they're equal.  */
    if (val1 == NULL && val2 == NULL)
***************
*** 1470,1475 ****
--- 1471,1479 ----
    /* The contents of VAL1 are supposed to be known.  */
    gdb_assert (!value_lazy (val1));
  
+   /* Make sure we get the contents of VAL1.  */
+   val3 = coerce_array (val1);
+ 
    /* Make sure we also know the contents of VAL2.  */
    val2 = coerce_array (val2);
    TRY_CATCH (except, RETURN_MASK_ERROR)
***************
*** 1484,1490 ****
      }
    gdb_assert (!value_lazy (val2));
  
!   return value_contents_equal (val1, val2);
  }
  
  /* FIXME: The following should be generic for any pointer */
--- 1488,1494 ----
      }
    gdb_assert (!value_lazy (val2));
  
!   return value_contents_equal (val3, val2);
  }
  
  /* FIXME: The following should be generic for any pointer */

Re: [PATCH] -var-update [was Re: Variable objects: references formatting]

[Jim Blandy]

Nick Roberts <nickrob@snap.net.nz> writes:
>  > > 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)
>  > ...
>
>  >   ...Alas, quick attempt to do that results in segfault, and I'm out of 
>  > time for today. Feel free to beat me to it ;-)
>
> I think this patch works.  My reasoning is one of symmetry: whatever is done to
> val2 should also be done to val1, and that you probably don't want to change
> the contents of val1 (hence val3).  I don't know exactly what coerce_array
> does, apart from convert the type from TYPE_CODE_REF to TYPE_CODE_INT or
> TYPE_CODE_FLOAT or whatever, so the comment might not be quite
> right.

Is there any reason you declared a new variable instead of just saying:

   val1 = coerce_array (val1)

?  That would make the symmetry more apparent.

(I don't know enough about the context to review the patch more
substantially.)

Re: [PATCH] -var-update [was Re: Variable objects: references formatting]

[Nick Roberts]

 > > I think this patch works.  My reasoning is one of symmetry: whatever is
 > > done to val2 should also be done to val1, and that you probably don't want
 > > to change the contents of val1 (hence val3).  I don't know exactly what
 > > coerce_array does, apart from convert the type from TYPE_CODE_REF to
 > > TYPE_CODE_INT or TYPE_CODE_FLOAT or whatever, so the comment might not be
 > > quite right.
 > 
 > Is there any reason you declared a new variable instead of just saying:
 >
 >    val1 = coerce_array (val1)
 > 
 > ?  That would make the symmetry more apparent.

I thought I had tried and it had failed.  Of course, now you suggest it, it
works ;-).  I had thought that maybe coerce_array kept changing val1, as it was
done for each update, but indeed it looks like the first call finds the
underlying type and further calls do nothing.

 > (I don't know enough about the context to review the patch more
 > substantially.)

I'm in no hurry for this one.  I can re-submit later when I have more
confidence that it's right, if no-one can endorse it for me.


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

Re: [PATCH] -var-update [was Re: Variable objects: references formatting]

[Daniel Jacobowitz]

On Tue, May 09, 2006 at 12:40:43AM +1200, Nick Roberts wrote:
> I think this patch works.  My reasoning is one of symmetry: whatever is done to
> val2 should also be done to val1, and that you probably don't want to change
> the contents of val1 (hence val3).  I don't know exactly what coerce_array
> does, apart from convert the type from TYPE_CODE_REF to TYPE_CODE_INT or
> TYPE_CODE_FLOAT or whatever, so the comment might not be quite right.

I don't think this is in the right place: you're using an argument of
symmetry, but in fact, the comments in my_value_equal suggest that
symmetry is inappropriate.  For instance:

  /* The contents of VAL1 are supposed to be known.  */
  gdb_assert (!value_lazy (val1));

If val1 is the reference at this point, then we haven't checked what we
think we have.

Every time my_value_equal is called its first argument comes from a
varobj's ->value.  It seems to me that if we want to properly know
whether the varobj has changed, we'd better have read its value into
GDB.

I spent a little while looking around and the right places to fix
weren't entirely obvious, but I am tentatively thinking after the
gdb_evaluate_expression call in varobj_create and before the assignment
to ->value in varobj_update.  But there might be more; really you'd
have to think about each time the address of ->value is taken or
->value is directly assigned to (basically grep for each ->value and
look at the context).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update [was Re: Variable objects: references formatting]

[Nick Roberts]

 > > I think this patch works.  My reasoning is one of symmetry: whatever is
 > > done to val2 should also be done to val1, and that you probably don't want
 > > to change the contents of val1 (hence val3).  I don't know exactly what
 > > coerce_array does, apart from convert the type from TYPE_CODE_REF to
 > > TYPE_CODE_INT or TYPE_CODE_FLOAT or whatever, so the comment might not be
 > > quite right.
 > 
 > I don't think this is in the right place: you're using an argument of
 > symmetry, but in fact, the comments in my_value_equal suggest that
 > symmetry is inappropriate.  

Also because, its a safe one, particularly if a dummy value variable is used,
because nothing gets changed outhside my_value_equal.

Anyway I now see its the wrong change, it just detects when the reference is
assigned an address, not when the value at that address changes.

 >                             For instance:
 > 
 >   /* The contents of VAL1 are supposed to be known.  */
 >   gdb_assert (!value_lazy (val1));
 > 
 > If val1 is the reference at this point, then we haven't checked what we
 > think we have.

Trying to interpret the code, given that val1 doesn't hold the value I
wonder why this assertion is true i.e why is value->lazy=0 even though
the value isn't in the contents field.

 > Every time my_value_equal is called its first argument comes from a
 > varobj's ->value.  It seems to me that if we want to properly know
 > whether the varobj has changed, we'd better have read its value into
 > GDB.

Could it not doing that because GDB's value mechanism isn't working properly
for references?

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

Re: [PATCH] -var-update [was Re: Variable objects: references formatting]

[Daniel Jacobowitz]

On Wed, May 17, 2006 at 11:52:31AM +1200, Nick Roberts wrote:
>  >                             For instance:
>  > 
>  >   /* The contents of VAL1 are supposed to be known.  */
>  >   gdb_assert (!value_lazy (val1));
>  > 
>  > If val1 is the reference at this point, then we haven't checked what we
>  > think we have.
> 
> Trying to interpret the code, given that val1 doesn't hold the value I
> wonder why this assertion is true i.e why is value->lazy=0 even though
> the value isn't in the contents field.

I am not sure what you mean.

!value_lazy (val1) means the contents of val1 have already been read
from the target.  At this point, val1 is the _reference_.  Nothing is
arranging to cache the value pointed to; that's what needs to happen.

>  > Every time my_value_equal is called its first argument comes from a
>  > varobj's ->value.  It seems to me that if we want to properly know
>  > whether the varobj has changed, we'd better have read its value into
>  > GDB.
> 
> Could it not doing that because GDB's value mechanism isn't working properly
> for references?

The value mechanism is working fine; it's varobj that does not support
references.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update [was Re: Variable objects: references formatting]

[Nick Roberts]

 > >  > Every time my_value_equal is called its first argument comes from a
 > >  > varobj's ->value.  It seems to me that if we want to properly know
 > >  > whether the varobj has changed, we'd better have read its value into
 > >  > GDB.
 > > 
 > > Could it not doing that because GDB's value mechanism isn't working
 > > properly for references?
 > 
 > The value mechanism is working fine; it's varobj that does not support
 > references.

Well GDB appears to interpret the address as being the value, its actual value
doesn't seem to be stored in the value structure (or if it is, I can't find
it).

When the actual value gets printed it seems to come out of smething like
print_address_demangle.

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

Re: [PATCH] -var-update [was Re: Variable objects: references formatting]

[Daniel Jacobowitz]

On Thu, May 18, 2006 at 01:28:05PM +1200, Nick Roberts wrote:
> Well GDB appears to interpret the address as being the value, its actual value
> doesn't seem to be stored in the value structure (or if it is, I can't find
> it).

The contents are a _different_ value, that's the point - one without a
reference type.  coerce_ref converts the reference to its value.

I do not quite understand why coerce_array is used; I suspect the
contents of an array are checked via the children instead of the
parent.  coerce_array happens to also call coerce_ref.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update

[Nick Roberts]

 > I do not quite understand why coerce_array is used; I suspect the
 > contents of an array are checked via the children instead of the
 > parent.  coerce_array happens to also call coerce_ref.

Maybe it's historical, as varobj_update used value_equal previously and this
uses coerce_array.

Anyway here's a patch that actually seems to work.  I've taken code from
c_val_print.  Maybe there should be more checks and I've not tested
varobj_set_value (-var-assign) yet, but I thought I'd sound out the genaral
approcah first.

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


*** varobj.c	19 May 2006 11:37:28 +1200	1.60
--- varobj.c	19 May 2006 16:27:29 +1200	
*************** varobj_create (char *objname,
*** 517,522 ****
--- 517,532 ----
  
        var->type = value_type (var->value);
  
+       if (TYPE_CODE (var->type) == TYPE_CODE_REF)
+ 	{
+ 	  var->value = value_at (TYPE_TARGET_TYPE (var->type),
+ 				 unpack_pointer
+ 				 (lookup_pointer_type (builtin_type_void),
+ 				  value_contents_all (var->value)
+ 				  + value_embedded_offset (var->value)));
+ 	  release_value (var->value);
+ 	}
+ 
        /* Set language info */
        lang = variable_language (var);
        var->root->lang = languages[lang];
*************** varobj_set_value (struct varobj *var, ch
*** 825,830 ****
--- 835,847 ----
  	  return 0;
  	}
  
+       if (TYPE_CODE (var->type) == TYPE_CODE_REF)
+ 	value = value_at (TYPE_TARGET_TYPE (var->type),
+ 			  unpack_pointer
+ 			  (lookup_pointer_type (builtin_type_void),
+ 			   value_contents_all (value)
+ 			   + value_embedded_offset (value)));
+ 
        if (!my_value_equal (var->value, value, &error))
          var->updated = 1;
        if (!gdb_value_assign (var->value, value, &val))
*************** c_value_of_root (struct varobj **var_han
*** 1915,1920 ****
--- 1932,1944 ----
        else
  	var->error = 1;
  
+       if (TYPE_CODE (var->type) == TYPE_CODE_REF)
+ 	new_val = value_at (TYPE_TARGET_TYPE (var->type),
+ 			    unpack_pointer
+ 			    (lookup_pointer_type (builtin_type_void),
+ 			     value_contents_all (new_val)
+ 			     + value_embedded_offset (new_val)));
+ 
        release_value (new_val);
        return new_val;
      }

Re: [PATCH] -var-update

[Vladimir Prus]

Nick Roberts wrote:

>  > I do not quite understand why coerce_array is used; I suspect the
>  > contents of an array are checked via the children instead of the
>  > parent.  coerce_array happens to also call coerce_ref.
> 
> Maybe it's historical, as varobj_update used value_equal previously and
> this uses coerce_array.
> 
> Anyway here's a patch that actually seems to work.  I've taken code from
> c_val_print.  Maybe there should be more checks and I've not tested
> varobj_set_value (-var-assign) yet, but I thought I'd sound out the
> genaral approcah first.

I think that in any case, you should add a function and call it, instead of
adding three identical code fragments. BTW, why coerce_ref is not suitable
here?

- Volodya


 

Re: [PATCH] -var-update

[Nick Roberts]

 >  > Try calling coerce_ref in here:
 >  > 
 >  >       if (gdb_evaluate_expression (var->root->exp, &var->value))
 >  >         {
 >  >           /* no error */
 >  > 
 >  > /* HERE */
 >  > 
 >  >           release_value (var->value);
 >  >           if (value_lazy (var->value))
 >  >             gdb_value_fetch_lazy (var->value);
 >  >         }
 >  >       else
 >  >         var->value = evaluate_type (var->root->exp);
 > 
 > I've tried to follow this suggestion in the patch below,...

Actually I don't think this can be quite right because if I have:

int i;
int& ri = i;

and I do:
-var-create - * ri

I get:
^done,name="var1",numchild="0",type="int"

when I really should have:
^done,name="var1",numchild="0",type="int &"

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

Re: [PATCH] -var-update

[Nick Roberts]

 > If you take a look at the code you're patching, there should be a
 > nearby call to value_fetch_lazy or gdb_value_fetch_lazy in each case.
 > You want to be calling coerce_ref before you do that.

OK.  Perhaps I called it after value_fetch_lazy.  

 > Try calling coerce_ref in here:
 > 
 >       if (gdb_evaluate_expression (var->root->exp, &var->value))
 >         {
 >           /* no error */
 > 
 > /* HERE */
 > 
 >           release_value (var->value);
 >           if (value_lazy (var->value))
 >             gdb_value_fetch_lazy (var->value);
 >         }
 >       else
 >         var->value = evaluate_type (var->root->exp);

I've tried to follow this suggestion in the patch below, which acts slightly
differently to the previous patch.  Itjust returns the actual value in the
value field instead of the address and value (which is what -stack-list-locals
does).  This is because the previously execution went through case
TYPE_CODE_REF in c-val-print, while now it goes through TYPE_CODE_INT say (for
a variable referencing an integer) because of the call to coerce_ref.  I'm
not sure which is preferable.

Now the change occurs earlier in varobj_create, I'm not sure what happens when
gdb_evaluate_expression returns 0 (or why it would return 0).

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


*** varobj.c	19 May 2006 11:37:28 +1200	1.60
--- varobj.c	21 May 2006 16:06:23 +1200	
*************** varobj_create (char *objname,
*** 502,513 ****
  	  select_frame (fi);
  	}
  
!       /* We definitively need to catch errors here.
           If evaluate_expression succeeds we got the value we wanted.
           But if it fails, we still go on with a call to evaluate_type()  */
        if (gdb_evaluate_expression (var->root->exp, &var->value))
  	{
  	  /* no error */
  	  release_value (var->value);
  	  if (value_lazy (var->value))
  	    gdb_value_fetch_lazy (var->value);
--- 502,514 ----
  	  select_frame (fi);
  	}
  
!       /* We definitely need to catch errors here.
           If evaluate_expression succeeds we got the value we wanted.
           But if it fails, we still go on with a call to evaluate_type()  */
        if (gdb_evaluate_expression (var->root->exp, &var->value))
  	{
  	  /* no error */
+ 	  var->value = coerce_ref (var->value);
  	  release_value (var->value);
  	  if (value_lazy (var->value))
  	    gdb_value_fetch_lazy (var->value);
*************** varobj_set_value (struct varobj *var, ch
*** 820,825 ****
--- 821,830 ----
        exp = parse_exp_1 (&s, 0, 0);
        if (!gdb_evaluate_expression (exp, &value))
  	{
+ 	  value = coerce_ref (value);
+ 	  if (value_lazy (value))
+ 	    gdb_value_fetch_lazy (value);
+ 
  	  /* We cannot proceed without a valid expression. */
  	  xfree (exp);
  	  return 0;
*************** c_value_of_root (struct varobj **var_han
*** 1898,1903 ****
--- 1903,1909 ----
           go on */
        if (gdb_evaluate_expression (var->root->exp, &new_val))
  	{
+ 	  new_val = coerce_ref (new_val);
  	  if (value_lazy (new_val))
  	    {
  	      /* We need to catch errors because if

Re: [PATCH] -var-update

[Daniel Jacobowitz]

On Sun, May 21, 2006 at 10:25:24AM +1200, Nick Roberts wrote:
> Yes.  Perhaps my summary isn't precise, but it didn't work.  My patch uses
> value_at while coerce_ref uses value_at_lazy.  The comment says:
> 
>    Call value_at only if the data needs to be fetched immediately;
>    if we can be 'lazy' and defer the fetch, perhaps indefinately, call
>                                                     ^^^^^^^^^^^^
>    value_at_lazy instead.  value_at_lazy simply records the address of
>    the data and sets the lazy-evaluation-required flag.  The lazy flag
>    is tested in the value_contents macro, which is used if and when
>    the contents are actually required.
> 
> value_contents is not a macro (VALUE_CONTENTS used to be one) but a function,
> and doesn't test the lazy flag.

No:

const gdb_byte *
value_contents (struct value *value)
{ 
  return value_contents_writeable (value);
}

gdb_byte *
value_contents_writeable (struct value *value)
{ 
  if (value->lazy)
    value_fetch_lazy (value);
  return value_contents_raw (value);
}

If you take a look at the code you're patching, there should be a
nearby call to value_fetch_lazy or gdb_value_fetch_lazy in each case.
You want to be calling coerce_ref before you do that.  Also, see the
existing calls to release_value?  If you change var->value after that,
you're going to leak memory.

Try calling coerce_ref in here:

      if (gdb_evaluate_expression (var->root->exp, &var->value))
        {
          /* no error */

/* HERE */

          release_value (var->value);
          if (value_lazy (var->value))
            gdb_value_fetch_lazy (var->value);
        }
      else
        var->value = evaluate_type (var->root->exp);


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update

[Nick Roberts]

 > > >                                         BTW, why coerce_ref is not
 > > > suitable here?
 > > 
 > > coerce_ref ensures that the address is placed in the value's contents, not
 > > the actual value which is being referred to.
 > 
 > Did you try that?  That certainly shouldn't be true!

Yes.  Perhaps my summary isn't precise, but it didn't work.  My patch uses
value_at while coerce_ref uses value_at_lazy.  The comment says:

   Call value_at only if the data needs to be fetched immediately;
   if we can be 'lazy' and defer the fetch, perhaps indefinately, call
                                                    ^^^^^^^^^^^^
   value_at_lazy instead.  value_at_lazy simply records the address of
   the data and sets the lazy-evaluation-required flag.  The lazy flag
   is tested in the value_contents macro, which is used if and when
   the contents are actually required.

value_contents is not a macro (VALUE_CONTENTS used to be one) but a function,
and doesn't test the lazy flag.  So the comment is out of date, but perhaps
the `data' (I don't know what that means exactly) is never fetched when
-var-update is issued.

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

Re: [PATCH] -var-update

[Daniel Jacobowitz]

On Sat, May 20, 2006 at 04:23:04PM +1200, Nick Roberts wrote:
> >                                         BTW, why coerce_ref is not suitable
> > here?
> 
> coerce_ref ensures that the address is placed in the value's contents, not
> the actual value which is being referred to.

Did you try that?  That certainly shouldn't be true!

struct value *
coerce_ref (struct value *arg)
{
  struct type *value_type_arg_tmp = check_typedef (value_type (arg));
  if (TYPE_CODE (value_type_arg_tmp) == TYPE_CODE_REF)
    arg = value_at_lazy (TYPE_TARGET_TYPE (value_type_arg_tmp),
                         unpack_pointer (value_type (arg),              
                                         value_contents (arg)));
  return arg;
}


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update

[Nick Roberts]

> > Anyway here's a patch that actually seems to work.  I've taken code from
> > c_val_print.  Maybe there should be more checks and I've not tested
> > varobj_set_value (-var-assign) yet, but I thought I'd sound out the
> > genaral approcah first.

> I think that in any case, you should add a function and call it, instead of
> adding three identical code fragments.

It's just for discussion, not a finalised patch for approval.

>                                         BTW, why coerce_ref is not suitable
> here?

coerce_ref ensures that the address is placed in the value's contents, not
the actual value which is being referred to.

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