[3/3] Use signal information to determine SIGTRAP type for FreeBSD.

Message ID 20180228014656.32372-4-jhb@FreeBSD.org
State New, archived
Headers

Commit Message

John Baldwin Feb. 28, 2018, 1:46 a.m. UTC
  Use the signal code from siginfo_t to distinguish SIGTRAP events due
to trace traps (TRAP_TRACE) and software breakpoints (TRAP_BRKPT).
For software breakpoints, adjust the PC when the event is reported as
part of the API when supplying "stopped_by_sw_breakpoint".  Currently
FreeBSD only supports hardware watchpoints and breakpoints on x86
which are reported as trace traps.  Signal information is not used on
MIPS and sparc64 kernels which do not reliably report TRAP_BRKPT for
software breakpoints.

gdb/ChangeLog:

	* fbsd-nat.c: Include "inf-ptrace.h".
	(USE_SIGTRAP_SIGINFO): Conditionally define.
	[USE_SIGTRAP_SIGINFO] (fbsd_handle_debug_trap): New function.
	(fbsd_wait) [USE_SIGTRAP_SIGINFO]: Call "fbsd_handle_debug_trap".
	[USE_SIGTRAP_SIGINFO] (fbsd_stopped_by_sw_breakpoint): New
	function.
	[USE_SIGTRAP_SIGINFO] (fbsd_supports_stopped_by_sw_breakpoint):
	Likewise.
	[USE_SIGTRAP_SIGINFO] (fbsd_supports_stopped_by_hw_breakpoint):
	Likewise.
	(fbsd_nat_add_target) [USE_SIGTRAP_SIGINFO]: Set
	"stopped_by_sw_breakpoint", "supports_stopped_by_sw_breakpoint",
	"supports_stopped_by_hw_breakpoint" target methods.
---
 gdb/ChangeLog  |  16 +++++++++
 gdb/fbsd-nat.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)
  

Comments

Pedro Alves March 2, 2018, 8:19 p.m. UTC | #1
Hi John,

LGTM, with nits below.  Please go ahead and push with
these fixed.

On 02/28/2018 01:46 AM, John Baldwin wrote:

> +#ifdef TRAP_BRKPT
> +/*
> + * MIPS does not set si_code for SIGTRAP.  sparc64 reports
> + * non-standard values in si_code for SIGTRAP.
> + */

GNU formatting, please.

> +#if !defined(__mips__) && !defined(__sparc64__)
> +#define	USE_SIGTRAP_SIGINFO
          ^^^

tab vs space?


> +#endif
> +#endif
> +

I'd suggest indenting the directives, like:

#ifdef TRAP_BRKPT
...
# if !defined(__mips__) && !defined(__sparc64__)
#  define USE_SIGTRAP_SIGINFO
# endif
#endif


>  /* Wait for the child specified by PTID to do something.  Return the
>     process ID of the child, or MINUS_ONE_PTID in case of error; store
>     the status in *OURSTATUS.  */
> @@ -1372,6 +1433,13 @@ fbsd_wait (struct target_ops *ops,
>  	    }
>  #endif
>  
> +#ifdef USE_SIGTRAP_SIGINFO
> +	  if (fbsd_handle_debug_trap (ops, wptid, pl))
> +	    {
> +	      return wptid;
> +	    }

Remove redundant braces.

> +#endif
> +
>  	  /* Note that PL_FLAG_SCE is set for any event reported while
>  	     a thread is executing a system call in the kernel.  In
>  	     particular, signals that interrupt a sleep in a system
> @@ -1410,6 +1478,41 @@ fbsd_wait (struct target_ops *ops,
>      }
>  }
>  
> +#ifdef USE_SIGTRAP_SIGINFO
> +/* Implement the "to_stopped_by_sw_breakpoint" target_ops method.  */
> +
> +static int
> +fbsd_stopped_by_sw_breakpoint (struct target_ops *ops)
> +{
> +  struct ptrace_lwpinfo pl;
> +
> +  if (ptrace (PT_LWPINFO, get_ptrace_pid (inferior_ptid), (caddr_t) &pl,
> +	      sizeof pl) == -1)
> +    return 0;
> +
> +  return (pl.pl_flags & PL_FLAG_SI) && pl.pl_siginfo.si_signo == SIGTRAP
> +    && pl.pl_siginfo.si_code == TRAP_BRKPT;

For multi-line expressions, the convention is to wrap in ()s so
that the && aligns under the (.  Like:

  return ((pl.pl_flags & PL_FLAG_SI)
          && pl.pl_siginfo.si_signo == SIGTRAP
          && pl.pl_siginfo.si_code == TRAP_BRKPT);

> +#ifdef USE_SIGTRAP_SIGINFO
> +  t->to_stopped_by_sw_breakpoint = fbsd_stopped_by_sw_breakpoint;
> +  t->to_supports_stopped_by_sw_breakpoint =
> +    fbsd_supports_stopped_by_sw_breakpoint;
> +  t->to_supports_stopped_by_hw_breakpoint =
> +    fbsd_supports_stopped_by_hw_breakpoint;

'=' goes on the next line, like:

 t->to_supports_stopped_by_sw_breakpoint 
   = fbsd_supports_stopped_by_sw_breakpoint;

Thanks,
Pedro Alves
  
John Baldwin March 4, 2018, 5:34 a.m. UTC | #2
On 3/2/18 12:19 PM, Pedro Alves wrote:
> Hi John,
> 
> LGTM, with nits below.  Please go ahead and push with
> these fixed.

Thanks for the review.  I fixed the things mentioned (they are
generally just me falling back to FreeBSD style rules out of habit
instead of GNU rules).  I took your suggested indentation for the
nested cpp directives as well.
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b8412304b6..50363d5e60 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@ 
+2018-02-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* fbsd-nat.c: Include "inf-ptrace.h".
+	(USE_SIGTRAP_SIGINFO): Conditionally define.
+	[USE_SIGTRAP_SIGINFO] (fbsd_handle_debug_trap): New function.
+	(fbsd_wait) [USE_SIGTRAP_SIGINFO]: Call "fbsd_handle_debug_trap".
+	[USE_SIGTRAP_SIGINFO] (fbsd_stopped_by_sw_breakpoint): New
+	function.
+	[USE_SIGTRAP_SIGINFO] (fbsd_supports_stopped_by_sw_breakpoint):
+	Likewise.
+	[USE_SIGTRAP_SIGINFO] (fbsd_supports_stopped_by_hw_breakpoint):
+	Likewise.
+	(fbsd_nat_add_target) [USE_SIGTRAP_SIGINFO]: Set
+	"stopped_by_sw_breakpoint", "supports_stopped_by_sw_breakpoint",
+	"supports_stopped_by_hw_breakpoint" target methods.
+
 2018-02-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* NEWS (Changes since GDB 8.1): Add "set/show debug fbsd-nat".
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 2516ac5552..d36d4c9299 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -26,6 +26,7 @@ 
 #include "gdbcmd.h"
 #include "gdbthread.h"
 #include "gdb_wait.h"
+#include "inf-ptrace.h"
 #include <sys/types.h>
 #include <sys/procfs.h>
 #include <sys/ptrace.h>
@@ -45,6 +46,16 @@ 
 
 #include <list>
 
+#ifdef TRAP_BRKPT
+/*
+ * MIPS does not set si_code for SIGTRAP.  sparc64 reports
+ * non-standard values in si_code for SIGTRAP.
+ */
+#if !defined(__mips__) && !defined(__sparc64__)
+#define	USE_SIGTRAP_SIGINFO
+#endif
+#endif
+
 /* Return the name of a file that can be opened to get the symbols for
    the child process identified by PID.  */
 
@@ -1187,6 +1198,56 @@  fbsd_resume (struct target_ops *ops,
   super_resume (ops, ptid, step, signo);
 }
 
+#ifdef USE_SIGTRAP_SIGINFO
+/* Handle breakpoint and trace traps reported via SIGTRAP.  If the
+   trap was a breakpoint or trace trap that should be reported to the
+   core, return true.  */
+
+static bool
+fbsd_handle_debug_trap (struct target_ops *ops, ptid_t ptid,
+			const struct ptrace_lwpinfo &pl)
+{
+
+  /* Ignore traps without valid siginfo or for signals other than
+     SIGTRAP.  */
+  if (! (pl.pl_flags & PL_FLAG_SI) || pl.pl_siginfo.si_signo != SIGTRAP)
+    return false;
+
+  /* Trace traps are either a single step or a hardware watchpoint or
+     breakpoint.  */
+  if (pl.pl_siginfo.si_code == TRAP_TRACE)
+    {
+      if (debug_fbsd_nat)
+	fprintf_unfiltered (gdb_stdlog,
+			    "FNAT: trace trap for LWP %ld\n", ptid.lwp ());
+      return true;
+    }
+
+  if (pl.pl_siginfo.si_code == TRAP_BRKPT)
+    {
+      /* Fixup PC for the software breakpoint.  */
+      struct regcache *regcache = get_thread_regcache (ptid);
+      struct gdbarch *gdbarch = regcache->arch ();
+      int decr_pc = gdbarch_decr_pc_after_break (gdbarch);
+
+      if (debug_fbsd_nat)
+	fprintf_unfiltered (gdb_stdlog,
+			    "FNAT: sw breakpoint trap for LWP %ld\n",
+			    ptid.lwp ());
+      if (decr_pc != 0)
+	{
+	  CORE_ADDR pc;
+
+	  pc = regcache_read_pc (regcache);
+	  regcache_write_pc (regcache, pc - decr_pc);
+	}
+      return true;
+    }
+
+  return false;
+}
+#endif
+
 /* Wait for the child specified by PTID to do something.  Return the
    process ID of the child, or MINUS_ONE_PTID in case of error; store
    the status in *OURSTATUS.  */
@@ -1372,6 +1433,13 @@  fbsd_wait (struct target_ops *ops,
 	    }
 #endif
 
+#ifdef USE_SIGTRAP_SIGINFO
+	  if (fbsd_handle_debug_trap (ops, wptid, pl))
+	    {
+	      return wptid;
+	    }
+#endif
+
 	  /* Note that PL_FLAG_SCE is set for any event reported while
 	     a thread is executing a system call in the kernel.  In
 	     particular, signals that interrupt a sleep in a system
@@ -1410,6 +1478,41 @@  fbsd_wait (struct target_ops *ops,
     }
 }
 
+#ifdef USE_SIGTRAP_SIGINFO
+/* Implement the "to_stopped_by_sw_breakpoint" target_ops method.  */
+
+static int
+fbsd_stopped_by_sw_breakpoint (struct target_ops *ops)
+{
+  struct ptrace_lwpinfo pl;
+
+  if (ptrace (PT_LWPINFO, get_ptrace_pid (inferior_ptid), (caddr_t) &pl,
+	      sizeof pl) == -1)
+    return 0;
+
+  return (pl.pl_flags & PL_FLAG_SI) && pl.pl_siginfo.si_signo == SIGTRAP
+    && pl.pl_siginfo.si_code == TRAP_BRKPT;
+}
+
+/* Implement the "to_supports_stopped_by_sw_breakpoint" target_ops
+   method.  */
+
+static int
+fbsd_supports_stopped_by_sw_breakpoint (struct target_ops *ops)
+{
+  return 1;
+}
+
+/* Implement the "to_supports_stopped_by_hw_breakpoint" target_ops
+   method.  */
+
+static int
+fbsd_supports_stopped_by_hw_breakpoint (struct target_ops *ops)
+{
+  return ops->to_stopped_by_hw_breakpoint != NULL;
+}
+#endif
+
 #ifdef TDP_RFPPWAIT
 /* Target hook for follow_fork.  On entry and at return inferior_ptid is
    the ptid of the followed inferior.  */
@@ -1560,6 +1663,13 @@  fbsd_nat_add_target (struct target_ops *t)
   t->to_wait = fbsd_wait;
   t->to_post_startup_inferior = fbsd_post_startup_inferior;
   t->to_post_attach = fbsd_post_attach;
+#ifdef USE_SIGTRAP_SIGINFO
+  t->to_stopped_by_sw_breakpoint = fbsd_stopped_by_sw_breakpoint;
+  t->to_supports_stopped_by_sw_breakpoint =
+    fbsd_supports_stopped_by_sw_breakpoint;
+  t->to_supports_stopped_by_hw_breakpoint =
+    fbsd_supports_stopped_by_hw_breakpoint;
+#endif
 #ifdef TDP_RFPPWAIT
   t->to_follow_fork = fbsd_follow_fork;
   t->to_insert_fork_catchpoint = fbsd_insert_fork_catchpoint;