[PATCH 0/8] Step over instruction branches to itself

[PATCH 0/8] Step over instruction branches to itself

[Yao Qi]

When I test arm linux range stepping patches, fails in
gdb.base/gdb-sigterm.exp lead me taking a look.  I find gcc is quite
smart to genreate *single* branch instruction for such endless for
loop,

  for (;;); /* loop-line */

the instruction branches to itself.

   0x00008638 <+28>:	b	0x8638 <main+28>

However, current GDB and GDBserver doesn't handle this kind of
instruction very well when stepping over breakpoint on top of it.
This patch series fixes the problems when stepping over
"branch to self" instruction, and it paves the way for arm linux range
stepping patches and tracepoint patches.

Patch 1 and 2 are refactor patch.  Patch 4 is a GDB patch, and the rest
of them are GDBserver patches.  Test case is in the last patch.  Patch 3
and patch 7 are about signal delivery when stepping over, I am not very
sure about them, so these two can be treated as RFC.

Regression tested on x86_64-linux and arm-linux.

*** BLURB HERE ***

Yao Qi (8):
  Set signal to 0 after enqueue_pending_signal
  Check LWP_SIGNAL_CAN_BE_DELIVERED for enqueue/dequeue pending signals
  Deliver signal in hardware single step
  Force to insert software single step breakpoint
  Insert breakpoint even when the raw breakpoint is found
  [GDBserver] Don't error in reinsert_raw_breakpoint if bp->inserted
  Resume the inferior with signal rather than stepping over
  New test case gdb.base/branch-to-self.exp

 gdb/breakpoint.c                          |  5 ++-
 gdb/gdbserver/linux-low.c                 | 66 +++++++++++++++++-------------
 gdb/gdbserver/mem-break.c                 | 19 ++++++++-
 gdb/testsuite/gdb.base/branch-to-self.c   | 44 ++++++++++++++++++++
 gdb/testsuite/gdb.base/branch-to-self.exp | 67 +++++++++++++++++++++++++++++++
 5 files changed, 171 insertions(+), 30 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/branch-to-self.c
 create mode 100644 gdb/testsuite/gdb.base/branch-to-self.exp

-- 
1.9.1

[PATCH 3/8] Deliver signal in hardware single step

[Yao Qi]

GDBserver doesn't deliver signal when stepping over a breakpoint even
hardware single step is used.  When GDBserver started to step over
(thread creation) breakpoint for mutlit-threaded debugging in 2002 [1],
GDBserver behaves this way.

This behaviour gets trouble on conditional breakpoints on branch to
self instruction like this,

   0x00000000004005b6 <+29>:	jmp    0x4005b6 <main+29>

and I set breakpoint

$(gdb) break branch-to-self.c:43 if counter > 3

and the variable counter will be set to 5 in SIGALRM signal handler.
Since GDBserver keeps stepping over breakpoint, the SIGALRM can never
be dequeued and delivered to the inferior, so the program can't stop.
The test can be found in gdb.base/branch-to-self.exp.

I can understand why does GDBserver queue signal for software single
step, but I can't figure out a reason we should queue signal for
hardware single step.  With this patch applied, GDBserver forward the
signal to inferior and the program can stop correctly.

[1] PATCH: Multithreaded debugging for gdbserver
    https://sourceware.org/ml/gdb-patches/2002-06/msg00157.html

gdb/gdbserver:

2016-03-04  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): Don't deliver
	signal when stepping over breakpoint with software single
	step.
---
 gdb/gdbserver/linux-low.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index dcf58db..2330e67 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4119,11 +4119,12 @@ single_step (struct lwp_info* lwp)
 }
 
 /* The signal can not be delivered to the inferior if we are trying to
-   reinsert a breakpoint or we're trying to finish a fast tracepoint
-   collect.  */
+   reinsert a breakpoint for software single step or we're trying to
+   finish a fast tracepoint collect.  */
 
-#define LWP_SIGNAL_CAN_BE_DELIVERED(LWP) \
-  !((LWP)->bp_reinsert != 0 || (LWP)->collecting_fast_tracepoint)
+#define LWP_SIGNAL_CAN_BE_DELIVERED(LWP)			\
+  !(((LWP)->bp_reinsert != 0 && can_software_single_step ())	\
+    || (LWP)->collecting_fast_tracepoint)
 
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
    SIGNAL is nonzero, give it that signal.  */
-- 
1.9.1

Re: [PATCH 3/8] Deliver signal in hardware single step

[Pedro Alves]

On 03/04/2016 10:44 AM, Yao Qi wrote:
> GDBserver doesn't deliver signal when stepping over a breakpoint even
> hardware single step is used.  When GDBserver started to step over
> (thread creation) breakpoint for mutlit-threaded debugging in 2002 [1],
> GDBserver behaves this way.
> 
> This behaviour gets trouble on conditional breakpoints on branch to
> self instruction like this,
> 
>     0x00000000004005b6 <+29>:	jmp    0x4005b6 <main+29>
> 
> and I set breakpoint
> 
> $(gdb) break branch-to-self.c:43 if counter > 3
> 
> and the variable counter will be set to 5 in SIGALRM signal handler.
> Since GDBserver keeps stepping over breakpoint, the SIGALRM can never
> be dequeued and delivered to the inferior, so the program can't stop.
> The test can be found in gdb.base/branch-to-self.exp.
> 
> I can understand why does GDBserver queue signal for software single
> step, but I can't figure out a reason we should queue signal for
> hardware single step.  With this patch applied, GDBserver forward the
> signal to inferior and the program can stop correctly.
> 
> [1] PATCH: Multithreaded debugging for gdbserver
>      https://sourceware.org/ml/gdb-patches/2002-06/msg00157.html
> 

Because the signal handler might recurse and call the same code
that had the breakpoint (or some other removed breakpoint), and thus
we'd miss a breakpoint hit in the signal handler.

GDB / infrun.c handles it here:

      if (ecs->event_thread->prev_pc == stop_pc
	  && ecs->event_thread->control.trap_expected
	  && ecs->event_thread->control.step_resume_breakpoint == NULL)
	{
	  int was_in_line;

	  /* We were just starting a new sequence, attempting to
	     single-step off of a breakpoint and expecting a SIGTRAP.
	     Instead this signal arrives.  This signal will take us out
	     of the stepping range so GDB needs to remember to, when
	     the signal handler returns, resume stepping off that
	     breakpoint.  */
	  /* To simplify things, "continue" is forced to use the same
	     code paths as single-step - set a breakpoint at the
	     signal return address and then, once hit, step off that
	     breakpoint.  */

IIRC, some of sigstep.exp, signull.exp, signest.exp exercise this.

Note that this also lets all threads run while the signal
handler runs.

Thanks,
Pedro Alves

Re: [PATCH 3/8] Deliver signal in hardware single step

[Pedro Alves]

On 03/11/2016 11:05 AM, Pedro Alves wrote:
> On 03/04/2016 10:44 AM, Yao Qi wrote:
>> GDBserver doesn't deliver signal when stepping over a breakpoint even
>> hardware single step is used.  When GDBserver started to step over
>> (thread creation) breakpoint for mutlit-threaded debugging in 2002 [1],
>> GDBserver behaves this way.
>>
>> This behaviour gets trouble on conditional breakpoints on branch to
>> self instruction like this,
>>
>>      0x00000000004005b6 <+29>:	jmp    0x4005b6 <main+29>
>>
>> and I set breakpoint
>>
>> $(gdb) break branch-to-self.c:43 if counter > 3
>>
>> and the variable counter will be set to 5 in SIGALRM signal handler.
>> Since GDBserver keeps stepping over breakpoint, the SIGALRM can never
>> be dequeued and delivered to the inferior, so the program can't stop.
>> The test can be found in gdb.base/branch-to-self.exp.
>>
>> I can understand why does GDBserver queue signal for software single
>> step, but I can't figure out a reason we should queue signal for
>> hardware single step.  With this patch applied, GDBserver forward the
>> signal to inferior and the program can stop correctly.
>>
>> [1] PATCH: Multithreaded debugging for gdbserver
>>       https://sourceware.org/ml/gdb-patches/2002-06/msg00157.html
>>
>
> Because the signal handler might recurse and call the same code
> that had the breakpoint (or some other removed breakpoint), and thus
> we'd miss a breakpoint hit in the signal handler.

Hmm, no, I got confused.  We'll stop in first instruction in the signal 
handler.  Let me go back and take a fresh look.

Thanks,
Pedro Alves

Re: [PATCH 3/8] Deliver signal in hardware single step

[Pedro Alves]

On 03/11/2016 11:09 AM, Pedro Alves wrote:
> On 03/11/2016 11:05 AM, Pedro Alves wrote:
>> On 03/04/2016 10:44 AM, Yao Qi wrote:
>>> GDBserver doesn't deliver signal when stepping over a breakpoint even
>>> hardware single step is used.  When GDBserver started to step over
>>> (thread creation) breakpoint for mutlit-threaded debugging in 2002 [1],
>>> GDBserver behaves this way.
>>>
>>> This behaviour gets trouble on conditional breakpoints on branch to
>>> self instruction like this,
>>>
>>>      0x00000000004005b6 <+29>:    jmp    0x4005b6 <main+29>
>>>
>>> and I set breakpoint
>>>
>>> $(gdb) break branch-to-self.c:43 if counter > 3
>>>
>>> and the variable counter will be set to 5 in SIGALRM signal handler.
>>> Since GDBserver keeps stepping over breakpoint, the SIGALRM can never
>>> be dequeued and delivered to the inferior, so the program can't stop.
>>> The test can be found in gdb.base/branch-to-self.exp.
>>>
>>> I can understand why does GDBserver queue signal for software single
>>> step, but I can't figure out a reason we should queue signal for
>>> hardware single step.  With this patch applied, GDBserver forward the
>>> signal to inferior and the program can stop correctly.
>>>
>>> [1] PATCH: Multithreaded debugging for gdbserver
>>>       https://sourceware.org/ml/gdb-patches/2002-06/msg00157.html
>>>
>>
>> Because the signal handler might recurse and call the same code
>> that had the breakpoint (or some other removed breakpoint), and thus
>> we'd miss a breakpoint hit in the signal handler.
> 
> Hmm, no, I got confused.  We'll stop in first instruction in the signal 
> handler.  Let me go back and take a fresh look.

OK, trying to think of scenarios that might break.  Haven't actually
tried them, I may have missed something.

- If there's no handler installed at all (SIG_IGN), then we'll
  manage to step over the breakpoint successfully and stop at the
  next instruction.  So this case is not an issue.

- If there's a signal handler installed, we'll stop at its entry,
  but we haven't stepped over the original breakpoint yet.  If we
  were already stopped at the entry of the signal handler, and it
  nested, we'll find the program stopped at the same PC it had
  started at.  But we won't know whether the step-over finished
  successfully (could be a "jmp $pc" instruction), or if instead we're
  in a new handler invocation, and thus this is a new,
  separate breakpoint hit.

  A signal handler that segfaults in the first instruction
  would be the easiest way to reproduce this that I can think of.
  (If it's crashing, you'd expect the user might try putting a
  breakpoint there.)

  Now, if after stepping into the handler, we immediately reinsert the 
  breakpoint and continue, we'll retrap it, it ends up OK.

  But if we immediately instead start a new step-over for the same 
  breakpoint address, we will miss the new hit, and nest a new handler,
  on and on.

  Not sure which happens, but I suspect the latter.

  Maybe we could detect this by comparing before / after
  stack pointer.

Thanks,
Pedro Alves

Re: [PATCH 3/8] Deliver signal in hardware single step

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> - If there's a signal handler installed, we'll stop at its entry,
>   but we haven't stepped over the original breakpoint yet.  If we
>   were already stopped at the entry of the signal handler, and it
>   nested, we'll find the program stopped at the same PC it had
>   started at.  But we won't know whether the step-over finished
>   successfully (could be a "jmp $pc" instruction), or if instead we're
>   in a new handler invocation, and thus this is a new,
>   separate breakpoint hit.

GDBserver doesn't have to tell the program stopped at the same PC
it had started at when step over is finished.  When step over, GDBserver
uninsert breakpoint, resumes the lwp, and wait the next event from the
lwp.  Once the event from the lwp arrives, GDBserver reinsert breakpoint
and thinks step over is finished.  That is all ...

>
>   A signal handler that segfaults in the first instruction
>   would be the easiest way to reproduce this that I can think of.
>   (If it's crashing, you'd expect the user might try putting a
>   breakpoint there.)

I write a case like this, set the conditional breakpoint on the first
instruction of handler, and looks breakpoint condition in target side
still works properly.

(gdb) disassemble handler
Dump of assembler code for function handler:
   0x0000000000400586 <+0>:     movb   $0x0,0x0
   0x000000000040058e <+8>:     retq

(gdb) p/x $rsp
$1 = 0x7fffffffde00
(gdb) b *handler if $rsp < 0x7fffffffde00 - 3300

the condition like this can guarantee that the condition will be true
after several recursions.

>
>   Now, if after stepping into the handler, we immediately reinsert the 
>   breakpoint and continue, we'll retrap it, it ends up OK.

breakpoint is reinserted after stepping into the handler, because
GDBserver thinks step over is finished, as I said above.

>   But if we immediately instead start a new step-over for the same 
>   breakpoint address, we will miss the new hit, and nest a new handler,
>   on and on.

This won't happen.  After hardware single step with signal delivered,
the program stops at the entry of handler, and GDBserver gets an event.
Then, GDBserver thinks step over is finished, and then proceed all lwps.
However, the thread still stopped at the breakpoint (of different
stack frames), so it needs step over again.  The loop like this will go
on until the breakpoint condition is true, need_step_over_p decides to
resume rather than step over.  The program will hit the breakpoint, and
GDBserver reports the event back to GDB.

Note in my experiment with the test case above, I don't let GDBserver
treat SIGSEGV as SIGTRAP, otherwise SIGSEGV will be reported back to GDB.

-- 
Yao (齐尧)

Re: [PATCH 3/8] Deliver signal in hardware single step

[Pedro Alves]

On 03/16/2016 10:47 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> 
>> - If there's a signal handler installed, we'll stop at its entry,
>>    but we haven't stepped over the original breakpoint yet.  If we
>>    were already stopped at the entry of the signal handler, and it
>>    nested, we'll find the program stopped at the same PC it had
>>    started at.  But we won't know whether the step-over finished
>>    successfully (could be a "jmp $pc" instruction), or if instead we're
>>    in a new handler invocation, and thus this is a new,
>>    separate breakpoint hit.
> 
> GDBserver doesn't have to tell the program stopped at the same PC
> it had started at when step over is finished.  When step over, GDBserver
> uninsert breakpoint, resumes the lwp, and wait the next event from the
> lwp.  Once the event from the lwp arrives, GDBserver reinsert breakpoint
> and thinks step over is finished.  That is all ...
> 
>>
>>    A signal handler that segfaults in the first instruction
>>    would be the easiest way to reproduce this that I can think of.
>>    (If it's crashing, you'd expect the user might try putting a
>>    breakpoint there.)
> 
> I write a case like this, set the conditional breakpoint on the first
> instruction of handler, and looks breakpoint condition in target side
> still works properly.
> 
> (gdb) disassemble handler
> Dump of assembler code for function handler:
>     0x0000000000400586 <+0>:     movb   $0x0,0x0
>     0x000000000040058e <+8>:     retq
> 
> (gdb) p/x $rsp
> $1 = 0x7fffffffde00
> (gdb) b *handler if $rsp < 0x7fffffffde00 - 3300
> 
> the condition like this can guarantee that the condition will be true
> after several recursions.
> 
>>
>>    Now, if after stepping into the handler, we immediately reinsert the
>>    breakpoint and continue, we'll retrap it, it ends up OK.
> 
> breakpoint is reinserted after stepping into the handler, because
> GDBserver thinks step over is finished, as I said above.
> 
>>    But if we immediately instead start a new step-over for the same
>>    breakpoint address, we will miss the new hit, and nest a new handler,
>>    on and on.
> 
> This won't happen.  After hardware single step with signal delivered,
> the program stops at the entry of handler, and GDBserver gets an event.
> Then, GDBserver thinks step over is finished, and then proceed all lwps.
> However, the thread still stopped at the breakpoint (of different
> stack frames), so it needs step over again. 

> The loop like this will go
> on until the breakpoint condition is true, need_step_over_p decides to
> resume rather than step over.  The program will hit the breakpoint, and
> GDBserver reports the event back to GDB.
> 
> Note in my experiment with the test case above, I don't let GDBserver
> treat SIGSEGV as SIGTRAP, otherwise SIGSEGV will be reported back to GDB.
> 

OK.

We need to think of unconditional tracepoints as well,
not just conditional breakpoints, however.

Here's a likely scenario, that I think will happen:

#1 - tracepoint set at 0xf00.
#2 - Program stops at 0xf00, tracepoint is collected.
#3 - gdbserver starts a step-over
#4 - instead, a signal arrives, step-over cancelled.
#5 - signal should be passed, not reported to gdb.
#6 - gdbserver starts a new step-over, passing signal as well.
#7 - step lands in handler. step over finished.
#8 - gdbserver proceeds/continues
#9 - signal handler returns, again to 0xf00, tracepoint
     traps again and is thus collected again. This is a
     spurious collection.

gdb avoids the spurious double-hit in #9 by remembering that
it was stepping over a breakpoint when a signal arrived, in
order to continue the step over once the handler returns
(tp->step_after_step_resume_breakpoint).  This only works if the handler 
doesn't cause some other unrelated user-visible stop -- in that case,
then the next time the user continues the program, and the
handler returns to the original breakpoint address, we'll still
report a new breakpoint hit.  I think.

Maybe we can just not bother and live with the spurious double
tracepoint hit/collect in gdbserver.  (Likewise dprintf.)
Dunno, but I'm now starting to lean toward that.

Thanks,
Pedro Alves

[PATCH 1/8] Set signal to 0 after enqueue_pending_signal

[Yao Qi]

Today, we enqueue signal in linux_resume_one_lwp_throw, but set
variable 'signal' many lines below with the comment

      /* Postpone any pending signal.  It was enqueued above.  */
      signal = 0;

I feel difficult to associate code across many lines, and we should
move the code close to enqueue_pending_signal call.  This is what
this patch does in general.  After this change, variable 'signal'
is set to zero very early, so the 'signal' value in the following
debugging message makes no sense, so I remove it from the debugging
message.  The function returns early if lwp->status_pending_p is
true, so 'signal' value in the debugging message doesn't matter,
AFAICS.  Also, I move one debugging message several lines below to
make it close the real ptrace call,

  if (debug_threads)
    debug_printf ("Resuming lwp %ld (%s, signal %d, stop %s)\n",
		  lwpid_of (thread), step ? "step" : "continue", signal,
		  lwp->stop_expected ? "expected" : "not expected");

so that the debugging message can reflect what GDBserver does.  This
is a code refactor and only debugging messages are affected.

gdb/gdbserver:

2016-03-04  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_resume_one_lwp_throw): Set 'signal' to
	0 if signal is enqueued.  Remove 'signal' from one debugging
	message.  Move one debugging message to some lines below.
	Remove code setting 'signal' to 0.
---
 gdb/gdbserver/linux-low.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index b96248c..4520a4a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4164,14 +4164,19 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 	  || lwp->pending_signals != NULL
 	  || lwp->bp_reinsert != 0
 	  || fast_tp_collecting))
-    enqueue_pending_signal (lwp, signal, info);
+    {
+      enqueue_pending_signal (lwp, signal, info);
+
+      /* Postpone any pending signal.  It was enqueued above.  */
+      signal = 0;
+    }
 
   if (lwp->status_pending_p)
     {
       if (debug_threads)
-	debug_printf ("Not resuming lwp %ld (%s, signal %d, stop %s);"
+	debug_printf ("Not resuming lwp %ld (%s, stop %s);"
 		      " has pending status\n",
-		      lwpid_of (thread), step ? "step" : "continue", signal,
+		      lwpid_of (thread), step ? "step" : "continue",
 		      lwp->stop_expected ? "expected" : "not expected");
       return;
     }
@@ -4179,11 +4184,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
   saved_thread = current_thread;
   current_thread = thread;
 
-  if (debug_threads)
-    debug_printf ("Resuming lwp %ld (%s, signal %d, stop %s)\n",
-		  lwpid_of (thread), step ? "step" : "continue", signal,
-		  lwp->stop_expected ? "expected" : "not expected");
-
   /* This bit needs some thinking about.  If we get a signal that
      we must report while a single-step reinsert is still pending,
      we often end up resuming the thread.  It might be better to
@@ -4213,9 +4213,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
 	  step = 1;
 	}
-
-      /* Postpone any pending signal.  It was enqueued above.  */
-      signal = 0;
     }
 
   if (fast_tp_collecting == 1)
@@ -4224,9 +4221,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 	debug_printf ("lwp %ld wants to get out of fast tracepoint jump pad"
 		      " (exit-jump-pad-bkpt)\n",
 		      lwpid_of (thread));
-
-      /* Postpone any pending signal.  It was enqueued above.  */
-      signal = 0;
     }
   else if (fast_tp_collecting == 2)
     {
@@ -4243,9 +4237,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 			  "moving out of jump pad single-stepping"
 			  " not implemented on this target");
 	}
-
-      /* Postpone any pending signal.  It was enqueued above.  */
-      signal = 0;
     }
 
   /* If we have while-stepping actions in this thread set it stepping.
@@ -4300,6 +4291,11 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
       *p_sig = NULL;
     }
 
+  if (debug_threads)
+    debug_printf ("Resuming lwp %ld (%s, signal %d, stop %s)\n",
+		  lwpid_of (thread), step ? "step" : "continue", signal,
+		  lwp->stop_expected ? "expected" : "not expected");
+
   if (the_low_target.prepare_to_resume != NULL)
     the_low_target.prepare_to_resume (lwp);
 
-- 
1.9.1

Re: [PATCH 1/8] Set signal to 0 after enqueue_pending_signal

[Pedro Alves]

On 03/04/2016 10:44 AM, Yao Qi wrote:
> Today, we enqueue signal in linux_resume_one_lwp_throw, but set
> variable 'signal' many lines below with the comment
>
>        /* Postpone any pending signal.  It was enqueued above.  */
>        signal = 0;
>
> I feel difficult to associate code across many lines, and we should
> move the code close to enqueue_pending_signal call.  This is what
> this patch does in general.  After this change, variable 'signal'
> is set to zero very early, so the 'signal' value in the following
> debugging message makes no sense, so I remove it from the debugging
> message.  The function returns early if lwp->status_pending_p is
> true, so 'signal' value in the debugging message doesn't matter,
> AFAICS.  Also, I move one debugging message several lines below to
> make it close the real ptrace call,
>
>    if (debug_threads)
>      debug_printf ("Resuming lwp %ld (%s, signal %d, stop %s)\n",
> 		  lwpid_of (thread), step ? "step" : "continue", signal,
> 		  lwp->stop_expected ? "expected" : "not expected");
>
> so that the debugging message can reflect what GDBserver does.  This
> is a code refactor and only debugging messages are affected.
>
> gdb/gdbserver:
>
> 2016-03-04  Yao Qi  <yao.qi@linaro.org>
>
> 	* linux-low.c (linux_resume_one_lwp_throw): Set 'signal' to
> 	0 if signal is enqueued.  Remove 'signal' from one debugging
> 	message.  Move one debugging message to some lines below.
> 	Remove code setting 'signal' to 0.

OK.

Thanks,
Pedro Alves

Re: [PATCH 1/8] Set signal to 0 after enqueue_pending_signal

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> gdb/gdbserver:
>>
>> 2016-03-04  Yao Qi  <yao.qi@linaro.org>
>>
>> 	* linux-low.c (linux_resume_one_lwp_throw): Set 'signal' to
>> 	0 if signal is enqueued.  Remove 'signal' from one debugging
>> 	message.  Move one debugging message to some lines below.
>> 	Remove code setting 'signal' to 0.
>
> OK.

Patch is pushed in.

-- 
Yao (齐尧)

[PATCH 6/8] [GDBserver] Don't error in reinsert_raw_breakpoint if bp->inserted

[Yao Qi]

GDBserver steps over a breakpoint while the single step breakpoint
is inserted at the same address, there are two breakpoint objects
using single raw breakpoint, which is inserted (for single step).
When step over is finished, GDBserver reinsert the breakpoint, but
it finds the raw breakpoint is already inserted, and error out
"Breakpoint already inserted at reinsert time."  Even if I change the
order to delete reinsert breakpoints first (which only decreases the
refcount, but leave inserted flag unchanged), the error is still
there.

The fix is to remove the error and return instead.

gdb/gdbserver:

2016-03-04  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (reinsert_raw_breakpoint): If bp->inserted is true
	return instead of error.
---
 gdb/gdbserver/mem-break.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 73c5e8a..5443379 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -1522,7 +1522,7 @@ reinsert_raw_breakpoint (struct raw_breakpoint *bp)
   int err;
 
   if (bp->inserted)
-    error ("Breakpoint already inserted at reinsert time.");
+    return;
 
   err = the_target->insert_point (bp->raw_type, bp->pc, bp->kind, bp);
   if (err == 0)
-- 
1.9.1

[PATCH 2/8] Check LWP_SIGNAL_CAN_BE_DELIVERED for enqueue/dequeue pending signals

[Yao Qi]

The enqueue and dequeue signals in linux_resume_one_lwp_throw use one
condition and its inverted one.  This patch is to move the condition
into a macro LWP_SIGNAL_CAN_BE_DELIVERED, so that the next patch can
change the condition in one place.

gdb/gdbserver:

2016-03-04  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): New macro.
	(linux_resume_one_lwp_throw): Use LWP_SIGNAL_CAN_BE_DELIVERED.
---
 gdb/gdbserver/linux-low.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 4520a4a..dcf58db 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4118,6 +4118,13 @@ single_step (struct lwp_info* lwp)
   return step;
 }
 
+/* The signal can not be delivered to the inferior if we are trying to
+   reinsert a breakpoint or we're trying to finish a fast tracepoint
+   collect.  */
+
+#define LWP_SIGNAL_CAN_BE_DELIVERED(LWP) \
+  !((LWP)->bp_reinsert != 0 || (LWP)->collecting_fast_tracepoint)
+
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
    SIGNAL is nonzero, give it that signal.  */
 
@@ -4157,13 +4164,12 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
     }
 
   /* If we have pending signals or status, and a new signal, enqueue the
-     signal.  Also enqueue the signal if we are waiting to reinsert a
-     breakpoint; it will be picked up again below.  */
+     signal.  Also enqueue the signal if it can't be delivered to the
+     inferior right now.  */
   if (signal != 0
       && (lwp->status_pending_p
 	  || lwp->pending_signals != NULL
-	  || lwp->bp_reinsert != 0
-	  || fast_tp_collecting))
+	  || !LWP_SIGNAL_CAN_BE_DELIVERED (lwp)))
     {
       enqueue_pending_signal (lwp, signal, info);
 
@@ -4269,12 +4275,9 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 	}
     }
 
-  /* If we have pending signals, consume one unless we are trying to
-     reinsert a breakpoint or we're trying to finish a fast tracepoint
-     collect.  */
-  if (lwp->pending_signals != NULL
-      && lwp->bp_reinsert == 0
-      && fast_tp_collecting == 0)
+  /* If we have pending signals, consume one if it can be delivered to
+     the inferior.  */
+  if (lwp->pending_signals != NULL && LWP_SIGNAL_CAN_BE_DELIVERED (lwp))
     {
       struct pending_signals **p_sig;
 
-- 
1.9.1

Re: [PATCH 2/8] Check LWP_SIGNAL_CAN_BE_DELIVERED for enqueue/dequeue pending signals

[Pedro Alves]

On 03/04/2016 10:44 AM, Yao Qi wrote:
> The enqueue and dequeue signals in linux_resume_one_lwp_throw use one
> condition and its inverted one.  This patch is to move the condition
> into a macro LWP_SIGNAL_CAN_BE_DELIVERED, so that the next patch can
> change the condition in one place.

I like the idea, but why a macro instead of a function?

Thanks,
Pedro Alves

Re: [PATCH 2/8] Check LWP_SIGNAL_CAN_BE_DELIVERED for enqueue/dequeue pending signals

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I like the idea, but why a macro instead of a function?

I thought the condition is simple enough to be a macro.

Here is the version to change it to function and also update comments as
Luis suggested.

-- 
Yao (齐尧)

From 6c086204b3259e576ad9d4abe687ac6f697432fa Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 3 Mar 2016 08:44:01 +0000
Subject: [PATCH] Check lwp_signal_can_be_delivered for enqueue/dequeue pending signals

The enqueue and dequeue signals in linux_resume_one_lwp_throw use one
condition and its inverted one.  This patch is to move the condition
into a function lwp_signal_can_be_delivered, so that the next patch can
change the condition in one place.

gdb/gdbserver:

2016-03-17  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (lwp_signal_can_be_delivered): New function.
	(linux_resume_one_lwp_throw): Use lwp_signal_can_be_delivered.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 4520a4a..ee8e42a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4118,6 +4118,16 @@ single_step (struct lwp_info* lwp)
   return step;
 }
 
+/* The signal can be delivered to the inferior if we are not trying to
+   reinsert a breakpoint and not trying to finish a fast tracepoint
+   collect.  */
+
+static int
+lwp_signal_can_be_delivered (struct lwp_info *lwp)
+{
+  return !(lwp->bp_reinsert != 0 || lwp->collecting_fast_tracepoint);
+}
+
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
    SIGNAL is nonzero, give it that signal.  */
 
@@ -4157,13 +4167,12 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
     }
 
   /* If we have pending signals or status, and a new signal, enqueue the
-     signal.  Also enqueue the signal if we are waiting to reinsert a
-     breakpoint; it will be picked up again below.  */
+     signal.  Also enqueue the signal if it can't be delivered to the
+     inferior right now.  */
   if (signal != 0
       && (lwp->status_pending_p
 	  || lwp->pending_signals != NULL
-	  || lwp->bp_reinsert != 0
-	  || fast_tp_collecting))
+	  || !lwp_signal_can_be_delivered (lwp)))
     {
       enqueue_pending_signal (lwp, signal, info);
 
@@ -4269,12 +4278,9 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 	}
     }
 
-  /* If we have pending signals, consume one unless we are trying to
-     reinsert a breakpoint or we're trying to finish a fast tracepoint
-     collect.  */
-  if (lwp->pending_signals != NULL
-      && lwp->bp_reinsert == 0
-      && fast_tp_collecting == 0)
+  /* If we have pending signals, consume one if it can be delivered to
+     the inferior.  */
+  if (lwp->pending_signals != NULL && lwp_signal_can_be_delivered (lwp))
     {
       struct pending_signals **p_sig;
 

Re: [PATCH 2/8] Check LWP_SIGNAL_CAN_BE_DELIVERED for enqueue/dequeue pending signals

[Pedro Alves]

On 03/17/2016 08:40 AM, Yao Qi wrote:

 >
 > Here is the version to change it to function and also update comments as
 > Luis suggested.

Thanks.


> +/* The signal can be delivered to the inferior if we are not trying to
> +   reinsert a breakpoint and not trying to finish a fast tracepoint
> +   collect.  */
> +
> +static int
> +lwp_signal_can_be_delivered (struct lwp_info *lwp)
> +{
> +  return !(lwp->bp_reinsert != 0 || lwp->collecting_fast_tracepoint);

The comment is written in terms of "and", but the implementation is in
terms of "or", negated.

If you write it like:

   return (lwp->bp_reinsert == 0 && !lwp->collecting_fast_tracepoint);

then it matches exactly what the comment says, making it easier to
reason about.

Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/8] Check LWP_SIGNAL_CAN_BE_DELIVERED for enqueue/dequeue pending signals

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> The comment is written in terms of "and", but the implementation is in
> terms of "or", negated.
>
> If you write it like:
>
>   return (lwp->bp_reinsert == 0 && !lwp->collecting_fast_tracepoint);
>
> then it matches exactly what the comment says, making it easier to
> reason about.
>
> Otherwise LGTM.

OK, patch below is what I pushed in,

-- 
Yao (齐尧)

From 35ac8b3e2dbbe1fcd107dfcc6bbc4faed6bdc63f Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 18 Mar 2016 14:34:37 +0000
Subject: [PATCH] Check lwp_signal_can_be_delivered for enqueue/dequeue pending
 signals

The enqueue and dequeue signals in linux_resume_one_lwp_throw use one
condition and its inverted one.  This patch is to move the condition
into a function lwp_signal_can_be_delivered, so that the next patch can
change the condition in one place.

gdb/gdbserver:

2016-03-18  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (lwp_signal_can_be_delivered): New function.
	(linux_resume_one_lwp_throw): Use lwp_signal_can_be_delivered.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index b210971..a63d7fb 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,10 @@
 2016-03-18  Yao Qi  <yao.qi@linaro.org>
 
+	* linux-low.c (lwp_signal_can_be_delivered): New function.
+	(linux_resume_one_lwp_throw): Use lwp_signal_can_be_delivered.
+
+2016-03-18  Yao Qi  <yao.qi@linaro.org>
+
 	* linux-low.c (linux_resume_one_lwp_throw): Set 'signal' to
 	0 if signal is enqueued.  Remove 'signal' from one debugging
 	message.  Move one debugging message to some lines below.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index f267a96..7630f9f 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4118,6 +4118,16 @@ single_step (struct lwp_info* lwp)
   return step;
 }
 
+/* The signal can be delivered to the inferior if we are not trying to
+   reinsert a breakpoint and not trying to finish a fast tracepoint
+   collect.  */
+
+static int
+lwp_signal_can_be_delivered (struct lwp_info *lwp)
+{
+  return (lwp->bp_reinsert == 0 && !lwp->collecting_fast_tracepoint);
+}
+
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
    SIGNAL is nonzero, give it that signal.  */
 
@@ -4157,13 +4167,12 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
     }
 
   /* If we have pending signals or status, and a new signal, enqueue the
-     signal.  Also enqueue the signal if we are waiting to reinsert a
-     breakpoint; it will be picked up again below.  */
+     signal.  Also enqueue the signal if it can't be delivered to the
+     inferior right now.  */
   if (signal != 0
       && (lwp->status_pending_p
 	  || lwp->pending_signals != NULL
-	  || lwp->bp_reinsert != 0
-	  || fast_tp_collecting))
+	  || !lwp_signal_can_be_delivered (lwp)))
     {
       enqueue_pending_signal (lwp, signal, info);
 
@@ -4269,12 +4278,9 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 	}
     }
 
-  /* If we have pending signals, consume one unless we are trying to
-     reinsert a breakpoint or we're trying to finish a fast tracepoint
-     collect.  */
-  if (lwp->pending_signals != NULL
-      && lwp->bp_reinsert == 0
-      && fast_tp_collecting == 0)
+  /* If we have pending signals, consume one if it can be delivered to
+     the inferior.  */
+  if (lwp->pending_signals != NULL && lwp_signal_can_be_delivered (lwp))
     {
       struct pending_signals **p_sig;
 

Re: [PATCH 2/8] Check LWP_SIGNAL_CAN_BE_DELIVERED for enqueue/dequeue pending signals

[Luis Machado]

On 03/04/2016 07:44 AM, Yao Qi wrote:
> The enqueue and dequeue signals in linux_resume_one_lwp_throw use one
> condition and its inverted one.  This patch is to move the condition
> into a macro LWP_SIGNAL_CAN_BE_DELIVERED, so that the next patch can
> change the condition in one place.
>
> gdb/gdbserver:
>
> 2016-03-04  Yao Qi  <yao.qi@linaro.org>
>
> 	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): New macro.
> 	(linux_resume_one_lwp_throw): Use LWP_SIGNAL_CAN_BE_DELIVERED.
> ---
>   gdb/gdbserver/linux-low.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 4520a4a..dcf58db 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4118,6 +4118,13 @@ single_step (struct lwp_info* lwp)
>     return step;
>   }
>
> +/* The signal can not be delivered to the inferior if we are trying to
> +   reinsert a breakpoint or we're trying to finish a fast tracepoint
> +   collect.  */
> +
> +#define LWP_SIGNAL_CAN_BE_DELIVERED(LWP) \
> +  !((LWP)->bp_reinsert != 0 || (LWP)->collecting_fast_tracepoint)
> +
>   /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
>      SIGNAL is nonzero, give it that signal.  */
>

Should we state when a signal can be delivered as opposed to describing 
when it cannot be delivered? Or keep the description and change the 
macro/function to LWP_SIGNAL_CANNOT_BE_DELIVERED(LWP)? Otherwise the 
description won't match the macro/function.

Otherwise looks fine to me.

[PATCH 7/8] Resume the inferior with signal rather than stepping over

[Yao Qi]

When GDBserver steps over a breakpoint using software single step, it
enqueues the signal, single step and deliver the signal in the next
resume if step over is not needed.  In this way, the program won't
receive the signal if the conditional breakpoint is set a branch to
self instruction, because the step over is always needed.

This patch removes the restriction that don't deliver the signal to
the inferior if we are trying to reinsert a breakpoint for software
single step and change the decision on resume vs. step-over when the
LWP has pending signals to deliver.

gdb/gdbserver:

2016-03-04  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): Adjust.
	(need_step_over_p): Return zero if the LWP has pending signals
	can be delivered on software single step target.
---
 gdb/gdbserver/linux-low.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 2330e67..9bae787 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4119,12 +4119,10 @@ single_step (struct lwp_info* lwp)
 }
 
 /* The signal can not be delivered to the inferior if we are trying to
-   reinsert a breakpoint for software single step or we're trying to
    finish a fast tracepoint collect.  */
 
 #define LWP_SIGNAL_CAN_BE_DELIVERED(LWP)			\
-  !(((LWP)->bp_reinsert != 0 && can_software_single_step ())	\
-    || (LWP)->collecting_fast_tracepoint)
+  !((LWP)->collecting_fast_tracepoint)
 
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
    SIGNAL is nonzero, give it that signal.  */
@@ -4572,6 +4570,20 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
       return 0;
     }
 
+  /* On software single step target, resume the inferior with signal
+     rather than stepping over.  */
+  if (can_software_single_step ()
+      && lwp->pending_signals != NULL
+      && LWP_SIGNAL_CAN_BE_DELIVERED (lwp))
+    {
+      if (debug_threads)
+	debug_printf ("Need step over [LWP %ld]? Ignoring, has pending"
+		      " signals.\n",
+		      lwpid_of (thread));
+
+      return 0;
+    }
+
   saved_thread = current_thread;
   current_thread = thread;
 
-- 
1.9.1

Re: [PATCH 7/8] Resume the inferior with signal rather than stepping over

[Pedro Alves]

On 03/04/2016 10:44 AM, Yao Qi wrote:
> When GDBserver steps over a breakpoint using software single step, it
> enqueues the signal, single step and deliver the signal in the next
> resume if step over is not needed.  In this way, the program won't
> receive the signal if the conditional breakpoint is set a branch to
> self instruction, because the step over is always needed.
> 
> This patch removes the restriction that don't deliver the signal to
> the inferior if we are trying to reinsert a breakpoint for software
> single step and change the decision on resume vs. step-over when the
> LWP has pending signals to deliver.

Once the handler returns, it'll retrap the same breakpoint we already stopped
for, and thus I think we'll count a spurious tracepoint/breakpoint hit.

Sounds like we'll need to do like GDB does and remember that we are advancing
past a signal handler, and need to get back to stepping over the breakpoint
if/when the handler returns successfully?

> 
> gdb/gdbserver:
> 
> 2016-03-04  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): Adjust.
> 	(need_step_over_p): Return zero if the LWP has pending signals
> 	can be delivered on software single step target.
> ---
>   gdb/gdbserver/linux-low.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 2330e67..9bae787 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4119,12 +4119,10 @@ single_step (struct lwp_info* lwp)
>   }
>   
>   /* The signal can not be delivered to the inferior if we are trying to
> -   reinsert a breakpoint for software single step or we're trying to
>      finish a fast tracepoint collect.  */
>   
>   #define LWP_SIGNAL_CAN_BE_DELIVERED(LWP)			\
> -  !(((LWP)->bp_reinsert != 0 && can_software_single_step ())	\
> -    || (LWP)->collecting_fast_tracepoint)
> +  !((LWP)->collecting_fast_tracepoint)
>   
>   /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
>      SIGNAL is nonzero, give it that signal.  */
> @@ -4572,6 +4570,20 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
>         return 0;
>       }
>   
> +  /* On software single step target, resume the inferior with signal
> +     rather than stepping over.  */
> +  if (can_software_single_step ()
> +      && lwp->pending_signals != NULL
> +      && LWP_SIGNAL_CAN_BE_DELIVERED (lwp))
> +    {
> +      if (debug_threads)
> +	debug_printf ("Need step over [LWP %ld]? Ignoring, has pending"
> +		      " signals.\n",
> +		      lwpid_of (thread));
> +
> +      return 0;

I notice this missed restoring the current thread (below).

> +    }
> +
>     saved_thread = current_thread;
>     current_thread = thread;
>   
> 


-- 
Thanks,
Pedro Alves

Re: [PATCH 7/8] Resume the inferior with signal rather than stepping over

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Once the handler returns, it'll retrap the same breakpoint we already stopped
> for, and thus I think we'll count a spurious tracepoint/breakpoint hit.
>
> Sounds like we'll need to do like GDB does and remember that we are advancing
> past a signal handler, and need to get back to stepping over the breakpoint
> if/when the handler returns successfully?

I think it is no longer an issue as we live with the spurious double
trap, right?

>>   
>> +  /* On software single step target, resume the inferior with signal
>> +     rather than stepping over.  */
>> +  if (can_software_single_step ()
>> +      && lwp->pending_signals != NULL
>> +      && LWP_SIGNAL_CAN_BE_DELIVERED (lwp))
>> +    {
>> +      if (debug_threads)
>> +	debug_printf ("Need step over [LWP %ld]? Ignoring, has pending"
>> +		      " signals.\n",
>> +		      lwpid_of (thread));
>> +
>> +      return 0;
>
> I notice this missed restoring the current thread (below).
>

The code below is to switch current_thread to thread, not restoring.
Since we don't switch current_thread here, we don't have to do anything.

>> +    }
>> +
>>     saved_thread = current_thread;
>>     current_thread = thread;

-- 
Yao (齐尧)

[PATCH 4/8] Force to insert software single step breakpoint

[Yao Qi]

GDB doesn't insert software single step breakpoint if the instruction
branches to itself, so that the program can't stop after command "si".

(gdb) b 32
Breakpoint 2 at 0x8680: file git/gdb/testsuite/gdb.base/branch-to-self.c, line 32.
(gdb) c
Continuing.

Breakpoint 2, main () at gdb/git/gdb/testsuite/gdb.base/branch-to-self.c:32
32	  asm (".Lhere: " BRANCH_INSN " .Lhere"); /* loop-line */
(gdb) si
infrun: clear_proceed_status_thread (Thread 3991.3991)
infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT)
infrun: step-over queue now empty
infrun: resuming [Thread 3991.3991] for step-over
infrun: skipping breakpoint: stepping past insn at: 0x8680
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sending packet: $Z0,8678,4#f3...Packet received: OK
infrun: skipping breakpoint: stepping past insn at: 0x8680
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sending packet: $Z0,b6fe86c8,4#82...Packet received: OK
infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [Thread 3991.3991] at 0x868

breakpoint.c:should_be_inserted thinks the breakpoint shouldn't be
inserted, which is wrong.  This patch restrict the condition that only
return false if breakpoint is NOT single step breakpoint.

gdb:

2016-03-02  Yao Qi  <yao.qi@linaro.org>

	* breakpoint.c (should_be_inserted): Don't return 0 if single
	step breakpoint is inserted at the address we're stepping over.
---
 gdb/breakpoint.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f99a7ab..72a3fdb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2219,9 +2219,12 @@ should_be_inserted (struct bp_location *bl)
     return 0;
 
   /* Don't insert a breakpoint if we're trying to step past its
-     location.  */
+     location except single step breakpoint, because the single step
+     breakpoint may be inserted at the location we're trying to step
+     if the instruction branches to itself.  */
   if ((bl->loc_type == bp_loc_software_breakpoint
        || bl->loc_type == bp_loc_hardware_breakpoint)
+      && bl->owner->type != bp_single_step
       && stepping_past_instruction_at (bl->pspace->aspace,
 				       bl->address))
     {
-- 
1.9.1

Re: [PATCH 4/8] Force to insert software single step breakpoint

[Pedro Alves]

On 03/04/2016 10:44 AM, Yao Qi wrote:
> GDB doesn't insert software single step breakpoint if the instruction
> branches to itself, so that the program can't stop after command "si".
>
> (gdb) b 32
> Breakpoint 2 at 0x8680: file git/gdb/testsuite/gdb.base/branch-to-self.c, line 32.
> (gdb) c
> Continuing.
>
> Breakpoint 2, main () at gdb/git/gdb/testsuite/gdb.base/branch-to-self.c:32
> 32	  asm (".Lhere: " BRANCH_INSN " .Lhere"); /* loop-line */
> (gdb) si
> infrun: clear_proceed_status_thread (Thread 3991.3991)
> infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT)
> infrun: step-over queue now empty
> infrun: resuming [Thread 3991.3991] for step-over
> infrun: skipping breakpoint: stepping past insn at: 0x8680
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Sending packet: $Z0,8678,4#f3...Packet received: OK
> infrun: skipping breakpoint: stepping past insn at: 0x8680
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Sending packet: $Z0,b6fe86c8,4#82...Packet received: OK
> infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [Thread 3991.3991] at 0x868
>
> breakpoint.c:should_be_inserted thinks the breakpoint shouldn't be
> inserted, which is wrong.  This patch restrict the condition that only
> return false if breakpoint is NOT single step breakpoint.

Hmm, I think we might need to do something else.

If you put a breakpoint there, then the instruction under
the breakpoint won't execute at all.

If it's a conditional branch, and the condition is false,
we will fail to ever advance past the instruction.

Similarly if the branch instruction happens to have important
side effects (flags? counters?).

Thanks,
Pedro Alves

Re: [PATCH 4/8] Force to insert software single step breakpoint

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Hmm, I think we might need to do something else.
>
> If you put a breakpoint there, then the instruction under
> the breakpoint won't execute at all.

That is intended, because if the instruction is executed, it can't be
stopped.

>
> If it's a conditional branch, and the condition is false,
> we will fail to ever advance past the instruction.
>
> Similarly if the branch instruction happens to have important
> side effects (flags? counters?).

We can switch to displaced stepping if we find the instruction may
branch to itself.  Say, we can change gdbarch software_single_step to
return a vector of dest addresses of current pc and caller inserts
software single step breakpoints to these dest addresses.  If any
element of vector equals to the current pc, switch to displaced
stepping if supported.  What do you think?

-- 
Yao (齐尧)

Re: [PATCH 4/8] Force to insert software single step breakpoint

[Pedro Alves]

On 03/16/2016 11:47 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Hmm, I think we might need to do something else.
>>
>> If you put a breakpoint there, then the instruction under
>> the breakpoint won't execute at all.
> 
> That is intended, because if the instruction is executed, it can't be
> stopped.
> 
>>
>> If it's a conditional branch, and the condition is false,
>> we will fail to ever advance past the instruction.
>>
>> Similarly if the branch instruction happens to have important
>> side effects (flags? counters?).
> 
> We can switch to displaced stepping if we find the instruction may
> branch to itself.  Say, we can change gdbarch software_single_step to
> return a vector of dest addresses of current pc and caller inserts
> software single step breakpoints to these dest addresses.  If any
> element of vector equals to the current pc, switch to displaced
> stepping if supported.  What do you think?

That's not possible on the gdbserver side, however.

Maybe what we need to do is firmly declare (and add comments in that
direction) that the arch's get_next_pcs implementation must always evaluate 
the condition of conditional branches, and not put a breakpoint at the
branch destination if the condition is false, thus ensuring forward progress.
The ARM implementation does this, though I haven't checked whether all the
branch instructions are covered.  Some other archs don't, and always put
a break at the branch destination, like e.g., moxie_software_single_step.

If we find some instruction where that is still not be sufficient,
due to side effects, then maybe gdb and gdbserver could first
try emulating the instruction's side effects manually.  And only
if that doesn't work, then try displaced stepping.  We could leave
that for later, until we find a need.

WDYT?

Thanks,
Pedro Alves

Re: [PATCH 4/8] Force to insert software single step breakpoint

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Maybe what we need to do is firmly declare (and add comments in that
> direction) that the arch's get_next_pcs implementation must always evaluate 
> the condition of conditional branches, and not put a breakpoint at the
> branch destination if the condition is false, thus ensuring forward progress.
> The ARM implementation does this, though I haven't checked whether all the
> branch instructions are covered.  Some other archs don't, and always put
> a break at the branch destination, like e.g., moxie_software_single_step.

Some targets doesn't evaluate condition and simply insert breakpoint on
possible destinations.  They are cris, moxie, sparc and spu.  I'll add
condition evaluation to these software single step implementations.

>
> If we find some instruction where that is still not be sufficient,
> due to side effects, then maybe gdb and gdbserver could first
> try emulating the instruction's side effects manually.  And only
> if that doesn't work, then try displaced stepping.  We could leave
> that for later, until we find a need.

What does "that" mean in "We could leave that for later"?  Is it
"instruction emulation + displaced stepping" or "displaced stepping"?

It is difficult to do instruction emulation for these targets, because I
need to understand the details all these targets.  Just make sure I
correctly understand the scope of the work.

-- 
Yao (齐尧)

Re: [PATCH 4/8] Force to insert software single step breakpoint

[Pedro Alves]

On 03/18/2016 02:24 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Maybe what we need to do is firmly declare (and add comments in that
>> direction) that the arch's get_next_pcs implementation must always evaluate
>> the condition of conditional branches, and not put a breakpoint at the
>> branch destination if the condition is false, thus ensuring forward progress.
>> The ARM implementation does this, though I haven't checked whether all the
>> branch instructions are covered.  Some other archs don't, and always put
>> a break at the branch destination, like e.g., moxie_software_single_step.
> 
> Some targets doesn't evaluate condition and simply insert breakpoint on
> possible destinations.  They are cris, moxie, sparc and spu.  I'll add
> condition evaluation to these software single step implementations.
> 

Hmm, that is more work than I'd imagine you'd do.  I wasn't really thinking you'd
go hack those ports.  I'd be content with just adding the comment, and leaving the
ports be, being happy that we know about a path forward if necessary.

I think before my software single-step rework, these archs may already have had
this issue, as I think old gdb would insert the software single-step breakpoint in
this scenario.   We may not see it happen in practice on those archs, as I think
usually spinlock-style asm needs at least two instructions; one to load from an
address, another to conditionally branch.

>>
>> If we find some instruction where that is still not be sufficient,
>> due to side effects, then maybe gdb and gdbserver could first
>> try emulating the instruction's side effects manually.  And only
>> if that doesn't work, then try displaced stepping.  We could leave
>> that for later, until we find a need.
> 
> What does "that" mean in "We could leave that for later"?  Is it
> "instruction emulation + displaced stepping" or "displaced stepping"?

The whole "instruction emulation + displaced stepping".

> It is difficult to do instruction emulation for these targets, because I
> need to understand the details all these targets.  Just make sure I
> correctly understand the scope of the work.

Thanks,
Pedro Alves

[PATCH 8/8] New test case gdb.base/branch-to-self.exp

[Yao Qi]

gdb/testsuite:

2016-03-02  Yao Qi  <yao.qi@linaro.org>

	* gdb.base/branch-to-self.c: New file.
	* gdb.base/branch-to-self.exp: New file.
---
 gdb/testsuite/gdb.base/branch-to-self.c   | 44 ++++++++++++++++++++
 gdb/testsuite/gdb.base/branch-to-self.exp | 67 +++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/branch-to-self.c
 create mode 100644 gdb/testsuite/gdb.base/branch-to-self.exp

diff --git a/gdb/testsuite/gdb.base/branch-to-self.c b/gdb/testsuite/gdb.base/branch-to-self.c
new file mode 100644
index 0000000..c2ec564
--- /dev/null
+++ b/gdb/testsuite/gdb.base/branch-to-self.c
@@ -0,0 +1,44 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <signal.h>
+
+#if defined(__arm__) || defined(__aarch64__)
+#define BRANCH_INSN "b"
+#elif defined(__x86_64__) || defined(__i386__)
+#define BRANCH_INSN "jmp"
+#else
+#endif
+
+volatile int counter = 0;
+
+static void
+handler (int sig)
+{
+  counter = 5;
+}
+
+int
+main (void)
+{
+  signal (SIGALRM, handler);
+  alarm (5);
+
+  asm (".Lhere: " BRANCH_INSN " .Lhere"); /* loop-line */
+}
diff --git a/gdb/testsuite/gdb.base/branch-to-self.exp b/gdb/testsuite/gdb.base/branch-to-self.exp
new file mode 100644
index 0000000..c4b5458
--- /dev/null
+++ b/gdb/testsuite/gdb.base/branch-to-self.exp
@@ -0,0 +1,67 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [build_executable ${testfile}.exp ${testfile} $srcfile {debug}] == -1 } {
+    return -1
+}
+
+with_test_prefix "single-step" {
+    global testfile
+
+    clean_restart ${testfile}
+    if ![runto_main] {
+	return -1
+    }
+    set line_num [gdb_get_line_number "loop-line" ${testfile}.c]
+
+    gdb_test "break ${testfile}.c:${line_num}"
+    gdb_continue_to_breakpoint "hit breakpoint"
+    gdb_test "si" ".*${testfile}.c:${line_num}.*"
+}
+
+with_test_prefix "break-cond" {
+    global testfile
+
+    foreach_with_prefix side {"host" "target"} {
+
+	clean_restart ${testfile}
+
+	if ![runto_main] {
+	    return -1
+	}
+
+	set test "set breakpoint condition-evaluation $side"
+	gdb_test_multiple $test $test {
+	    -re "warning: Target does not support breakpoint condition evaluation.\r\nUsing host evaluation mode instead.\r\n$gdb_prompt $" {
+		# Target doesn't support breakpoint condition
+		# evaluation on its side.  Skip the test.
+		continue
+	    }
+	    -re "^$test\r\n$gdb_prompt $" {
+	    }
+	}
+
+	set line_num [gdb_get_line_number "loop-line" ${testfile}.c]
+	gdb_test "break ${testfile}.c:${line_num} if counter > 3"
+
+	gdb_continue_to_breakpoint "continue to break"
+
+	gdb_test "p counter" ".* = 5"
+    }
+}
-- 
1.9.1

[PATCH 5/8] Insert breakpoint even when the raw breakpoint is found

[Yao Qi]

When GDBserver inserts a breakpoint, it looks for raw breakpoint, if
the raw breakpoint is found, increase its refcount, and return.  This
doesn't work when it steps over a breakpoint using software single
step and the underneath instruction of breakpoint is branch to self.

When stepping over a breakpoint on ADDR using software single step,
GDBserver uninsert the breakpoint, so the corresponding raw breakpoint
RAW's 'inserted' flag is zero.  Then, GDBserver insert single step
breakpoint at the same address ADDR because the instruction is branch
to self, the same raw brekapoint RAW is found, and increase the
refcount.  However, the raw breakpoint is not inserted, and the
program won't stop.

gdb/gdbserver:

2016-03-04  Yao Qi  <yao.qi@linaro.org>

	* mem-break.c (set_raw_breakpoint_at): Insert raw breakpoint
	when its refcount is increased.
---
 gdb/gdbserver/mem-break.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index b06f8e9..73c5e8a 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -411,7 +411,22 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int kind,
   if (bp != NULL)
     {
       bp->refcount++;
-      return bp;
+
+      if (!bp->inserted)
+	{
+	  *err = the_target->insert_point (bp->raw_type, bp->pc, bp->kind,
+					   bp);
+	  if (*err != 0)
+	    {
+	      if (debug_threads)
+		debug_printf ("Failed to insert breakpoint at 0x%s (%d).\n",
+			      paddress (where), *err);
+	      free (bp);
+	      return NULL;
+	    }
+	  bp->inserted = 1;
+	}
+	return bp;
     }
 
   bp = XCNEW (struct raw_breakpoint);
-- 
1.9.1

Re: [PATCH 5/8] Insert breakpoint even when the raw breakpoint is found

[Pedro Alves]

On 03/04/2016 10:44 AM, Yao Qi wrote:
> When GDBserver inserts a breakpoint, it looks for raw breakpoint, if
> the raw breakpoint is found, increase its refcount, and return.  This
> doesn't work when it steps over a breakpoint using software single
> step and the underneath instruction of breakpoint is branch to self.
> 
> When stepping over a breakpoint on ADDR using software single step,
> GDBserver uninsert the breakpoint, so the corresponding raw breakpoint
> RAW's 'inserted' flag is zero.  Then, GDBserver insert single step
> breakpoint at the same address ADDR because the instruction is branch
> to self, the same raw brekapoint RAW is found, and increase the
> refcount.  However, the raw breakpoint is not inserted, and the
> program won't stop.
> 
> gdb/gdbserver:
> 
> 2016-03-04  Yao Qi  <yao.qi@linaro.org>
> 
> 	* mem-break.c (set_raw_breakpoint_at): Insert raw breakpoint
> 	when its refcount is increased.
> ---
>   gdb/gdbserver/mem-break.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
> index b06f8e9..73c5e8a 100644
> --- a/gdb/gdbserver/mem-break.c
> +++ b/gdb/gdbserver/mem-break.c
> @@ -411,7 +411,22 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int kind,
>     if (bp != NULL)
>       {
>         bp->refcount++;
> -      return bp;
> +
> +      if (!bp->inserted)
> +	{
> +	  *err = the_target->insert_point (bp->raw_type, bp->pc, bp->kind,
> +					   bp);
> +	  if (*err != 0)
> +	    {
> +	      if (debug_threads)
> +		debug_printf ("Failed to insert breakpoint at 0x%s (%d).\n",
> +			      paddress (where), *err);
> +	      free (bp);

If this raw breapoint already existed, and we just bumped the
refcount, then this free leaves those other breakpoints with
a stale pointer.

But I'd like to defer this patch until we reach a conclusion on the
previous one on the gdb side.

> +	      return NULL;
> +	    }
> +	  bp->inserted = 1;
> +	}
> +	return bp;
>       }
>   
>     bp = XCNEW (struct raw_breakpoint);
> 


-- 
Thanks,
Pedro Alves