Re: [PATCH 11/11] ELFv2 ABI: skip global entry point code

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Joel Brobecker wrote:

> > Now, when setting a breakpoint on a function by name, you really want
> > that breakpoint to trigger either way, no matter whether the function
> > is called via its local or global entry point.  Since the global entry
> > point will always fall through into the local entry point, the way to
> > achieve that is to simply set the breakpoint at the local entry point.
> 
> I think ppc-aix has the same kind issue? Some inter-unit function calls
> can get optimized, even by the linker. I can't remember the details,
> but maybe the optimization is make possible when the jump distance
> is not exceeding a certain value?

No, there are no multiple entry points on AIX.  (What you are refering
to are mabye linker stubs?  Those exist on Linux too, but they're not
causing any issues with setting break points ...)

> > Unfortunately, I don't really see any way to express the notion of
> > local entry points with the current set of gdbarch callbacks.
> > 
> > Thus this patch adds a new callback, skip_entrypoint, that is
> > somewhat analogous to skip_prologue, but is called every time
> > GDB needs to determine a function start address, even in those
> > cases where GDB decides to not call skip_prologue.
> 
> I don't feel very qualified to review this addition. However, I couldn't
> help but notice that this new callback is very close to
> deprecated_function_start_offset - which, btw, appears to be only used
> in the case of VAX.
> 
> If this method was to be added, I am wondering whether we could
> replace deprecated_function_start_offset by this new method.
> The implementation in vax-tdep would simply return pc+2 instead
> of plain 2.

I first thought of using deprecated_function_start_offset too, but this
hasn't quite the correct semantics.  In particular, function_start_offset
is also applied when evaluating a symbol name as function pointer expression
and when doing inferior calls; see infcall.c:find_function_addr.

This is exactly wrong for powerpc64le-linux, because when using function
pointers or doing inferior calls, we definitely must use the *global*
entry point, not the local one.

Maybe it would be possible to change VAX to now use gdbarch_skip_entrypoint
plus in addition define gdbarch_convert_from_func_ptr_addr to handle the
function pointer expression case, and thereby get rid of the
deprecated_function_start_offset callback ...   I'm not sure I feel
confident to do that without actual testing on VAX though.

> Can you add a comment documenting precisely the purpose of this method,
> what its parameters are, and what the return value is expected to be?
> That's really going to help us out in the future.

Of course, see below.

> > +/* If the ELF symbol has a local entry point, use it as SYMBOL_VALUE_ADDRESS
> > +   for the msymbol.  This matches the DWARF function start if present.  */
> > +
> > +static void
> > +ppc_elfv2_elf_make_msymbol_special (asymbol *sym, struct minimal_symbol *msym)
> 
> You probably meant this comment for the next function
> (ppc_elfv2_skip_entrypoint)?

Oops, that comment is buggy anyway, it's a remains of an earlier, different
attempt of solving the problem.  I've fixed / added comments below.

> > -gdb_test "advance *bowler" "bowler.*" "advance to the bowler"
> > +gdb_test "advance bowler" "bowler.*" "advance to the bowler"
> >  set test "stepping to fault"
> 
> Agreed on this one. I did some archeology as well as looking at
> the test. There is no real reason to stop at the start of the function.
> Despite stopping after the prologue, the code is written in a way
> that we should still have multiple instructions to step before
> causing the SEGV.

Exactly, that was my thinking.

> > -gdb_test "break *hello" \
> > +gdb_test "break hello" \
> >           "Breakpoint.*at.* file .*$srcfile, line .*" \
> >           "breakpoint at first instruction of hello()"
> 
> I am not sure about this one, however. It seems to me that the purpose
> of this test is to verify the behavior of the "bt" command while
> still inside a function's prologue.  That is something useful that
> I wouldn't change.

Hmm, looks like you're right.  I had been under the impression that
this test was about testing optimized code where we wouldn't do
prologue skipping even with "break hello", but it is actually not
optimized ...

> For ppc64 using ELFv2, my suggestion would be to break at *hello+8?

I don't particularly like this, since it hard-codes that:
- this routine has a local entry point (not all routines do)
- and it is at offset 8 (while all current compilers only ever create
  global entry point prologues of size 8, this might change in theory)

I'd rather either skip the test, or modify it so that it also works on
powerpc64le-linux.  One way to do that would be to ensure that "hello"
is entered via its global entry point, e.g. by calling it via a
function pointer.  This is what I've implemented now.  Does this look
reasonable to you?


Since there's a number of changes, I've appended the full version of
this patch below (excluding generated files).

Bye,
Ulrich


ChangeLog:

	* gdbarch.sh (skip_entrypoint): New callback.
	* gdbarch.c, gdbarch.h: Regenerate.
	* symtab.c (skip_prologue_sal): Call gdbarch_skip_entrypoint.
	* infrun.c (fill_in_stop_func): Likewise.
	* ppc-linux-tdep.c: Include "elf/ppc64.h".
	(ppc_elfv2_elf_make_msymbol_special): New function.
	(ppc_elfv2_skip_entrypoint): Likewise.
	(ppc_linux_init_abi): Install them for ELFv2.

testsuite/ChangeLog:

	* gdb.base/sigbpt.exp: Do not use "*" when setting breakpoint
	on a function.
	* gdb.base/step-bt.c: Call hello via function pointer to make
	sure its first instruction is executed on powerpc64le-linux.


Index: binutils-gdb/gdb/gdbarch.sh
===================================================================
--- binutils-gdb.orig/gdb/gdbarch.sh
+++ binutils-gdb/gdb/gdbarch.sh
@@ -530,6 +530,19 @@ m:int:return_in_first_hidden_param_p:str
 
 m:CORE_ADDR:skip_prologue:CORE_ADDR ip:ip:0:0
 M:CORE_ADDR:skip_main_prologue:CORE_ADDR ip:ip
+# On some platforms, a single function may provide multiple entry points,
+# e.g. one that is used for function-pointer calls and a different one
+# that is used for direct function calls.
+# In order to ensure that breakpoints set on the function will trigger
+# no matter via which entry point the function is entered, a platform
+# may provide the skip_entrypoint callback.  It is called with IP set
+# to the main entry point of a function (as determined by the symbol table),
+# and should return the address of the innermost entry point, where the
+# actual breakpoint needs to be set.  Note that skip_entrypoint is used
+# by GDB common code even when debugging optimized code, where skip_prologue
+# is not used.
+M:CORE_ADDR:skip_entrypoint:CORE_ADDR ip:ip
+
 f:int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs:0:0
 m:const gdb_byte *:breakpoint_from_pc:CORE_ADDR *pcptr, int *lenptr:pcptr, lenptr::0:
 # Return the adjusted address and kind to use for Z0/Z1 packets.
Index: binutils-gdb/gdb/infrun.c
===================================================================
--- binutils-gdb.orig/gdb/infrun.c
+++ binutils-gdb/gdb/infrun.c
@@ -3159,6 +3159,10 @@ fill_in_stop_func (struct gdbarch *gdbar
       ecs->stop_func_start
 	+= gdbarch_deprecated_function_start_offset (gdbarch);
 
+      if (gdbarch_skip_entrypoint_p (gdbarch))
+	ecs->stop_func_start = gdbarch_skip_entrypoint (gdbarch,
+							ecs->stop_func_start);
+
       ecs->stop_func_filled_in = 1;
     }
 }
Index: binutils-gdb/gdb/symtab.c
===================================================================
--- binutils-gdb.orig/gdb/symtab.c
+++ binutils-gdb/gdb/symtab.c
@@ -2950,6 +2950,8 @@ skip_prologue_sal (struct symtab_and_lin
 
       /* Skip "first line" of function (which is actually its prologue).  */
       pc += gdbarch_deprecated_function_start_offset (gdbarch);
+      if (gdbarch_skip_entrypoint_p (gdbarch))
+        pc = gdbarch_skip_entrypoint (gdbarch, pc);
       if (skip)
 	pc = gdbarch_skip_prologue (gdbarch, pc);
 
Index: binutils-gdb/gdb/ppc-linux-tdep.c
===================================================================
--- binutils-gdb.orig/gdb/ppc-linux-tdep.c
+++ binutils-gdb/gdb/ppc-linux-tdep.c
@@ -44,6 +44,7 @@
 #include "observer.h"
 #include "auxv.h"
 #include "elf/common.h"
+#include "elf/ppc64.h"
 #include "exceptions.h"
 #include "arch-utils.h"
 #include "spu-tdep.h"
@@ -876,6 +877,55 @@ ppc_linux_core_read_description (struct
     }
 }
 
+
+/* Implementation of `gdbarch_elf_make_msymbol_special', as defined in
+   gdbarch.h.  This implementation is used for the ELFv2 ABI only.  */
+
+static void
+ppc_elfv2_elf_make_msymbol_special (asymbol *sym, struct minimal_symbol *msym)
+{
+  elf_symbol_type *elf_sym = (elf_symbol_type *)sym;
+
+  /* If the symbol is marked as having a local entry point, set a target
+     flag in the msymbol.  We currently only support local entry point
+     offsets of 8 bytes, which is the only entry point offset ever used
+     by current compilers.  If/when other offsets are ever used, we will
+     have to use additional target flag bits to store them.  */
+  switch (PPC64_LOCAL_ENTRY_OFFSET (elf_sym->internal_elf_sym.st_other))
+    {
+    default:
+      break;
+    case 8:
+      MSYMBOL_TARGET_FLAG_1 (msym) = 1;
+      break;
+    }
+}
+
+/* Implementation of `gdbarch_skip_entrypoint', as defined in
+   gdbarch.h.  This implementation is used for the ELFv2 ABI only.  */
+
+static CORE_ADDR
+ppc_elfv2_skip_entrypoint (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  struct bound_minimal_symbol fun;
+  int local_entry_offset = 0;
+
+  fun = lookup_minimal_symbol_by_pc (pc);
+  if (fun.minsym == NULL)
+    return pc;
+
+  /* See ppc_elfv2_elf_make_msymbol_special for how local entry point
+     offset values are encoded.  */
+  if (MSYMBOL_TARGET_FLAG_1 (fun.minsym))
+    local_entry_offset = 8;
+
+  if (SYMBOL_VALUE_ADDRESS (fun.minsym) <= pc
+      && pc < SYMBOL_VALUE_ADDRESS (fun.minsym) + local_entry_offset)
+    return SYMBOL_VALUE_ADDRESS (fun.minsym) + local_entry_offset;
+
+  return pc;
+}
+
 /* Implementation of `gdbarch_stap_is_single_operand', as defined in
    gdbarch.h.  */
 
@@ -1349,6 +1399,13 @@ ppc_linux_init_abi (struct gdbarch_info
 	  set_gdbarch_elf_make_msymbol_special
 	    (gdbarch, ppc64_elf_make_msymbol_special);
 	}
+      else
+	{
+	  set_gdbarch_elf_make_msymbol_special
+	    (gdbarch, ppc_elfv2_elf_make_msymbol_special);
+
+	  set_gdbarch_skip_entrypoint (gdbarch, ppc_elfv2_skip_entrypoint);
+	}
 
       /* Shared library handling.  */
       set_gdbarch_skip_trampoline_code (gdbarch, ppc64_skip_trampoline_code);
Index: binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp
===================================================================
--- binutils-gdb.orig/gdb/testsuite/gdb.base/sigbpt.exp
+++ binutils-gdb/gdb/testsuite/gdb.base/sigbpt.exp
@@ -76,7 +76,7 @@ gdb_test "break keeper"
 set bowler_addrs bowler
 set segv_addr none
 gdb_test {display/i $pc}
-gdb_test "advance *bowler" "bowler.*" "advance to the bowler"
+gdb_test "advance bowler" "bowler.*" "advance to the bowler"
 set test "stepping to fault"
 set signame "SIGSEGV"
 gdb_test_multiple "stepi" "$test" {
Index: binutils-gdb/gdb/testsuite/gdb.base/step-bt.c
===================================================================
--- binutils-gdb.orig/gdb/testsuite/gdb.base/step-bt.c
+++ binutils-gdb/gdb/testsuite/gdb.base/step-bt.c
@@ -23,10 +23,19 @@ hello (void)
   printf ("Hello world.\n");
 }
 
+/* The test case uses "break *hello" to make sure to step at the very
+   first instruction of the function.  This causes a problem running
+   the test on powerpc64le-linux, since the first instruction belongs
+   to the global entry point prologue, which is skipped when doing a
+   local direct function call.  To make sure that first instruction is
+   indeed being executed and the breakpoint hits, we make sure to call
+   the routine via an indirect call.  */
+void (*ptr) (void) = hello;
+
 int
 main (void)
 {
-  hello ();
+  ptr ();
 
   return 0;
 }


-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com