[PATCH 0/3] Use signal information to determine SIGTRAP type on FreeBSD

[PATCH 0/3] Use signal information to determine SIGTRAP type on FreeBSD

[John Baldwin]

Use the signal information for a SIGTRAP signal to determine when an
event in a child process is a breakpoint event versus another event.
FreeBSD currently doesn't have a TRAP_HWBKPT (though I'm working on
fixing that), so this patchset queries the x86 debug registers on x86
platforms for a SIGTRAP signal a TRAP_TRACE signal code.  (TRAP_TRACE is
currently used in FreeBSD for any DB# exception on x86 including a single
step trap or a watchpoint trigger.)

This fixed 27 previously failing tests in the testsuite for me on
FreeBSD/amd64.

John Baldwin (3):
  Implement "to_stopped_by_hw_breakpoint" for x86 debug registers.
  Add a new debug knob for the FreeBSD native target.
  Use signal information to determine SIGTRAP type for FreeBSD.

 gdb/ChangeLog       |  32 ++++++++++++
 gdb/NEWS            |   6 +++
 gdb/doc/ChangeLog   |   5 ++
 gdb/doc/gdb.texinfo |   5 ++
 gdb/fbsd-nat.c      | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/nat/x86-dregs.c |  45 +++++++++++++++++
 gdb/nat/x86-dregs.h |   4 ++
 gdb/x86-nat.c       |  13 +++++
 8 files changed, 249 insertions(+)

-- 
2.15.1

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

[John Baldwin]

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(+)

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

[PATCH 2/3] Add a new debug knob for the FreeBSD native target.

[John Baldwin]

For now this just logs information about the state of the current LWP
for each STOPPED event in fbsd_wait().

gdb/ChangeLog:

	* NEWS (Changes since GDB 8.1): Add "set/show debug fbsd-nat".
	* fbsd-nat.c (debug_fbsd_nat): New variable.
	(show_fbsd_nat_debug): New function.
	(fbsd_wait): Log LWP info if "debug_fbsd_nat" is enabled.
	(_initialize_fbsd_nat): Add "fbsd-nat" debug boolean command.

gdb/doc/ChangeLog:

	* gdb.texinfo (Debugging Output): Document "set/show debug
	fbsd-nat".
---
 gdb/ChangeLog       |  8 ++++++++
 gdb/NEWS            |  6 ++++++
 gdb/doc/ChangeLog   |  5 +++++
 gdb/doc/gdb.texinfo |  5 +++++
 gdb/fbsd-nat.c      | 29 +++++++++++++++++++++++++++++
 5 files changed, 53 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0b4a308ef5..b8412304b6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-02-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* NEWS (Changes since GDB 8.1): Add "set/show debug fbsd-nat".
+	* fbsd-nat.c (debug_fbsd_nat): New variable.
+	(show_fbsd_nat_debug): New function.
+	(fbsd_wait): Log LWP info if "debug_fbsd_nat" is enabled.
+	(_initialize_fbsd_nat): Add "fbsd-nat" debug boolean command.
+
 2018-02-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* nat/x86-dregs.c (x86_dr_stopped_by_breakpoint): New function.
diff --git a/gdb/NEWS b/gdb/NEWS
index 1767cef920..867e268a2a 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -6,6 +6,12 @@
 * 'info proc' now works on running processes on FreeBSD systems and core
   files created on FreeBSD systems.
 
+* New commands
+
+set debug fbsd-nat
+show debug fbsd-nat
+  Control display of debugging info regarding the FreeBSD native target.
+
 *** Changes in GDB 8.1
 
 * GDB now supports dynamically creating arbitrary register groups specified
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 037173ce0a..29757481b3 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2018-02-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* gdb.texinfo (Debugging Output): Document "set/show debug
+	fbsd-nat".
+
 2018-02-19  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.texinfo (Machine Code): Additional information about "info
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ee7adc8df2..74e0fdb4a4 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -24554,6 +24554,11 @@ Displays the current state of displaying debugging info about
 Turns on or off debugging messages from the FreeBSD LWP debug support.
 @item show debug fbsd-lwp
 Show the current state of FreeBSD LWP debugging messages.
+@item set debug fbsd-nat
+@cindex FreeBSD native target debug messages
+Turns on or off debugging messages from the FreeBSD native target.
+@item show debug fbsd-nat
+Show the current state of FreeBSD native target debugging messages.
 @item set debug frame
 @cindex frame debugging info
 Turns on or off display of @value{GDBN} frame debugging info.  The
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 3a216abf18..2516ac5552 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -765,6 +765,7 @@ fbsd_xfer_partial (struct target_ops *ops, enum target_object object,
 
 #ifdef PT_LWPINFO
 static int debug_fbsd_lwp;
+static int debug_fbsd_nat;
 
 static void (*super_resume) (struct target_ops *,
 			     ptid_t,
@@ -782,6 +783,14 @@ show_fbsd_lwp_debug (struct ui_file *file, int from_tty,
   fprintf_filtered (file, _("Debugging of FreeBSD lwp module is %s.\n"), value);
 }
 
+static void
+show_fbsd_nat_debug (struct ui_file *file, int from_tty,
+		     struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("Debugging of FreeBSD native target is %s.\n"),
+		    value);
+}
+
 /*
   FreeBSD's first thread support was via a "reentrant" version of libc
   (libc_r) that first shipped in 2.2.7.  This library multiplexed all
@@ -1212,6 +1221,18 @@ fbsd_wait (struct target_ops *ops,
 
 	  wptid = ptid_build (pid, pl.pl_lwpid, 0);
 
+	  if (debug_fbsd_nat)
+	    {
+	      fprintf_unfiltered (gdb_stdlog,
+				  "FNAT: stop for LWP %u event %d flags %#x\n",
+				  pl.pl_lwpid, pl.pl_event, pl.pl_flags);
+	      if (pl.pl_flags & PL_FLAG_SI)
+		fprintf_unfiltered (gdb_stdlog,
+				    "FNAT: si_signo %u si_code %u\n",
+				    pl.pl_siginfo.si_signo,
+				    pl.pl_siginfo.si_code);
+	    }
+
 #ifdef PT_LWP_EVENTS
 	  if (pl.pl_flags & PL_FLAG_EXITED)
 	    {
@@ -1569,5 +1590,13 @@ Enables printf debugging output."),
 			   NULL,
 			   &show_fbsd_lwp_debug,
 			   &setdebuglist, &showdebuglist);
+  add_setshow_boolean_cmd ("fbsd-nat", class_maintenance,
+			   &debug_fbsd_nat, _("\
+Set debugging of FreeBSD native target."), _("\
+Show debugging of FreeBSD native target."), _("\
+Enables printf debugging output."),
+			   NULL,
+			   &show_fbsd_nat_debug,
+			   &setdebuglist, &showdebuglist);
 #endif
 }
-- 
2.15.1

[PATCH 1/3] Implement "to_stopped_by_hw_breakpoint" for x86 debug registers.

[John Baldwin]

Report that a thread is stopped by a hardware breakpoint if a non-data
watchpoint is set in DR6.  This change should be a no-op since a target
still needs to implement the "to_supports_stopped_by_hw_breakpoint"
method before this function is used.

gdb/ChangeLog:

	* nat/x86-dregs.c (x86_dr_stopped_by_breakpoint): New function.
	* nat/x86-dregs.h (x86_dr_stopped_by_breakpoint): New prototype.
	* x86-nat.c (x86_stopped_by_hw_breakpoint): New function.
	(x86_use_watchpoints): Set "stopped_by_hw_breakpoint" target
	method.
---
 gdb/ChangeLog       |  8 ++++++++
 gdb/nat/x86-dregs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/nat/x86-dregs.h |  4 ++++
 gdb/x86-nat.c       | 13 +++++++++++++
 4 files changed, 70 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8ab6381add..0b4a308ef5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-02-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* nat/x86-dregs.c (x86_dr_stopped_by_breakpoint): New function.
+	* nat/x86-dregs.h (x86_dr_stopped_by_breakpoint): New prototype.
+	* x86-nat.c (x86_stopped_by_hw_breakpoint): New function.
+	(x86_use_watchpoints): Set "stopped_by_hw_breakpoint" target
+	method.
+
 2018-02-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-nat.c (fbsd_resume): Use PT_SETSTEP for stepping and a
diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
index c816473628..514fde0253 100644
--- a/gdb/nat/x86-dregs.c
+++ b/gdb/nat/x86-dregs.c
@@ -649,3 +649,48 @@ x86_dr_stopped_by_watchpoint (struct x86_debug_reg_state *state)
   CORE_ADDR addr = 0;
   return x86_dr_stopped_data_address (state, &addr);
 }
+
+/* Return non-zero if the inferior has some breakpoint that triggered.
+   Otherwise return zero.  */
+
+int
+x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state)
+{
+  CORE_ADDR addr = 0;
+  int i;
+  int rc = 0;
+  /* The current thread's DR_STATUS.  We always need to read this to
+     check whether some watchpoint caused the trap.  */
+  unsigned status;
+  /* We need DR_CONTROL as well, but only iff DR_STATUS indicates a
+     breakpoint trap.  Only fetch it when necessary, to avoid an
+     unnecessary extra syscall when no watchpoint triggered.  */
+  int control_p = 0;
+  unsigned control = 0;
+
+  /* As above, always read the current thread's debug registers rather
+     than trusting dr_mirror.  */
+  status = x86_dr_low_get_status ();
+
+  ALL_DEBUG_ADDRESS_REGISTERS (i)
+    {
+      if (!X86_DR_WATCH_HIT (status, i))
+	continue;
+
+      if (!control_p)
+	{
+	  control = x86_dr_low_get_control ();
+	  control_p = 1;
+	}
+
+      if (X86_DR_GET_RW_LEN (control, i) == 0)
+	{
+	  addr = x86_dr_low_get_addr (i);
+	  rc = 1;
+	  if (show_debug_regs)
+	    x86_show_dr (state, "watchpoint_hit", addr, -1, hw_execute);
+	}
+    }
+
+  return rc;
+}
diff --git a/gdb/nat/x86-dregs.h b/gdb/nat/x86-dregs.h
index dd6242eda9..84d710c34e 100644
--- a/gdb/nat/x86-dregs.h
+++ b/gdb/nat/x86-dregs.h
@@ -128,4 +128,8 @@ extern int x86_dr_stopped_data_address (struct x86_debug_reg_state *state,
    Otherwise return false.  */
 extern int x86_dr_stopped_by_watchpoint (struct x86_debug_reg_state *state);
 
+/* Return true if the inferior has some breakpoint that triggered.
+   Otherwise return false.  */
+extern int x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state);
+
 #endif /* X86_DREGS_H */
diff --git a/gdb/x86-nat.c b/gdb/x86-nat.c
index b126c47c94..e32450afdf 100644
--- a/gdb/x86-nat.c
+++ b/gdb/x86-nat.c
@@ -260,6 +260,18 @@ x86_can_use_hw_breakpoint (struct target_ops *self,
   return 1;
 }
 
+/* Return non-zero if the inferior has some breakpoint that triggered.
+   Otherwise return zero.  */
+
+static int
+x86_stopped_by_hw_breakpoint (struct target_ops *ops)
+{
+  struct x86_debug_reg_state *state
+    = x86_debug_reg_state (ptid_get_pid (inferior_ptid));
+
+  return x86_dr_stopped_by_breakpoint (state);
+}
+
 static void
 add_show_debug_regs_command (void)
 {
@@ -293,6 +305,7 @@ x86_use_watchpoints (struct target_ops *t)
   t->to_region_ok_for_hw_watchpoint = x86_region_ok_for_watchpoint;
   t->to_stopped_by_watchpoint = x86_stopped_by_watchpoint;
   t->to_stopped_data_address = x86_stopped_data_address;
+  t->to_stopped_by_hw_breakpoint = x86_stopped_by_hw_breakpoint;
   t->to_insert_watchpoint = x86_insert_watchpoint;
   t->to_remove_watchpoint = x86_remove_watchpoint;
   t->to_insert_hw_breakpoint = x86_insert_hw_breakpoint;
-- 
2.15.1

Re: [PATCH 2/3] Add a new debug knob for the FreeBSD native target.

[Eli Zaretskii]

> From: John Baldwin <jhb@FreeBSD.org>
> Date: Tue, 27 Feb 2018 17:46:55 -0800
> 
> For now this just logs information about the state of the current LWP
> for each STOPPED event in fbsd_wait().
> 
> gdb/ChangeLog:
> 
> 	* NEWS (Changes since GDB 8.1): Add "set/show debug fbsd-nat".
> 	* fbsd-nat.c (debug_fbsd_nat): New variable.
> 	(show_fbsd_nat_debug): New function.
> 	(fbsd_wait): Log LWP info if "debug_fbsd_nat" is enabled.
> 	(_initialize_fbsd_nat): Add "fbsd-nat" debug boolean command.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Debugging Output): Document "set/show debug
> 	fbsd-nat".

The documentation parts of this are approved.

Thanks.

Re: [PATCH 1/3] Implement "to_stopped_by_hw_breakpoint" for x86 debug registers.

[Pedro Alves]

Hi John,

This LGTM, with the nits below addressed.

On 02/28/2018 01:46 AM, John Baldwin wrote:
> This change should be a no-op since a target
> still needs to implement the "to_supports_stopped_by_hw_breakpoint"
> method before this function is used.

I think it'd be good to dd something like this as a comment somewhere.
Maybe next to where t->to_stopped_by_hw_breakpoint is set?

Because while looking at the patch, I didn't notice that comment in
the git log, and it took me a bit to realize that this does not affect
all x86 ports as is.

> +/* Return non-zero if the inferior has some breakpoint that triggered.
> +   Otherwise return zero.  */
> +
> +int
> +x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state)
> +{

I was also slightly confused by this until I realized that you
meant _hardware_ breakpoint.  Can you rename this to
x86_dr_stopped_by_hw_breakpoint, and update the comment too?

Thanks,
Pedro Alves

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

[Pedro Alves]

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

Re: [PATCH 2/3] Add a new debug knob for the FreeBSD native target.

[Pedro Alves]

LGTM, but one small question below.

On 02/28/2018 01:46 AM, John Baldwin wrote:
> @@ -765,6 +765,7 @@ fbsd_xfer_partial (struct target_ops *ops, enum target_object object,
>  
>  #ifdef PT_LWPINFO
>  static int debug_fbsd_lwp;
> +static int debug_fbsd_nat;

Should this be guarded by PT_LWPINFO?  Wouldn't you want to
enable fbsd-nat debugging on FreeBSD systems without PT_LWPINFO?

(Hmm, does GDB actually build today if PT_LWPINFO is not defined?)

Thanks,
Pedro Alves

Re: [PATCH 2/3] Add a new debug knob for the FreeBSD native target.

[John Baldwin]

On Friday, March 02, 2018 08:19:33 PM Pedro Alves wrote:
> LGTM, but one small question below.
> 
> On 02/28/2018 01:46 AM, John Baldwin wrote:
> > @@ -765,6 +765,7 @@ fbsd_xfer_partial (struct target_ops *ops, enum target_object object,
> >  
> >  #ifdef PT_LWPINFO
> >  static int debug_fbsd_lwp;
> > +static int debug_fbsd_nat;
> 
> Should this be guarded by PT_LWPINFO?  Wouldn't you want to
> enable fbsd-nat debugging on FreeBSD systems without PT_LWPINFO?

So fbsd-nat omits a lot of functionality including custom to_wait/to_resume
methods where debug_fbsd_nat is used both in this patch and the next one for
systems without PT_LWPINFO.  There isn't a way to fetch the siginfo to parse
the trap code without PT_LWPINFO for example.  I don't expect to be adding
any new features that would use this debugging on systems without PT_LWPINFO
at this point.  For reference, PT_LWPINFO was added to FreeBSD in 5.0 release
which was released back in 2003.  The last release which didn't support it
was 4.11 released in January 2005.

> (Hmm, does GDB actually build today if PT_LWPINFO is not defined?)

I believe it should, but given how old of a FreeBSD version you'd need for
this to matter, I should perhaps require it to simplify the #ifdef forest
in the FreeBSD native target a bit.

-- 
John Baldwin

Re: [PATCH 1/3] Implement "to_stopped_by_hw_breakpoint" for x86 debug registers.

[John Baldwin]

On 3/2/18 12:19 PM, Pedro Alves wrote:
> Hi John,
> 
> This LGTM, with the nits below addressed.
> 
> On 02/28/2018 01:46 AM, John Baldwin wrote:
>> This change should be a no-op since a target
>> still needs to implement the "to_supports_stopped_by_hw_breakpoint"
>> method before this function is used.
> 
> I think it'd be good to dd something like this as a comment somewhere.
> Maybe next to where t->to_stopped_by_hw_breakpoint is set?
> 
> Because while looking at the patch, I didn't notice that comment in
> the git log, and it took me a bit to realize that this does not affect
> all x86 ports as is.

Ok, will do.  The comment is also useful to help clarify the difference
with 'to_stopped_by_watchpoint' which doesn't have a matching
'to_supports_stopped_by_watchpoint'.  (I found this difference a bit
confusing myself.)

>> +/* Return non-zero if the inferior has some breakpoint that triggered.
>> +   Otherwise return zero.  */
>> +
>> +int
>> +x86_dr_stopped_by_breakpoint (struct x86_debug_reg_state *state)
>> +{
> 
> I was also slightly confused by this until I realized that you
> meant _hardware_ breakpoint.  Can you rename this to
> x86_dr_stopped_by_hw_breakpoint, and update the comment too?

Yes.  I had originally modeled this on the 'stopped_by_watchpoint' naming
and had just assumed 'hw_' since x86 debug registers aren't used for
software breakpoints, but I agree that the more explicit name is clearer.

-- 
John Baldwin

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

[John Baldwin]

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.

-- 
John Baldwin