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

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

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.

> 
> > ---
> >  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