[gdbserver] Disable conditional breakpoints on no-hardware-single-step targets

GDBserver steps over breakpoint if the condition is false, but if target
doesn't support hardware single step, the step over is very simple, if
not incorrect, in linux-arm-low.c:

/* We only place breakpoints in empty marker functions, and thread locking
   is outside of the function.  So rather than importing software single-step,
   we can just run until exit.  */
static CORE_ADDR
arm_reinsert_addr (void)
{
  struct regcache *regcache = get_thread_regcache (current_thread, 1);
  unsigned long pc;
  collect_register_by_name (regcache, "lr", &pc);
  return pc;
}

and linux-mips-low.c does the same.  GDBserver sets a breakpoint at the
return address of the current function, resume and wait the program hits
the breakpoint in order to achieve "breakpoint step over".  What if
program hits other user breakponits during this "step over"?

It is worse if the arm/thumb interworking is considered.  Nowadays,
GDBserver arm backend unconditionally inserts arm breakpoint,

  /* Define an ARM-mode breakpoint; we only set breakpoints in the C
     library, which is most likely to be ARM.  If the kernel supports
     clone events, we will never insert a breakpoint, so even a Thumb
     C library will work; so will mixing EABI/non-EABI gdbserver and
     application.  */
  (const unsigned char *) &arm_breakpoint,
  (const unsigned char *) &arm_eabi_breakpoint,

note that the comments are no longer valid as C library can be compiled
in thumb mode.

When GDBserver steps over a breakpoint in arm mode function, which
returns to thumb mode, GDBserver will insert arm mode breakpoint by
mistake and the program will crash.  GDBserver alone is unable to
determine the arm/thumb mode given a PC address.  See how GDB does
it in arm-tdep.c:arm_pc_is_thumb.

After thinking about how to teach GDBserver inserting right breakpoint
(arm or thumb) for a while, I reconsider it from a different direction
that it may be unreasonable to run target-side conditional breakpoint for
targets without hardware single step.  Pedro also pointed this out here
https://sourceware.org/ml/gdb-patches/2015-04/msg00337.html

This patch is to add a new target_ops hook
supports_conditional_breakpoints, and only reply
";ConditionalBreakpoints+" if it is true.  On linux targets,
supports_conditional_breakpoints returns true if target has hardware
single step, on other targets, (win32, lynx, nto, spu), set it to NULL,
because conditional breakpoint is a linux-specific feature.

gdb/gdbserver:

2015-05-08  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_supports_conditional_breakpoints): New
	function.
	(linux_target_ops): Install new target method.
	* lynx-low.c (lynx_target_ops): Install NULL hook for
	supports_conditional_breakpoints.
	* nto-low.c (nto_target_ops): Likewise.
	* spu-low.c (spu_target_ops): Likewise.
	* win32-low.c (win32_target_ops): Likewise.
	* server.c (handle_query): Check
	target_supports_conditional_breakpoints.
	* target.h (struct target_ops) <supports_conditional_breakpoints>:
	New field.
	(target_supports_conditional_breakpoints): New macro.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index c5312d021810c3113a8c8c36d52ac734e587d5d4..191c90e8b02a9ef8edfbbb3fc47a96f7d9945e7f 100644 (file)
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,19 @@
+2015-05-08  Yao Qi  <yao.qi@linaro.org>
+
+       * linux-low.c (linux_supports_conditional_breakpoints): New
+       function.
+       (linux_target_ops): Install new target method.
+       * lynx-low.c (lynx_target_ops): Install NULL hook for
+       supports_conditional_breakpoints.
+       * nto-low.c (nto_target_ops): Likewise.
+       * spu-low.c (spu_target_ops): Likewise.
+       * win32-low.c (win32_target_ops): Likewise.
+       * server.c (handle_query): Check
+       target_supports_conditional_breakpoints.
+       * target.h (struct target_ops) <supports_conditional_breakpoints>:
+       New field.
+       (target_supports_conditional_breakpoints): New macro.
+
 2015-05-06  Pedro Alves  <palves@redhat.com>
 
        PR server/18081

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 1c4c2d73bd341ca5cd3e0b5537b9356647b0c945..bc76ffc126661c4dba1b100bdf75b5b73c675042 100644 (file)
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5177,6 +5177,19 @@ linux_supports_stopped_by_hw_breakpoint (void)
   return USE_SIGTRAP_SIGINFO;
 }
 
+/* Implement the supports_conditional_breakpoints target_ops
+   method.  */
+
+static int
+linux_supports_conditional_breakpoints (void)
+{
+  /* GDBserver needs to step over the breakpoint if the condition is
+     false.  GDBserver software single step is too simple, so disable
+     conditional breakpoints if the target doesn't have hardware single
+     step.  */
+  return can_hardware_single_step ();
+}
+
 static int
 linux_stopped_by_watchpoint (void)
 {
@@ -6375,6 +6388,7 @@ static struct target_ops linux_target_ops = {
   linux_supports_stopped_by_sw_breakpoint,
   linux_stopped_by_hw_breakpoint,
   linux_supports_stopped_by_hw_breakpoint,
+  linux_supports_conditional_breakpoints,
   linux_stopped_by_watchpoint,
   linux_stopped_data_address,
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)        \

diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 2f858297d0bdddd4f70ee86ae7a4950f5bf14ec0..364c79f363cfd5f1b1c892fb69728f2cda1e2000 100644 (file)
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -746,6 +746,10 @@ static struct target_ops lynx_target_ops = {
   NULL,  /* supports_stopped_by_sw_breakpoint */
   NULL,  /* stopped_by_hw_breakpoint */
   NULL,  /* supports_stopped_by_hw_breakpoint */
+  /* Although lynx has hardware single step, still disable this
+     feature for lynx, because it is implemented in linux-low.c instead
+     of in generic code.  */
+  NULL,  /* supports_conditional_breakpoints */
   NULL,  /* stopped_by_watchpoint */
   NULL,  /* stopped_data_address */
   NULL,  /* read_offsets */

diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 801d76a7c4491ad84c9c2c24be1f2ac21dea5836..92767362043f5a8dfaa68074fdaa9f38842a8664 100644 (file)
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -949,6 +949,10 @@ static struct target_ops nto_target_ops = {
   NULL, /* supports_stopped_by_sw_breakpoint */
   NULL, /* stopped_by_hw_breakpoint */
   NULL, /* supports_stopped_by_hw_breakpoint */
+  /* Although nto has hardware single step, still disable this
+     feature for not, because it is implemented in linux-low.c instead
+     of in generic code.  */
+  NULL, /* supports_conditional_breakpoints */
   nto_stopped_by_watchpoint,
   nto_stopped_data_address,
   NULL, /* nto_read_offsets */

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 62287d47573f9dbcbd275f5ab50f2c6611794939..3f9bb8938d6131c5f5724e2164853f393dafe190 100644 (file)
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2107,7 +2107,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
        }
 
       /* Support target-side breakpoint conditions and commands.  */
-      strcat (own_buf, ";ConditionalBreakpoints+");
+      if (target_supports_conditional_breakpoints ())
+       strcat (own_buf, ";ConditionalBreakpoints+");
       strcat (own_buf, ";BreakpointCommands+");
 
       if (target_supports_agent ())

diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index 73f17860807b2de5b385f863d9687eec585c7f60..a56a8893c4a3179b22a73025121e4f17653e451f 100644 (file)
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -662,6 +662,7 @@ static struct target_ops spu_target_ops = {
   NULL, /* supports_stopped_by_sw_breakpoint */
   NULL, /* stopped_by_hw_breakpoint */
   NULL, /* supports_stopped_by_hw_breakpoint */
+  NULL, /* supports_conditional_breakpoints */
   NULL,
   NULL,
   NULL,

diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 6280c267377413f6ca9770f79206012ddbbd7443..b3d08cddd1a18cb14decbfed3e9cb59aec8fbd23 100644 (file)
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -222,6 +222,10 @@ struct target_ops
      HW breakpoint triggering.  */
   int (*supports_stopped_by_hw_breakpoint) (void);
 
+  /* Returns true if the target can evaluate conditions of
+     breakpoints.  */
+  int (*supports_conditional_breakpoints) (void);
+
   /* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise.  */
 
   int (*stopped_by_watchpoint) (void);
@@ -550,6 +554,10 @@ int kill_inferior (int);
   (the_target->supports_stopped_by_hw_breakpoint ? \
    (*the_target->supports_stopped_by_hw_breakpoint) () : 0)
 
+#define target_supports_conditional_breakpoints() \
+  (the_target->supports_conditional_breakpoints ? \
+   (*the_target->supports_conditional_breakpoints) () : 0)
+
 #define target_stopped_by_hw_breakpoint() \
   (the_target->stopped_by_hw_breakpoint ? \
    (*the_target->stopped_by_hw_breakpoint) () : 0)

diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 6c86765b2db03984a2ea4866df7a431caa115181..6cf56bd54d358950b0601234367238dbd1018ffb 100644 (file)
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -1811,6 +1811,10 @@ static struct target_ops win32_target_ops = {
   NULL, /* supports_stopped_by_sw_breakpoint */
   NULL, /* stopped_by_hw_breakpoint */
   NULL, /* supports_stopped_by_hw_breakpoint */
+  /* Although win32-i386 has hardware single step, still disable this
+     feature for win32, because it is implemented in linux-low.c instead
+     of in generic code.  */
+  NULL, /* supports_conditional_breakpoints */
   win32_stopped_by_watchpoint,
   win32_stopped_data_address,
   NULL, /* read_offsets */