Re: [PATCH] gdb+gdbserver/Linux: Remove USE_SIGTRAP_SIGINFO fallback

[Pedro Alves <pedro@palves.net>]

On 2022-06-27 22:10, Pedro Alves wrote:
> It's been over 7 years (since commit faf09f0119da) since Linux GDB and
> GDBserver started relying on SIGTRAP si_code to tell whether a
> breakpoint triggered, which is important for non-stop mode.  When that
> then-new code was added, I had left the then-old code as fallback, in
> case some architectured still needed it.  Given AFAIK there haven't
> been complaints since, this commit finally removes the fallback code,
> along with USE_SIGTRAP_SIGINFO.
> 
> Change-Id: I140a5333a9fe70e90dbd186aca1f081549b2e63d

W00t.  I was looking at linux-nat.c today, and noticed the USE_SIGTRAP_SIGINFO
code was still around, which made me go like "but hadn't I removed this before??".
Well, turns out I forgot to merge this.

I'm going to merge it, after I download the patch from the list and
update that "7 years".  :-)

Pedro Alves

> ---
>  gdb/linux-nat.c        | 53 ++----------------------------------------
>  gdb/nat/linux-ptrace.h | 11 ++++-----
>  gdbserver/linux-low.cc | 39 ++-----------------------------
>  3 files changed, 8 insertions(+), 95 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index b9164e621db..008791c12dc 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -2439,17 +2439,6 @@ status_callback (struct lwp_info *lp)
>  	  discard = 1;
>  	}
>  
> -#if !USE_SIGTRAP_SIGINFO
> -      else if (!breakpoint_inserted_here_p (regcache->aspace (), pc))
> -	{
> -	  linux_nat_debug_printf ("previous breakpoint of %s, at %s gone",
> -				  lp->ptid.to_string ().c_str (),
> -				  paddress (target_gdbarch (), lp->stop_pc));
> -
> -	  discard = 1;
> -	}
> -#endif
> -
>        if (discard)
>  	{
>  	  linux_nat_debug_printf ("pending event of %s cancelled.",
> @@ -2529,9 +2518,7 @@ save_stop_reason (struct lwp_info *lp)
>    struct gdbarch *gdbarch;
>    CORE_ADDR pc;
>    CORE_ADDR sw_bp_pc;
> -#if USE_SIGTRAP_SIGINFO
>    siginfo_t siginfo;
> -#endif
>  
>    gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
>    gdb_assert (lp->status != 0);
> @@ -2545,7 +2532,6 @@ save_stop_reason (struct lwp_info *lp)
>    pc = regcache_read_pc (regcache);
>    sw_bp_pc = pc - gdbarch_decr_pc_after_break (gdbarch);
>  
> -#if USE_SIGTRAP_SIGINFO
>    if (linux_nat_get_siginfo (lp->ptid, &siginfo))
>      {
>        if (siginfo.si_signo == SIGTRAP)
> @@ -2588,22 +2574,6 @@ save_stop_reason (struct lwp_info *lp)
>  	    }
>  	}
>      }
> -#else
> -  if ((!lp->step || lp->stop_pc == sw_bp_pc)
> -      && software_breakpoint_inserted_here_p (regcache->aspace (),
> -					      sw_bp_pc))
> -    {
> -      /* The LWP was either continued, or stepped a software
> -	 breakpoint instruction.  */
> -      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -    }
> -
> -  if (hardware_breakpoint_inserted_here_p (regcache->aspace (), pc))
> -    lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -
> -  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
> -    check_stopped_by_watchpoint (lp);
> -#endif
>  
>    if (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
>      {
> @@ -2649,7 +2619,7 @@ linux_nat_target::stopped_by_sw_breakpoint ()
>  bool
>  linux_nat_target::supports_stopped_by_sw_breakpoint ()
>  {
> -  return USE_SIGTRAP_SIGINFO;
> +  return true;
>  }
>  
>  /* Returns true if the LWP had stopped for a hardware
> @@ -2670,7 +2640,7 @@ linux_nat_target::stopped_by_hw_breakpoint ()
>  bool
>  linux_nat_target::supports_stopped_by_hw_breakpoint ()
>  {
> -  return USE_SIGTRAP_SIGINFO;
> +  return true;
>  }
>  
>  /* Select one LWP out of those that have events pending.  */
> @@ -3252,25 +3222,6 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
>  
>    gdb_assert (lp != NULL);
>  
> -  /* Now that we've selected our final event LWP, un-adjust its PC if
> -     it was a software breakpoint, and we can't reliably support the
> -     "stopped by software breakpoint" stop reason.  */
> -  if (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
> -      && !USE_SIGTRAP_SIGINFO)
> -    {
> -      struct regcache *regcache = get_thread_regcache (linux_target, lp->ptid);
> -      struct gdbarch *gdbarch = regcache->arch ();
> -      int decr_pc = gdbarch_decr_pc_after_break (gdbarch);
> -
> -      if (decr_pc != 0)
> -	{
> -	  CORE_ADDR pc;
> -
> -	  pc = regcache_read_pc (regcache);
> -	  regcache_write_pc (regcache, pc + decr_pc);
> -	}
> -    }
> -
>    /* We'll need this to determine whether to report a SIGSTOP as
>       GDB_SIGNAL_0.  Need to take a copy because resume_clear_callback
>       clears it.  */
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index 4694046e1e8..3edc2363aa2 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -97,9 +97,9 @@ struct buffer;
>  #define __WALL          0x40000000 /* Wait for any child.  */
>  #endif
>  
> -/* True if whether a breakpoint/watchpoint triggered can be determined
> -   from the si_code of SIGTRAP's siginfo_t (TRAP_BRKPT/TRAP_HWBKPT).
> -   That is, if the kernel can tell us whether the thread executed a
> +/* Whether a breakpoint/watchpoint triggered can be determined from
> +   the si_code of SIGTRAP's siginfo_t (TRAP_BRKPT/TRAP_HWBKPT).  That
> +   is, since the kernel can tell us whether the thread executed a
>     software breakpoint, we trust it.  The kernel will be determining
>     that from the hardware (e.g., from which exception was raised in
>     the CPU).  Relying on whether a breakpoint is planted in memory at
> @@ -112,10 +112,7 @@ struct buffer;
>     architectures.  The moribund location mechanism helps with that
>     somewhat but it is an heuristic, and can well fail.  Getting that
>     information out of the kernel and ultimately out of the CPU is the
> -   way to go.  That said, some architecture may get the si_code wrong,
> -   and as such we're leaving fallback code in place.  We'll remove
> -   this after a while if no problem is reported.  */
> -#define USE_SIGTRAP_SIGINFO 1
> +   way to go.  */
>  
>  /* The x86 kernel gets some of the si_code values backwards, like
>     this:
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 8b8614f6ed4..9305928bbbf 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -790,9 +790,7 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
>  {
>    CORE_ADDR pc;
>    CORE_ADDR sw_breakpoint_pc;
> -#if USE_SIGTRAP_SIGINFO
>    siginfo_t siginfo;
> -#endif
>  
>    if (!low_supports_breakpoints ())
>      return false;
> @@ -804,7 +802,6 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
>    scoped_restore_current_thread restore_thread;
>    switch_to_thread (get_lwp_thread (lwp));
>  
> -#if USE_SIGTRAP_SIGINFO
>    if (ptrace (PTRACE_GETSIGINFO, lwpid_of (current_thread),
>  	      (PTRACE_TYPE_ARG3) 0, &siginfo) == 0)
>      {
> @@ -846,21 +843,6 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
>  	    }
>  	}
>      }
> -#else
> -  /* We may have just stepped a breakpoint instruction.  E.g., in
> -     non-stop mode, GDB first tells the thread A to step a range, and
> -     then the user inserts a breakpoint inside the range.  In that
> -     case we need to report the breakpoint PC.  */
> -  if ((!lwp->stepping || lwp->stop_pc == sw_breakpoint_pc)
> -      && low_breakpoint_at (sw_breakpoint_pc))
> -    lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -
> -  if (hardware_breakpoint_inserted_here (pc))
> -    lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -
> -  if (lwp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
> -    check_stopped_by_watchpoint (lwp);
> -#endif
>  
>    if (lwp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
>      {
> @@ -1648,23 +1630,6 @@ linux_process_target::thread_still_has_status_pending (thread_info *thread)
>  	  discard = 1;
>  	}
>  
> -#if !USE_SIGTRAP_SIGINFO
> -      else if (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
> -	       && !low_breakpoint_at (pc))
> -	{
> -	  threads_debug_printf ("previous SW breakpoint of %ld gone",
> -				lwpid_of (thread));
> -	  discard = 1;
> -	}
> -      else if (lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT
> -	       && !hardware_breakpoint_inserted_here (pc))
> -	{
> -	  threads_debug_printf ("previous HW breakpoint of %ld gone",
> -				lwpid_of (thread));
> -	  discard = 1;
> -	}
> -#endif
> -
>        if (discard)
>  	{
>  	  threads_debug_printf ("discarding pending breakpoint status");
> @@ -5555,7 +5520,7 @@ linux_process_target::stopped_by_sw_breakpoint ()
>  bool
>  linux_process_target::supports_stopped_by_sw_breakpoint ()
>  {
> -  return USE_SIGTRAP_SIGINFO;
> +  return true;
>  }
>  
>  /* Implement the stopped_by_hw_breakpoint target_ops
> @@ -5575,7 +5540,7 @@ linux_process_target::stopped_by_hw_breakpoint ()
>  bool
>  linux_process_target::supports_stopped_by_hw_breakpoint ()
>  {
> -  return USE_SIGTRAP_SIGINFO;
> +  return true;
>  }
>  
>  /* Implement the supports_hardware_single_step target_ops method.  */
> 
> base-commit: f18acc9c4e5d18f4783f3a7d59e3ec95d7af0199