[PATCH] gdbserver gnu/linux: stepping over breakpoint

[PATCH] gdbserver gnu/linux: stepping over breakpoint

[Yao Qi]

From: Yao Qi <yao.qi@linaro.org>

Hi,
I see the following error on arm linux gdbserver,

continue^M
Continuing.^M
../../../binutils-gdb/gdb/gdbserver/linux-arm-low.c:458: A problem internal to GDBserver has been detected.^M
raw_bkpt_type_to_arm_hwbp_type: unhandled raw type^M
Remote connection closed^M
(gdb) FAIL: gdb.base/cond-eval-mode.exp: hbreak: continue

After we make GDBserver handling Zx/zx packet idempotent,

  [PATCH 3/3] [GDBserver] Make Zx/zx packet handling idempotent.
  https://sourceware.org/ml/gdb-patches/2014-04/msg00480.html

> Now removal/insertion of all kinds of breakpoints/watchpoints, either
> internal, or from GDB, always go through the target methods.

GDBserver handles all kinds of breakpoints/watchpoints through target
methods.  However, some target backends, such as arm, don't support Z0
packet but need software breakpoint to do breakpoint stepping over in
linux-low.c:start_step_over,

  if (can_hardware_single_step ())
    {
      step = 1;
    }
  else
    {
      CORE_ADDR raddr = (*the_low_target.breakpoint_reinsert_addr) ();
      set_reinsert_breakpoint (raddr);
      step = 0;
    }

a software breakpoint is requested to the backend, and the error is
triggered.  This problem should affect targets having
breakpoint_reinsert_addr hooked.

Instead of handling memory breakpoint in these affected linux backend,
this patch handles memory breakpoint in linux_{insert,remove}_point,
that, if memory breakpoint is requested, call
{insert,remove}_memory_breakpoint respectively.  Then, it becomes
unnecessary to handle memory breakpoint for linux x86 backend, so
this patch removes the code there.

This patch is tested with GDBserver on x86_64-linux and arm-linux
(-marm, -mthumb).  Testing this patch on mips, nios2, bfin, and sparc
is welcome.

Note that there are still some fails in gdb.base/cond-eval-mode.exp
with -mthumb, because GDBserver doesn't know how to select the
correct breakpoint instruction according to the arm-or-thumb-mode of
requested address.  This is a separate issue, anyway.

gdb/gdbserver:

2015-04-09  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_insert_point): Call
	insert_memory_breakpoint if TYPE is raw_bkpt_type_sw.
	(linux_remove_point): Call remove_memory_breakpoint if type is
	raw_bkpt_type_sw.
	* linux-x86-low.c (x86_insert_point): Don't call
	insert_memory_breakpoint.
	(x86_remove_point): Don't call remove_memory_breakpoint.
---
 gdb/gdbserver/linux-low.c     | 8 ++++++--
 gdb/gdbserver/linux-x86-low.c | 6 ------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e4c5420..6dd9224 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5115,7 +5115,9 @@ static int
 linux_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
 		    int size, struct raw_breakpoint *bp)
 {
-  if (the_low_target.insert_point != NULL)
+  if (type == raw_bkpt_type_sw)
+    return insert_memory_breakpoint (bp);
+  else if (the_low_target.insert_point != NULL)
     return the_low_target.insert_point (type, addr, size, bp);
   else
     /* Unsupported (see target.h).  */
@@ -5126,7 +5128,9 @@ static int
 linux_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
 		    int size, struct raw_breakpoint *bp)
 {
-  if (the_low_target.remove_point != NULL)
+  if (type == raw_bkpt_type_sw)
+    return remove_memory_breakpoint (bp);
+  else if (the_low_target.remove_point != NULL)
     return the_low_target.remove_point (type, addr, size, bp);
   else
     /* Unsupported (see target.h).  */
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index e293ba4..763df08 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -561,9 +561,6 @@ x86_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
 
   switch (type)
     {
-    case raw_bkpt_type_sw:
-      return insert_memory_breakpoint (bp);
-
     case raw_bkpt_type_hw:
     case raw_bkpt_type_write_wp:
     case raw_bkpt_type_access_wp:
@@ -590,9 +587,6 @@ x86_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
 
   switch (type)
     {
-    case raw_bkpt_type_sw:
-      return remove_memory_breakpoint (bp);
-
     case raw_bkpt_type_hw:
     case raw_bkpt_type_write_wp:
     case raw_bkpt_type_access_wp:
-- 
1.9.1

Re: [PATCH] gdbserver gnu/linux: stepping over breakpoint

[Pedro Alves]

On 04/09/2015 09:45 AM, Yao Qi wrote:

> gdb/gdbserver:
> 
> 2015-04-09  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (linux_insert_point): Call
> 	insert_memory_breakpoint if TYPE is raw_bkpt_type_sw.
> 	(linux_remove_point): Call remove_memory_breakpoint if type is
> 	raw_bkpt_type_sw.
> 	* linux-x86-low.c (x86_insert_point): Don't call
> 	insert_memory_breakpoint.
> 	(x86_remove_point): Don't call remove_memory_breakpoint.

LGTM.

Thanks,
Pedro Alves

Re: [PATCH] gdbserver gnu/linux: stepping over breakpoint

[Yao Qi]

On 09/04/15 10:04, Pedro Alves wrote:
> On 04/09/2015 09:45 AM, Yao Qi wrote:
>
>> gdb/gdbserver:
>>
>> 2015-04-09  Yao Qi  <yao.qi@linaro.org>
>>
>> 	* linux-low.c (linux_insert_point): Call
>> 	insert_memory_breakpoint if TYPE is raw_bkpt_type_sw.
>> 	(linux_remove_point): Call remove_memory_breakpoint if type is
>> 	raw_bkpt_type_sw.
>> 	* linux-x86-low.c (x86_insert_point): Don't call
>> 	insert_memory_breakpoint.
>> 	(x86_remove_point): Don't call remove_memory_breakpoint.
>
> LGTM.
>

Patch is pushed in.

-- 
Yao (齐尧)

Re: [PATCH] gdbserver gnu/linux: stepping over breakpoint

[Pedro Alves]

On 04/09/2015 09:45 AM, Yao Qi wrote:
> From: Yao Qi <yao.qi@linaro.org>
> 
> Hi,
> I see the following error on arm linux gdbserver,
> 
> continue^M
> Continuing.^M
> ../../../binutils-gdb/gdb/gdbserver/linux-arm-low.c:458: A problem internal to GDBserver has been detected.^M
> raw_bkpt_type_to_arm_hwbp_type: unhandled raw type^M
> Remote connection closed^M
> (gdb) FAIL: gdb.base/cond-eval-mode.exp: hbreak: continue
> 
> After we make GDBserver handling Zx/zx packet idempotent,
> 
>   [PATCH 3/3] [GDBserver] Make Zx/zx packet handling idempotent.
>   https://sourceware.org/ml/gdb-patches/2014-04/msg00480.html
> 
>> Now removal/insertion of all kinds of breakpoints/watchpoints, either
>> internal, or from GDB, always go through the target methods.
> 
> GDBserver handles all kinds of breakpoints/watchpoints through target
> methods.  However, some target backends, such as arm, don't support Z0
> packet but need software breakpoint to do breakpoint stepping over in
> linux-low.c:start_step_over,
> 
>   if (can_hardware_single_step ())
>     {
>       step = 1;
>     }
>   else
>     {
>       CORE_ADDR raddr = (*the_low_target.breakpoint_reinsert_addr) ();
>       set_reinsert_breakpoint (raddr);
>       step = 0;
>     }
> 
> a software breakpoint is requested to the backend, and the error is
> triggered.  This problem should affect targets having
> breakpoint_reinsert_addr hooked.

BTW, I think the patch is OK on its own right, as it would be
needed to step over thread event breakpoints on older kernels (that's
all breakpoint_reinsert_addr handles, see comment above start_step_over),
but gdbserver shouldn't be using those event breakpoints nowadays
(thread_db_use_events ends up false), so it's surprising that that's
even reached.  The test isn't even threaded.  It sounds like
gdbserver is trying to step over the breakpoint at "foo"?  Didn't
gdb itself step over it?  How come that was reached in gdbserver?
Did we mishandle the breakpoint's reference count in gdbserver?

Thanks,
Pedro Alves

Re: [PATCH] gdbserver gnu/linux: stepping over breakpoint

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> even reached.  The test isn't even threaded.  It sounds like
> gdbserver is trying to step over the breakpoint at "foo"?  Didn't
> gdb itself step over it?  How come that was reached in gdbserver?
> Did we mishandle the breakpoint's reference count in gdbserver?

Shouldn't GDBserver step over breakpoint when the target side condition
is false?

-- 
Yao (齐尧)

Re: [PATCH] gdbserver gnu/linux: stepping over breakpoint

[Pedro Alves]

On 04/09/2015 04:06 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> even reached.  The test isn't even threaded.  It sounds like
>> gdbserver is trying to step over the breakpoint at "foo"?  Didn't
>> gdb itself step over it?  How come that was reached in gdbserver?
>> Did we mishandle the breakpoint's reference count in gdbserver?
> 
> Shouldn't GDBserver step over breakpoint when the target side condition
> is false?

Oh, this is stepping past an hardware breakpoint, not software
breakpoint.  Yes, GDBserver should be stepping past such
breakpoints.  But, given GDBserver's software single-step
support is really really really really too simple:

/* 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;
}

... we should probably disable target side conditions on software
single-step gdbserver ports.  E.g., try "si" through this function:

   void
   function ()
   {
     i = 0;
     i = 0; // set cond breakpoint that evals false here
     i = 0;
   }

I'd guess the "si" over the breakpoint ends in the caller
of "function"...

Thanks,
Pedro Alves

Re: [PATCH] gdbserver gnu/linux: stepping over breakpoint

[Pedro Alves]

On 04/09/2015 04:22 PM, Pedro Alves wrote:
> On 04/09/2015 04:06 PM, Yao Qi wrote:
>> Pedro Alves <palves@redhat.com> writes:
>>
>>> even reached.  The test isn't even threaded.  It sounds like
>>> gdbserver is trying to step over the breakpoint at "foo"?  Didn't
>>> gdb itself step over it?  How come that was reached in gdbserver?
>>> Did we mishandle the breakpoint's reference count in gdbserver?
>>
>> Shouldn't GDBserver step over breakpoint when the target side condition
>> is false?
> 
> Oh, this is stepping past an hardware breakpoint, not software
> breakpoint.  Yes, GDBserver should be stepping past such
> breakpoints.  But, given GDBserver's software single-step
> support is really really really really too simple:
> 
> /* 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;
> }
> 
> ... we should probably disable target side conditions on software
> single-step gdbserver ports.  E.g., try "si" through this function:

Sorry, "si" probably works as gdb steps over the breakpoint
itself.  Try "step" or "next" instead, which kick in the range
stepping support, which then causes gdbserver to handle the
step-over itself.

> 
>    void
>    function ()
>    {
>      i = 0;
>      i = 0; // set cond breakpoint that evals false here
>      i = 0;
>    }
> 
> I'd guess the "si" over the breakpoint ends in the caller
> of "function"...

Thanks,
Pedro Alves

Re: [PATCH] gdbserver gnu/linux: stepping over breakpoint

[Pedro Alves]

On 04/09/2015 04:24 PM, Pedro Alves wrote:
> On 04/09/2015 04:22 PM, Pedro Alves wrote:
>> On 04/09/2015 04:06 PM, Yao Qi wrote:
>>> Pedro Alves <palves@redhat.com> writes:
>>>
>>>> even reached.  The test isn't even threaded.  It sounds like
>>>> gdbserver is trying to step over the breakpoint at "foo"?  Didn't
>>>> gdb itself step over it?  How come that was reached in gdbserver?
>>>> Did we mishandle the breakpoint's reference count in gdbserver?
>>>
>>> Shouldn't GDBserver step over breakpoint when the target side condition
>>> is false?
>>
>> Oh, this is stepping past an hardware breakpoint, not software
>> breakpoint.  Yes, GDBserver should be stepping past such
>> breakpoints.  But, given GDBserver's software single-step
>> support is really really really really too simple:
>>
>> /* 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;
>> }
>>
>> ... we should probably disable target side conditions on software
>> single-step gdbserver ports.  E.g., try "si" through this function:
> 
> Sorry, "si" probably works as gdb steps over the breakpoint
> itself.  Try "step" or "next" instead, which kick in the range
> stepping support, which then causes gdbserver to handle the
> step-over itself.

Hmm^2, no, range stepping couldn't possibly work correctly with
that limited software single-step support.  Duh.  We only
enable range stepping on x86, even.

So maybe that's "good enough" for stepping past conditional
breakpoints in practice.  Guess we should update the comments
to mention this.

Thanks,
Pedro Alves