Re: [PATCH] -var-update

Re: [PATCH] -var-update

[Nick Roberts]

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

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

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

>                                         BTW, why coerce_ref is not suitable
> here?

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

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

Re: [PATCH] -var-update

[Daniel Jacobowitz]

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

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

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


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update

[Nick Roberts]

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

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

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

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

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

Re: [PATCH] -var-update

[Daniel Jacobowitz]

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

No:

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

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

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

Try calling coerce_ref in here:

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

/* HERE */

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


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update

[Nick Roberts]

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

OK.  Perhaps I called it after value_fetch_lazy.  

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

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

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

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


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

Re: [PATCH] -var-update

[Nick Roberts]

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

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

int i;
int& ri = i;

and I do:
-var-create - * ri

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

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

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

[patch] Fixes problem setting breakpoint in dynamic loader

[PAUL GILLIAM]

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

On PowerPC-64, with 64-bit executables, GDB has been giving this message
for a while:

        warning: Unable to find dynamic linker breakpoint function.
        GDB will be unable to debug shared library initializers
        and track explicitly loaded dynamic code.

This is because "enable_break()" in solib-svr4.c was looking for the
symbol "._dl_debug_state" in the 64-bit dynamic loader and not finding
it.  This should not be a surprise because these 'dot' symbols have not
been used for a while.

The reason that the non-'dot' symbol was also passed by, is that it
points into a data section and the existing code only checked code
sections.

If none of the symbols on the list was found in a code section, the
attached patch looks in data sections.  When it finds one, and the data
section is either '.plt' or '.opd', then the address points to a
function descriptor which is then used to find the corresponding code
address where the breakpoint can be set.

OK to commit?

-=# Paul #=-



[-- Attachment #2: loader_break.diff --]
[-- Type: text/x-patch, Size: 4204 bytes --]

2006-05-24  Paul Gilliam  <pgilliam@us.ibm.com

	* solib-svr4.c (enable_break): Resolve break address when the
	symbol is found in the data section.

diff -Naur old/solib-svr4.c new/solib-svr4.c
--- old/solib-svr4.c	2006-05-24 15:50:42.000000000 -0700
+++ new/solib-svr4.c	2006-05-24 15:51:36.000000000 -0700
@@ -85,16 +85,6 @@
   "rtld_db_dlactivity",
   "_rtld_debug_state",
 
-  /* On the 64-bit PowerPC, the linker symbol with the same name as
-     the C function points to a function descriptor, not to the entry
-     point.  The linker symbol whose name is the C function name
-     prefixed with a '.' points to the function's entry point.  So
-     when we look through this table, we ignore symbols that point
-     into the data section (thus skipping the descriptor's symbol),
-     and eventually try this one, giving us the real entry point
-     address.  */
-  "._dl_debug_state",
-
   NULL
 };
 
@@ -1043,20 +1033,75 @@
       /* Now try to set a breakpoint in the dynamic linker.  */
       for (bkpt_namep = solib_break_names; *bkpt_namep != NULL; bkpt_namep++)
 	{
-          /* On ABI's that use function descriptors, there are usually
-             two linker symbols associated with each C function: one
-             pointing at the actual entry point of the machine code,
-             and one pointing at the function's descriptor.  The
-             latter symbol has the same name as the C function.
-
-             What we're looking for here is the machine code entry
-             point, so we are only interested in symbols in code
-             sections.  */
+	  /* What we're looking for here is the machine code entry point,
+	     so we are only interested in symbols in code sections.
+
+	     On ABI's that use function descriptors, the linker symbol with
+	     the same name as a C funtion points to that functions descriptor.
+	     when those function descriptors are in the code section, they
+	     contain executable code and we can set a breakpoint there. */
 	  sym_addr = bfd_lookup_symbol (tmp_bfd, *bkpt_namep, SEC_CODE);
 	  if (sym_addr != 0)
 	    break;
 	}
 
+      if (sym_addr == 0)
+        {
+	  CORE_ADDR sect_offset;
+	  
+	  /* No symbol was found in a code section, so look in the data
+             sections.  This will only happen when the linker symbol points
+	     to a function descriptor that is in a data section. */
+	  for (bkpt_namep = solib_break_names; *bkpt_namep!=NULL; bkpt_namep++)
+	    {
+	      /* On ABI's that use function descriptors that are in the data
+	         section,
+	      sym_addr = bfd_lookup_symbol (tmp_bfd, *bkpt_namep, SEC_DATA);
+	      if (sym_addr != 0)
+		break;
+	    }
+	  if (sym_addr == 0)
+	    {
+	      target_close (tmp_bfd_target, 0);
+	      goto bkpt_at_symbol;
+	    }
+
+	  /* On some ABI's, the function descriptor we need will be in the
+	     ".plt" section.  In others, it will be in the ".opd" section. */
+	  if (sym_addr + load_addr >= interp_plt_sect_low
+              && sym_addr + load_addr < interp_plt_sect_high)
+	    {
+	      interp_sect = bfd_get_section_by_name (tmp_bfd, ".plt");
+	      sect_offset = interp_plt_sect_low - load_addr;
+	    }
+	  else
+	    {
+	      interp_sect = bfd_get_section_by_name (tmp_bfd, ".opd");
+	      if (interp_sect != 0)
+		{
+		  sect_offset = bfd_section_vma (tmp_bfd, interp_sect);
+		  if (sym_addr < sect_offset)
+		    interp_sect == 0;
+		  else if (sym_addr - sect_offset >=
+				       bfd_section_size (tmp_bfd, interp_sect))
+		    interp_sect == 0;
+		}
+	    }
+	  if (interp_sect != 0)
+	    {
+	      /* Try to convert the function descriptor we found above, into
+		 the address we need.  It will be relocated below by adding
+		 "load_addr" to it. */
+	      char *buf = alloca (sizeof (LONGEST));
+	      if (bfd_get_section_contents (tmp_bfd, interp_sect, buf,
+					    sym_addr - sect_offset,
+					    sizeof (LONGEST)))
+		sym_addr = extract_unsigned_integer (buf, sizeof (LONGEST));
+	      else
+		sym_addr = 0;
+	    }
+        }
+
       /* We're done with both the temporary bfd and target.  Remember,
          closing the target closes the underlying bfd.  */
       target_close (tmp_bfd_target, 0);

Re: [patch] Fixes problem setting breakpoint in dynamic loader

[PAUL GILLIAM]

Sorry, this got put in the wrong mail list thread.  Let me try again.
-=# Paul #=-

Re: Variable objects: references formatting

[Nick Roberts]

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

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

> Changelog:

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

> Patch attached.

> Thanks,
> Volodya

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

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

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

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

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

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

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


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

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

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


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

Re: Variable objects: references formatting

[Vladimir Prus]

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

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

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

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

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

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

- Volodya

Re: Variable objects: references formatting

[Nick Roberts]

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

Thanks.

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

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

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

Re: Variable objects: references formatting

[Vladimir Prus]

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

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

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

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

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

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

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

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

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


- Volodya

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

[Nick Roberts]

 > > However, do you see the problem with references that I mentioned earlier
 > > (that they don't seem to disappear from the changelist with -var-update)?
 > > This seems to be the case for any variable object made from a reference.
 > 
 > Yes, I see that for variable object created from reference, -var-update * 
 > always mentions that. Looking at this under debugger, it seems that the call 
 > to my_value_equal in varobj_update compares the value of *reference* to the 
 > value of new *referenced-to* object. This happens in my_value_equal 
 > (varobj.c):
 >
 >  static int
 >  my_value_equal (struct value *val1, struct value *volatile val2, int
 >   *error2)
 > ...

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

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

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


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



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

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


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

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

[Daniel Jacobowitz]

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

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

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

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

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

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

-- 
Daniel Jacobowitz
CodeSourcery

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

[Nick Roberts]

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

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

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

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

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

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

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

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

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

[Daniel Jacobowitz]

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

I am not sure what you mean.

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

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

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

-- 
Daniel Jacobowitz
CodeSourcery

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

[Nick Roberts]

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

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

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

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

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

[Daniel Jacobowitz]

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

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

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

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] -var-update

[Nick Roberts]

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

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

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

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


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

Re: [PATCH] -var-update

[Vladimir Prus]

Nick Roberts wrote:

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

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

- Volodya