Re: Variable objects laziness

Re: Variable objects laziness

[Nick Roberts]

I'm having trouble to understand this patch.  If it does more than one thing
perhaps you can break it into two patches.

	* varobj.c (struct varobj): Clarify comment.
	(my_value_equal): Remove.
	(install_new_value): New.

New function presumably.

	(type_of_child): Remove.
	(varobj_create): Use install_new_value.
	(varobj_set_value): Use value_contents_equal, not
	my_value_equal.

Previously someone (Mark Kettenis?) has gone to a lot of trouble to replace
value_contents_equal with my_value_equal why do you think it's not needed?

	(varobj_update): Use install_new_value.
	(create_child): Likewise. Inline type_of_child here.
	(value_of_child): Don't fetch the value.
	(c_value_of_root): Likewise.
	(c_value_of_variable): Likewise.

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.60
diff -u -p -r1.60 varobj.c
--- varobj.c	3 May 2006 22:59:38 -0000	1.60
+++ varobj.c	14 Nov 2006 13:38:35 -0000
@@ -101,7 +101,9 @@ struct varobj
   /* The type of this variable. This may NEVER be NULL. */
   struct type *type;
 
-  /* The value of this expression or subexpression.  This may be NULL. */
+  /* The value of this expression or subexpression.  This may be NULL. 
+     Invariant: if type_changeable (this) is non-zero, the value is either
+     NULL, or not lazy.  */


I don't understand the replacement comment

...

+/** Assign new value to a variable object.  If INITIAL is non-zero,
+    this is first assignement after the variable object was just
+    created, or changed type.  In that case, just assign the value 
+    and return 0.
+    Otherwise, assign the value and if type_changeable returns non-zero,
+    find if the new value is different from the current value.
+    Return 1 if so, and 0 is the values are equal.  */
+static int
+install_new_value (struct varobj *var, struct value *value, int initial)
+{ 

I don't understand this comment either.  INITIAL can be type_changed
i.e not really initial.  Can you give it more structure and not refer to
internals like type_changeable?

...

+  if (CPLUS_FAKE_CHILD (var))
+    changeable = 0;
+  else
+    changeable = type_changeable (var);

type_changeable returns 0 if CPLUS_FAKE_CHILD (var) is true anyway so do you
need this clause?

As a whole the patch seems to lack clarity (although that might partly be a
reflection on my abilities!)

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

Re: Variable objects laziness

[Vladimir Prus]

Nick Roberts wrote:

> Index: varobj.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/varobj.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 varobj.c
> --- varobj.c  3 May 2006 22:59:38 -0000       1.60
> +++ varobj.c  14 Nov 2006 13:38:35 -0000
> @@ -101,7 +101,9 @@ struct varobj
>    /* The type of this variable. This may NEVER be NULL. */
>    struct type *type;
>  
> -  /* The value of this expression or subexpression.  This may be NULL. */
> +  /* The value of this expression or subexpression.  This may be NULL.
> +     Invariant: if type_changeable (this) is non-zero, the value is
> either
> +     NULL, or not lazy.  */
> 
> 
> I don't understand the replacement comment

Do you don't understand the comment proper, or high-level meaning of it? The
comment itself says that after each varobj-related function exits, if
type_changeable for some varobj returns true, the the varobj must be either
NULL or non-lazy.

The higher-level implication is that type_changeable indicates if the values
of this varobjs will be compared by -var-update. If it will be compared,
then the values may not be lazy. Otherwise, after we've made the step and
call -var-update, we don't have the old value in gdb's memory, and it might
have changed in target's memory, so we've lost the old value.

Does this clarifies things?

> 
> ...
> 
> +/** Assign new value to a variable object.  If INITIAL is non-zero,
> +    this is first assignement after the variable object was just
> +    created, or changed type.  In that case, just assign the value
> +    and return 0.
> +    Otherwise, assign the value and if type_changeable returns non-zero,
> +    find if the new value is different from the current value.
> +    Return 1 if so, and 0 is the values are equal.  */
> +static int
> +install_new_value (struct varobj *var, struct value *value, int initial)
> +{
> 
> I don't understand this comment either.  INITIAL can be type_changed
> i.e not really initial.  

Sorry, I don't understand. Do you refer to the fact that somewhere else, a
variable 'type_changed' is passed as INITIAL parameter? That's correct,
because if the type changed, gdb basically recreated varobj afresh.
Assignment to that recreated varobj is indeed initial assignment -- you
should not compare new value with any old value.

> Can you give it more structure and not refer to 
> internals like type_changeable?

Why do you call type_changeable "internals"? Just like install_new_value,
it's static function in varobj.c -- it's no more internal than
install_new_value itself. It is important aspect of install_new value that
it consults type_changeable. To maintain invariant of struct varobj,
install_new_value *must* check type_changeable.

> ...
> 
> +  if (CPLUS_FAKE_CHILD (var))
> +    changeable = 0;
> +  else
> +    changeable = type_changeable (var);
> 
> type_changeable returns 0 if CPLUS_FAKE_CHILD (var) is true anyway so do
> you need this clause?

Ah, right. Changed.

> As a whole the patch seems to lack clarity (although that might partly be
> a reflection on my abilities!)

Maybe I can try explaining it from a different angle? The -var-update
command must compare old values and new values for some variable objects --
namely those for which type_changeable returns true. In order to do this,
old value of such variable objects must not be lazy. At the same time,
there's no point to fetch values of other variable objects. So, this leads
to invariant I've documented.

Present code has a number of calls to value_fetch_lazy. It's troublesome to
even examine all those places. So, there's new "install_new_value" function
that's the only place where new value of varobj is assigned, and the only
place where value_fetch_lazy is called. That function also takes care about
maintaining invariant.

> (type_of_child): Remove.
> (varobj_create): Use install_new_value.
> (varobj_set_value): Use value_contents_equal, not
> my_value_equal.
> 
> Previously someone (Mark Kettenis?) has gone to a lot of trouble to
> replace value_contents_equal with my_value_equal why do you think it's not
> needed?

Because now install_new_value makes sure that var->value is not lazy when it
needs be not lazy. At the same time, install_new_value is handling making
new value non-lazy. So essentially, it's guaranteed that both arguments to
value_contents_equal are not lazy.

> * varobj.c (struct varobj): Clarify comment.
> (my_value_equal): Remove.
> (install_new_value): New.
> 
> New function presumably.

You mean, "New function." as comment in ChangeLog? OK.

> I'm having trouble to understand this patch.  If it does more than one
> thing perhaps you can break it into two patches.

I don't think I see any reasonable breakdown. This patch at the same time
introduces new invariant, new function that keeps it, and makes all code
use the new function. Breaking this apart will mean you have new function
that is not used everywhere and so basically useless.

- Volodya

Re: Variable objects laziness

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Sat, Nov 18, 2006 at 12:48:05PM +0300, Vladimir Prus wrote:
>> Daniel Jacobowitz wrote:
>> 
>> >> +     rather value_contents, will take care of this.
>> >> +     It might throw, but unlike var-update for -var-assign
>> >> +     there's just one variable we're working it, so we don't
>> >> +     need to catch the exception here.  */
>> > 
>> > Wait, what?  gdb_value_assign will never throw.  value_contents
>> > might, but gdb_value_assign will catch it.
>> 
>> Yep, confused gdb_value_assign with value_assign.
>> 
>> Updated patch attached. I also attach the delta to the previous patch.
> 
> This version is OK now (for HEAD only).  Thanks for your patience.
> The frozen varobj patch had some further discussion; I'll have to
> get back to it, but I'm afraid not today.  Poke me if I haven't done it
> in a few days, please.

*POKE*.

- Volodya

Re: Variable objects laziness

[Nick Roberts]

 > FWIW, I do think this counted as an obvious fix, but it's near the
 > border indeed.

I guess it depends what the obvious applies to: it wan't obvious to me _how_ it
should be fixed, but it was obvious to me that it _needed_ to be fixed.

It was a _sensible_ fix, particularly as the next release is so far away, and
seeking approval for each iteration would be tedious.  Perhaps the obvious fix
rule could be extended to bug fixes to one's own recently approved patches.

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

Re: Variable objects laziness

[Vladimir Prus]

On Wednesday 29 November 2006 16:44, Daniel Jacobowitz wrote:
> FWIW, I do think this counted as an obvious fix, but it's near the
> border indeed.  And, Nick is right; Vladimir, please do add yourself
> to MAINTAINERS as write after approval.

Done.

> On Wed, Nov 29, 2006 at 10:25:11AM +0300, Vladimir Prus wrote:
> > > I think a further call to coerce_array is needed
> >
> > No, please no! Calls to coerce_array is exactly the reason for the other
> > bug I'm fixing. This function has a nice property of silently
> > coercing_refs, but that property is not documented, not obvious from
> > function name and therefore should be considered a bug.
>
> Let's please not change it though.  Too much of GDB expects the current
> behavior...

I know.

> > Attached (references.diff) is the patch that makes gdb sense the changes
> > in reference values, and eliminates the address from the output. Any
> > opinions?
>
> IMVHO, we should still print the value, 

You meant address?

> but only update if the contents 
> change; is that going to be a real pain to implement?

Well, this might end up tricky, so I'd rather take "V" in IMVHO to mean that 
the patch is ok even without this change.

- Volodya

Re: Variable objects laziness

[Daniel Jacobowitz]

FWIW, I do think this counted as an obvious fix, but it's near the
border indeed.  And, Nick is right; Vladimir, please do add yourself
to MAINTAINERS as write after approval.

On Wed, Nov 29, 2006 at 10:25:11AM +0300, Vladimir Prus wrote:
> > I think a further call to coerce_array is needed 
> 
> No, please no! Calls to coerce_array is exactly the reason for the other bug
> I'm fixing. This function has a nice property of silently coercing_refs,
> but that property is not documented, not obvious from function name and
> therefore should be considered a bug.

Let's please not change it though.  Too much of GDB expects the current
behavior...

> Attached (references.diff) is the patch that makes gdb sense the changes in
> reference values, and eliminates the address from the output. Any opinions?

IMVHO, we should still print the value, but only update if the contents
change; is that going to be a real pain to implement?
> +      /* If the value has changed, record it, so that next -var-update can
> +	 report this change.  If a variable had a value of '1', we've set it
> +	 to '333' and then set again to '1', when -var-update will report this

"then" rather than "when".  Otherwise patch is fine.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Variable objects laziness

[Vladimir Prus]

On Wednesday 29 November 2006 11:55, Nick Roberts wrote:
> > > With the new changes to varobj.c, -var-assign doesn't work for
> > > references.
> >
> > This is embarrassing. However, it also validates my claim that we should
> > a single invariant-preserving function to assign new value. This crash
> > happens, for all appearances, because varobj_set_value directly sets new
> > value.
> >
> > I've checked in the attached, that fixes the crash, and causes no
> > regressions.
>
> I find this way works well but it's not how things are done here.  You need
> to post the patch first and get approval from an appropriate maintainer
> _before_ committing it (I don't think your change counts as an obvious
> fix).  See the MAINTAINERS file.  I'm assuming that you have Write After
> Approval (clearly you have write access) but AFAICS you've not added your 
> name to MAINTAINERS.

I apologise if I've bypassed the procedures. I this case, however, this was a 
rather serious regression caused by immediately preceding patch of mine. 

>
> Anyway the patch does indeed seem to do what you say.  Thanks.
>
> > Attached (references.diff) is the patch that makes gdb sense the changes
> > in reference values, and eliminates the address from the output. Any
> > opinions?
>
> Doesn't appear to be attached but I'm only reading the archives.  

I'll post it separately in a second -- with a testcase.

> If you 
> reply to an e-mail from me on gdb-patches could you please include me as
> I'm not subscribed to the mailing list (I'd rather receive two than none
> anyway). I think that's general accepted protocol.

Alas, I'm also not subscribed and reading the list via gmane.org and my NNTP 
reader does not provide a way to reply both to the list and to the author.
Is there any way you can try gmane.org?

- Volodya

Re: Variable objects laziness

[Nick Roberts]

> > With the new changes to varobj.c, -var-assign doesn't work for references.

> This is embarrassing. However, it also validates my claim that we should a
> single invariant-preserving function to assign new value. This crash
> happens, for all appearances, because varobj_set_value directly sets new
> value.

> I've checked in the attached, that fixes the crash, and causes no
> regressions.

I find this way works well but it's not how things are done here.  You need to
post the patch first and get approval from an appropriate maintainer _before_
committing it (I don't think your change counts as an obvious fix).  See the
MAINTAINERS file.  I'm assuming that you have Write After Approval (clearly you
have write access) but AFAICS you've not added your name to MAINTAINERS.

Anyway the patch does indeed seem to do what you say.  Thanks.

> Attached (references.diff) is the patch that makes gdb sense the changes in
> reference values, and eliminates the address from the output. Any opinions?

Doesn't appear to be attached but I'm only reading the archives.  If you
reply to an e-mail from me on gdb-patches could you please include me as I'm
not subscribed to the mailing list (I'd rather receive two than none anyway).
I think that's general accepted protocol.

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

Re: Variable objects laziness

[Vladimir Prus]

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

Nick Roberts wrote:

> 
> With the new changes to varobj.c, -var-assign doesn't work for references.

This is embarrassing. However, it also validates my claim that we should a
single invariant-preserving function to assign new value. This crash
happens, for all appearances, because varobj_set_value directly sets new
value.

I've checked in the attached, that fixes the crash, and causes no
regressions.

I did not add no testcase for this. I've run into some grave
reference-related bug reported by KDevelop user (not related to my patch)
and is going to fix that, and then add a new test for C++ features.

> Also -var-update only reports a change when the address changes and not
> the
> value.  Variable objects were broken before but in a different way
> (-var-assign worked but -var-update always reported that the reference
> value had changed).

Apparently, not reporting a change is worse

> 
> I think a further call to coerce_array is needed 

No, please no! Calls to coerce_array is exactly the reason for the other bug
I'm fixing. This function has a nice property of silently coercing_refs,
but that property is not documented, not obvious from function name and
therefore should be considered a bug.

Here, call to coerce_ref would work.

> and the output to 
> -var-update should look like:
> 
> 
> -var-update --all-values var1
> ^done,changelist=[{name="var1",value="4",in_scope="true",type_chan
> ged="false"}]
> 
> i.e the address shouldn't appear in the value field.

Attached (references.diff) is the patch that makes gdb sense the changes in
reference values, and eliminates the address from the output. Any opinions?


- Volodya

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

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.7995
diff -u -p -r1.7995 ChangeLog
--- ChangeLog	28 Nov 2006 22:21:23 -0000	1.7995
+++ ChangeLog	29 Nov 2006 06:40:51 -0000
@@ -1,3 +1,9 @@
+2006-11-29  Vladimir Prus  <vladimir@codesourcery.com>
+
+	* varobj.c (varobj_set_value): Don't compare the old
+	and the new value here.  Don't assign new value here.
+	Instead, call install_new_value.
+
 2006-11-28  Daniel Jacobowitz  <dan@codesourcery.com>
 
 	* regformats/reg-mips64.dat: New file.
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.61
diff -u -p -r1.61 varobj.c
--- varobj.c	28 Nov 2006 17:23:09 -0000	1.61
+++ varobj.c	29 Nov 2006 06:40:52 -0000
@@ -841,18 +841,22 @@ varobj_set_value (struct varobj *var, ch
 	 array's content.  */
       value = coerce_array (value);
 
-      if (!value_contents_equal (var->value, value))
-        var->updated = 1;
-
       /* The new value may be lazy.  gdb_value_assign, or 
 	 rather value_contents, will take care of this.
 	 If fetching of the new value will fail, gdb_value_assign
 	 with catch the exception.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
-      value_free (var->value);
+
       release_value (val);
-      var->value = val;
+      
+      /* If the value has changed, record it, so that next -var-update can
+	 report this change.  If a variable had a value of '1', we've set it
+	 to '333' and then set again to '1', when -var-update will report this
+	 variable as changed -- because the first assignment has set the
+	 'updated' flag.  There's no need to optimize that, because return value
+	 of -var-update should be considered an approximation.  */
+      var->updated = install_new_value (var, val, 0 /* Compare values. */);
       input_radix = saved_input_radix;
       return 1;
     }

Re: Variable objects laziness

[Nick Roberts]

 > > Also -var-update only reports a change when the address changes and not
 > > the value.  Variable objects were broken before but in a different way
 > > (-var-assign worked but -var-update always reported that the reference
 > > value had changed).
 > 
 > Just so we're all on the same page, could you post a compilable test
 > case?  I don't need a dejagnu-ified one, just some C++.

Any C++ program with references (I've never used them in earnest) would do e.g


1:  main()
2:  {
3:    int x = 4;
4:    int& rx = x;
5:    x = 7;
6:  }

1) Do -var-create * - rx before line 4.

2) -var-update --all-values after line 4 picks up the change of address.

3) -var-update --all-values after line 5 doesn't pick up the change of value.

4) -var-assign var1 generates an internal error.

 > > I think a further call to coerce_array is needed and the output to -var-update
 > > should look like:
 > > 
 > > 
 > > -var-update --all-values var1
 > > ^done,changelist=[{name="var1",value="4",in_scope="true",type_chan ged="false"}]
 > > 
 > > i.e the address shouldn't appear in the value field.
 > 
 > Is that what it did before?  I guess it's not surprising.

No, before it would give the address too:

^done,changelist=[{name="var1",value="@0xbff72ebc: 4",in_scope="true",type_chan ged="false"}]

like now, but all the time e.g if you issued -var-update twice without doing
anything in between.


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

Re: Variable objects laziness

[Daniel Jacobowitz]

On Wed, Nov 29, 2006 at 03:04:10PM +1300, Nick Roberts wrote:
> Also -var-update only reports a change when the address changes and not the
> value.  Variable objects were broken before but in a different way (-var-assign
> worked but -var-update always reported that the reference value had changed).

Just so we're all on the same page, could you post a compilable test
case?  I don't need a dejagnu-ified one, just some C++.

> I think a further call to coerce_array is needed and the output to -var-update
> should look like:
> 
> 
> -var-update --all-values var1
> ^done,changelist=[{name="var1",value="4",in_scope="true",type_chan ged="false"}]
> 
> i.e the address shouldn't appear in the value field.

Is that what it did before?  I guess it's not surprising.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Variable objects laziness

[Nick Roberts]

With the new changes to varobj.c, -var-assign doesn't work for references.

(gdb)
-var-create - * rx
^done,name="var1",numchild="0",type="int &"

...
(gdb)
-var-update --all-values var1
^done,changelist=[{name="var1",value="@0xbff72ebc: 4",in_scope="true",type_chan ged="false"}]
(gdb)
-var-evaluate-expression var1
^done,value="@0xbff72ebc: 4"
(gdb)
--var-assign var1 8
~"varobj.c:2143: internal-error: c_value_of_variable: Assertion `!value_lazy (v ar->value)' failed.\n"
~"A problem internal to GDB has been detected,\n"
~"further debugging may prove unreliable.\n"
~"Quit this debugging session? (y or n) "
~"\n"

Also -var-update only reports a change when the address changes and not the
value.  Variable objects were broken before but in a different way (-var-assign
worked but -var-update always reported that the reference value had changed).

I think a further call to coerce_array is needed and the output to -var-update
should look like:


-var-update --all-values var1
^done,changelist=[{name="var1",value="4",in_scope="true",type_chan ged="false"}]

i.e the address shouldn't appear in the value field.

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

Re: Variable objects laziness

[Daniel Jacobowitz]

On Sat, Nov 18, 2006 at 12:48:05PM +0300, Vladimir Prus wrote:
> Daniel Jacobowitz wrote:
> 
> >> +     rather value_contents, will take care of this.
> >> +     It might throw, but unlike var-update for -var-assign
> >> +     there's just one variable we're working it, so we don't
> >> +     need to catch the exception here.  */
> > 
> > Wait, what?  gdb_value_assign will never throw.  value_contents
> > might, but gdb_value_assign will catch it.
> 
> Yep, confused gdb_value_assign with value_assign.
> 
> Updated patch attached. I also attach the delta to the previous patch.

This version is OK now (for HEAD only).  Thanks for your patience.
The frozen varobj patch had some further discussion; I'll have to
get back to it, but I'm afraid not today.  Poke me if I haven't done it
in a few days, please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Variable objects laziness

[Vladimir Prus]

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

Daniel Jacobowitz wrote:

>> +     rather value_contents, will take care of this.
>> +     It might throw, but unlike var-update for -var-assign
>> +     there's just one variable we're working it, so we don't
>> +     need to catch the exception here.  */
> 
> Wait, what?  gdb_value_assign will never throw.  value_contents
> might, but gdb_value_assign will catch it.

Yep, confused gdb_value_assign with value_assign.

Updated patch attached. I also attach the delta to the previous patch.

-  Volodya

        Fetch values from memory in a single place,
        and only fetch the values that are really needed.
        * varobj.c (struct varobj): Clarify comment.
        (my_value_equal): Remove.
        (install_new_value): New function.
        (type_of_child): Remove.
        (varobj_create): Use install_new_value.
        (varobj_set_value): Use value_contents_equal, not
        my_value_equal.
        (varobj_update): Use install_new_value.
        (create_child): Likewise. Inline type_of_child here.
        (value_of_child): Don't fetch the value.
        (c_value_of_root): Likewise.
        (c_value_of_variable): Likewise.
        (type_changeable): Improve comments.

        gdb/testsuite/
        * gdb.mi/mi-var-cmd.exp: Check -var-update after
        assignement of arrays and function pointers.
        * gdb.mi/var-cmd.c: Add declaration necessary for above
        tests.


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

=== gdb/varobj.c
==================================================================
--- gdb/varobj.c	(revision 2144)
+++ gdb/varobj.c	(local)
@@ -507,11 +507,9 @@
       /* 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, &value))
-	{
-	  /* no error */
-	}
-      else
+      if (!gdb_evaluate_expression (var->root->exp, &value))
+	/* Error getting the value.  Try to at least get the
+	   right type.  */
 	value = evaluate_type (var->root->exp);
 
       release_value (value);
@@ -837,20 +835,19 @@
       /* Need to coerce the input.  We want to check if the
 	 value of the variable object will be different
 	 after assignment, and the first thing value_assign
-	 does is coercing the input.
-	 For example, it means that if assigning array to
-	 a pointer variable, we'll be comparing pointer with array's
-	 address, not pointer with array's content.  */
+	 does is coerce the input.
+	 For example, if we are assigning an array to a pointer variable we
+	 should compare the pointer with the the array's address, not with the
+	 array's content.  */
       value = coerce_array (value);
 
       if (!value_contents_equal (var->value, value))
         var->updated = 1;
 
-      /* The new value may be lazy. gdb_value_assign, or 
-	 rather value_contents, will take care of this.  
-	 It might throw, but unlike var-update for -var-assign
-	 there's just one variable we're working it, so we don't
-	 need to catch the exception here.  */
+      /* The new value may be lazy.  gdb_value_assign, or 
+	 rather value_contents, will take care of this.
+	 If fetching of the new value will fail, gdb_value_assign
+	 with catch the exception.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
       value_free (var->value);
@@ -900,7 +897,7 @@
    and return 0.
    Otherwise, assign the value and if type_changeable returns non-zero,
    find if the new value is different from the current value.
-   Return 1 if so, and 0 is the values are equal.  */
+   Return 1 if so, and 0 if the values are equal.  */
 static int
 install_new_value (struct varobj *var, struct value *value, int initial)
 { 
@@ -979,7 +976,7 @@
 	}
     }
     
-  /* We must always keep the new value, since children depend on it. */
+  /* We must always keep the new value, since children depend on it.  */
   if (var->value != NULL)
     value_free (var->value);
   var->value = value;

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

=== gdb/testsuite/gdb.mi/mi-var-cmd.exp
==================================================================
--- gdb/testsuite/gdb.mi/mi-var-cmd.exp	(/mirrors/gdb_mainline)	(revision 2148)
+++ gdb/testsuite/gdb.mi/mi-var-cmd.exp	(/patches/gdb/laziness/gdb_mainline)	(revision 2148)
@@ -382,6 +382,43 @@
 	"\\^done,value=\"333\"" \
 	"assign to lsimple.integer"
 
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=.*" \
+	"var update"
+
+# Check that assignment of function and array values
+# promotes the assigned value to function pointer/data
+# pointer before comparing with the existing value, 
+# and does not incorrectly make the value as changed.
+mi_gdb_test "-var-assign func do_block_tests" \
+	"\\^done,value=\"$hex <do_block_tests>\"" \
+	"assign same value to func"
+
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=\\\[\\\]" \
+	"assign same value to func (update)"
+
+mi_gdb_test "-var-create array_ptr * array_ptr" \
+	"\\^done,name=\"array_ptr\",numchild=\"1\",type=\"int \\*\"" \
+	"create global variable array_ptr"
+
+mi_gdb_test "-var-assign array_ptr array2" \
+	"\\^done,value=\"$hex\"" \
+	"assign array to pointer"
+
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=\\\[\{name=\"array_ptr\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
+	"assign array to pointer (update)"
+
+mi_gdb_test "-var-assign array_ptr array2" \
+	"\\^done,value=\"$hex\"" \
+	"assign same array to pointer"
+
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=\\\[\\\]" \
+	"assign same array to pointer (update)"
+
+
 ######
 # End of assign tests 
 #####
=== gdb/testsuite/gdb.mi/var-cmd.c
==================================================================
--- gdb/testsuite/gdb.mi/var-cmd.c	(/mirrors/gdb_mainline)	(revision 2148)
+++ gdb/testsuite/gdb.mi/var-cmd.c	(/patches/gdb/laziness/gdb_mainline)	(revision 2148)
@@ -106,6 +106,10 @@
   b = a;
 }
 
+int array[] = {1,2,3};
+int array2[] = {4,5,6};
+int *array_ptr = array;
+
 void
 do_locals_tests ()
 {
=== gdb/varobj.c
==================================================================
--- gdb/varobj.c	(/mirrors/gdb_mainline)	(revision 2148)
+++ gdb/varobj.c	(/patches/gdb/laziness/gdb_mainline)	(revision 2148)
@@ -101,7 +101,9 @@
   /* The type of this variable. This may NEVER be NULL. */
   struct type *type;
 
-  /* The value of this expression or subexpression.  This may be NULL. */
+  /* The value of this expression or subexpression.  This may be NULL. 
+     Invariant: if type_changeable (this) is non-zero, the value is either
+     NULL, or not lazy.  */
   struct value *value;
 
   /* Did an error occur evaluating the expression or getting its value? */
@@ -202,8 +204,6 @@
 
 static enum varobj_display_formats variable_default_display (struct varobj *);
 
-static int my_value_equal (struct value *, struct value *, int *);
-
 static void vpush (struct vstack **pstack, struct varobj *var);
 
 static struct varobj *vpop (struct vstack **pstack);
@@ -212,6 +212,9 @@
 
 static char *cppop (struct cpstack **pstack);
 
+static int install_new_value (struct varobj *var, struct value *value, 
+			      int initial);
+
 /* Language-specific routines. */
 
 static enum varobj_languages variable_language (struct varobj *var);
@@ -226,8 +229,6 @@
 
 static struct value *value_of_child (struct varobj *parent, int index);
 
-static struct type *type_of_child (struct varobj *var);
-
 static int variable_editable (struct varobj *var);
 
 static char *my_value_of_variable (struct varobj *var);
@@ -445,6 +446,7 @@
     {
       char *p;
       enum varobj_languages lang;
+      struct value *value;
 
       /* Parse and evaluate the expression, filling in as much
          of the variable's data as possible */
@@ -505,18 +507,17 @@
       /* 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);
-	}
-      else
-	var->value = evaluate_type (var->root->exp);
+      if (!gdb_evaluate_expression (var->root->exp, &value))
+	/* Error getting the value.  Try to at least get the
+	   right type.  */
+	value = evaluate_type (var->root->exp);
 
-      var->type = value_type (var->value);
+      release_value (value);
 
+      var->type = value_type (value);
+
+      install_new_value (var, value, 1 /* Initial assignment */);
+
       /* Set language info */
       lang = variable_language (var);
       var->root->lang = languages[lang];
@@ -825,8 +826,28 @@
 	  return 0;
 	}
 
-      if (!my_value_equal (var->value, value, &error))
+      /* All types that are editable must also be changeable.  */
+      gdb_assert (type_changeable (var));
+
+      /* The value of a changeable variable object must not be lazy.  */
+      gdb_assert (!value_lazy (var->value));
+
+      /* Need to coerce the input.  We want to check if the
+	 value of the variable object will be different
+	 after assignment, and the first thing value_assign
+	 does is coerce the input.
+	 For example, if we are assigning an array to a pointer variable we
+	 should compare the pointer with the the array's address, not with the
+	 array's content.  */
+      value = coerce_array (value);
+
+      if (!value_contents_equal (var->value, value))
         var->updated = 1;
+
+      /* The new value may be lazy.  gdb_value_assign, or 
+	 rather value_contents, will take care of this.
+	 If fetching of the new value will fail, gdb_value_assign
+	 with catch the exception.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
       value_free (var->value);
@@ -870,6 +891,101 @@
   return rootcount;
 }
 
+/* Assign a new value to a variable object.  If INITIAL is non-zero,
+   this is the first assignement after the variable object was just
+   created, or changed type.  In that case, just assign the value 
+   and return 0.
+   Otherwise, assign the value and if type_changeable returns non-zero,
+   find if the new value is different from the current value.
+   Return 1 if so, and 0 if the values are equal.  */
+static int
+install_new_value (struct varobj *var, struct value *value, int initial)
+{ 
+  int changeable;
+  int need_to_fetch;
+  int changed = 0;
+
+  var->error = 0;
+  /* We need to know the varobj's type to decide if the value should
+     be fetched or not.  C++ fake children (public/protected/private) don't have
+     a type. */
+  gdb_assert (var->type || CPLUS_FAKE_CHILD (var));
+  changeable = type_changeable (var);
+  need_to_fetch = changeable;
+
+  if (var->type && TYPE_CODE (var->type) == TYPE_CODE_UNION)
+    /* For unions, we need to fetch the value implicitly because
+       of implementation of union member fetch.  When gdb
+       creates a value for a field and the value of the enclosing
+       structure is not lazy,  it immediately copies the necessary
+       bytes from the enclosing values.  If the enclosing value is
+       lazy, the call to value_fetch_lazy on the field will read
+       the data from memory.  For unions, that means we'll read the
+       same memory more than once, which is not desirable.  So
+       fetch now.  */
+    need_to_fetch = 1;
+
+  /* The new value might be lazy.  If the type is changeable,
+     that is we'll be comparing values of this type, fetch the
+     value now.  Otherwise, on the next update the old value
+     will be lazy, which means we've lost that old value.  */
+  if (need_to_fetch && value && value_lazy (value))
+    {
+      if (!gdb_value_fetch_lazy (value))
+	{
+	  var->error = 1;
+	  /* Set the value to NULL, so that for the next -var-update,
+	     we don't try to compare the new value with this value,
+	     that we couldn't even read.  */
+	  value = NULL;
+	}
+      else
+	var->error = 0;
+    }
+
+  /* If the type is changeable, compare the old and the new values.
+     If this is the initial assignment, we don't have any old value
+     to compare with.  */
+  if (!initial && changeable)
+    {
+      /* If the value of the varobj was changed by -var-set-value, then the 
+	 value in the varobj and in the target is the same.  However, that value
+	 is different from the value that the varobj had after the previous
+	 -var-update. So need to the varobj as changed.  */	 
+      if (var->updated)
+	changed = 1;
+      else 
+	{
+	  /* Try to compare the values.  That requires that both
+	     values are non-lazy.  */
+	  
+	  /* Quick comparison of NULL values.  */
+	  if (var->value == NULL && value == NULL)
+	    /* Equal. */
+	    ;
+	  else if (var->value == NULL || value == NULL)
+	    changed = 1;
+	  else
+	    {
+	      gdb_assert (!value_lazy (var->value));
+	      gdb_assert (!value_lazy (value));
+	      
+	      if (!value_contents_equal (var->value, value))
+		changed = 1;
+	    }
+	}
+    }
+    
+  /* We must always keep the new value, since children depend on it.  */
+  if (var->value != NULL)
+    value_free (var->value);
+  var->value = value;
+  var->updated = 0;
+
+  return changed;
+}
+  
+
 /* Update the values for a variable and its children.  This is a
    two-pronged attack.  First, re-parse the value for the root's
    expression to see if it's changed.  Then go all the way
@@ -939,24 +1055,16 @@
       vpush (&result, *varp);
       changed++;
     }
-  /* If values are not equal, note that it's changed.
-     There a couple of exceptions here, though.
-     We don't want some types to be reported as "changed". */
-  else if (type_changeable (*varp) &&
-	   ((*varp)->updated || !my_value_equal ((*varp)->value, new, &error)))
+
+  if (install_new_value ((*varp), new, type_changed))
     {
-      vpush (&result, *varp);
-      (*varp)->updated = 0;
+      /* If type_changed is 1, install_new_value will never return
+	 non-zero, so we'll never report the same variable twice.  */
+      gdb_assert (!type_changed);
+      vpush (&result, (*varp));
       changed++;
-      /* Its value is going to be updated to NEW.  */
-      (*varp)->error = error;
     }
 
-  /* We must always keep around the new value for this root
-     variable expression, or we lose the updated children! */
-  value_free ((*varp)->value);
-  (*varp)->value = new;
-
   /* Initialize a stack */
   vpush (&stack, NULL);
 
@@ -982,22 +1090,14 @@
 
       /* Update this variable */
       new = value_of_child (v->parent, v->index);
-      if (type_changeable (v) && 
-          (v->updated || !my_value_equal (v->value, new, &error)))
-	{
+      if (install_new_value (v, new, 0 /* type not changed */))
+ 	{
 	  /* Note that it's changed */
 	  vpush (&result, v);
 	  v->updated = 0;
 	  changed++;
 	}
-      /* Its value is going to be updated to NEW.  */
-      v->error = error;
 
-      /* We must always keep new values, since children depend on it. */
-      if (v->value != NULL)
-	value_free (v->value);
-      v->value = new;
-
       /* Get next child */
       v = vpop (&stack);
     }
@@ -1257,15 +1357,14 @@
 {
   struct varobj *child;
   char *childs_name;
+  struct value *value;
 
   child = new_variable ();
 
   /* name is allocated by name_of_child */
   child->name = name;
   child->index = index;
-  child->value = value_of_child (parent, index);
-  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
-    child->error = 1;
+  value = value_of_child (parent, index);
   child->parent = parent;
   child->root = parent->root;
   childs_name = xstrprintf ("%s.%s", parent->obj_name, name);
@@ -1275,9 +1374,21 @@
   /* Save a pointer to this child in the parent */
   save_child_in_parent (parent, child);
 
-  /* Note the type of this child */
-  child->type = type_of_child (child);
+  /* Compute the type of the child.  Must do this before
+     calling install_new_value.  */
+  if (value != NULL)
+    /* If the child had no evaluation errors, var->value
+       will be non-NULL and contain a valid type. */
+    child->type = value_type (value);
+  else
+    /* Otherwise, we must compute the type. */
+    child->type = (*child->root->lang->type_of_child) (child->parent, 
+						       child->index);
+  install_new_value (child, value, 1);
 
+  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
+    child->error = 1;
+
   return child;
 }
 
@@ -1451,42 +1562,6 @@
   return FORMAT_NATURAL;
 }
 
-/* This function is similar to GDB's value_contents_equal, except that
-   this one is "safe"; it never longjmps.  It determines if the VAL1's
-   value is the same as VAL2.  If for some reason the value of VAR2
-   can't be established, *ERROR2 is set to non-zero.  */
-
-static int
-my_value_equal (struct value *val1, struct value *volatile val2, int *error2)
-{
-  volatile struct gdb_exception except;
-
-  /* As a special case, if both are null, we say they're equal.  */
-  if (val1 == NULL && val2 == NULL)
-    return 1;
-  else if (val1 == NULL || val2 == NULL)
-    return 0;
-
-  /* The contents of VAL1 are supposed to be known.  */
-  gdb_assert (!value_lazy (val1));
-
-  /* Make sure we also know the contents of VAL2.  */
-  val2 = coerce_array (val2);
-  TRY_CATCH (except, RETURN_MASK_ERROR)
-    {
-      if (value_lazy (val2))
-	value_fetch_lazy (val2);
-    }
-  if (except.reason < 0)
-    {
-      *error2 = 1;
-      return 0;
-    }
-  gdb_assert (!value_lazy (val2));
-
-  return value_contents_equal (val1, val2);
-}
-
 /* FIXME: The following should be generic for any pointer */
 static void
 vpush (struct vstack **pstack, struct varobj *var)
@@ -1678,33 +1753,9 @@
 
   value = (*parent->root->lang->value_of_child) (parent, index);
 
-  /* If we're being lazy, fetch the real value of the variable. */
-  if (value != NULL && value_lazy (value))
-    {
-      /* If we fail to fetch the value of the child, return
-         NULL so that callers notice that we're leaving an
-         error message. */
-      if (!gdb_value_fetch_lazy (value))
-	value = NULL;
-    }
-
   return value;
 }
 
-/* What is the type of VAR? */
-static struct type *
-type_of_child (struct varobj *var)
-{
-
-  /* If the child had no evaluation errors, var->value
-     will be non-NULL and contain a valid type. */
-  if (var->value != NULL)
-    return value_type (var->value);
-
-  /* Otherwise, we must compute the type. */
-  return (*var->root->lang->type_of_child) (var->parent, var->index);
-}
-
 /* Is this variable editable? Use the variable's type to make
    this determination. */
 static int
@@ -1720,9 +1771,15 @@
   return (*var->root->lang->value_of_variable) (var);
 }
 
-/* Is VAR something that can change? Depending on language,
-   some variable's values never change. For example,
-   struct and unions never change values. */
+/* Return non-zero if changes in value of VAR
+   must be detected and reported by -var-update.
+   Return zero is -var-update should never report
+   changes of such values.  This makes sense for structures
+   (since the changes in children values will be reported separately),
+   or for artifical objects (like 'public' pseudo-field in C++).
+
+   Return value of 0 means that gdb need not call value_fetch_lazy
+   for the value of this variable object.  */
 static int
 type_changeable (struct varobj *var)
 {
@@ -1898,24 +1955,12 @@
          go on */
       if (gdb_evaluate_expression (var->root->exp, &new_val))
 	{
-	  if (value_lazy (new_val))
-	    {
-	      /* We need to catch errors because if
-	         value_fetch_lazy fails we still want to continue
-	         (after making val->error = 1) */
-	      /* FIXME: Shouldn't be using value_contents()?  The
-	         comment on value_fetch_lazy() says it is only called
-	         from the macro... */
-	      if (!gdb_value_fetch_lazy (new_val))
-		var->error = 1;
-	      else
-		var->error = 0;
-	    }
+	  var->error = 0;
+	  release_value (new_val);
 	}
       else
 	var->error = 1;
 
-      release_value (new_val);
       return new_val;
     }
 
@@ -2094,8 +2139,8 @@
 	    struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
 	    char *thevalue;
 
-	    if (value_lazy (var->value))
-	      gdb_value_fetch_lazy (var->value);
+	    gdb_assert (type_changeable (var));
+	    gdb_assert (!value_lazy (var->value));
 	    common_val_print (var->value, stb,
 			      format_code[(int) var->format], 1, 0, 0);
 	    thevalue = ui_file_xstrdup (stb, &dummy);

Property changes on: 
___________________________________________________________________
Name: csl:base
 +/all/mirrors/gdb_mainline
Name: svk:merge
 +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:154913

Re: Variable objects laziness

[Daniel Jacobowitz]

On Fri, Nov 17, 2006 at 08:17:04PM +0300, Vladimir Prus wrote:
> -      if (gdb_evaluate_expression (var->root->exp, &var->value))
> +      if (gdb_evaluate_expression (var->root->exp, &value))
>  	{
>  	  /* no error */
> -	  if (value_lazy (var->value))
> -	    gdb_value_fetch_lazy (var->value);
>  	}
>        else
> -	var->value = evaluate_type (var->root->exp);
> +	value = evaluate_type (var->root->exp);

Noticed while working on another patch: can you kill the empty braces
here, please (by negating the if/dropping the else).

> +      /* Need to coerce the input.  We want to check if the
> +	 value of the variable object will be different
> +	 after assignment, and the first thing value_assign
> +	 does is coercing the input.

"is coerce"

> +	 For example, it means that if assigning array to
> +	 a pointer variable, we'll be comparing pointer with array's
> +	 address, not pointer with array's content.  */

For example, if we are assigning an array to a pointer variable we
should compare the pointer with the the array's address, not with the
array's content.

> +      /* The new value may be lazy. gdb_value_assign, or 

Two spaces after period please.

> +	 rather value_contents, will take care of this.  
> +	 It might throw, but unlike var-update for -var-assign
> +	 there's just one variable we're working it, so we don't
> +	 need to catch the exception here.  */

Wait, what?  gdb_value_assign will never throw.  value_contents
might, but gdb_value_assign will catch it.

> +   Return 1 if so, and 0 is the values are equal.  */

if, not is.

> +  /* We must always keep the new value, since children depend on it. */

Two spaces after periods please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Variable objects laziness

[Vladimir Prus]

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


> Daniel Jacobowitz wrote:
>> I think this is the key bit - my_value_equal called value_fetch_lazy,
>> the new code calls gdb_value_fetch_lazy.
>> 
>> Vlad, I noticed that the old code used coerce_array and the new one
>> doesn't.  Is that a problem?
> 
> This should not be a problem on -var-update path, because we never try
> to compare values of array types, since for them type_changeable returns
> false.
> 
> However, it looks to be a problem on -var-assign path. Given:
> 
>    int b[] = {1,2,3};
>    int *a  = b;
> 
> if we create varobj for 'a' and assign it the value of 'b', we should not
> mark this variable as changed.
> 
> I'll double-check this (and other coercions that coerce_array silently
> does) and add new test cases as needed.

This version of patch fixes the problem and adds new tests.

- Volodya

        Fetch values from memory in a single place,
        and only fetch the values that are really needed.
        * varobj.c (struct varobj): Clarify comment.
        (my_value_equal): Remove.
        (install_new_value): New function.
        (type_of_child): Remove.
        (varobj_create): Use install_new_value.
        (varobj_set_value): Use value_contents_equal, not
        my_value_equal.
        (varobj_update): Use install_new_value.
        (create_child): Likewise. Inline type_of_child here.
        (value_of_child): Don't fetch the value.
        (c_value_of_root): Likewise.
        (c_value_of_variable): Likewise.
        (type_changeable): Improve comments.

        gdb/testsuite/
        * gdb.mi/mi-var-cmd.exp: Check -var-update after
        assignement of arrays and function pointers.
        * gdb.mi/var-cmd.c: Add declaration necessary for above
        tests.

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

=== gdb/testsuite/gdb.mi/mi-var-cmd.exp
==================================================================
--- gdb/testsuite/gdb.mi/mi-var-cmd.exp	(/patches/gdb/varobj_value_fixes/gdb_mainline)	(revision 2144)
+++ gdb/testsuite/gdb.mi/mi-var-cmd.exp	(/patches/gdb/laziness/gdb_mainline)	(revision 2144)
@@ -382,6 +382,43 @@
 	"\\^done,value=\"333\"" \
 	"assign to lsimple.integer"
 
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=.*" \
+	"var update"
+
+# Check that assignment of function and array values
+# promotes the assigned value to function pointer/data
+# pointer before comparing with the existing value, 
+# and does not incorrectly make the value as changed.
+mi_gdb_test "-var-assign func do_block_tests" \
+	"\\^done,value=\"$hex <do_block_tests>\"" \
+	"assign same value to func"
+
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=\\\[\\\]" \
+	"assign same value to func (update)"
+
+mi_gdb_test "-var-create array_ptr * array_ptr" \
+	"\\^done,name=\"array_ptr\",numchild=\"1\",type=\"int \\*\"" \
+	"create global variable array_ptr"
+
+mi_gdb_test "-var-assign array_ptr array2" \
+	"\\^done,value=\"$hex\"" \
+	"assign array to pointer"
+
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=\\\[\{name=\"array_ptr\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
+	"assign array to pointer (update)"
+
+mi_gdb_test "-var-assign array_ptr array2" \
+	"\\^done,value=\"$hex\"" \
+	"assign same array to pointer"
+
+mi_gdb_test "-var-update *" \
+	"\\^done,changelist=\\\[\\\]" \
+	"assign same array to pointer (update)"
+
+
 ######
 # End of assign tests 
 #####
=== gdb/testsuite/gdb.mi/var-cmd.c
==================================================================
--- gdb/testsuite/gdb.mi/var-cmd.c	(/patches/gdb/varobj_value_fixes/gdb_mainline)	(revision 2144)
+++ gdb/testsuite/gdb.mi/var-cmd.c	(/patches/gdb/laziness/gdb_mainline)	(revision 2144)
@@ -106,6 +106,10 @@
   b = a;
 }
 
+int array[] = {1,2,3};
+int array2[] = {4,5,6};
+int *array_ptr = array;
+
 void
 do_locals_tests ()
 {
=== gdb/varobj.c
==================================================================
--- gdb/varobj.c	(/patches/gdb/varobj_value_fixes/gdb_mainline)	(revision 2144)
+++ gdb/varobj.c	(/patches/gdb/laziness/gdb_mainline)	(revision 2144)
@@ -101,7 +101,9 @@
   /* The type of this variable. This may NEVER be NULL. */
   struct type *type;
 
-  /* The value of this expression or subexpression.  This may be NULL. */
+  /* The value of this expression or subexpression.  This may be NULL. 
+     Invariant: if type_changeable (this) is non-zero, the value is either
+     NULL, or not lazy.  */
   struct value *value;
 
   /* Did an error occur evaluating the expression or getting its value? */
@@ -202,8 +204,6 @@
 
 static enum varobj_display_formats variable_default_display (struct varobj *);
 
-static int my_value_equal (struct value *, struct value *, int *);
-
 static void vpush (struct vstack **pstack, struct varobj *var);
 
 static struct varobj *vpop (struct vstack **pstack);
@@ -212,6 +212,9 @@
 
 static char *cppop (struct cpstack **pstack);
 
+static int install_new_value (struct varobj *var, struct value *value, 
+			      int initial);
+
 /* Language-specific routines. */
 
 static enum varobj_languages variable_language (struct varobj *var);
@@ -226,8 +229,6 @@
 
 static struct value *value_of_child (struct varobj *parent, int index);
 
-static struct type *type_of_child (struct varobj *var);
-
 static int variable_editable (struct varobj *var);
 
 static char *my_value_of_variable (struct varobj *var);
@@ -445,6 +446,7 @@
     {
       char *p;
       enum varobj_languages lang;
+      struct value *value;
 
       /* Parse and evaluate the expression, filling in as much
          of the variable's data as possible */
@@ -505,19 +507,19 @@
       /* 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))
+      if (gdb_evaluate_expression (var->root->exp, &value))
 	{
 	  /* no error */
-	  if (value_lazy (var->value))
-	    gdb_value_fetch_lazy (var->value);
 	}
       else
-	var->value = evaluate_type (var->root->exp);
+	value = evaluate_type (var->root->exp);
 
-      release_value (var->value);
+      release_value (value);
 
-      var->type = value_type (var->value);
+      var->type = value_type (value);
 
+      install_new_value (var, value, 1 /* Initial assignment */);
+
       /* Set language info */
       lang = variable_language (var);
       var->root->lang = languages[lang];
@@ -826,8 +828,29 @@
 	  return 0;
 	}
 
-      if (!my_value_equal (var->value, value, &error))
+      /* All types that are editable must also be changeable.  */
+      gdb_assert (type_changeable (var));
+
+      /* The value of a changeable variable object must not be lazy.  */
+      gdb_assert (!value_lazy (var->value));
+
+      /* Need to coerce the input.  We want to check if the
+	 value of the variable object will be different
+	 after assignment, and the first thing value_assign
+	 does is coercing the input.
+	 For example, it means that if assigning array to
+	 a pointer variable, we'll be comparing pointer with array's
+	 address, not pointer with array's content.  */
+      value = coerce_array (value);
+
+      if (!value_contents_equal (var->value, value))
         var->updated = 1;
+
+      /* The new value may be lazy. gdb_value_assign, or 
+	 rather value_contents, will take care of this.  
+	 It might throw, but unlike var-update for -var-assign
+	 there's just one variable we're working it, so we don't
+	 need to catch the exception here.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
       value_free (var->value);
@@ -871,6 +894,101 @@
   return rootcount;
 }
 
+/* Assign a new value to a variable object.  If INITIAL is non-zero,
+   this is the first assignement after the variable object was just
+   created, or changed type.  In that case, just assign the value 
+   and return 0.
+   Otherwise, assign the value and if type_changeable returns non-zero,
+   find if the new value is different from the current value.
+   Return 1 if so, and 0 is the values are equal.  */
+static int
+install_new_value (struct varobj *var, struct value *value, int initial)
+{ 
+  int changeable;
+  int need_to_fetch;
+  int changed = 0;
+
+  var->error = 0;
+  /* We need to know the varobj's type to decide if the value should
+     be fetched or not.  C++ fake children (public/protected/private) don't have
+     a type. */
+  gdb_assert (var->type || CPLUS_FAKE_CHILD (var));
+  changeable = type_changeable (var);
+  need_to_fetch = changeable;
+
+  if (var->type && TYPE_CODE (var->type) == TYPE_CODE_UNION)
+    /* For unions, we need to fetch the value implicitly because
+       of implementation of union member fetch.  When gdb
+       creates a value for a field and the value of the enclosing
+       structure is not lazy,  it immediately copies the necessary
+       bytes from the enclosing values.  If the enclosing value is
+       lazy, the call to value_fetch_lazy on the field will read
+       the data from memory.  For unions, that means we'll read the
+       same memory more than once, which is not desirable.  So
+       fetch now.  */
+    need_to_fetch = 1;
+
+  /* The new value might be lazy.  If the type is changeable,
+     that is we'll be comparing values of this type, fetch the
+     value now.  Otherwise, on the next update the old value
+     will be lazy, which means we've lost that old value.  */
+  if (need_to_fetch && value && value_lazy (value))
+    {
+      if (!gdb_value_fetch_lazy (value))
+	{
+	  var->error = 1;
+	  /* Set the value to NULL, so that for the next -var-update,
+	     we don't try to compare the new value with this value,
+	     that we couldn't even read.  */
+	  value = NULL;
+	}
+      else
+	var->error = 0;
+    }
+
+  /* If the type is changeable, compare the old and the new values.
+     If this is the initial assignment, we don't have any old value
+     to compare with.  */
+  if (!initial && changeable)
+    {
+      /* If the value of the varobj was changed by -var-set-value, then the 
+	 value in the varobj and in the target is the same.  However, that value
+	 is different from the value that the varobj had after the previous
+	 -var-update. So need to the varobj as changed.  */	 
+      if (var->updated)
+	changed = 1;
+      else 
+	{
+	  /* Try to compare the values.  That requires that both
+	     values are non-lazy.  */
+	  
+	  /* Quick comparison of NULL values.  */
+	  if (var->value == NULL && value == NULL)
+	    /* Equal. */
+	    ;
+	  else if (var->value == NULL || value == NULL)
+	    changed = 1;
+	  else
+	    {
+	      gdb_assert (!value_lazy (var->value));
+	      gdb_assert (!value_lazy (value));
+	      
+	      if (!value_contents_equal (var->value, value))
+		changed = 1;
+	    }
+	}
+    }
+    
+  /* We must always keep the new value, since children depend on it. */
+  if (var->value != NULL)
+    value_free (var->value);
+  var->value = value;
+  var->updated = 0;
+
+  return changed;
+}
+  
+
 /* Update the values for a variable and its children.  This is a
    two-pronged attack.  First, re-parse the value for the root's
    expression to see if it's changed.  Then go all the way
@@ -940,24 +1058,16 @@
       vpush (&result, *varp);
       changed++;
     }
-  /* If values are not equal, note that it's changed.
-     There a couple of exceptions here, though.
-     We don't want some types to be reported as "changed". */
-  else if (type_changeable (*varp) &&
-	   ((*varp)->updated || !my_value_equal ((*varp)->value, new, &error)))
+
+  if (install_new_value ((*varp), new, type_changed))
     {
-      vpush (&result, *varp);
-      (*varp)->updated = 0;
+      /* If type_changed is 1, install_new_value will never return
+	 non-zero, so we'll never report the same variable twice.  */
+      gdb_assert (!type_changed);
+      vpush (&result, (*varp));
       changed++;
-      /* Its value is going to be updated to NEW.  */
-      (*varp)->error = error;
     }
 
-  /* We must always keep around the new value for this root
-     variable expression, or we lose the updated children! */
-  value_free ((*varp)->value);
-  (*varp)->value = new;
-
   /* Initialize a stack */
   vpush (&stack, NULL);
 
@@ -983,22 +1093,14 @@
 
       /* Update this variable */
       new = value_of_child (v->parent, v->index);
-      if (type_changeable (v) && 
-          (v->updated || !my_value_equal (v->value, new, &error)))
-	{
+      if (install_new_value (v, new, 0 /* type not changed */))
+ 	{
 	  /* Note that it's changed */
 	  vpush (&result, v);
 	  v->updated = 0;
 	  changed++;
 	}
-      /* Its value is going to be updated to NEW.  */
-      v->error = error;
 
-      /* We must always keep new values, since children depend on it. */
-      if (v->value != NULL)
-	value_free (v->value);
-      v->value = new;
-
       /* Get next child */
       v = vpop (&stack);
     }
@@ -1258,15 +1360,14 @@
 {
   struct varobj *child;
   char *childs_name;
+  struct value *value;
 
   child = new_variable ();
 
   /* name is allocated by name_of_child */
   child->name = name;
   child->index = index;
-  child->value = value_of_child (parent, index);
-  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
-    child->error = 1;
+  value = value_of_child (parent, index);
   child->parent = parent;
   child->root = parent->root;
   childs_name = xstrprintf ("%s.%s", parent->obj_name, name);
@@ -1276,9 +1377,21 @@
   /* Save a pointer to this child in the parent */
   save_child_in_parent (parent, child);
 
-  /* Note the type of this child */
-  child->type = type_of_child (child);
+  /* Compute the type of the child.  Must do this before
+     calling install_new_value.  */
+  if (value != NULL)
+    /* If the child had no evaluation errors, var->value
+       will be non-NULL and contain a valid type. */
+    child->type = value_type (value);
+  else
+    /* Otherwise, we must compute the type. */
+    child->type = (*child->root->lang->type_of_child) (child->parent, 
+						       child->index);
+  install_new_value (child, value, 1);
 
+  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
+    child->error = 1;
+
   return child;
 }
 
@@ -1452,42 +1565,6 @@
   return FORMAT_NATURAL;
 }
 
-/* This function is similar to GDB's value_contents_equal, except that
-   this one is "safe"; it never longjmps.  It determines if the VAL1's
-   value is the same as VAL2.  If for some reason the value of VAR2
-   can't be established, *ERROR2 is set to non-zero.  */
-
-static int
-my_value_equal (struct value *val1, struct value *volatile val2, int *error2)
-{
-  volatile struct gdb_exception except;
-
-  /* As a special case, if both are null, we say they're equal.  */
-  if (val1 == NULL && val2 == NULL)
-    return 1;
-  else if (val1 == NULL || val2 == NULL)
-    return 0;
-
-  /* The contents of VAL1 are supposed to be known.  */
-  gdb_assert (!value_lazy (val1));
-
-  /* Make sure we also know the contents of VAL2.  */
-  val2 = coerce_array (val2);
-  TRY_CATCH (except, RETURN_MASK_ERROR)
-    {
-      if (value_lazy (val2))
-	value_fetch_lazy (val2);
-    }
-  if (except.reason < 0)
-    {
-      *error2 = 1;
-      return 0;
-    }
-  gdb_assert (!value_lazy (val2));
-
-  return value_contents_equal (val1, val2);
-}
-
 /* FIXME: The following should be generic for any pointer */
 static void
 vpush (struct vstack **pstack, struct varobj *var)
@@ -1679,33 +1756,9 @@
 
   value = (*parent->root->lang->value_of_child) (parent, index);
 
-  /* If we're being lazy, fetch the real value of the variable. */
-  if (value != NULL && value_lazy (value))
-    {
-      /* If we fail to fetch the value of the child, return
-         NULL so that callers notice that we're leaving an
-         error message. */
-      if (!gdb_value_fetch_lazy (value))
-	value = NULL;
-    }
-
   return value;
 }
 
-/* What is the type of VAR? */
-static struct type *
-type_of_child (struct varobj *var)
-{
-
-  /* If the child had no evaluation errors, var->value
-     will be non-NULL and contain a valid type. */
-  if (var->value != NULL)
-    return value_type (var->value);
-
-  /* Otherwise, we must compute the type. */
-  return (*var->root->lang->type_of_child) (var->parent, var->index);
-}
-
 /* Is this variable editable? Use the variable's type to make
    this determination. */
 static int
@@ -1721,9 +1774,15 @@
   return (*var->root->lang->value_of_variable) (var);
 }
 
-/* Is VAR something that can change? Depending on language,
-   some variable's values never change. For example,
-   struct and unions never change values. */
+/* Return non-zero if changes in value of VAR
+   must be detected and reported by -var-update.
+   Return zero is -var-update should never report
+   changes of such values.  This makes sense for structures
+   (since the changes in children values will be reported separately),
+   or for artifical objects (like 'public' pseudo-field in C++).
+
+   Return value of 0 means that gdb need not call value_fetch_lazy
+   for the value of this variable object.  */
 static int
 type_changeable (struct varobj *var)
 {
@@ -1899,19 +1958,7 @@
          go on */
       if (gdb_evaluate_expression (var->root->exp, &new_val))
 	{
-	  if (value_lazy (new_val))
-	    {
-	      /* We need to catch errors because if
-	         value_fetch_lazy fails we still want to continue
-	         (after making val->error = 1) */
-	      /* FIXME: Shouldn't be using value_contents()?  The
-	         comment on value_fetch_lazy() says it is only called
-	         from the macro... */
-	      if (!gdb_value_fetch_lazy (new_val))
-		var->error = 1;
-	      else
-		var->error = 0;
-	    }
+	  var->error = 0;
 	  release_value (new_val);
 	}
       else
@@ -2095,8 +2142,8 @@
 	    struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
 	    char *thevalue;
 
-	    if (value_lazy (var->value))
-	      gdb_value_fetch_lazy (var->value);
+	    gdb_assert (type_changeable (var));
+	    gdb_assert (!value_lazy (var->value));
 	    common_val_print (var->value, stb,
 			      format_code[(int) var->format], 1, 0, 0);
 	    thevalue = ui_file_xstrdup (stb, &dummy);

Property changes on: 
___________________________________________________________________
Name: csl:base
 +/all/patches/gdb/varobj_value_fixes/gdb_mainline
Name: svk:merge
 +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:154913

Re: Variable objects laziness

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Fri, Nov 17, 2006 at 01:44:53PM +0300, Vladimir Prus wrote:
>> On Friday 17 November 2006 13:40, Mark Kettenis wrote:
>> > >  Daniel Jacobowitz wrote:
>> > > > Sorry, you're right!
>> > > >
>> > > > When you're ready, please repost.  Might want to finish talking
>> > > > with Nick first.
>> >
>> > As Nick indicated, I'm not sure whether the removal of my_value_equal
>> > is ok. The whole point of my_value_equal is to check equality of values
>> > without error()ing out if we can't read a value.  Your replacement code
>> > doesn't seem to take that possibility into account.
>> 
>> Are you sure? The code has exactly one place where value is fetched, and
>> it does that by the call to gdb_value_fetch_lazy -- that calls
>> value_fetch_lazy in a try block. Any errors will be caught and cause the
>> value to be set to NULL.
> 
> I think this is the key bit - my_value_equal called value_fetch_lazy,
> the new code calls gdb_value_fetch_lazy.
> 
> Vlad, I noticed that the old code used coerce_array and the new one
> doesn't.  Is that a problem?

This should not be a problem on -var-update path, because we never try
to compare values of array types, since for them type_changeable returns
false.

However, it looks to be a problem on -var-assign path. Given:

   int b[] = {1,2,3};
   int *a  = b;

if we create varobj for 'a' and assign it the value of 'b', we should not
mark this variable as changed.

I'll double-check this (and other coercions that coerce_array silently does)
and add new test cases as needed.

Thanks,
Volodya

Re: Variable objects laziness

[Daniel Jacobowitz]

On Fri, Nov 17, 2006 at 01:44:53PM +0300, Vladimir Prus wrote:
> On Friday 17 November 2006 13:40, Mark Kettenis wrote:
> > >  Daniel Jacobowitz wrote:
> > > > Sorry, you're right!
> > > >
> > > > When you're ready, please repost.  Might want to finish talking with
> > > > Nick first.
> >
> > As Nick indicated, I'm not sure whether the removal of my_value_equal is
> > ok. The whole point of my_value_equal is to check equality of values
> > without error()ing out if we can't read a value.  Your replacement code
> > doesn't seem to take that possibility into account.  
> 
> Are you sure? The code has exactly one place where value is fetched, and it 
> does that by the call to gdb_value_fetch_lazy -- that calls value_fetch_lazy 
> in a try block. Any errors will be caught and cause the value to be set to 
> NULL. 

I think this is the key bit - my_value_equal called value_fetch_lazy,
the new code calls gdb_value_fetch_lazy.

Vlad, I noticed that the old code used coerce_array and the new one
doesn't.  Is that a problem?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Variable objects laziness

[Vladimir Prus]

On Friday 17 November 2006 13:40, Mark Kettenis wrote:
> >  Daniel Jacobowitz wrote:
> > > Sorry, you're right!
> > >
> > > When you're ready, please repost.  Might want to finish talking with
> > > Nick first.
>
> As Nick indicated, I'm not sure whether the removal of my_value_equal is
> ok. The whole point of my_value_equal is to check equality of values
> without error()ing out if we can't read a value.  Your replacement code
> doesn't seem to take that possibility into account.  

Are you sure? The code has exactly one place where value is fetched, and it 
does that by the call to gdb_value_fetch_lazy -- that calls value_fetch_lazy 
in a try block. Any errors will be caught and cause the value to be set to 
NULL. 

> Or is it that your new 
> code no longer needs to do this comparison?  If that's the case, do
> watchpoints still work?

Sorry, what watchpoint have to do with this? my_value_equal is not used to 
implement watchpoints -- only var-update and -var-assign.

- Volodya

Re: Variable objects laziness

[Mark Kettenis]

>  Daniel Jacobowitz wrote:
>
> > Sorry, you're right!
> >
> > When you're ready, please repost.  Might want to finish talking with
> > Nick first.

As Nick indicated, I'm not sure whether the removal of my_value_equal is ok.
The whole point of my_value_equal is to check equality of values without
error()ing out if we can't read a value.  Your replacement code doesn't
seem to take that possibility into account.  Or is it that your new code
no longer needs to do this comparison?  If that's the case, do watchpoints
still work?

>  - Volodya
>
>          Fetch values from memory in a single place,
>          and only fetch the values that are really needed.
>          * varobj.c (struct varobj): Clarify comment.
>          (my_value_equal): Remove.
>          (install_new_value): New function.
>          (type_of_child): Remove.
>          (varobj_create): Use install_new_value.
>          (varobj_set_value): Use value_contents_equal, not
>          my_value_equal.
>          (varobj_update): Use install_new_value.
>          (create_child): Likewise. Inline type_of_child here.
>          (value_of_child): Don't fetch the value.
>          (c_value_of_root): Likewise.
>          (c_value_of_variable): Likewise.
>          (type_changeable): Improve comments.

Re: Variable objects laziness

[Vladimir Prus]

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

Daniel Jacobowitz wrote:

> Sorry, you're right!
> 
> When you're ready, please repost.  Might want to finish talking with
> Nick first.

Here's revised patch. 

- Volodya

        Fetch values from memory in a single place,
        and only fetch the values that are really needed.
        * varobj.c (struct varobj): Clarify comment.
        (my_value_equal): Remove.
        (install_new_value): New function.
        (type_of_child): Remove.
        (varobj_create): Use install_new_value.
        (varobj_set_value): Use value_contents_equal, not
        my_value_equal.
        (varobj_update): Use install_new_value.
        (create_child): Likewise. Inline type_of_child here.
        (value_of_child): Don't fetch the value.
        (c_value_of_root): Likewise.
        (c_value_of_variable): Likewise.
        (type_changeable): Improve comments.

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

=== gdb/varobj.c
==================================================================
--- gdb/varobj.c	(/mirrors/gdb_mainline)	(revision 2128)
+++ gdb/varobj.c	(/patches/gdb/laziness/gdb_mainline)	(revision 2128)
@@ -101,7 +101,9 @@
   /* The type of this variable. This may NEVER be NULL. */
   struct type *type;
 
-  /* The value of this expression or subexpression.  This may be NULL. */
+  /* The value of this expression or subexpression.  This may be NULL. 
+     Invariant: if type_changeable (this) is non-zero, the value is either
+     NULL, or not lazy.  */
   struct value *value;
 
   /* Did an error occur evaluating the expression or getting its value? */
@@ -202,8 +204,6 @@
 
 static enum varobj_display_formats variable_default_display (struct varobj *);
 
-static int my_value_equal (struct value *, struct value *, int *);
-
 static void vpush (struct vstack **pstack, struct varobj *var);
 
 static struct varobj *vpop (struct vstack **pstack);
@@ -212,6 +212,9 @@
 
 static char *cppop (struct cpstack **pstack);
 
+static int install_new_value (struct varobj *var, struct value *value, 
+			      int initial);
+
 /* Language-specific routines. */
 
 static enum varobj_languages variable_language (struct varobj *var);
@@ -226,8 +229,6 @@
 
 static struct value *value_of_child (struct varobj *parent, int index);
 
-static struct type *type_of_child (struct varobj *var);
-
 static int variable_editable (struct varobj *var);
 
 static char *my_value_of_variable (struct varobj *var);
@@ -445,6 +446,7 @@
     {
       char *p;
       enum varobj_languages lang;
+      struct value *value;
 
       /* Parse and evaluate the expression, filling in as much
          of the variable's data as possible */
@@ -505,18 +507,18 @@
       /* 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))
+      if (gdb_evaluate_expression (var->root->exp, &value))
 	{
 	  /* no error */
-	  release_value (var->value);
-	  if (value_lazy (var->value))
-	    gdb_value_fetch_lazy (var->value);
+	  release_value (value);
 	}
       else
-	var->value = evaluate_type (var->root->exp);
+	value = evaluate_type (var->root->exp);
 
-      var->type = value_type (var->value);
+      var->type = value_type (value);
 
+      install_new_value (var, value, 1 /* Initial assignment */);
+
       /* Set language info */
       lang = variable_language (var);
       var->root->lang = languages[lang];
@@ -825,8 +827,20 @@
 	  return 0;
 	}
 
-      if (!my_value_equal (var->value, value, &error))
+      /* All types that are editable must also be changeable.  */
+      gdb_assert (type_changeable (var));
+
+      /* The value of a changeable variable object must not be lazy.  */
+      gdb_assert (!value_lazy (var->value));
+
+      if (!value_contents_equal (var->value, value))
         var->updated = 1;
+
+      /* The new value may be lazy. gdb_value_assign, or 
+	 rather value_contents, will take care of this.  
+	 It might throw, but unlike var-update for -var-assign
+	 there's just one variable we're working it, so we don't
+	 need to catch the exception here.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
       value_free (var->value);
@@ -870,6 +884,101 @@
   return rootcount;
 }
 
+/* Assign a new value to a variable object.  If INITIAL is non-zero,
+   this is the first assignement after the variable object was just
+   created, or changed type.  In that case, just assign the value 
+   and return 0.
+   Otherwise, assign the value and if type_changeable returns non-zero,
+   find if the new value is different from the current value.
+   Return 1 if so, and 0 is the values are equal.  */
+static int
+install_new_value (struct varobj *var, struct value *value, int initial)
+{ 
+  int changeable;
+  int need_to_fetch;
+  int changed = 0;
+
+  var->error = 0;
+  /* We need to know the varobj's type to decide if the value should
+     be fetched or not.  C++ fake children (public/protected/private) don't have
+     a type. */
+  gdb_assert (var->type || CPLUS_FAKE_CHILD (var));
+  changeable = type_changeable (var);
+  need_to_fetch = changeable;
+
+  if (var->type && TYPE_CODE (var->type) == TYPE_CODE_UNION)
+    /* For unions, we need to fetch the value implicitly because
+       of implementation of union member fetch.  When gdb
+       creates a value for a field and the value of the enclosing
+       structure is not lazy,  it immediately copies the necessary
+       bytes from the enclosing values.  If the enclosing value is
+       lazy, the call to value_fetch_lazy on the field will read
+       the data from memory.  For unions, that means we'll read the
+       same memory more than once, which is not desirable.  So
+       fetch now.  */
+    need_to_fetch = 1;
+
+  /* The new value might be lazy.  If the type is changeable,
+     that is we'll be comparing values of this type, fetch the
+     value now.  Otherwise, on the next update the old value
+     will be lazy, which means we've lost that old value.  */
+  if (need_to_fetch && value && value_lazy (value))
+    {
+      if (!gdb_value_fetch_lazy (value))
+	{
+	  var->error = 1;
+	  /* Set the value to NULL, so that for the next -var-update,
+	     we don't try to compare the new value with this value,
+	     that we couldn't even read.  */
+	  value = NULL;
+	}
+      else
+	var->error = 0;
+    }
+
+  /* If the type is changeable, compare the old and the new values.
+     If this is the initial assignment, we don't have any old value
+     to compare with.  */
+  if (!initial && changeable)
+    {
+      /* If the value of the varobj was changed by -var-set-value, then the 
+	 value in the varobj and in the target is the same.  However, that value
+	 is different from the value that the varobj had after the previous
+	 -var-update. So need to the varobj as changed.  */	 
+      if (var->updated)
+	changed = 1;
+      else 
+	{
+	  /* Try to compare the values.  That requires that both
+	     values are non-lazy.  */
+	  
+	  /* Quick comparison of NULL values.  */
+	  if (var->value == NULL && value == NULL)
+	    /* Equal. */
+	    ;
+	  else if (var->value == NULL || value == NULL)
+	    changed = 1;
+	  else
+	    {
+	      gdb_assert (!value_lazy (var->value));
+	      gdb_assert (!value_lazy (value));
+	      
+	      if (!value_contents_equal (var->value, value))
+		changed = 1;
+	    }
+	}
+    }
+    
+  /* We must always keep the new value, since children depend on it. */
+  if (var->value != NULL)
+    value_free (var->value);
+  var->value = value;
+  var->updated = 0;
+
+  return changed;
+}
+  
+
 /* Update the values for a variable and its children.  This is a
    two-pronged attack.  First, re-parse the value for the root's
    expression to see if it's changed.  Then go all the way
@@ -939,24 +1048,16 @@
       vpush (&result, *varp);
       changed++;
     }
-  /* If values are not equal, note that it's changed.
-     There a couple of exceptions here, though.
-     We don't want some types to be reported as "changed". */
-  else if (type_changeable (*varp) &&
-	   ((*varp)->updated || !my_value_equal ((*varp)->value, new, &error)))
+
+  if (install_new_value ((*varp), new, type_changed))
     {
-      vpush (&result, *varp);
-      (*varp)->updated = 0;
+      /* If type_changed is 1, install_new_value will never return
+	 non-zero, so we'll never report the same variable twice.  */
+      gdb_assert (!type_changed);
+      vpush (&result, (*varp));
       changed++;
-      /* Its value is going to be updated to NEW.  */
-      (*varp)->error = error;
     }
 
-  /* We must always keep around the new value for this root
-     variable expression, or we lose the updated children! */
-  value_free ((*varp)->value);
-  (*varp)->value = new;
-
   /* Initialize a stack */
   vpush (&stack, NULL);
 
@@ -982,22 +1083,14 @@
 
       /* Update this variable */
       new = value_of_child (v->parent, v->index);
-      if (type_changeable (v) && 
-          (v->updated || !my_value_equal (v->value, new, &error)))
-	{
+      if (install_new_value (v, new, 0 /* type not changed */))
+ 	{
 	  /* Note that it's changed */
 	  vpush (&result, v);
 	  v->updated = 0;
 	  changed++;
 	}
-      /* Its value is going to be updated to NEW.  */
-      v->error = error;
 
-      /* We must always keep new values, since children depend on it. */
-      if (v->value != NULL)
-	value_free (v->value);
-      v->value = new;
-
       /* Get next child */
       v = vpop (&stack);
     }
@@ -1257,15 +1350,14 @@
 {
   struct varobj *child;
   char *childs_name;
+  struct value *value;
 
   child = new_variable ();
 
   /* name is allocated by name_of_child */
   child->name = name;
   child->index = index;
-  child->value = value_of_child (parent, index);
-  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
-    child->error = 1;
+  value = value_of_child (parent, index);
   child->parent = parent;
   child->root = parent->root;
   childs_name = xstrprintf ("%s.%s", parent->obj_name, name);
@@ -1275,9 +1367,21 @@
   /* Save a pointer to this child in the parent */
   save_child_in_parent (parent, child);
 
-  /* Note the type of this child */
-  child->type = type_of_child (child);
+  /* Compute the type of the child.  Must do this before
+     calling install_new_value.  */
+  if (value != NULL)
+    /* If the child had no evaluation errors, var->value
+       will be non-NULL and contain a valid type. */
+    child->type = value_type (value);
+  else
+    /* Otherwise, we must compute the type. */
+    child->type = (*child->root->lang->type_of_child) (child->parent, 
+						       child->index);
+  install_new_value (child, value, 1);
 
+  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
+    child->error = 1;
+
   return child;
 }
 
@@ -1451,42 +1555,6 @@
   return FORMAT_NATURAL;
 }
 
-/* This function is similar to GDB's value_contents_equal, except that
-   this one is "safe"; it never longjmps.  It determines if the VAL1's
-   value is the same as VAL2.  If for some reason the value of VAR2
-   can't be established, *ERROR2 is set to non-zero.  */
-
-static int
-my_value_equal (struct value *val1, struct value *volatile val2, int *error2)
-{
-  volatile struct gdb_exception except;
-
-  /* As a special case, if both are null, we say they're equal.  */
-  if (val1 == NULL && val2 == NULL)
-    return 1;
-  else if (val1 == NULL || val2 == NULL)
-    return 0;
-
-  /* The contents of VAL1 are supposed to be known.  */
-  gdb_assert (!value_lazy (val1));
-
-  /* Make sure we also know the contents of VAL2.  */
-  val2 = coerce_array (val2);
-  TRY_CATCH (except, RETURN_MASK_ERROR)
-    {
-      if (value_lazy (val2))
-	value_fetch_lazy (val2);
-    }
-  if (except.reason < 0)
-    {
-      *error2 = 1;
-      return 0;
-    }
-  gdb_assert (!value_lazy (val2));
-
-  return value_contents_equal (val1, val2);
-}
-
 /* FIXME: The following should be generic for any pointer */
 static void
 vpush (struct vstack **pstack, struct varobj *var)
@@ -1678,33 +1746,9 @@
 
   value = (*parent->root->lang->value_of_child) (parent, index);
 
-  /* If we're being lazy, fetch the real value of the variable. */
-  if (value != NULL && value_lazy (value))
-    {
-      /* If we fail to fetch the value of the child, return
-         NULL so that callers notice that we're leaving an
-         error message. */
-      if (!gdb_value_fetch_lazy (value))
-	value = NULL;
-    }
-
   return value;
 }
 
-/* What is the type of VAR? */
-static struct type *
-type_of_child (struct varobj *var)
-{
-
-  /* If the child had no evaluation errors, var->value
-     will be non-NULL and contain a valid type. */
-  if (var->value != NULL)
-    return value_type (var->value);
-
-  /* Otherwise, we must compute the type. */
-  return (*var->root->lang->type_of_child) (var->parent, var->index);
-}
-
 /* Is this variable editable? Use the variable's type to make
    this determination. */
 static int
@@ -1720,9 +1764,15 @@
   return (*var->root->lang->value_of_variable) (var);
 }
 
-/* Is VAR something that can change? Depending on language,
-   some variable's values never change. For example,
-   struct and unions never change values. */
+/* Return non-zero if changes in value of VAR
+   must be detected and reported by -var-update.
+   Return zero is -var-update should never report
+   changes of such values.  This makes sense for structures
+   (since the changes in children values will be reported separately),
+   or for artifical objects (like 'public' pseudo-field in C++).
+
+   Return value of 0 means that gdb need not call value_fetch_lazy
+   for the value of this variable object.  */
 static int
 type_changeable (struct varobj *var)
 {
@@ -1898,19 +1948,7 @@
          go on */
       if (gdb_evaluate_expression (var->root->exp, &new_val))
 	{
-	  if (value_lazy (new_val))
-	    {
-	      /* We need to catch errors because if
-	         value_fetch_lazy fails we still want to continue
-	         (after making val->error = 1) */
-	      /* FIXME: Shouldn't be using value_contents()?  The
-	         comment on value_fetch_lazy() says it is only called
-	         from the macro... */
-	      if (!gdb_value_fetch_lazy (new_val))
-		var->error = 1;
-	      else
-		var->error = 0;
-	    }
+	  var->error = 0;
 	}
       else
 	var->error = 1;
@@ -2094,8 +2132,8 @@
 	    struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
 	    char *thevalue;
 
-	    if (value_lazy (var->value))
-	      gdb_value_fetch_lazy (var->value);
+	    gdb_assert (type_changeable (var));
+	    gdb_assert (!value_lazy (var->value));
 	    common_val_print (var->value, stb,
 			      format_code[(int) var->format], 1, 0, 0);
 	    thevalue = ui_file_xstrdup (stb, &dummy);

Property changes on: 
___________________________________________________________________
Name: svk:merge
 +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:154913

Re: Variable objects laziness

[Daniel Jacobowitz]

On Wed, Nov 15, 2006 at 12:04:08PM +0300, Vladimir Prus wrote:
> > Neither before nor after this patch do I really understand what
> > "type_changeable" means so I'll trust you've got it right :-)
> 
> Do you want to me add a comment to that function?

Since you seem to know what it's used for and why it relates to
fetching things, yes please.

> > Will we do eight reads from the target for this?
> > 
> >   struct x {
> >     unsigned char a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1;
> >   };
> 
> No. Look at value_primitive_field. For bitfeilds, it calls
> unpack_field_as_long, passing value_contents (args1) to it. The
> value_contents function will read entire structure from memory on the first
> time. For the second bitfield, the already-read data will be used.

Oh.  So non-bitfield members will be read lazily, but bitfield members
will cause a read of their immediate container.  That's a little
strange, but since I think it's exactly what we want to happen right
now, let's leave it alone :-)

> I think I can address this issue by making install_new_value always fetch
> values of unions.
> 
> Or I can do nothing, because in CLI, if your program is:
> 
>     struct S
>     {
>         union {
>            int i;
>            int j;
>         };
>     };
> 
>     int main()
>     {
>         S s;
>         .....
> 
> And you do:
> 
>     print s.i
>     print s.j
> 
> it will generate two reads of memory too.

Right - but only one read for "print s".  If you think fetching the
value of unions is reasonable, let's do that.

> What do you mean? The second line is indented 6 spaces, so it can use tab
> for indentation.

Sorry, you're right!

When you're ready, please repost.  Might want to finish talking with
Nick first.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Variable objects laziness

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Tue, Nov 14, 2006 at 04:43:24PM +0300, Vladimir Prus wrote:
>> At the moment, MI's variable objects are too eager. For example, if you
>> create a variable object corresponding to a structure, gdb will read the
>> entire structure from the memory, even though it never does anything with
>> that value directly. Structure values are not compared, and you can't
>> access them other than by creating children variable objects.
>> 
>> For example, if you have a huge structure containing two structure
>> fields, gdb will read all the outer structure even though you never open
>> one of the children structures.
>> 
>> Worse, the code for fetching the values is scattered over a number of
>> places.
> 
> I think this is a good patch.  I'm going to wait a few days before
> approving this, however, in case any of the other MI users on the list
> have comments.
> 
> Neither before nor after this patch do I really understand what
> "type_changeable" means so I'll trust you've got it right :-)

Do you want to me add a comment to that function?

> How does reading the children instead of the parent affect bitfields?

Good question, but I have a good answer -- the patch does not change this.
The only effect that instead of reading the structure right when you create
it, the structure will be read when you list the children.

> Will we do eight reads from the target for this?
> 
>   struct x {
>     unsigned char a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1;
>   };

No. Look at value_primitive_field. For bitfeilds, it calls
unpack_field_as_long, passing value_contents (args1) to it. The
value_contents function will read entire structure from memory on the first
time. For the second bitfield, the already-read data will be used.

> How about unions?

Here's what will happen for a union:

1. The value_primitive_field will be called to create values for all
elements of a union.

2. When value_fetch_lazy is called on each value, it will directly read
the content from the memory. Once for each value.

If the union's value is not lazy (like it is today), value_primitive_field
will immediately memcpy the value, and target memory will be read just once.

I think I can address this issue by making install_new_value always fetch
values of unions.

Or I can do nothing, because in CLI, if your program is:

    struct S
    {
        union {
           int i;
           int j;
        };
    };

    int main()
    {
        S s;
        .....

And you do:

    print s.i
    print s.j

it will generate two reads of memory too.

The only way to consistently fix this problem everywhere is to add 'parent'
field to struct value, so that we can lazily fetch parent, and do it just
once.


>> +    value = evaluate_type (var->root->exp);
>> +
>> +      var->type = value_type (value);
> 
> Space/tabs mismatch.

What do you mean? The second line is indented 6 spaces, so it can use tab
for indentation.

>> +/** Assign new value to a variable object.  If INITIAL is non-zero,
> 
> Javadoc?  Not here :-)  

Why not? ;-) It would be great to get API docs fro gdb. But year, it's
probably easier to teach doxygen to handle ordinary comments.

>> +  /* If the type is changeable, compare the old and the new values.
>> +     If the type of varobj changed, no point in comparing values.  */
> 
> "the type of the varobj".

In fact, the second sentence is stale, I've reworded it like this:

   If this is the initial assignment, we don't have any old value
   to compare with.

>> +  if (!initial && changeable)
>> +    {
>> +      /* If the value of the varobj was set by -var-set-value, then the
>> +     value in the varobj and in the target is the same.  However, that
>> value
>> +     is different from the value that the varobj had after the previous
>> +     -var-update. So need to the varobj as changed.  */
>> +      if (var->updated)
>> +    changed = 1;
> 
> "need to mark the".  Why is this true?  I would have thought we only
> needed to mark the varobj as changed if the value assigned to it was
> different from the previous one.  

True, the 'updated' flag is set only in that case. I've modified the comment
to read:

    /* If the value of the varobj was changed by -var-set-value, then the

>> +  /* We must always keep new values, since children depend on it. */
> 
> "values" is plural, "it" is singular, so this sounds strange; probably
> "must always keep the new value".

This comment was present in the old code. Fixed anyway.

Thanks,
Volodya

Re: Variable objects laziness

[Daniel Jacobowitz]

On Tue, Nov 14, 2006 at 04:43:24PM +0300, Vladimir Prus wrote:
> At the moment, MI's variable objects are too eager. For example, if you create 
> a variable object corresponding to a structure, gdb will read the entire 
> structure from the memory, even though it never does anything with that value 
> directly. Structure values are not compared, and you can't access them other 
> than by creating children variable objects.
> 
> For example, if you have a huge structure containing two structure fields, gdb 
> will read all the outer structure even though you never open one of the 
> children structures.
> 
> Worse, the code for fetching the values is scattered over a number of places.

I think this is a good patch.  I'm going to wait a few days before
approving this, however, in case any of the other MI users on the list
have comments.

Neither before nor after this patch do I really understand what
"type_changeable" means so I'll trust you've got it right :-)

How does reading the children instead of the parent affect bitfields?
Will we do eight reads from the target for this?

  struct x {
    unsigned char a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1;
  };

How about unions?

> +static int
> +install_new_value (struct varobj *var, struct value *value, int initial);

No newline, since this is a prototype.

> +	value = evaluate_type (var->root->exp);
> +
> +      var->type = value_type (value);

Space/tabs mismatch.

> -      var->type = value_type (var->value);
> +      install_new_value (var, value, 1 /* Initial assignment. */);

Probably don't need the period here, since this isn't supposed to be a
full sentence.  If you did need the period, you'd need another space
after it.

> +      /* All types are the editable must also be changeable.  */

"All types that are editable"

> +      /* Value of a changeable variable object must not be lazy.  */

"The value of"

> +      /* The new value may be lazy, gdb_value_assign, or 
> +	 rather value_contents, will take care of this.  */

Don't use a comma to separate what are basically independent sentences;
you should use a period between "lazy" and "gdb_value_assign".

> +/** Assign new value to a variable object.  If INITIAL is non-zero,

Javadoc?  Not here :-)  Article missing, "Assign a new value" or
"Assign VALUE to VAR".

> +    this is first assignement after the variable object was just

"is the first".

> +  /* The new value might be lazy.  If the type is changeable,
> +     that is we'll be comparing values of this type, fetch the
> +     value now.  Otherwise, on the next update the old value
> +     will be lazy, which means we've lost that old value.
> +     We do this for frozen varobjs as well, since this
> +     function is only called when it's decided that we need
> +     to fetch the new value of a frozen variable.  */

Stray frozen varobjs comment.

> +	  /* Set the value to NULL, so that for the next -var-update,
> +	     we don't try to compare the new value with this value,
> +	     that we can't even read.  */

"this value that we couldn't".

> +  /* If the type is changeable, compare the old and the new values.
> +     If the type of varobj changed, no point in comparing values.  */

"the type of the varobj".

> +  if (!initial && changeable)
> +    {
> +      /* If the value of the varobj was set by -var-set-value, then the 
> +	 value in the varobj and in the target is the same.  However, that value
> +	 is different from the value that the varobj had after the previous
> +	 -var-update. So need to the varobj as changed.  */	 
> +      if (var->updated)
> +	changed = 1;

"need to mark the".  Why is this true?  I would have thought we only
needed to mark the varobj as changed if the value assigned to it was
different from the previous one.  Oh, I see this was what it did
before.  OK.

> +  /* We must always keep new values, since children depend on it. */

"values" is plural, "it" is singular, so this sounds strange; probably
"must always keep the new value".

-- 
Daniel Jacobowitz
CodeSourcery

Variable objects laziness

[Vladimir Prus]

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


Hello!

At the moment, MI's variable objects are too eager. For example, if you create 
a variable object corresponding to a structure, gdb will read the entire 
structure from the memory, even though it never does anything with that value 
directly. Structure values are not compared, and you can't access them other 
than by creating children variable objects.

For example, if you have a huge structure containing two structure fields, gdb 
will read all the outer structure even though you never open one of the 
children structures.

Worse, the code for fetching the values is scattered over a number of places.

This patch cleans up all this:

       - There's just one function that decides if we must fetch the value
       - The decision is consistent with the way gdb uses variable objects

No regressions on gdb.mi testsuite on x86. OK?

- Volodya

	* varobj.c (struct varobj): Clarify comment.
	(my_value_equal): Remove.
	(install_new_value): New.
	(type_of_child): Remove.
	(varobj_create): Use install_new_value.
	(varobj_set_value): Use value_contents_equal, not
	my_value_equal.
	(varobj_update): Use install_new_value.
	(create_child): Likewise. Inline type_of_child here.
	(value_of_child): Don't fetch the value.
	(c_value_of_root): Likewise.
	(c_value_of_variable): Likewise.

[-- Attachment #2: laziness.diff --]
[-- Type: text/x-diff, Size: 13160 bytes --]

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.60
diff -u -p -r1.60 varobj.c
--- varobj.c	3 May 2006 22:59:38 -0000	1.60
+++ varobj.c	14 Nov 2006 13:38:35 -0000
@@ -101,7 +101,9 @@ struct varobj
   /* The type of this variable. This may NEVER be NULL. */
   struct type *type;
 
-  /* The value of this expression or subexpression.  This may be NULL. */
+  /* The value of this expression or subexpression.  This may be NULL. 
+     Invariant: if type_changeable (this) is non-zero, the value is either
+     NULL, or not lazy.  */
   struct value *value;
 
   /* Did an error occur evaluating the expression or getting its value? */
@@ -202,8 +204,6 @@ static struct type *get_target_type (str
 
 static enum varobj_display_formats variable_default_display (struct varobj *);
 
-static int my_value_equal (struct value *, struct value *, int *);
-
 static void vpush (struct vstack **pstack, struct varobj *var);
 
 static struct varobj *vpop (struct vstack **pstack);
@@ -212,6 +212,9 @@ static void cppush (struct cpstack **pst
 
 static char *cppop (struct cpstack **pstack);
 
+static int
+install_new_value (struct varobj *var, struct value *value, int initial);
+
 /* Language-specific routines. */
 
 static enum varobj_languages variable_language (struct varobj *var);
@@ -226,8 +229,6 @@ static struct value *value_of_root (stru
 
 static struct value *value_of_child (struct varobj *parent, int index);
 
-static struct type *type_of_child (struct varobj *var);
-
 static int variable_editable (struct varobj *var);
 
 static char *my_value_of_variable (struct varobj *var);
@@ -445,6 +446,7 @@ varobj_create (char *objname,
     {
       char *p;
       enum varobj_languages lang;
+      struct value *value;
 
       /* Parse and evaluate the expression, filling in as much
          of the variable's data as possible */
@@ -505,17 +507,17 @@ varobj_create (char *objname,
       /* 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))
+      if (gdb_evaluate_expression (var->root->exp, &value))
 	{
 	  /* no error */
-	  release_value (var->value);
-	  if (value_lazy (var->value))
-	    gdb_value_fetch_lazy (var->value);
+	  release_value (value);
 	}
       else
-	var->value = evaluate_type (var->root->exp);
+	value = evaluate_type (var->root->exp);
+
+      var->type = value_type (value);
 
-      var->type = value_type (var->value);
+      install_new_value (var, value, 1 /* Initial assignment. */);
 
       /* Set language info */
       lang = variable_language (var);
@@ -825,8 +827,17 @@ varobj_set_value (struct varobj *var, ch
 	  return 0;
 	}
 
-      if (!my_value_equal (var->value, value, &error))
+      /* All types are the editable must also be changeable.  */
+      gdb_assert (type_changeable (var));
+
+      /* Value of a changeable variable object must not be lazy.  */
+      gdb_assert (!value_lazy (var->value));
+
+      if (!value_contents_equal (var->value, value))
         var->updated = 1;
+
+      /* The new value may be lazy, gdb_value_assign, or 
+	 rather value_contents, will take care of this.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
       value_free (var->value);
@@ -870,6 +881,92 @@ varobj_list (struct varobj ***varlist)
   return rootcount;
 }
 
+/** Assign new value to a variable object.  If INITIAL is non-zero,
+    this is first assignement after the variable object was just
+    created, or changed type.  In that case, just assign the value 
+    and return 0.
+    Otherwise, assign the value and if type_changeable returns non-zero,
+    find if the new value is different from the current value.
+    Return 1 if so, and 0 is the values are equal.  */
+static int
+install_new_value (struct varobj *var, struct value *value, int initial)
+{ 
+  int changeable;
+  int changed = 0;
+
+  var->error = 0;
+  /* We need to know the varobj's type to decide if the value should
+     be fetched or not.  C++ fake children (public/protected/private) don't have
+     a type. */
+  gdb_assert (var->type || CPLUS_FAKE_CHILD (var));
+  if (CPLUS_FAKE_CHILD (var))
+    changeable = 0;
+  else
+    changeable = type_changeable (var);
+
+  /* The new value might be lazy.  If the type is changeable,
+     that is we'll be comparing values of this type, fetch the
+     value now.  Otherwise, on the next update the old value
+     will be lazy, which means we've lost that old value.
+     We do this for frozen varobjs as well, since this
+     function is only called when it's decided that we need
+     to fetch the new value of a frozen variable.  */
+  if (changeable && value && value_lazy (value))
+    {
+      if (!gdb_value_fetch_lazy (value))
+	{
+	  var->error = 1;
+	  /* Set the value to NULL, so that for the next -var-update,
+	     we don't try to compare the new value with this value,
+	     that we can't even read.  */
+	  value = NULL;
+	}
+      else
+	var->error = 0;
+    }
+
+  /* If the type is changeable, compare the old and the new values.
+     If the type of varobj changed, no point in comparing values.  */
+  if (!initial && changeable)
+    {
+      /* If the value of the varobj was set by -var-set-value, then the 
+	 value in the varobj and in the target is the same.  However, that value
+	 is different from the value that the varobj had after the previous
+	 -var-update. So need to the varobj as changed.  */	 
+      if (var->updated)
+	changed = 1;
+      else 
+	{
+	  /* Try to compare the values.  That requires that both
+	     values are non-lazy.  */
+	  
+	  /* Quick comparison of NULL values.  */
+	  if (var->value == NULL && value == NULL)
+	    /* Equal. */
+	    ;
+	  else if (var->value == NULL || value == NULL)
+	    changed = 1;
+	  else
+	    {
+	      gdb_assert (!value_lazy (var->value));
+	      gdb_assert (!value_lazy (value));
+	      
+	      if (!value_contents_equal (var->value, value))
+		changed = 1;
+	    }
+	}
+    }
+    
+  /* We must always keep new values, since children depend on it. */
+  if (var->value != NULL)
+    value_free (var->value);
+  var->value = value;
+  var->updated = 0;
+
+  return changed;
+}
+  
+
 /* Update the values for a variable and its children.  This is a
    two-pronged attack.  First, re-parse the value for the root's
    expression to see if it's changed.  Then go all the way
@@ -939,24 +1036,16 @@ varobj_update (struct varobj **varp, str
       vpush (&result, *varp);
       changed++;
     }
-  /* If values are not equal, note that it's changed.
-     There a couple of exceptions here, though.
-     We don't want some types to be reported as "changed". */
-  else if (type_changeable (*varp) &&
-	   ((*varp)->updated || !my_value_equal ((*varp)->value, new, &error)))
+
+  if (install_new_value ((*varp), new, type_changed))
     {
-      vpush (&result, *varp);
-      (*varp)->updated = 0;
+      /* If type_changed is 1, install_new_value will never return
+	 non-zero, so we'll never report the same variable twice.  */
+      gdb_assert (!type_changed);
+      vpush (&result, (*varp));
       changed++;
-      /* Its value is going to be updated to NEW.  */
-      (*varp)->error = error;
     }
 
-  /* We must always keep around the new value for this root
-     variable expression, or we lose the updated children! */
-  value_free ((*varp)->value);
-  (*varp)->value = new;
-
   /* Initialize a stack */
   vpush (&stack, NULL);
 
@@ -982,21 +1071,13 @@ varobj_update (struct varobj **varp, str
 
       /* Update this variable */
       new = value_of_child (v->parent, v->index);
-      if (type_changeable (v) && 
-          (v->updated || !my_value_equal (v->value, new, &error)))
-	{
+      if (install_new_value (v, new, 0 /* type not changed */))
+ 	{
 	  /* Note that it's changed */
 	  vpush (&result, v);
 	  v->updated = 0;
 	  changed++;
 	}
-      /* Its value is going to be updated to NEW.  */
-      v->error = error;
-
-      /* We must always keep new values, since children depend on it. */
-      if (v->value != NULL)
-	value_free (v->value);
-      v->value = new;
 
       /* Get next child */
       v = vpop (&stack);
@@ -1257,15 +1338,14 @@ create_child (struct varobj *parent, int
 {
   struct varobj *child;
   char *childs_name;
+  struct value *value;
 
   child = new_variable ();
 
   /* name is allocated by name_of_child */
   child->name = name;
   child->index = index;
-  child->value = value_of_child (parent, index);
-  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
-    child->error = 1;
+  value = value_of_child (parent, index);
   child->parent = parent;
   child->root = parent->root;
   childs_name = xstrprintf ("%s.%s", parent->obj_name, name);
@@ -1275,8 +1355,20 @@ create_child (struct varobj *parent, int
   /* Save a pointer to this child in the parent */
   save_child_in_parent (parent, child);
 
-  /* Note the type of this child */
-  child->type = type_of_child (child);
+  /* Compute the type of the child.  Must do this before
+     calling install_new_value.  */
+  if (value != NULL)
+    /* If the child had no evaluation errors, var->value
+       will be non-NULL and contain a valid type. */
+    child->type = value_type (value);
+  else
+    /* Otherwise, we must compute the type. */
+    child->type = (*child->root->lang->type_of_child) (child->parent, 
+						       child->index);
+  install_new_value (child, value, 1);
+
+  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
+    child->error = 1;
 
   return child;
 }
@@ -1451,42 +1543,6 @@ variable_default_display (struct varobj 
   return FORMAT_NATURAL;
 }
 
-/* This function is similar to GDB's value_contents_equal, except that
-   this one is "safe"; it never longjmps.  It determines if the VAL1's
-   value is the same as VAL2.  If for some reason the value of VAR2
-   can't be established, *ERROR2 is set to non-zero.  */
-
-static int
-my_value_equal (struct value *val1, struct value *volatile val2, int *error2)
-{
-  volatile struct gdb_exception except;
-
-  /* As a special case, if both are null, we say they're equal.  */
-  if (val1 == NULL && val2 == NULL)
-    return 1;
-  else if (val1 == NULL || val2 == NULL)
-    return 0;
-
-  /* The contents of VAL1 are supposed to be known.  */
-  gdb_assert (!value_lazy (val1));
-
-  /* Make sure we also know the contents of VAL2.  */
-  val2 = coerce_array (val2);
-  TRY_CATCH (except, RETURN_MASK_ERROR)
-    {
-      if (value_lazy (val2))
-	value_fetch_lazy (val2);
-    }
-  if (except.reason < 0)
-    {
-      *error2 = 1;
-      return 0;
-    }
-  gdb_assert (!value_lazy (val2));
-
-  return value_contents_equal (val1, val2);
-}
-
 /* FIXME: The following should be generic for any pointer */
 static void
 vpush (struct vstack **pstack, struct varobj *var)
@@ -1678,33 +1734,9 @@ value_of_child (struct varobj *parent, i
 
   value = (*parent->root->lang->value_of_child) (parent, index);
 
-  /* If we're being lazy, fetch the real value of the variable. */
-  if (value != NULL && value_lazy (value))
-    {
-      /* If we fail to fetch the value of the child, return
-         NULL so that callers notice that we're leaving an
-         error message. */
-      if (!gdb_value_fetch_lazy (value))
-	value = NULL;
-    }
-
   return value;
 }
 
-/* What is the type of VAR? */
-static struct type *
-type_of_child (struct varobj *var)
-{
-
-  /* If the child had no evaluation errors, var->value
-     will be non-NULL and contain a valid type. */
-  if (var->value != NULL)
-    return value_type (var->value);
-
-  /* Otherwise, we must compute the type. */
-  return (*var->root->lang->type_of_child) (var->parent, var->index);
-}
-
 /* Is this variable editable? Use the variable's type to make
    this determination. */
 static int
@@ -1898,19 +1930,7 @@ c_value_of_root (struct varobj **var_han
          go on */
       if (gdb_evaluate_expression (var->root->exp, &new_val))
 	{
-	  if (value_lazy (new_val))
-	    {
-	      /* We need to catch errors because if
-	         value_fetch_lazy fails we still want to continue
-	         (after making val->error = 1) */
-	      /* FIXME: Shouldn't be using value_contents()?  The
-	         comment on value_fetch_lazy() says it is only called
-	         from the macro... */
-	      if (!gdb_value_fetch_lazy (new_val))
-		var->error = 1;
-	      else
-		var->error = 0;
-	    }
+	  var->error = 0;
 	}
       else
 	var->error = 1;
@@ -2094,8 +2114,8 @@ c_value_of_variable (struct varobj *var)
 	    struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
 	    char *thevalue;
 
-	    if (value_lazy (var->value))
-	      gdb_value_fetch_lazy (var->value);
+	    gdb_assert (type_changeable (var));
+	    gdb_assert (!value_lazy (var->value));
 	    common_val_print (var->value, stb,
 			      format_code[(int) var->format], 1, 0, 0);
 	    thevalue = ui_file_xstrdup (stb, &dummy);