Re: [PATCH 1/2, ver3] PowerPC, Fix-test-gdb.base-store.exp

[Andrew Burgess <aburgess@redhat.com>]

Carl Love <cel@linux.ibm.com> writes:

> Andrew, Ulrich:
>
> Per the discussions, the commit message has been updated to make it
> clear that the patch fixes the DWARF register mapping which fixes two
> of the test failures in step.exp.  The fix exposed a dormant issue with
> the signal handling on PowerPC.  The underlying issue which is then
> exposed causes the signal handling test sigstep.exp to fail.  The test
> sigstep.exp tests stepping into a signal handler.  The patch also fixes
> the underlying signal handler issue.  
>
> This new verion of the patch only updates the commit log to make it
> clear the patch is fixing multiple issues.
>
> Please let me know if the commit description is clear and acceptable.
>
> Thanks.
>
>                             Carl 
> ------------------------------------------------------------
> rs6000, Fix Linux DWARF register mapping
>
> Overview of issues fixed by the patch.
>
> The primary issue this patch fixes is the DWARF register mapping for
> Linux.  The changes in ppc-linux-tdep.c fix the DWARF register mapping
> issues.  The register mapping issue is responsible for two of the
> five regression bugs seen in gdb.base/store.exp.
>
> Once the register mapping is fixed, an underlying issue with the unwinding
> of the signal trampoline in common-code in ifrun.c was found.  This
> underlying bug is best described by Ulrich in the following description.
>
>   The unwinder bug shows up on platforms where the kernel uses a trampoline
>   to dispatch "calls to" the signal handler (not just *returns from* the
>   signal handler).  Many platforms use a trampoline for signal return, and
>   that is working fine, but the only platform I'm (Ulrich) aware of that
>   uses a trampoline for signal handler calls is (recent kernels for)
>   PowerPC.  I believe the rationale for using a trampoline here
>   is to improve performance by avoiding unbalancing of the
>   branch predictor's call/return stack.
>
>   However, on PowerPC the bug is dormant as well as it is hidden
>   by *another* bug that prevents correct unwinding out of the
>   signal trampoline.  This is because the custom CFI for the
>   trampoline uses a register number (VSCR) that is not ever used
>   by compiler-generated CFI, and that particular register is
>   mapped to an invalid number by the current PowerPC DWARF mapper.
>
> The underlying unwinder bug is exposed by the "new" regression failures
> in gdb.base/sigstep.exp.  These failures were previously masked by
> the fact that GDB was not seeing a valid frame when it tried to unwind
> the frames.  The sigstep.exp test is specifically testing stepping into
> a signal handler.  With the correct DWARF register mapping in place,
> specifically the VSCR mapping, the signal trampoline code now unwinds to a
> valid frame exposing the pre-existing bug in how the signal handler on
> PowerPC works.  The one line change infrun.c fixes the exiting bug in
> the common-code for platforms that use a trampoline to dispatch calls
> to the signal handler by not stopping in the SIGTRAMP_FRAME.
>
> Detailed description of the DWARF register mapping fix.
>
> The PowerPC DWARF register mapping is the same for the .eh_frame and
> .debug_frame on Linux.  PowerPC uses different mapping for .eh_frame and
> .debug_frame on other operating systems.  The current GDB support for
> mapping the DWARF registers in rs6000_linux_dwarf2_reg_to_regnum and
> rs6000_adjust_frame_regnum file gdb/rs6000-tdep.c is not correct for Linux.
> The files have some legacy mappings for spe_acc, spefscr, EV which was
> removed from GCC in 2017.
>
> This patch adds a two new functions rs6000_linux_dwarf2_reg_to_regnum,
> and rs6000_linux_adjust_frame_regnum in file gdb/ppc-linux-tdep.c to handle
> the DWARF register mappings on Linux.  Function
> rs6000_linux_dwarf2_reg_to_regnum is installed for both gdb_dwarf_to_regnum
> and gdbarch_stab_reg_to_regnum since the mappings are the same.
>
> The ppc_linux_init_abi function in gdb/ppc-linux-tdep.c is updated to
> call set_gdbarch_dwarf2_reg_to_regnum map the new function
> rs6000_linux_dwarf2_reg_to_regnum for the architecture.  Similarly,
> dwarf2_frame_set_adjust_regnum is called to map
> rs6000_linux_adjust_frame_regnum into the architecture.
>
> Additional detail on the signal handling fix.
>
> The specific sequence of events for handling a signal on most
> architectures is as follows:
>
>   1) Some code is running when a signal arrives.
>   2) The kernel handles the signal and dispatches to the handler.
>     ...
>
> However on PowerPC the sequence of events is:
>
>   1) Some code is running when a signal arrives.
>   2) The kernel handles the signal and dispatches to the trampoline.
>   3) The trampoline performs a normal function call to the handler.
>       ...
>
> We want the "nexti" to step into, not over, signal handlers invoked by
> the kernel.  This is the case for most platforms as the kernel puts a
> signal trampoline frame onto the stack to handle proper return after the
> handler.  However, on some platforms such as PowerPC, the kernel actually
> uses a trampoline to handle *invocation* of the handler.  We do not
> want GDB to stop in the SIGTRAMP_FRAME.  The issue is fixed in function
> process_event_stop_test by adding a check that the frame is not a
> SIGTRAMP_FRAME to the if statement to stop in a subroutine call.  This
> prevents GDB from erroneously detecting the trampoline invocation as a
> subroutine call.
>
> This patch fixes two regression test failures in gdb.base/store.exp.
>
> The patch then fixes an exposed, dormant, signal handling issue that
> is exposed in the signal handling test gdb.base/sigstep.exp.
>
> The patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with
> no new regressions.  Note, only two of the five failures in store.exp
> are fixed.  The remaining three failures are fixed in a following
> patch.

Carl,

Sorry for the slow delay, I had to find some time to play with this a
little.  But thanks to the expanded explanation, it's now clear what's
going on.  This all looks great.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


> ---
>  gdb/infrun.c         | 13 +++++++++++
>  gdb/ppc-linux-tdep.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 4730d290442..922d8a6913d 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7334,8 +7334,21 @@ process_event_stop_test (struct execution_control_state *ecs)
>       initial outermost frame, before sp was valid, would
>       have code_addr == &_start.  See the comment in frame_id::operator==
>       for more.  */
> +
> +  /* We want "nexti" to step into, not over, signal handlers invoked
> +     by the kernel, therefore this subroutine check should not trigger
> +     for a signal handler invocation.  On most platforms, this is already
> +     not the case, as the kernel puts a signal trampoline frame onto the
> +     stack to handle proper return after the handler, and therefore at this
> +     point, the current frame is a grandchild of the step frame, not a
> +     child.  However, on some platforms, the kernel actually uses a
> +     trampoline to handle *invocation* of the handler.  In that case,
> +     when executing the first instruction of the trampoline, this check
> +     would erroneously detect the trampoline invocation as a subroutine
> +     call.  Fix this by checking for SIGTRAMP_FRAME.  */
>    if ((get_stack_frame_id (frame)
>         != ecs->event_thread->control.step_stack_frame_id)
> +      && get_frame_type (frame) != SIGTRAMP_FRAME
>        && ((frame_unwind_caller_id (get_current_frame ())
>  	   == ecs->event_thread->control.step_stack_frame_id)
>  	  && ((ecs->event_thread->control.step_stack_frame_id
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 784dafa59db..8d975336fe5 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -83,6 +83,7 @@
>  #include "features/rs6000/powerpc-isa207-vsx64l.c"
>  #include "features/rs6000/powerpc-isa207-htm-vsx64l.c"
>  #include "features/rs6000/powerpc-e500l.c"
> +#include "dwarf2/frame.h"
>  
>  /* Shared library operations for PowerPC-Linux.  */
>  static struct target_so_ops powerpc_so_ops;
> @@ -2088,6 +2089,49 @@ ppc_linux_displaced_step_prepare  (gdbarch *arch, thread_info *thread,
>    return per_inferior->disp_step_buf->prepare (thread, displaced_pc);
>  }
>  
> +/* Convert a Dwarf 2 register number to a GDB register number for Linux.  */
> +
> +static int
> +rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num)
> +{
> +  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep>(gdbarch);
> +
> +  if (0 <= num && num <= 31)
> +    return tdep->ppc_gp0_regnum + num;
> +  else if (32 <= num && num <= 63)
> +    return tdep->ppc_fp0_regnum + (num - 32);
> +  else if (77 <= num && num < 77 + 32)
> +    return tdep->ppc_vr0_regnum + (num - 77);
> +  else
> +    switch (num)
> +      {
> +      case 65:
> +	return tdep->ppc_lr_regnum;
> +      case 66:
> +	return tdep->ppc_ctr_regnum;
> +      case 76:
> +	return tdep->ppc_xer_regnum;
> +      case 109:
> +	return tdep->ppc_vrsave_regnum;
> +      case 110:
> +	return tdep->ppc_vrsave_regnum - 1; /* vscr */
> +      }
> +
> +  /* Unknown DWARF register number.  */
> +  return -1;
> +}
> +
> +/* Translate a .eh_frame register to DWARF register, or adjust a
> +   .debug_frame register.  */
> +
> +static int
> +rs6000_linux_adjust_frame_regnum (struct gdbarch *gdbarch, int num,
> +				  int eh_frame_p)
> +{
> +  /* Linux uses the same numbering for .debug_frame numbering as .eh_frame.  */
> +  return num;
> +}
> +
>  static void
>  ppc_linux_init_abi (struct gdbarch_info info,
>  		    struct gdbarch *gdbarch)
> @@ -2135,6 +2179,15 @@ ppc_linux_init_abi (struct gdbarch_info info,
>    set_gdbarch_stap_is_single_operand (gdbarch, ppc_stap_is_single_operand);
>    set_gdbarch_stap_parse_special_token (gdbarch,
>  					ppc_stap_parse_special_token);
> +  /* Linux DWARF register mapping is different from the other OSes.  */
> +  set_gdbarch_dwarf2_reg_to_regnum (gdbarch,
> +				    rs6000_linux_dwarf2_reg_to_regnum);
> +  /* Note on Linux the mapping for the DWARF registers and the stab registers
> +     use the same numbers.  Install rs6000_linux_dwarf2_reg_to_regnum for the
> +     stab register mappings as well.  */
> +  set_gdbarch_stab_reg_to_regnum (gdbarch,
> +				    rs6000_linux_dwarf2_reg_to_regnum);
> +  dwarf2_frame_set_adjust_regnum (gdbarch, rs6000_linux_adjust_frame_regnum);
>  
>    if (tdep->wordsize == 4)
>      {
> -- 
> 2.37.2