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: [PATCH] -var-update

[Vladimir Prus]

Nick Roberts wrote:

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

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

- Volodya


 

Re: [PATCH] -var-update

[Nick Roberts]

 > 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;
      }