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

[Andrew Burgess <aburgess@redhat.com>]

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

> GDB maintainers:
>
> Ver2, added a missing blank line, removed an extra blank line, fixed
> spelling mistakes.  Removed comment in ChangeLog about fixing py-
> thread-exited.exp test errors.  Patch just fixes issues with the
> store.exp.  Did not change the comment:
>
>> +    /* FIXME: jimb/2004-05-05: What should we do when the debug info
>> +       specifies registers the architecture doesn't have?  Our
>> +       callers don't check the value we return.  */
>   
> Will create a new patch to address the comment in all three places in
> the source code.
>
> Note, fixing the register mapping fixes two of the five test failures
> for the store.exp test.
>
> Patch was retested on Power10.
>
> This is the first patch in the series which fixes the DWWARF register
> mapping and signal handling issues on PowerPC.
>
>                   Carl
>
>
> -------------------------------------------------
> Fix Linux DWARF register mapping
>
> 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.
>
> The second issue fixed by this patch is the check for subroutine
> process_event_stop_test.  Need to make sure the frame is not the
> SIGTRAMP_FRAME.  The sequence of events on most platforms is:
>
>   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 "nexti" to step into, not over, signal handlers invoked
> by the kernel.  This is the case 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.
>
> 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.
>
> Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10 with no
> new regressions.

Hi Carl,

Sorry for being such a pain w.r.t. this patch.  Honestly, it's mostly
because you touched infrun.c that I'm so interested here.

I left some notes here that need addressing:

  https://inbox.sourceware.org/gdb-patches/6f9c8fa20962129048384d74f6f15d1b37d255ed.camel@us.ibm.com/T/#m83b4f0d7da45f15c2df44344fbf3e326cf7435e3

But I wonder if you could be more specific about when you would expect
to see the failures in gdb.base/store.exp.  I grabbed a random Power9
Linux machine and tried running store.exp with an unpatched GDB, and see
no failures.  I guess there's a specific architecture/compiler combo
that I need in order to see failures -- but if those details are in the
above text, I'm just not seeing them.

Could you give some more details about the setup needed to see failures
on store.exp.

And I'd like to see clearer details about which tests the infrun.c
changes will fix.

Thanks,
Andrew




> ---
>  gdb/infrun.c         | 13 ++++++++++
>  gdb/ppc-linux-tdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 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..dc03430e2af 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,52 @@ 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)
> +    /* FIXME: jimb/2004-05-05: What should we do when the debug info
> +       specifies registers the architecture doesn't have?  Our
> +       callers don't check the value we return.  */
> +    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 +2182,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