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

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

[Klaus Gerlicher]

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.

Limit the check for a breakpoint at the current PC to internal breakpoints.
Internal breakpoints have a number less than or equal to zero.
---
 gdb/breakpoint.c | 18 ++++++++++++++++++
 gdb/breakpoint.h |  5 +++++
 gdb/infrun.c     |  8 +++++---
 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

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

[Andrew Burgess]

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.

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

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

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

[Gerlicher, Klaus]

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

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

[Gerlicher, Klaus]

Hi Andrew,

Just a little friendly reminder. 

Thanks
K-

> -----Original Message-----
> From: Gerlicher, Klaus <klaus.gerlicher@intel.com>
> Sent: Tuesday, October 8, 2024 9:04 AM
> To: Andrew Burgess <aburgess@redhat.com>; gdb-patches@sourceware.org
> Subject: [PATCH] gdb: avoid conversion of SIGSEGV to SIGTRAP on user
> breakpoints
> 
> 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
> 

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

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

[Gerlicher, Klaus]

Kindly pinging.

Thanks
Klaus

> -----Original Message-----
> From: Gerlicher, Klaus <klaus.gerlicher@intel.com>
> Sent: Tuesday, October 22, 2024 1:50 PM
> To: Andrew Burgess <aburgess@redhat.com>; gdb-patches@sourceware.org
> Subject: [PING][PATCH] gdb: avoid conversion of SIGSEGV to SIGTRAP on user
> breakpoints
> 
> Hi Andrew,
> 
> Just a little friendly reminder.
> 
> Thanks
> K-
> 
> > -----Original Message-----
> > From: Gerlicher, Klaus <klaus.gerlicher@intel.com>
> > Sent: Tuesday, October 8, 2024 9:04 AM
> > To: Andrew Burgess <aburgess@redhat.com>; gdb-
> patches@sourceware.org
> > Subject: [PATCH] gdb: avoid conversion of SIGSEGV to SIGTRAP on user
> > breakpoints
> >
> > 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
> >
> 
> 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

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

[Andrew Burgess]

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

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

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

[Gerlicher, Klaus]

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

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

[Andrew Burgess]

Klaus Gerlicher <klaus.gerlicher@intel.com> writes:

> 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.
>
> Limit the check for a breakpoint at the current PC to internal breakpoints.
> Internal breakpoints have a number less than or equal to zero.
> ---
>  gdb/breakpoint.c | 18 ++++++++++++++++++
>  gdb/breakpoint.h |  5 +++++
>  gdb/infrun.c     |  8 +++++---
>  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)))

Klaus,

First, huge thanks for your excellent explanation of this patch in your
other reply, that really helped.

I think what you're doing above is not quite right though, but maybe I'm
still not understanding things.

The comment was truncated in the diff above, so let me quote it here
again:

  /* 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....

In your explanation you said:

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

I think the SIGSEGV case was added to support inferior function calls,
but I think the other two cases are not.  The SIGILL and SIGEMT cases
are there to support targets where any software breakpoint might
generate those signals instead of SIGTRAP, at least, that's my reading
of the comment.

With your change, for those targets (if I'm right), we'll no longer
convert the e.g. SIGILL to SIGTRAP for a user breakpoint.

So I think at a minimum you need to restrict the use of
internal_breakpoint_inserted_here_p to the SIGSEGV case only.

But, I wonder if there's a different approach that could be used?

I assume that your out of tree architecture with these imprecise page
faults has a new gdbarch to represent it.

You are limiting the SIGSEGV -> SIGTRAP conversion because, I assume,
you've hit cases where a SIGSEGV occurs and then your target has run on
and reported the stop at the address of a user breakpoint.  But all
you're really doing is reducing the scope for errors.  It could be the
case that the SIGSEGV is reported at the site of an internal breakpoint,
and then you'll still have issues.

So what if, instead, you added a new gdbarch method, something like:

  bool gdbarch_has_imprecise_page_faults (struct gdbarch *gdbarch);

The default for this, and for all currently in-tree targets, would be to
return true.  For your target this will return false.

Then in this code we would do:

  if (ecs->ws.kind () == TARGET_WAITKIND_STOPPED
      && (ecs->ws.sig () == GDB_SIGNAL_ILL
	  || (ecs->ws.sig () == GDB_SIGNAL_SEGV
              !gdbarch_has_imprecise_page_faults (gdbarch))
	  || ecs->ws.sig () == GDB_SIGNAL_EMT))

How does this approach sound?

Thanks,
Andrew


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

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

[Gerlicher, Klaus]

Hi Andrew,

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Wednesday, November 6, 2024 3:24 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
> 
> Klaus Gerlicher <klaus.gerlicher@intel.com> writes:
> 
> > 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.
> >
> > Limit the check for a breakpoint at the current PC to internal breakpoints.
> > Internal breakpoints have a number less than or equal to zero.
> > ---
> >  gdb/breakpoint.c | 18 ++++++++++++++++++
> >  gdb/breakpoint.h |  5 +++++
> >  gdb/infrun.c     |  8 +++++---
> >  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)))
> 
> Klaus,
> 
> First, huge thanks for your excellent explanation of this patch in your
> other reply, that really helped.
> 
> I think what you're doing above is not quite right though, but maybe I'm
> still not understanding things.
> 
> The comment was truncated in the diff above, so let me quote it here
> again:
> 
>   /* 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....
> 
> In your explanation you said:
> 
>   "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."
> 
> I think the SIGSEGV case was added to support inferior function calls,
> but I think the other two cases are not.  The SIGILL and SIGEMT cases
> are there to support targets where any software breakpoint might
> generate those signals instead of SIGTRAP, at least, that's my reading
> of the comment.
> 
> With your change, for those targets (if I'm right), we'll no longer
> convert the e.g. SIGILL to SIGTRAP for a user breakpoint.
> 
> So I think at a minimum you need to restrict the use of
> internal_breakpoint_inserted_here_p to the SIGSEGV case only.
> 
> But, I wonder if there's a different approach that could be used?
> 
> I assume that your out of tree architecture with these imprecise page
> faults has a new gdbarch to represent it.
> 
> You are limiting the SIGSEGV -> SIGTRAP conversion because, I assume,
> you've hit cases where a SIGSEGV occurs and then your target has run on
> and reported the stop at the address of a user breakpoint.  But all
> you're really doing is reducing the scope for errors.  It could be the
> case that the SIGSEGV is reported at the site of an internal breakpoint,
> and then you'll still have issues.
> 
> So what if, instead, you added a new gdbarch method, something like:
> 
>   bool gdbarch_has_imprecise_page_faults (struct gdbarch *gdbarch);
> 
> The default for this, and for all currently in-tree targets, would be to
> return true.  For your target this will return false.
> 
> Then in this code we would do:
> 
>   if (ecs->ws.kind () == TARGET_WAITKIND_STOPPED
>       && (ecs->ws.sig () == GDB_SIGNAL_ILL
> 	  || (ecs->ws.sig () == GDB_SIGNAL_SEGV
>               !gdbarch_has_imprecise_page_faults (gdbarch))
> 	  || ecs->ws.sig () == GDB_SIGNAL_EMT))
> 
> How does this approach sound?
> 
> Thanks,
> Andrew
> 
> 
Incidentally this was brought up here internally yesterday and I will look at the gdbarch 
approach, might really be less troublesome.

Thanks
Klaus
> >  	{
> >  	  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