This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Variable objects laziness


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




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]