RE: [Patch 1/2] PowerPC, Fix-test-gdb.base-store.exp

[Carl Love <cel@linux.ibm.com>]

Andrew:

Just wanted to make sure you were OK with response, particularly about
creating a new patch to address the issues with the comment in the code
before I commit the two patches.

Sorry for the slow response.  My email is being migrated from 
cel@us.ibm.com to cel@linux.ibm.com.  There have been some issues with
the migration.  I don't seem to be getting email at the old address but
the new address does seem to work.  So any replies need to include the
new email address.  Thanks.

                  Carl 

On Mon, 2023-10-16 at 08:51 -0700, Carl Love wrote:
> Andrew:
> 
> Thanks for the review and comments.  See responses below.
> 
> On Mon, 2023-10-16 at 15:31 +0100, Andrew Burgess wrote:
> > Carl Love <cel@us.ibm.com> writes:
> > 
> > > GDB maintainers:
> > > 
> > > This is the first patch in the series which fixes the DWWARF
> > > register
> > > mapping and signal handling issues on PowerPC.
> > > 
> > >                   Carl
> > > 
> > > -----------------------------------------------
> > > 
> > > rs6000, 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:
> > 
> > Usually for GDB we avoid bundling unrelated changes into a single
> > commit.  Each commit should address one self contained issue (as
> > far
> > as
> > possible).
> > 
> > I really struggling to see any connection between the two fixes you
> > have
> > here.
> 
> As mentioned in the commit log, the first patch which fixes the DWARF
> register mapping fixes two of the 5 failures seen in store.exp. 
> Specifically the patch fixes the issue of GDB not stopping in the
> handler which results in the step.exp test failing.  The second patch
> then fixes the rest of the issues with the step.exp test.
> > >   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.  It
> > > also fixes two regression failures in gdb.python/py-thread-
> > > exited.exp.
> > 
> > On the one random PPC box I tried this patch on, I'm not seeing any
> > failures in gdb.python/py-thread-exited.exp either before, or after
> > this
> > commit.
> > 
> > Which tests in gdb.python/py-thread-exited.exp are you seeing as
> > broken?
> > And which of the two fixes in this commit fix the problems you're
> > seeing?
> 
> The comment about the py-thread-exited.exp should have been
> removed.  I
> found that sometimes that test fails and sometime passes.  I have yet
> to dig into it and figure out why the test is inconsistent.  The
> inconsistent behavior exists with and without these patches.
> 
> I will remove the comment about the gdb.python/py-thread-exited.exp
> fixes as that is erroneous.
> 
> 
> > > Patch has been tested on Power 8 LE/BE, Power 9 LE/BE, Power 10
> > > with no
> > > new regressions.
> > > ---
> > >  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..7fb90799dff 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.  */
> > 
> > I see this comment was just copied from else where, but isn't the
> > answer
> > just: return -1 ?
> > 
> > The comment about the 'return -1' at the trail of this function
> > seems
> > to
> > suggest that would be the correct thing to do.
> > 
> > I guess I'm asking: do we need to add another copy of this (I think
> > out
> > of date) fixme?
> 
> Yes, the function is based on rs6000_dwarf2_reg_to_regnum with the
> specific changes for the Linux DWARF mappings.  The comment looked
> out
> of date to me, but I left it for consistency with the other two
> functions that have the same comment. It would probably be best to
> investigate the comment further and update it in all places in a
> separate patch. 
> 
> Would it be acceptable to leave the comment as is for now, for
> consistency sake, and I will work on a separate cleanup patch to
> address removing the comment in the original two places and this
> additional place?  I will want to look into it a bit more but I think
> we can just remove the comment.  But I do want to verify that the
> return value is never used first.
> 
>                           Carl 
>