Re: [RFA] MIPS16 FP manual call/return fixes

["Maciej W. Rozycki" <macro@codesourcery.com>]

Jan,

> > We must have somehow established its address previously or we couldn't 
> > have called it.  Is it possible to cache it somehow for example?
> 
> Yes, we could.  In set_breakpoint_location_function is created
> bp_gnu_ifunc_resolver, so cache it into 'struct bp_location' there (or even
> into 'struct breakpoint', I do not see too much difference there)..

 Well, it's associated with a specific location really, so...

> Then transfer this info when bp_gnu_ifunc_resolver_return is created from that
> bp_gnu_ifunc_resolver.

 Given that we rely on having a reference to bp_gnu_ifunc_resolver from 
bp_gnu_ifunc_resolver_return I've decided that we don't really have to 
transfer anything here.

> I would be fine also with just some error there.

 I'm not sure what you mean, we do need to return something here.

> >  Thanks for raising the issue of unloading symbol tables, that's an 
> > important point as to how MIPS16 and microMIPS symbols should be treated 
> > in general -- here I think it will only matter in the asynchronous mode. 
> 
> No, even in synchronous mode.  If you still do "stepi", "stepi", "stepi"...
> you will get something like:
> 
> (gdb) start
> (gdb) b strcmp
> Breakpoint 2 at gnu-indirect-function resolver at 0x7ffff7aaa3d0: file ../sysdeps/x86_64/multiarch/strcmp.S, line 87.
> (gdb) b *strcmp
> Note: breakpoint 2 also set at pc 0x7ffff7aaa3d0.
> Breakpoint 3 at 0x7ffff7aaa3d0: file ../sysdeps/x86_64/multiarch/strcmp.S, line 87.
> (gdb) c
> Continuing.
> warning: Breakpoint 2 address previously adjusted from 0x004003c6 to 0x7ffff7aaa3d0.
> Breakpoint 2, strcmp () at ../sysdeps/x86_64/multiarch/strcmp.S:87
> 87		cmpl	$0, __cpu_features+KIND_OFFSET(%rip)
> (gdb) maintenance info breakpoints 
> Num     Type                          Disp Enb Address            What
> [...]
> 2       STT_GNU_IFUNC resolver        keep y   0x00007ffff7aaa3d0 ../sysdeps/x86_64/multiarch/strcmp.S:87 inf 1
> 	breakpoint already hit 1 time
> 3       breakpoint                    keep y   0x00007ffff7aaa3d0 ../sysdeps/x86_64/multiarch/strcmp.S:87 inf 1
> 	breakpoint already hit 1 time
> 0       STT_GNU_IFUNC resolver return keep y   0x00007ffff7deb3be <_dl_fixup+446> inf 1 thread 1
> 	stop only in thread 1
> 
> So you can see anything can happen now before we hit that breakpoint #0.
> I can for example unload the glibc symbol file by 'nosharedlibrary' (which has
> led to unrelated PR 14054 now).

 Good point, it's turned out I was confused what the use case of this code 
exactly is.

 So here's the new proposal, comprising only the relevant bits (I keep it 
all with the rest of the patch really, but decided not to clutter the list 
with), it doesn't cause any regressions compared to the original.

 I have decided to add a related_address field to bp_location; this could 
get converted to an opaque member in the future in case some other 
breakpoints need to carry other auxiliary information (just as we do with 
some other structures).  It's set to the address of the resolver as 
set_breakpoint_location_function sets up a bp_gnu_ifunc_resolver 
breakpoint; conveniently it already looks up the function, just did not 
request its address before.

 I have decided to keep the assertion you had concerns about after all.  
The reason is as set_breakpoint_location_function will never create a 
bp_gnu_ifunc_resolver breakpoint with more than one location assigned 
anyway, see the condition enclosing:

 	      b->type = bp_gnu_ifunc_resolver;

-- it already requires loc->next to be NULL or it doesn't select this 
special breakpoint type at all (the intent to avoid such breakpoints is 
commented on too; the comment even qualified for context of the patch 
below, so you can just read it here).  Is it ever possible for this 
breakpoint to be updated later on with additional locations?  What would 
the conditions be if so? -- I'd like to be able to reproduce them in the 
lab then.

 I have tested this change with the mips-sde-elf target observing no 
regressions compared to the previous proposal.  I did some manual checks 
running GDB under GDB to see the expected flow of changes is correct here, 
using the test program used by gdb.base/gnu-ifunc.exp; this was with the 
x86_64-linux-gnu target.  It worked exactly as expected.

 OK for this update?  I believe this is the last part of the whole change 
there are unresolved concerns about.

2012-05-08  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* breakpoint.h (bp_location): Add related_address member.
	* breakpoint.c (set_breakpoint_location_function): Initialize it 
	for bp_gnu_ifunc_resolver breakpoints.
	* elfread.c (elf_gnu_ifunc_resolver_return_stop): Pass the
	requested function's address to gdbarch_return_value.

  Maciej

gdb-mips16-calls-ifunc-new.diff
Index: gdb-fsf-trunk-quilt/gdb/elfread.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/elfread.c	2012-05-08 08:25:37.795639727 +0100
+++ gdb-fsf-trunk-quilt/gdb/elfread.c	2012-05-08 09:42:32.075560588 +0100
@@ -1020,21 +1020,15 @@ elf_gnu_ifunc_resolver_return_stop (stru
   struct type *func_func_type = builtin_type (gdbarch)->builtin_func_func;
   struct type *value_type = TYPE_TARGET_TYPE (func_func_type);
   struct regcache *regcache = get_thread_regcache (inferior_ptid);
+  struct value *func_func;
   struct value *value;
   CORE_ADDR resolved_address, resolved_pc;
   struct symtab_and_line sal;
   struct symtabs_and_lines sals, sals_end;
+  CORE_ADDR func_func_addr;
 
   gdb_assert (b->type == bp_gnu_ifunc_resolver_return);
 
-  value = allocate_value (value_type);
-  gdbarch_return_value (gdbarch, func_func_type, value_type, regcache,
-			value_contents_raw (value), NULL);
-  resolved_address = value_as_address (value);
-  resolved_pc = gdbarch_convert_from_func_ptr_addr (gdbarch,
-						    resolved_address,
-						    &current_target);
-
   while (b->related_breakpoint != b)
     {
       struct breakpoint *b_next = b->related_breakpoint;
@@ -1055,6 +1049,18 @@ elf_gnu_ifunc_resolver_return_stop (stru
       b = b_next;
     }
   gdb_assert (b->type == bp_gnu_ifunc_resolver);
+  gdb_assert (b->loc->next == NULL);
+
+  func_func = allocate_value (func_func_type);
+  set_value_address (func_func, b->loc->related_address);
+
+  value = allocate_value (value_type);
+  gdbarch_return_value (gdbarch, func_func, value_type, regcache,
+			value_contents_raw (value), NULL);
+  resolved_address = value_as_address (value);
+  resolved_pc = gdbarch_convert_from_func_ptr_addr (gdbarch,
+						    resolved_address,
+						    &current_target);
 
   gdb_assert (current_program_space == b->pspace || b->pspace == NULL);
   elf_gnu_ifunc_record_cache (b->addr_string, resolved_pc);
Index: gdb-fsf-trunk-quilt/gdb/breakpoint.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/breakpoint.c	2012-05-04 19:26:24.845606346 +0100
+++ gdb-fsf-trunk-quilt/gdb/breakpoint.c	2012-05-08 08:26:49.855603092 +0100
@@ -6586,9 +6586,10 @@ set_breakpoint_location_function (struct
     {
       int is_gnu_ifunc;
       const char *function_name;
+      CORE_ADDR func_addr;
 
       find_pc_partial_function_gnu_ifunc (loc->address, &function_name,
-					  NULL, NULL, &is_gnu_ifunc);
+					  &func_addr, NULL, &is_gnu_ifunc);
 
       if (is_gnu_ifunc && !explicit_loc)
 	{
@@ -6609,6 +6610,9 @@ set_breakpoint_location_function (struct
 	      /* Create only the whole new breakpoint of this type but do not
 		 mess more complicated breakpoints with multiple locations.  */
 	      b->type = bp_gnu_ifunc_resolver;
+	      /* Remember the resolver's address for use by the return
+	         breakpoint.  */
+	      loc->related_address = func_addr;
 	    }
 	}
 
Index: gdb-fsf-trunk-quilt/gdb/breakpoint.h
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/breakpoint.h	2012-05-04 19:26:24.845606346 +0100
+++ gdb-fsf-trunk-quilt/gdb/breakpoint.h	2012-05-08 08:26:49.875618479 +0100
@@ -418,6 +418,11 @@ struct bp_location
      processor's architectual constraints.  */
   CORE_ADDR requested_address;
 
+  /* An additional address assigned with this location.  This is currently
+     only used by STT_GNU_IFUNC resolver breakpoints to hold the address
+     of the resolver function.  */
+  CORE_ADDR related_address;
+
   /* If the location comes from a probe point, this is the probe associated
      with it.  */
   struct probe *probe;