diff mbox series

[v5,1/1] gdb: avoid conversion of SIGSEGV to SIGTRAP on user breakpoints

Message ID 20260828125003.253648-1-klaus.gerlicher@intel.com
State New
Headers
Series [v5,1/1] gdb: avoid conversion of SIGSEGV to SIGTRAP on user breakpoints |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Klaus Gerlicher Aug. 28, 2026, 12:50 p.m. UTC 
  From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>

GDB converts signals GDB_SIGNAL_ILL, GDB_SIGNAL_SEGV and GDB_SIGNAL_EMT to
GDB_SIGNAL_TRAP if a breakpoint is inserted at the fault location.  This
conversion logic assumes that when these signals occur at a breakpoint
location, the signal was actually caused by hitting the breakpoint rather
than a genuine fault.

However, some architectures have "imprecise page fault reporting," where a
memory access violation can be reported several instructions after the
faulting instruction.  For example:

  INSN1  <-- generates a SIGSEGV
  INSN2
  INSN3  <-- breakpoint installed here

If INSN1 causes a memory access violation, the backend may report the stop
at INSN3, where a breakpoint happens to be installed.  GDB's logic then
incorrectly assumes "there is a breakpoint at INSN3, so this SIGSEGV must
mean we hit the breakpoint" and converts it to SIGTRAP.  On architectures
with imprecise fault reporting, this is wrong: if a breakpoint is never
reported via SIGSEGV, then receiving a SIGSEGV at a breakpoint location
means we have a genuine SIGSEGV, not a breakpoint hit.

Add a new gdbarch function, imprecise_pagefault_reporting, that allows the
signal conversion from GDB_SIGNAL_SEGV to GDB_SIGNAL_TRAP to be skipped for
an architecture.  The default is false (conversion enabled), preserving
existing behavior.  Architectures with imprecise fault reporting should
override this to return true.
---
 gdb/gdbarch-gen.c         | 22 ++++++++++++++++++++++
 gdb/gdbarch-gen.h         | 24 ++++++++++++++++++++++++
 gdb/gdbarch_components.py | 29 +++++++++++++++++++++++++++++
 gdb/infrun.c              |  4 +++-
 4 files changed, 78 insertions(+), 1 deletion(-)

Comments

Aktemur, Baris Sept. 1, 2026, 7:02 a.m. UTC | #1 
AMD General

On Friday, August 28, 2026 3:50 PM, Klaus Gerlicher wrote:
> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
>
> GDB converts signals GDB_SIGNAL_ILL, GDB_SIGNAL_SEGV and GDB_SIGNAL_EMT to
> GDB_SIGNAL_TRAP if a breakpoint is inserted at the fault location.  This
> conversion logic assumes that when these signals occur at a breakpoint
> location, the signal was actually caused by hitting the breakpoint rather
> than a genuine fault.
>
> However, some architectures have "imprecise page fault reporting," where a
> memory access violation can be reported several instructions after the
> faulting instruction.  For example:
>
>   INSN1  <-- generates a SIGSEGV
>   INSN2
>   INSN3  <-- breakpoint installed here
>
> If INSN1 causes a memory access violation, the backend may report the stop
> at INSN3, where a breakpoint happens to be installed.  GDB's logic then
> incorrectly assumes "there is a breakpoint at INSN3, so this SIGSEGV must
> mean we hit the breakpoint" and converts it to SIGTRAP.  On architectures
> with imprecise fault reporting, this is wrong: if a breakpoint is never
> reported via SIGSEGV, then receiving a SIGSEGV at a breakpoint location
> means we have a genuine SIGSEGV, not a breakpoint hit.
>
> Add a new gdbarch function, imprecise_pagefault_reporting, that allows the
> signal conversion from GDB_SIGNAL_SEGV to GDB_SIGNAL_TRAP to be skipped for
> an architecture.  The default is false (conversion enabled), preserving
> existing behavior.  Architectures with imprecise fault reporting should
> override this to return true.

Looks good to me.  Thanks.

Reviewed-By: Tankut Baris Aktemur <TankutBaris.Aktemur@amd.com>

When this is merged, I'll submit a patch that overrides the gdbarch method
in the amdgpu target and includes a test.

-Baris
Tom Tromey Sept. 2, 2026, 7:20 p.m. UTC | #2 
>>>>> 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_TRAP if a breakpoint is inserted at the fault location.  This
> conversion logic assumes that when these signals occur at a breakpoint
> location, the signal was actually caused by hitting the breakpoint rather
> than a genuine fault.

Thanks for the patch.

> Add a new gdbarch function, imprecise_pagefault_reporting, that allows the
> signal conversion from GDB_SIGNAL_SEGV to GDB_SIGNAL_TRAP to be skipped for
> an architecture.  The default is false (conversion enabled), preserving
> existing behavior.  Architectures with imprecise fault reporting should
> override this to return true.

My first thought was that, without an implementation in the tree, this
will cause gdb/check-gdbarch.py to complain.  However, I saw that the
plan is a follow-up patch to use this.

Maybe temporarily marking this 'unused' would be good, provided that is
removed in the follow-up.

Also I was wondering, why a Function and not a Value?

> +    predefault="[] () -> bool {return false;}",

This doesn't take arguments, so presumably just a value would be
sufficient?

Tom
diff mbox series

Patch

diff --git a/gdb/gdbarch-gen.c b/gdb/gdbarch-gen.c
index 6008003466c..b3a699598f9 100644
--- a/gdb/gdbarch-gen.c
+++ b/gdb/gdbarch-gen.c
@@ -253,6 +253,7 @@  struct gdbarch
   gdbarch_core_parse_exec_context_ftype *core_parse_exec_context = default_core_parse_exec_context;
   gdbarch_shadow_stack_push_ftype *shadow_stack_push = nullptr;
   gdbarch_get_shadow_stack_pointer_ftype *get_shadow_stack_pointer = default_get_shadow_stack_pointer;
+  gdbarch_imprecise_pagefault_reporting_ftype *imprecise_pagefault_reporting = [] () -> bool {return false;};
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -513,6 +514,7 @@  verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of core_parse_exec_context, invalid_p == 0.  */
   /* Skip verify of shadow_stack_push, has predicate.  */
   /* Skip verify of get_shadow_stack_pointer, invalid_p == 0.  */
+  /* Skip verify of imprecise_pagefault_reporting, invalid_p == 0.  */
   if (!log.empty ())
     internal_error (_("verify_gdbarch: the following are invalid ...%s"),
 		    log.c_str ());
@@ -1339,6 +1341,9 @@  gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
 	      "gdbarch_dump: get_shadow_stack_pointer = <%s>\n",
 	      host_address_to_string (gdbarch->get_shadow_stack_pointer));
+  gdb_printf (file,
+	      "gdbarch_dump: imprecise_pagefault_reporting = <%s>\n",
+	      host_address_to_string (gdbarch->imprecise_pagefault_reporting));
   if (gdbarch->dump_tdep != nullptr)
     gdbarch->dump_tdep (gdbarch, file);
 }
@@ -5286,3 +5291,20 @@  set_gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch,
 {
   gdbarch->get_shadow_stack_pointer = get_shadow_stack_pointer;
 }
+
+bool
+gdbarch_imprecise_pagefault_reporting (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != nullptr);
+  gdb_assert (gdbarch->imprecise_pagefault_reporting != nullptr);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_imprecise_pagefault_reporting called\n");
+  return gdbarch->imprecise_pagefault_reporting ();
+}
+
+void
+set_gdbarch_imprecise_pagefault_reporting (struct gdbarch *gdbarch,
+					   gdbarch_imprecise_pagefault_reporting_ftype imprecise_pagefault_reporting)
+{
+  gdbarch->imprecise_pagefault_reporting = imprecise_pagefault_reporting;
+}
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index 6eda8693d58..3175e712c55 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1758,3 +1758,27 @@  void set_gdbarch_shadow_stack_push (struct gdbarch *gdbarch, gdbarch_shadow_stac
 using gdbarch_get_shadow_stack_pointer_ftype = std::optional<CORE_ADDR> (struct gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled);
 std::optional<CORE_ADDR> gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch, regcache *regcache, bool &shadow_stack_enabled);
 void set_gdbarch_get_shadow_stack_pointer (struct gdbarch *gdbarch, gdbarch_get_shadow_stack_pointer_ftype *get_shadow_stack_pointer);
+
+/* Returns true if architecture has imprecise page fault reporting, where a
+   memory access violation may be reported several instructions after the
+   faulting instruction.
+
+   On such architectures, if an instruction causes a memory access violation,
+   the backend may report the stop at a later instruction where a breakpoint
+   happens to be installed.  GDB's normal logic would incorrectly assume that
+   the SIGSEGV was caused by hitting the breakpoint and convert it to SIGTRAP.
+
+   However, if an architecture never reports breakpoints via SIGSEGV (e.g.,
+   breakpoints are always reported via a dedicated mechanism), then receiving
+   a SIGSEGV at a breakpoint location means we have a genuine SIGSEGV, not a
+   breakpoint hit.
+
+   Return true for architectures with imprecise fault reporting to disable the
+   SIGSEGV-to-SIGTRAP conversion.  The default is false (conversion enabled)
+   to preserve existing behavior for architectures where breakpoints may be
+   reported as SIGSEGV (e.g., executing a breakpoint instruction on a
+   non-executable stack). */
+
+using gdbarch_imprecise_pagefault_reporting_ftype = bool ();
+bool gdbarch_imprecise_pagefault_reporting (struct gdbarch *gdbarch);
+void set_gdbarch_imprecise_pagefault_reporting (struct gdbarch *gdbarch, gdbarch_imprecise_pagefault_reporting_ftype *imprecise_pagefault_reporting);
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index d8b2d114909..09d3130e753 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -2789,3 +2789,32 @@  SHADOW_STACK_ENABLED to false.
     predefault="default_get_shadow_stack_pointer",
     invalid=False,
 )
+
+Function(
+    comment="""
+Returns true if architecture has imprecise page fault reporting, where a
+memory access violation may be reported several instructions after the
+faulting instruction.
+
+On such architectures, if an instruction causes a memory access violation,
+the backend may report the stop at a later instruction where a breakpoint
+happens to be installed.  GDB's normal logic would incorrectly assume that
+the SIGSEGV was caused by hitting the breakpoint and convert it to SIGTRAP.
+
+However, if an architecture never reports breakpoints via SIGSEGV (e.g.,
+breakpoints are always reported via a dedicated mechanism), then receiving
+a SIGSEGV at a breakpoint location means we have a genuine SIGSEGV, not a
+breakpoint hit.
+
+Return true for architectures with imprecise fault reporting to disable the
+SIGSEGV-to-SIGTRAP conversion.  The default is false (conversion enabled)
+to preserve existing behavior for architectures where breakpoints may be
+reported as SIGSEGV (e.g., executing a breakpoint instruction on a
+non-executable stack).
+""",
+    type="bool",
+    name="imprecise_pagefault_reporting",
+    params=[],
+    predefault="[] () -> bool {return false;}",
+    invalid=False,
+)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c0767e7f764..25ed7e7523f 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6331,7 +6331,9 @@  handle_inferior_event (struct execution_control_state *ecs)
      stack.  */
   if (ecs->ws.kind () == TARGET_WAITKIND_STOPPED
       && (ecs->ws.sig () == GDB_SIGNAL_ILL
-	  || ecs->ws.sig () == GDB_SIGNAL_SEGV
+	  || (ecs->ws.sig () == GDB_SIGNAL_SEGV
+	      && !gdbarch_imprecise_pagefault_reporting
+		  (target_thread_architecture (ecs->event_thread->ptid)))
 	  || ecs->ws.sig () == GDB_SIGNAL_EMT))
     {
       struct regcache *regcache = get_thread_regcache (ecs->event_thread);