[PATCH] gdb: avoid conversion of SIGSEGV to SIGTRAP on user breakpoints

["Gerlicher, Klaus" <klaus.gerlicher@intel.com>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Tuesday, November 5, 2024 3:31 PM
> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH] gdb: avoid conversion of SIGSEGV to SIGTRAP on user
> breakpoints
> 
> "Gerlicher, Klaus" <klaus.gerlicher@intel.com> writes:
> 
> > Hi Andrew,
> >
> > Thanks for the feedback.
> >
> > I didn't expect that the explanation is too brief and so I'll try to explain a bit
> more Inline.
> >
> > Thanks
> > Klaus
> >
> >> -----Original Message-----
> >> From: Andrew Burgess <aburgess@redhat.com>
> >> Sent: Friday, October 4, 2024 11:22 AM
> >> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-
> >> patches@sourceware.org
> >> Subject: Re: [PATCH] gdb: avoid conversion of SIGSEGV to SIGTRAP on user
> >> breakpoints
> >>
> >> Klaus Gerlicher <klaus.gerlicher@intel.com> writes:
> >>
> >> Thanks for the patch.  Sorry if I'm asking really obvious questions, but
> >> there are a couple of things that are not clear to me from your commit
> >> message.
> >>
> >>
> >> > From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
> >> >
> >> > GDB converts signals GDB_SIGNAL_ILL, GDB_SIGNAL_SEGV and
> >> GDB_SIGNAL_EMT to
> >> > GDB_SIGNAL_SEGV if a breakpoint is inserted at the fault location.  If, due
> >> > to imprecise page fault reporting, a breakpoint is at the same address as
> >> > the fault address, this signal would always be reported as
> >> > GDB_SIGNAL_TRAP.
> >>
> >> I'm sure if I do enough digging I could probably figure this bit out,
> >> but is it feasible to give a brief explanation of exactly how/why/what
> >> impact "imprecise page fault reporting" is having here.  Honestly, it's
> >> not a phrase I'm familiar with.
> >>
> >> > Limit the check for a breakpoint at the current PC to internal breakpoints.
> >> > Internal breakpoints have a number less than or equal to zero.
> >>
> >> This is my biggest area of confusion.  I'm not seeing _why_ we only want
> >> to do this thing for internal b/p.  More detail on why this is needed
> >> would be really useful.
> >
> > In infrun.c at line #~6144, GDB converts signals EMT, ILL and SEGV to TRAP if
> a breakpoint is inserted
> > at the reported fault location.
> >
> > From the comment above the code section in question:
> >
> >   /* First, distinguish signals caused by the debugger from signals
> >      that have to do with the program's own actions.  Note that
> >      breakpoint insns may cause SIGTRAP or SIGILL or SIGEMT, depending
> >      on the operating system version.  Here we detect when a SIGILL or
> >      SIGEMT is really a breakpoint and change it to SIGTRAP.  We do
> >      something similar for SIGSEGV, since a SIGSEGV will be generated
> >      when we're trying to execute a breakpoint instruction on a
> >      non-executable stack.  This happens for call dummy breakpoints
> >      for architectures like SPARC that place call dummies on the
> >      stack.  */
> >
> > I'm assuming based on this that this was really added to support returns
> from inferior calls. From that
> > I deduce that we would not have to convert the signal for all breakpoint
> types.
> >
> > That being said, our specific case is that we have a target that reports the
> page fault not on the instruction
> > that causes the page fault but at some point after that ("imprecise page fault
> reporting"). If incidentally there is a breakpoint inserted
> > at this imprecise location, GDB will convert the actual SIGSEGV we get from
> the target into a TRAP and the user will be presented a
> > breakpoint stop. This of course is not what a user would expect, instead a
> SIGSEGV should be reported.
> >
> > I mulled several options to address this:
> >
> > 1) change the actually reported location back to an instruction earlier: our
> target has non-constant instruction size and so getting the
> >    Previous instruction could only be done by disassembling from a known
> start point to the previous instruction. This would potentially not
> >    always work.
> > 2) Instead of checking the inserted breakpoint always, only check for a
> dummy call frame used in inferior calls:  since my basic assumption is that we
> only need this
> >    for inferior calls, this would work but the check would have to be
> transplanted to a location where we already have the frame information.
> > 3) Only check breakpoints that were not set by the user: this is also based on
> the assumption that the conversion is only needed for inferior calls. Since
> >   this seemed to be the most straightforward way, I chose this and this is the
> current patch implementation.
> 
> Thanks, this explanation makes a lot of sense.  I'll take another look
> at this patch tomorrow.  I do have a follow up question, though the
> answer is not needed for the review, more for my own interest...
> 
> You say:
> 
>   "...we have a target that reports the page fault not on the instruction
>    that causes the page fault but at some point after that..."
> 
> Does this mean there's no way to know which instruction triggered the
> fault?  Or is there some state somewhere that does indicate the precise
> address?
> 

In our case, there currently is no mechanism to know the precise address. The user
would need to narrow down the code location by inserting breakpoints at some checkpoints
and/or by single-stepping.

Please note that a similar imprecise memory exception behavior exists for the AMD GPU
target, too (grep "precise" in amd-dbgapi-target.c).

Thanks
Klaus

> Thanks,
> Andrew
> 
> >
> >>
> >> > ---
> >> >  gdb/breakpoint.c | 18 ++++++++++++++++++
> >> >  gdb/breakpoint.h |  5 +++++
> >> >  gdb/infrun.c     |  8 +++++---
> >>
> >> Is there any chance we could write a test for this?  I can't really tell
> >> right now as I don't understand the problem or solution ... but tests
> >> are always good, right?
> >>
> >
> > We have a testcase for our specific target but this is not up-streamable so far
> since the target is not there yet.
> >
> > I also tried to repro with a more common CPU target but for instance x86
> and ARM will not exhibit this since
> > they are reporting at the fault address. If there is a breakpoint there, it will
> have been removed (i.e. not inserted) during
> > breakpoint dancing over the page faulting instruction and the signal
> conversion will not be done.
> >
> >> Thanks,
> >> Andrew
> >>
> >>
> >>
> >> >  3 files changed, 28 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> >> > index b80b3522dcb..09526fa557f 100644
> >> > --- a/gdb/breakpoint.c
> >> > +++ b/gdb/breakpoint.c
> >> > @@ -4569,6 +4569,24 @@ hardware_watchpoint_inserted_in_range
> >> (const address_space *aspace,
> >> >
> >> >  /* See breakpoint.h.  */
> >> >
> >> > +bool
> >> > +internal_breakpoint_inserted_here_p (const address_space *aspace,
> >> > +				     CORE_ADDR pc)
> >> > +{
> >> > +  for (bp_location *bl : all_bp_locations_at_addr (pc))
> >> > +    {
> >> > +      if (bl->owner == nullptr || user_breakpoint_p (bl->owner))
> >> > +	continue;
> >> > +
> >> > +      if (bp_location_inserted_here_p (bl, aspace, pc))
> >> > +	return true;
> >> > +    }
> >> > +
> >> > +  return false;
> >> > +}
> >> > +
> >> > +/* See breakpoint.h.  */
> >> > +
> >> >  bool
> >> >  is_catchpoint (struct breakpoint *b)
> >> >  {
> >> > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> >> > index cef1d727ed2..6134b546f4e 100644
> >> > --- a/gdb/breakpoint.h
> >> > +++ b/gdb/breakpoint.h
> >> > @@ -1499,6 +1499,11 @@ extern int
> >> hardware_watchpoint_inserted_in_range (const address_space *,
> >> >  						  CORE_ADDR addr,
> >> >  						  ULONGEST len);
> >> >
> >> > +/* Returns true if any non-user breakpoints are present and inserted
> >> > +   at ADDR.  Internal breakpoints have a number less than or equal to
> zero.
> >> */
> >> > +extern bool internal_breakpoint_inserted_here_p (const address_space
> >> *aspace,
> >> > +						 CORE_ADDR addr);
> >> > +
> >> >  /* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent
> the
> >> >     same breakpoint location.  In most targets, this can only be true
> >> >     if ASPACE1 matches ASPACE2.  On targets that have global
> >> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> >> > index 4ca15450afe..af9c6d291b7 100644
> >> > --- a/gdb/infrun.c
> >> > +++ b/gdb/infrun.c
> >> > @@ -6135,7 +6135,8 @@ handle_inferior_event (struct
> >> execution_control_state *ecs)
> >> >       when we're trying to execute a breakpoint instruction on a
> >> >       non-executable stack.  This happens for call dummy breakpoints
> >> >       for architectures like SPARC that place call dummies on the
> >> > -     stack.  */
> >> > +     stack.  Note that only internal breakpoints must be used for this
> >> > +     check.  */
> >> >    if (ecs->ws.kind () == TARGET_WAITKIND_STOPPED
> >> >        && (ecs->ws.sig () == GDB_SIGNAL_ILL
> >> >  	  || ecs->ws.sig () == GDB_SIGNAL_SEGV
> >> > @@ -6143,8 +6144,9 @@ handle_inferior_event (struct
> >> execution_control_state *ecs)
> >> >      {
> >> >        struct regcache *regcache = get_thread_regcache (ecs->event_thread);
> >> >
> >> > -      if (breakpoint_inserted_here_p (ecs->event_thread->inf->aspace.get
> (),
> >> > -				      regcache_read_pc (regcache)))
> >> > +      if (internal_breakpoint_inserted_here_p (
> >> > +	    ecs->event_thread->inf->aspace.get (),
> >> > +	    regcache_read_pc (regcache)))
> >> >  	{
> >> >  	  infrun_debug_printf ("Treating signal as SIGTRAP");
> >> >  	  ecs->ws.set_stopped (GDB_SIGNAL_TRAP);
> >> > --
> >> > 2.34.1
> >> >
> >> > Intel Deutschland GmbH
> >> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> >> > Tel: +49 89 99 8853-0, www.intel.de
> >> > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon
> Silva
> >> > Chairperson of the Supervisory Board: Nicole Lau
> >> > Registered Office: Munich
> >> > Commercial Register: Amtsgericht Muenchen HRB 186928
> >
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de
> > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928