[PATCH 0/7 V2] Step over instruction branches to itself

[PATCH 0/7 V2] Step over instruction branches to itself

[Yao Qi]

Here is the V2 of this series, and V1 can be found in
https://sourceware.org/ml/gdb-patches/2016-03/msg00067.html

In V2, there are some changes,

 - Add a new test case in patch 1,
 - Add more comments in patch 2 and 3, to explain some scenarios may
   happen,
 - Decrease refcount rather than free the raw breapoint in patch 4,

The original description to this patch series is:

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.

Yao Qi (7):
  New test case gdb.trace/signal.exp
  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                          |   9 +-
 gdb/gdbarch.h                             |   5 +-
 gdb/gdbarch.sh                            |   5 +-
 gdb/gdbserver/linux-low.c                 |  22 +++-
 gdb/gdbserver/mem-break.c                 |  19 +++-
 gdb/testsuite/gdb.base/branch-to-self.c   |  44 ++++++++
 gdb/testsuite/gdb.base/branch-to-self.exp |  67 +++++++++++
 gdb/testsuite/gdb.trace/signal.c          |  68 +++++++++++
 gdb/testsuite/gdb.trace/signal.exp        | 180 ++++++++++++++++++++++++++++++
 9 files changed, 411 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/branch-to-self.c
 create mode 100644 gdb/testsuite/gdb.base/branch-to-self.exp
 create mode 100644 gdb/testsuite/gdb.trace/signal.c
 create mode 100644 gdb/testsuite/gdb.trace/signal.exp

-- 
1.9.1

[PATCH 4/7] 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-23  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..af01288 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);
+	      bp->refcount--;
+	      return NULL;
+	    }
+	  bp->inserted = 1;
+	}
+	return bp;
     }
 
   bp = XCNEW (struct raw_breakpoint);
-- 
1.9.1

Re: [PATCH 4/7] Insert breakpoint even when the raw breakpoint is found

[Pedro Alves]

On 03/23/2016 04:09 PM, Yao Qi wrote:

> ---
>  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..af01288 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);
> +	      bp->refcount--;

Can we only increase the refcount if inserting succeeds?  gdbserver can
use gdb exceptions nowadays, and even though lots of current gdbserver
code isn't exception safe, it'd be nice to start considering that.

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

Thanks,
Pedro Alves

Re: [PATCH 4/7] Insert breakpoint even when the raw breakpoint is found

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Can we only increase the refcount if inserting succeeds?  gdbserver can
> use gdb exceptions nowadays, and even though lots of current gdbserver
> code isn't exception safe, it'd be nice to start considering that.

One thing I can think of is to properly decrease refcount if exception
is thrown.  Anything else?  I don't think we need to worry about the
atomicity of increment and decrement of refcount, because I don't figure
out a case the refcount is updated together in mainline code and in
exception.

-- 
Yao (齐尧)

Re: [PATCH 4/7] Insert breakpoint even when the raw breakpoint is found

[Pedro Alves]

On 04/12/2016 10:03 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Can we only increase the refcount if inserting succeeds?  gdbserver can
>> use gdb exceptions nowadays, and even though lots of current gdbserver
>> code isn't exception safe, it'd be nice to start considering that.
> 
> One thing I can think of is to properly decrease refcount if exception
> is thrown.  Anything else?

Nothing else, that's all I meant.

Actually, now that I look closer, I think we could merge the new
code with the code that handles inserting a new raw breakpoint just below.

For example:

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index b06f8e9..419db9e 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -390,6 +390,7 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int kind,
 {
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
 
   if (type == raw_bkpt_type_sw || type == raw_bkpt_type_hw)
     {
@@ -408,32 +409,39 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int kind,
   else
     bp = find_raw_breakpoint_at (where, type, kind);
 
-  if (bp != NULL)
+  if (bp == NULL)
     {
-      bp->refcount++;
-      return bp;
+      bp = XCNEW (struct raw_breakpoint);
+      bp->pc = where;
+      bp->kind = kind;
+      bp->raw_type = type;
+      make_cleanup (xfree, bp);
     }
 
-  bp = XCNEW (struct raw_breakpoint);
-  bp->pc = where;
-  bp->kind = kind;
-  bp->refcount = 1;
-  bp->raw_type = type;
-
-  *err = the_target->insert_point (bp->raw_type, bp->pc, bp->kind, bp);
-  if (*err != 0)
+  if (!bp->inserted)
     {
-      if (debug_threads)
-	debug_printf ("Failed to insert breakpoint at 0x%s (%d).\n",
-		      paddress (where), *err);
-      free (bp);
-      return NULL;
+      *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);
+
+	  do_cleanups (old_chain);
+	  return NULL;
+	}
+
+      bp->inserted = 1;
     }
 
-  bp->inserted = 1;
-  /* Link the breakpoint in.  */
-  bp->next = proc->raw_breakpoints;
-  proc->raw_breakpoints = bp;
+  discard_cleanups (old_chain);
+
+  /* Link the breakpoint in, if this is the first reference.  */
+  if (++bp->refcount == 1)
+    {
+      bp->next = proc->raw_breakpoints;
+      proc->raw_breakpoints = bp;
+    }
   return bp;
 }
 
-- 
2.5.5

Re: [PATCH 4/7] Insert breakpoint even when the raw breakpoint is found

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Nothing else, that's all I meant.
>
> Actually, now that I look closer, I think we could merge the new
> code with the code that handles inserting a new raw breakpoint just below.
>
> For example:

Looks yours is good to me.  I pushed it in.

-- 
Yao (齐尧)
From 20249ae4551ae7b2193caed73d9ce8d594f38754 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Mon, 25 Apr 2016 09:43:36 +0100
Subject: [PATCH] Insert breakpoint even when the raw breakpoint is found

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-04-25  Pedro Alves  <palves@redhat.com>
	    Yao Qi  <yao.qi@linaro.org>

	* mem-break.c (set_raw_breakpoint_at): Create a raw breakpoint
	object.  Insert it if it is not inserted yet.  Increase the
	refcount and link it into the proc's raw breakpoint list.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0b08605..5c94832 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2016-04-25  Pedro Alves  <palves@redhat.com>
+	    Yao Qi  <yao.qi@linaro.org>
+
+	* mem-break.c (set_raw_breakpoint_at): Create a raw breakpoint
+	object.  Insert it if it is not inserted yet.  Increase the
+	refcount and link it into the proc's raw breakpoint list.
+
 2016-04-25  Yao Qi  <yao.qi@linaro.org>
 
 	* breakpoint.c (should_be_inserted): Return 0 if the location's
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index b06f8e9..419db9e 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -390,6 +390,7 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int kind,
 {
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
 
   if (type == raw_bkpt_type_sw || type == raw_bkpt_type_hw)
     {
@@ -408,32 +409,39 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int kind,
   else
     bp = find_raw_breakpoint_at (where, type, kind);
 
-  if (bp != NULL)
+  if (bp == NULL)
     {
-      bp->refcount++;
-      return bp;
+      bp = XCNEW (struct raw_breakpoint);
+      bp->pc = where;
+      bp->kind = kind;
+      bp->raw_type = type;
+      make_cleanup (xfree, bp);
     }
 
-  bp = XCNEW (struct raw_breakpoint);
-  bp->pc = where;
-  bp->kind = kind;
-  bp->refcount = 1;
-  bp->raw_type = type;
-
-  *err = the_target->insert_point (bp->raw_type, bp->pc, bp->kind, bp);
-  if (*err != 0)
+  if (!bp->inserted)
     {
-      if (debug_threads)
-	debug_printf ("Failed to insert breakpoint at 0x%s (%d).\n",
-		      paddress (where), *err);
-      free (bp);
-      return NULL;
+      *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);
+
+	  do_cleanups (old_chain);
+	  return NULL;
+	}
+
+      bp->inserted = 1;
     }
 
-  bp->inserted = 1;
-  /* Link the breakpoint in.  */
-  bp->next = proc->raw_breakpoints;
-  proc->raw_breakpoints = bp;
+  discard_cleanups (old_chain);
+
+  /* Link the breakpoint in, if this is the first reference.  */
+  if (++bp->refcount == 1)
+    {
+      bp->next = proc->raw_breakpoints;
+      proc->raw_breakpoints = bp;
+    }
   return bp;
 }
 

[PATCH 6/7] 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-23  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 7e55349..6b72c91 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4118,7 +4118,6 @@ single_step (struct lwp_info* lwp)
 }
 
 /* The signal can be delivered to the inferior if we are not trying to
-   reinsert a breakpoint for software single step and not trying to
    finish a fast tracepoint collect.  Since signal can be delivered in
    the step-over, the program may go to signal handler and trap again
    after return from the signal handler.  We can live with the spurious
@@ -4127,8 +4126,7 @@ single_step (struct lwp_info* lwp)
 static int
 lwp_signal_can_be_delivered (struct lwp_info *lwp)
 {
-  return (!(lwp->bp_reinsert != 0 && can_software_single_step ())
-	  && !lwp->collecting_fast_tracepoint);
+  return !lwp->collecting_fast_tracepoint;
 }
 
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
@@ -4577,6 +4575,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 6/7] Resume the inferior with signal rather than stepping over

[Pedro Alves]

On 03/23/2016 04:09 PM, 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.
> 
> gdb/gdbserver:
> 
> 2016-03-23  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): Adjust.

It's not longer a macro.  :-)

> 	(need_step_over_p): Return zero if the LWP has pending signals
> 	can be delivered on software single step target.

OK.

Thanks,
Pedro Alves

[PATCH 2/7] 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 behavior 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.

GDBserver didn't deliver signal when stepping over a breakpoint because
a tracepoint is collected twice if GDBserver does so in the following
scenario, which can be reproduced by gdb.trace/signal.exp.

 - program stops at tracepoint, and tracepoint is collected,
 - gdbserver starts a step-over,
 - a signal arrives, step-over is canceled, and signal should be passed,
 - gdbserver starts a new step-over again, pass the signal as well,
 - program stops at the entry of signal handler, step-over finished,
 - gdbserver proceeds,
 - program returns from the signal handler, again to the tracepoint,
   and thus is collected again.

The spurious collection isn't that harmful, IMO, so it should be OK
to let GDBserver deliver signal when stepping over a breakpoint.

gdb/gdbserver:

2016-03-23  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/testsuite:

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

	* gdb.trace/signal.exp: Also pass if
	$tracepoint_hits($i) > $iterations.
---
 gdb/gdbserver/linux-low.c          | 10 +++++++---
 gdb/testsuite/gdb.trace/signal.exp | 12 ++++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 2cf192e..7e55349 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4118,13 +4118,17 @@ single_step (struct lwp_info* lwp)
 }
 
 /* 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.  */
+   reinsert a breakpoint for software single step and not trying to
+   finish a fast tracepoint collect.  Since signal can be delivered in
+   the step-over, the program may go to signal handler and trap again
+   after return from the signal handler.  We can live with the spurious
+   double traps.  */
 
 static int
 lwp_signal_can_be_delivered (struct lwp_info *lwp)
 {
-  return (lwp->bp_reinsert == 0 && !lwp->collecting_fast_tracepoint);
+  return (!(lwp->bp_reinsert != 0 && can_software_single_step ())
+	  && !lwp->collecting_fast_tracepoint);
 }
 
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
diff --git a/gdb/testsuite/gdb.trace/signal.exp b/gdb/testsuite/gdb.trace/signal.exp
index 85898c3..7eeb0f9 100644
--- a/gdb/testsuite/gdb.trace/signal.exp
+++ b/gdb/testsuite/gdb.trace/signal.exp
@@ -166,7 +166,15 @@ while { $loop } {
 
 for { set i 0 } { $i < [expr 20] } { incr i } {
     if {[info exists tracepoint_hits($i)]} {
-	gdb_assert { $tracepoint_hits($i) == $iterations } \
-	    "tracepoint $i hit $iterations times"
+
+	if { $tracepoint_hits($i) == $iterations } {
+	    pass "tracepoint $i hit $iterations times"
+	} elseif { $tracepoint_hits($i) > $iterations } {
+	    # GDBserver delivers the signal while stepping over tracepoint,
+	    # so it is possible that a tracepoint is collected twice.
+	    pass "tracepoint $i hit $iterations times: spurious collection"
+	} else {
+	    fail "tracepoint $i hit $iterations times"
+	}
     }
 }
-- 
1.9.1

Re: [PATCH 2/7] Deliver signal in hardware single step

[Pedro Alves]

On 03/23/2016 04:09 PM, Yao Qi wrote:

> +++ b/gdb/testsuite/gdb.trace/signal.exp
> @@ -166,7 +166,15 @@ while { $loop } {
>  
>  for { set i 0 } { $i < [expr 20] } { incr i } {
>      if {[info exists tracepoint_hits($i)]} {
> -	gdb_assert { $tracepoint_hits($i) == $iterations } \
> -	    "tracepoint $i hit $iterations times"
> +
> +	if { $tracepoint_hits($i) == $iterations } {
> +	    pass "tracepoint $i hit $iterations times"
> +	} elseif { $tracepoint_hits($i) > $iterations } {
> +	    # GDBserver delivers the signal while stepping over tracepoint,
> +	    # so it is possible that a tracepoint is collected twice.
> +	    pass "tracepoint $i hit $iterations times: spurious collection"

I think this will make some test runs print, e.g.:

 pass "tracepoint 2 hit 4 times: spurious collection"

while others runs print:

 pass "tracepoint 2 hit 4 times"

making test result diffing unstable.  If so, I think it should
either be written as:

   pass "tracepoint $i hit $iterations times (spurious collection)"

making use of the rule that terminating " (...)" bits don't really
count as test message, or always:

   pass "tracepoint $i hit $iterations times"

Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/7] Deliver signal in hardware single step

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> making test result diffing unstable.  If so, I think it should
> either be written as:
>
>    pass "tracepoint $i hit $iterations times (spurious collection)"
>

I prefer this.

> making use of the rule that terminating " (...)" bits don't really
> count as test message, or always:
>
>    pass "tracepoint $i hit $iterations times"
>
> Otherwise LGTM.

Patch below is pushed in.

-- 
Yao (齐尧)
From b3273b50e69dda8ac50278ab9cc57e67e4f45465 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 3 Mar 2016 08:58:52 +0000
Subject: [PATCH] Deliver signal in hardware single step

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 behavior 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.

GDBserver didn't deliver signal when stepping over a breakpoint because
a tracepoint is collected twice if GDBserver does so in the following
scenario, which can be reproduced by gdb.trace/signal.exp.

 - program stops at tracepoint, and tracepoint is collected,
 - gdbserver starts a step-over,
 - a signal arrives, step-over is canceled, and signal should be passed,
 - gdbserver starts a new step-over again, pass the signal as well,
 - program stops at the entry of signal handler, step-over finished,
 - gdbserver proceeds,
 - program returns from the signal handler, again to the tracepoint,
   and thus is collected again.

The spurious collection isn't that harmful, IMO, so it should be OK
to let GDBserver deliver signal when stepping over a breakpoint.

gdb/gdbserver:

2016-04-22  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/testsuite:

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

	* gdb.trace/signal.exp: Also pass if
	$tracepoint_hits($i) > $iterations.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 2cf192e..7e55349 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4118,13 +4118,17 @@ single_step (struct lwp_info* lwp)
 }
 
 /* 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.  */
+   reinsert a breakpoint for software single step and not trying to
+   finish a fast tracepoint collect.  Since signal can be delivered in
+   the step-over, the program may go to signal handler and trap again
+   after return from the signal handler.  We can live with the spurious
+   double traps.  */
 
 static int
 lwp_signal_can_be_delivered (struct lwp_info *lwp)
 {
-  return (lwp->bp_reinsert == 0 && !lwp->collecting_fast_tracepoint);
+  return (!(lwp->bp_reinsert != 0 && can_software_single_step ())
+	  && !lwp->collecting_fast_tracepoint);
 }
 
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
diff --git a/gdb/testsuite/gdb.trace/signal.exp b/gdb/testsuite/gdb.trace/signal.exp
index 7118a9f..48e495e 100644
--- a/gdb/testsuite/gdb.trace/signal.exp
+++ b/gdb/testsuite/gdb.trace/signal.exp
@@ -174,6 +174,14 @@ while { 1 } {
 # Step 3, check the number of collections on each tracepoint.
 
 for { set i $tpnum } { $i < [expr $tpnum + 2] } { incr i } {
-    gdb_assert { $tracepoint_hits($i) == $iterations } \
-	"tracepoint $i hit $iterations times"
+
+    if { $tracepoint_hits($i) == $iterations } {
+	pass "tracepoint $i hit $iterations times"
+    } elseif { $tracepoint_hits($i) > $iterations } {
+	# GDBserver deliver the signal while stepping over tracepoint,
+	# so it is possible that a tracepoint is collected twice.
+	pass "tracepoint $i hit $iterations times (spurious collection)"
+    } else {
+	fail "tracepoint $i hit $iterations times"
+    }
 }

[PATCH 1/7] New test case gdb.trace/signal.exp

[Yao Qi]

This is to test whether GDBserver deliver signal to the inferior while
doing the step over.  Nowadays, GDBserver doesn't deliver signal, so
there won't be spurious collection, however, if GDBserver does deliver
signal, there might be spurious collections.

gdb/testsuite:

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

	* gdb.trace/signal.c: New file.
	* gdb.trace/signal.exp: New file.
---
 gdb/testsuite/gdb.trace/signal.c   |  68 +++++++++++++++
 gdb/testsuite/gdb.trace/signal.exp | 172 +++++++++++++++++++++++++++++++++++++
 2 files changed, 240 insertions(+)
 create mode 100644 gdb/testsuite/gdb.trace/signal.c
 create mode 100644 gdb/testsuite/gdb.trace/signal.exp

diff --git a/gdb/testsuite/gdb.trace/signal.c b/gdb/testsuite/gdb.trace/signal.c
new file mode 100644
index 0000000..b2b976d
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/signal.c
@@ -0,0 +1,68 @@
+/* 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 <signal.h>
+#include <string.h>
+#include <stdio.h>
+
+static int counter = 0;
+
+static void
+handler (int sig)
+{
+  counter++;
+}
+
+static int iterations = 3;
+
+static void
+start (int pid)
+{
+  int i;
+
+  for (i = 0; i < iterations; i++)
+    {
+      kill (pid, SIGABRT);
+    }
+}
+
+static void
+end (void)
+{}
+
+int
+main (void)
+{
+  struct sigaction act;
+  int i;
+
+  memset (&act, 0, sizeof act);
+  act.sa_handler = handler;
+  act.sa_flags = SA_NODEFER;
+  sigaction (SIGABRT, &act, NULL);
+
+  for (i = 0; i < 3; i++)
+    {
+      kill (getpid (), SIGABRT);
+    }
+
+  counter = 0;
+  start (getpid ());
+
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/signal.exp b/gdb/testsuite/gdb.trace/signal.exp
new file mode 100644
index 0000000..85898c3
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/signal.exp
@@ -0,0 +1,172 @@
+#   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/>.
+
+set syscall_insn ""
+
+# Define the syscall instruction for each target.
+
+if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
+    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
+} elseif { [istarget "aarch64*-*-linux*"] || [istarget "arm*-*-linux*"] } {
+    set syscall_insn "\[ \t\](swi|svc)\[ \t\]"
+} else {
+    return -1
+}
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_test "break kill" "Breakpoint \[0-9\]* at .*"
+gdb_test "handle SIGABRT nostop noprint pass" ".*" "pass SIGABRT"
+
+gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, (.* in |__libc_|)kill \\(\\).*" \
+    "continue to kill (1st time)"
+# Hit the breakpoint on $syscall for the first time.  In this time,
+# we will let PLT resolution done, and the number single steps we will
+# do later will be reduced.
+
+gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, (.* in |__libc_|)kill \\(\\).*" \
+    "continue to kill (2nd time)"
+# Hit the breakpoint on $syscall for the second time.  In this time,
+# the address of syscall insn and next insn of syscall are recorded.
+
+gdb_test "display/i \$pc" ".*"
+
+# Single step until we see a syscall insn or we reach the
+# upper bound of loop iterations.
+set msg "find syscall insn in kill"
+set steps 0
+set max_steps 1000
+gdb_test_multiple "stepi" $msg {
+    -re ".*$syscall_insn.*$gdb_prompt $" {
+	pass $msg
+    }
+    -re "x/i .*=>.*\r\n$gdb_prompt $" {
+	incr steps
+	if {$steps == $max_steps} {
+	    fail $msg
+	} else {
+	    send_gdb "stepi\n"
+	    exp_continue
+	}
+    }
+}
+
+if {$steps == $max_steps} {
+    return
+}
+
+# Remove the display
+gdb_test_no_output "delete display 1"
+
+set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0"]
+set syscall_insn_next 0
+set test "x/2i \$pc"
+gdb_test_multiple $test $test {
+    -re "$hex .*:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" {
+	set syscall_insn_next $expect_out(1,string)
+    }
+}
+
+delete_breakpoints
+gdb_test "break start" "Breakpoint \[0-9\] at .*"
+gdb_continue_to_breakpoint "continue to start"
+
+gdb_assert { 0 == [get_integer_valueof "counter" "1"] }
+
+delete_breakpoints
+
+# Set tracepoints on syscall instruction and the next one.  It is
+# more likely to get signal on these two places when GDBserver is
+# doing step-over.
+gdb_test "trace *$syscall_insn_addr" "Tracepoint \[0-9\] at .*"
+gdb_test "trace *$syscall_insn_next" "Tracepoint \[0-9\] at .*"
+
+gdb_test "break end" "Breakpoint \[0-9\] at .*"
+
+gdb_test_no_output "tstart"
+gdb_test "continue" ".*Breakpoint.* end .*at.*$srcfile.*" \
+    "continue to end"
+gdb_test_no_output "tstop"
+
+set iterations [get_integer_valueof "iterations" "0"]
+
+gdb_assert { $iterations == [get_integer_valueof "counter" "0"] }
+
+# Record the hit times of each tracepoint in this array.
+array set tracepoint_hits { }
+
+set test "tfind 0"
+gdb_test_multiple $test $test {
+    -re "Found trace frame 0, tracepoint ($decimal).*\r\n$gdb_prompt $" {
+
+	set idx [expr $expect_out(1,string)]
+
+	if {[info exists tracepoint_hits($idx)]} {
+	    incr tracepoint_hits($idx)
+	} else {
+	    set tracepoint_hits($idx) 1
+	}
+    }
+}
+
+set loop 1
+while { $loop } {
+    set test "tfind"
+    gdb_test_multiple $test $test {
+	-re "Found trace frame $decimal, tracepoint ($decimal).*\r\n$gdb_prompt $" {
+	    set idx [expr $expect_out(1,string)]
+
+	    if {[info exists tracepoint_hits($idx)]} {
+		incr tracepoint_hits($idx)
+	    } else {
+		set tracepoint_hits($idx) 1
+	    }
+	}
+	-re "Target failed to find requested trace frame\..*\r\n$gdb_prompt $" {
+	    set loop 0
+	}
+    }
+}
+
+for { set i 0 } { $i < [expr 20] } { incr i } {
+    if {[info exists tracepoint_hits($i)]} {
+	gdb_assert { $tracepoint_hits($i) == $iterations } \
+	    "tracepoint $i hit $iterations times"
+    }
+}
-- 
1.9.1

Re: [PATCH 1/7] New test case gdb.trace/signal.exp

[Pedro Alves]

On 03/23/2016 04:09 PM, Yao Qi wrote:
> This is to test whether GDBserver deliver signal to the inferior while
> doing the step over.  Nowadays, GDBserver doesn't deliver signal, so
> there won't be spurious collection, however, if GDBserver does deliver
> signal, there might be spurious collections.

It'd be good to have an intro comment like this in the signal.exp
file itself.  Actually, it'd be good to add some general comments in
the .exp file explaining what the test is doing: both an overview,
and on the individual test parts.  From the quick skim I did, I
couldn't really figure out what is the test doing.

> 
> gdb/testsuite:
> 
> 2016-03-23  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.trace/signal.c: New file.
> 	* gdb.trace/signal.exp: New file.
> ---
>  gdb/testsuite/gdb.trace/signal.c   |  68 +++++++++++++++
>  gdb/testsuite/gdb.trace/signal.exp | 172 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 240 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.trace/signal.c
>  create mode 100644 gdb/testsuite/gdb.trace/signal.exp
> 
> diff --git a/gdb/testsuite/gdb.trace/signal.c b/gdb/testsuite/gdb.trace/signal.c
> new file mode 100644
> index 0000000..b2b976d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/signal.c
> @@ -0,0 +1,68 @@
> +/* 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 <signal.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +static int counter = 0;
> +
> +static void
> +handler (int sig)
> +{
> +  counter++;
> +}
> +
> +static int iterations = 3;
> +
> +static void
> +start (int pid)
> +{
> +  int i;
> +
> +  for (i = 0; i < iterations; i++)
> +    {
> +      kill (pid, SIGABRT);
> +    }
> +}
> +
> +static void
> +end (void)
> +{}
> +
> +int
> +main (void)
> +{
> +  struct sigaction act;
> +  int i;
> +
> +  memset (&act, 0, sizeof act);
> +  act.sa_handler = handler;
> +  act.sa_flags = SA_NODEFER;
> +  sigaction (SIGABRT, &act, NULL);
> +
> +  for (i = 0; i < 3; i++)
> +    {
> +      kill (getpid (), SIGABRT);
> +    }
> +
> +  counter = 0;
> +  start (getpid ());
> +
> +  end ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.trace/signal.exp b/gdb/testsuite/gdb.trace/signal.exp
> new file mode 100644
> index 0000000..85898c3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/signal.exp
> @@ -0,0 +1,172 @@
> +#   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/>.
> +
> +set syscall_insn ""
> +
> +# Define the syscall instruction for each target.
> +
> +if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
> +    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
> +} elseif { [istarget "aarch64*-*-linux*"] || [istarget "arm*-*-linux*"] } {
> +    set syscall_insn "\[ \t\](swi|svc)\[ \t\]"
> +} else {
> +    return -1

Call "unsupported" before returning, perhaps?

> +}
> +
> +load_lib "trace-support.exp"
> +
> +standard_testfile
> +
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
> +    untested $testfile.exp
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    fail "Can't run to main to check for trace support"
> +    return -1
> +}
> +
> +if ![gdb_target_supports_trace] {
> +    unsupported "target does not support trace"
> +    return -1
> +}
> +
> +# Start with a fresh gdb.
> +clean_restart ${testfile}
> +if ![runto_main] {
> +    fail "Can't run to main"
> +    return -1
> +}
> +
> +gdb_test "break kill" "Breakpoint \[0-9\]* at .*"
> +gdb_test "handle SIGABRT nostop noprint pass" ".*" "pass SIGABRT"
> +
> +gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, (.* in |__libc_|)kill \\(\\).*" \
> +    "continue to kill (1st time)"

Avoid using ()s at end of test message to differentiate tests:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

Suggest, e.g.:

    "continue to kill, 1st time"
    "continue to kill, 2nd time"


> +# Hit the breakpoint on $syscall for the first time.  In this time,
> +# we will let PLT resolution done, and the number single steps we will
> +# do later will be reduced.

I found this comment quite confusing, until I realized you were
talking about the previous line.

> +
> +gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, (.* in |__libc_|)kill \\(\\).*" \
> +    "continue to kill (2nd time)"
> +# Hit the breakpoint on $syscall for the second time.  In this time,
> +# the address of syscall insn and next insn of syscall are recorded.

And this too.  How about just moving the comments to just before
each of the corresponding gdb_test calls?


> +
> +gdb_test "display/i \$pc" ".*"
> +
> +# Single step until we see a syscall insn or we reach the
> +# upper bound of loop iterations.
> +set msg "find syscall insn in kill"
> +set steps 0
> +set max_steps 1000
> +gdb_test_multiple "stepi" $msg {
> +    -re ".*$syscall_insn.*$gdb_prompt $" {
> +	pass $msg
> +    }
> +    -re "x/i .*=>.*\r\n$gdb_prompt $" {
> +	incr steps
> +	if {$steps == $max_steps} {
> +	    fail $msg
> +	} else {
> +	    send_gdb "stepi\n"
> +	    exp_continue
> +	}
> +    }
> +}
> +
> +if {$steps == $max_steps} {
> +    return
> +}
> +
> +# Remove the display
> +gdb_test_no_output "delete display 1"
> +
> +set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0"]
> +set syscall_insn_next 0
> +set test "x/2i \$pc"
> +gdb_test_multiple $test $test {
> +    -re "$hex .*:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" {
> +	set syscall_insn_next $expect_out(1,string)
> +    }
> +}
> +
> +delete_breakpoints
> +gdb_test "break start" "Breakpoint \[0-9\] at .*"
> +gdb_continue_to_breakpoint "continue to start"
> +
> +gdb_assert { 0 == [get_integer_valueof "counter" "1"] }

Might be nice to give this gdb_assert a more friendly
test message.

> +
> +delete_breakpoints
> +
> +# Set tracepoints on syscall instruction and the next one.  It is
> +# more likely to get signal on these two places when GDBserver is
> +# doing step-over.
> +gdb_test "trace *$syscall_insn_addr" "Tracepoint \[0-9\] at .*"
> +gdb_test "trace *$syscall_insn_next" "Tracepoint \[0-9\] at .*"

Looks like this is putting addresses in the test messages, which
is not stable.

> +
> +gdb_test "break end" "Breakpoint \[0-9\] at .*"
> +
> +gdb_test_no_output "tstart"
> +gdb_test "continue" ".*Breakpoint.* end .*at.*$srcfile.*" \
> +    "continue to end"
> +gdb_test_no_output "tstop"
> +
> +set iterations [get_integer_valueof "iterations" "0"]
> +
> +gdb_assert { $iterations == [get_integer_valueof "counter" "0"] }

Like above.

> +
> +# Record the hit times of each tracepoint in this array.
> +array set tracepoint_hits { }
> +
> +set test "tfind 0"
> +gdb_test_multiple $test $test {
> +    -re "Found trace frame 0, tracepoint ($decimal).*\r\n$gdb_prompt $" {
> +
> +	set idx [expr $expect_out(1,string)]
> +
> +	if {[info exists tracepoint_hits($idx)]} {
> +	    incr tracepoint_hits($idx)
> +	} else {
> +	    set tracepoint_hits($idx) 1
> +	}
> +    }
> +}
> +
> +set loop 1
> +while { $loop } {
> +    set test "tfind"
> +    gdb_test_multiple $test $test {
> +	-re "Found trace frame $decimal, tracepoint ($decimal).*\r\n$gdb_prompt $" {
> +	    set idx [expr $expect_out(1,string)]
> +
> +	    if {[info exists tracepoint_hits($idx)]} {
> +		incr tracepoint_hits($idx)
> +	    } else {
> +		set tracepoint_hits($idx) 1
> +	    }
> +	}
> +	-re "Target failed to find requested trace frame\..*\r\n$gdb_prompt $" {
> +	    set loop 0
> +	}
> +    }
> +}
> +
> +for { set i 0 } { $i < [expr 20] } { incr i } {

Why "20" here?

> +    if {[info exists tracepoint_hits($i)]} {
> +	gdb_assert { $tracepoint_hits($i) == $iterations } \
> +	    "tracepoint $i hit $iterations times"
> +    }
> +}
> 

I noticed several places could use $decimal.

Thanks,
Pedro Alves

Re: [PATCH 1/7] New test case gdb.trace/signal.exp

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> It'd be good to have an intro comment like this in the signal.exp
> file itself.  Actually, it'd be good to add some general comments in
> the .exp file explaining what the test is doing: both an overview,
> and on the individual test parts.  From the quick skim I did, I
> couldn't really figure out what is the test doing.

Fixed.

>> +
>> +set syscall_insn ""
>> +
>> +# Define the syscall instruction for each target.
>> +
>> +if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
>> +    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
>> +} elseif { [istarget "aarch64*-*-linux*"] || [istarget "arm*-*-linux*"] } {
>> +    set syscall_insn "\[ \t\](swi|svc)\[ \t\]"
>> +} else {
>> +    return -1
>
> Call "unsupported" before returning, perhaps?
>

Done, and I move this block after gdb_target_supports_trace check.

>> +
>> +gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, (.* in
>> |__libc_|)kill \\(\\).*" \
>> +    "continue to kill (1st time)"
>
> Avoid using ()s at end of test message to differentiate tests:
>
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages
>
> Suggest, e.g.:
>
>     "continue to kill, 1st time"
>     "continue to kill, 2nd time"
>

I didn't know this rule.  Thanks for letting me know.  Fixed.


>> +
>> +for { set i 0 } { $i < [expr 20] } { incr i } {
>
> Why "20" here?
>

It is a leftover.  Fixed.

>> +    if {[info exists tracepoint_hits($i)]} {
>> +	gdb_assert { $tracepoint_hits($i) == $iterations } \
>> +	    "tracepoint $i hit $iterations times"
>> +    }
>> +}
>> 
>
> I noticed several places could use $decimal.

Done.

-- 
Yao (齐尧)
From 050feffa4e7335b68b27d24a32c08a0bcc6f5c84 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Wed, 23 Mar 2016 15:37:10 +0000
Subject: [PATCH] New test case gdb.trace/signal.exp

This is to test whether GDBserver deliver signal to the inferior while
doing the step over.  Nowadays, GDBserver doesn't deliver signal, so
there won't be spurious collection, however, if GDBserver does deliver
signal, there might be spurious collection.

gdb/testsuite:

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

	* gdb.trace/signal.c: New file.
	* gdb.trace/signal.exp: New file.

diff --git a/gdb/testsuite/gdb.trace/signal.c b/gdb/testsuite/gdb.trace/signal.c
new file mode 100644
index 0000000..b2b976d
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/signal.c
@@ -0,0 +1,68 @@
+/* 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 <signal.h>
+#include <string.h>
+#include <stdio.h>
+
+static int counter = 0;
+
+static void
+handler (int sig)
+{
+  counter++;
+}
+
+static int iterations = 3;
+
+static void
+start (int pid)
+{
+  int i;
+
+  for (i = 0; i < iterations; i++)
+    {
+      kill (pid, SIGABRT);
+    }
+}
+
+static void
+end (void)
+{}
+
+int
+main (void)
+{
+  struct sigaction act;
+  int i;
+
+  memset (&act, 0, sizeof act);
+  act.sa_handler = handler;
+  act.sa_flags = SA_NODEFER;
+  sigaction (SIGABRT, &act, NULL);
+
+  for (i = 0; i < 3; i++)
+    {
+      kill (getpid (), SIGABRT);
+    }
+
+  counter = 0;
+  start (getpid ());
+
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/signal.exp b/gdb/testsuite/gdb.trace/signal.exp
new file mode 100644
index 0000000..8883bfc
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/signal.exp
@@ -0,0 +1,196 @@
+#   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/>.
+
+# This is to test whether GDBserver or other remote stubs deliver signal
+# to the inferior while step over thread.  The program signal.c sends
+# signal SIGABRT to itself via kill syscall.  The test sets tracepoints
+# syscall instruction and the next one, and it is quite likely that
+# GDBserver gets the signal when it steps over thread and does the
+# collection.  If GDBserver doesn't deliver signal in thread step over,
+# one collection is got for one tracepoint hit.  Otherwise, there may
+# be two collections for one tracepoint hit, because tracepoint is
+# collected once before step over, the program goes into signal handler
+# (because signal is delivered in step over), and program goes back
+# to the tracepoint again (one more collection) after returns from
+# signal handler.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Step 1, find the syscall instruction address.
+
+set syscall_insn ""
+
+# Define the syscall instruction for each target.
+
+if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
+    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
+} elseif { [istarget "aarch64*-*-linux*"] || [istarget "arm*-*-linux*"] } {
+    set syscall_insn "\[ \t\](swi|svc)\[ \t\]"
+} else {
+    unsupported "unknown syscall instruction"
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_test "break kill" "Breakpoint $decimal at .*"
+gdb_test "handle SIGABRT nostop noprint pass" ".*" "pass SIGABRT"
+
+# Hit the breakpoint on $syscall for the first time.  In this time,
+# we will let PLT resolution done, and the number single steps we will
+# do later will be reduced.
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, (.* in |__libc_|)kill \\(\\).*" \
+    "continue to kill, 1st time"
+
+# Hit the breakpoint on $syscall for the second time.  In this time,
+# the address of syscall insn and next insn of syscall are recorded.
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, (.* in |__libc_|)kill \\(\\).*" \
+    "continue to kill, 2nd time"
+
+gdb_test "display/i \$pc" ".*"
+
+# Single step until we see a syscall insn or we reach the
+# upper bound of loop iterations.
+set msg "find syscall insn in kill"
+set steps 0
+set max_steps 1000
+gdb_test_multiple "stepi" $msg {
+    -re ".*$syscall_insn.*$gdb_prompt $" {
+	pass $msg
+    }
+    -re "x/i .*=>.*\r\n$gdb_prompt $" {
+	incr steps
+	if {$steps == $max_steps} {
+	    fail $msg
+	} else {
+	    send_gdb "stepi\n"
+	    exp_continue
+	}
+    }
+}
+
+if {$steps == $max_steps} {
+    return
+}
+
+# Remove the display
+gdb_test_no_output "delete display 1"
+
+set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0"]
+set syscall_insn_next 0
+set test "x/2i \$pc"
+gdb_test_multiple $test $test {
+    -re "$hex .*:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" {
+	set syscall_insn_next $expect_out(1,string)
+    }
+}
+
+delete_breakpoints
+gdb_test "break start" "Breakpoint $decimal at .*"
+gdb_continue_to_breakpoint "continue to start"
+
+gdb_assert { 0 == [get_integer_valueof "counter" "1"] } "counter is zero"
+
+delete_breakpoints
+
+# Step 2, set tracepoints on syscall instruction and the next one.
+# It is more likely to get signal on these two places when GDBserver
+# is doing step-over.
+gdb_test "trace *$syscall_insn_addr" "Tracepoint $decimal at .*" \
+    "tracepoint on syscall instruction"
+set tpnum [get_integer_valueof "\$bpnum" 0]
+gdb_test "trace *$syscall_insn_next" "Tracepoint $decimal at .*" \
+    "tracepoint on instruction following syscall instruction"
+
+gdb_test "break end" "Breakpoint $decimal at .*"
+
+gdb_test_no_output "tstart"
+gdb_test "continue" ".*Breakpoint.* end .*at.*$srcfile.*" \
+    "continue to end"
+gdb_test_no_output "tstop"
+
+set iterations [get_integer_valueof "iterations" "0"]
+
+gdb_assert { $iterations == [get_integer_valueof "counter" "0"] } \
+    "iterations equals to counter"
+
+# Record the hit times of each tracepoint in this array.
+array set tracepoint_hits { }
+
+set test "tfind 0"
+gdb_test_multiple $test $test {
+    -re "Found trace frame 0, tracepoint ($decimal).*\r\n$gdb_prompt $" {
+
+	set idx [expr $expect_out(1,string)]
+
+	if {[info exists tracepoint_hits($idx)]} {
+	    incr tracepoint_hits($idx)
+	} else {
+	    set tracepoint_hits($idx) 1
+	}
+    }
+}
+
+set loop 1
+while { $loop } {
+    set test "tfind"
+    gdb_test_multiple $test $test {
+	-re "Found trace frame $decimal, tracepoint ($decimal).*\r\n$gdb_prompt $" {
+	    set idx [expr $expect_out(1,string)]
+
+	    if {[info exists tracepoint_hits($idx)]} {
+		incr tracepoint_hits($idx)
+	    } else {
+		set tracepoint_hits($idx) 1
+	    }
+	}
+	-re "Target failed to find requested trace frame\..*\r\n$gdb_prompt $" {
+	    set loop 0
+	}
+    }
+}
+
+# Step 3, check the number of collections on each tracepoint.
+
+for { set i $tpnum } { $i < [expr $tpnum + 2] } { incr i } {
+    if {[info exists tracepoint_hits($i)]} {
+	gdb_assert { $tracepoint_hits($i) == $iterations } \
+	    "tracepoint $i hit $iterations times"
+    } else {
+	fail "can't find tracepoint $i hit"
+    }
+}

Re: [PATCH 1/7] New test case gdb.trace/signal.exp

[Pedro Alves]

On 04/11/2016 09:40 AM, Yao Qi wrote:



> +# Record the hit times of each tracepoint in this array.
> +array set tracepoint_hits { }
> +
> +set test "tfind 0"
> +gdb_test_multiple $test $test {

Why do we need this separate "tfind 0" step?  I'd think the
"tfind" loop below would be sufficient?

> +    -re "Found trace frame 0, tracepoint ($decimal).*\r\n$gdb_prompt $" {
> +
> +	set idx [expr $expect_out(1,string)]
> +
> +	if {[info exists tracepoint_hits($idx)]} {
> +	    incr tracepoint_hits($idx)
> +	} else {
> +	    set tracepoint_hits($idx) 1
> +	}
> +    }
> +}
> +
> +set loop 1
> +while { $loop } {
> +    set test "tfind"
> +    gdb_test_multiple $test $test {
> +	-re "Found trace frame $decimal, tracepoint ($decimal).*\r\n$gdb_prompt $" {
> +	    set idx [expr $expect_out(1,string)]
> +
> +	    if {[info exists tracepoint_hits($idx)]} {
> +		incr tracepoint_hits($idx)
> +	    } else {
> +		set tracepoint_hits($idx) 1
> +	    }
> +	}
> +	-re "Target failed to find requested trace frame\..*\r\n$gdb_prompt $" {
> +	    set loop 0
> +	}
> +    }

If this gdb_test_multiple FAILs or times out, the loop
will continue, over and over, forever.  So we need to reverse
the logic -- assume no looping, unless the "Found trace ..." regex matched.

I'd also suggest to preinitialize the array elements to 0, avoiding
the "info exists" calls in "Step 3", here:

> +# Step 3, check the number of collections on each tracepoint.
> +
> +for { set i $tpnum } { $i < [expr $tpnum + 2] } { incr i } {
> +    if {[info exists tracepoint_hits($i)]} {
> +	gdb_assert { $tracepoint_hits($i) == $iterations } \
> +	    "tracepoint $i hit $iterations times"
> +    } else {
> +	fail "can't find tracepoint $i hit"

Also, here I think it's nicer if PASS/FAIL messages are the same,
for test result diffing.

Thus, something like:

for { set i $tpnum } { $i < [expr $tpnum + 2] } { incr i } {
  set tracepoint_hits($idx) 0
}

while { 1 } {
   set test "tfind"
   set idx 0
   gdb_test_multiple $test $test {
	-re "Found trace frame $decimal, tracepoint ($decimal).*\r\n$gdb_prompt $" {
	    set idx [expr $expect_out(1,string)]
	    incr tracepoint_hits($idx)
	}
   }
   if {$idx == 0} {
	break
   }
}

# Step 3, check the number of collections on each tracepoint.
for { set i $tpnum } { $i < [expr $tpnum + 2] } { incr i } {
   gdb_assert { $tracepoint_hits($i) == $iterations } \
      "tracepoint $i hit $iterations times"
}


Otherwise looks good to me.

Thanks,
Pedro Alves

Re: [PATCH 1/7] New test case gdb.trace/signal.exp

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> +# Record the hit times of each tracepoint in this array.
>> +array set tracepoint_hits { }
>> +
>> +set test "tfind 0"
>> +gdb_test_multiple $test $test {
>
> Why do we need this separate "tfind 0" step?  I'd think the
> "tfind" loop below would be sufficient?

I don't know the first "tfind" can find the first snapshot , and I think
"tfind 0" should be the first one in all other "tfind X" commands.

The usage like this is not documented.  In current doc:

tfind
   
    No argument means find the next trace snapshot.

We probably need to change the doc to

    No argument means find the next trace snapshot or find the first
    one if no trace snapshot is selected.

> Otherwise looks good to me.

Patch below is what I'll push in.

-- 
Yao (齐尧)

From 8f78bf828e7df83f1fde7154bbfcd718a90bef36 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Wed, 23 Mar 2016 15:37:10 +0000
Subject: [PATCH] New test case gdb.trace/signal.exp

This is to test whether GDBserver deliver signal to the inferior while
doing the step over.  Nowadays, GDBserver doesn't deliver signal, so
there won't be spurious collection, however, if GDBserver does deliver
signal, there might be spurious collection.

gdb/testsuite:

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

	* gdb.trace/signal.c: New file.
	* gdb.trace/signal.exp: New file.

diff --git a/gdb/testsuite/gdb.trace/signal.c b/gdb/testsuite/gdb.trace/signal.c
new file mode 100644
index 0000000..4577a7b
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/signal.c
@@ -0,0 +1,68 @@
+/* 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 <signal.h>
+#include <string.h>
+#include <unistd.h>
+
+static int counter = 0;
+
+static void
+handler (int sig)
+{
+  counter++;
+}
+
+static int iterations = 3;
+
+static void
+start (int pid)
+{
+  int i;
+
+  for (i = 0; i < iterations; i++)
+    {
+      kill (pid, SIGABRT);
+    }
+}
+
+static void
+end (void)
+{}
+
+int
+main (void)
+{
+  struct sigaction act;
+  int i;
+
+  memset (&act, 0, sizeof act);
+  act.sa_handler = handler;
+  act.sa_flags = SA_NODEFER;
+  sigaction (SIGABRT, &act, NULL);
+
+  for (i = 0; i < 3; i++)
+    {
+      kill (getpid (), SIGABRT);
+    }
+
+  counter = 0;
+  start (getpid ());
+
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/signal.exp b/gdb/testsuite/gdb.trace/signal.exp
new file mode 100644
index 0000000..7118a9f
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/signal.exp
@@ -0,0 +1,179 @@
+#   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/>.
+
+# This is to test whether GDBserver or other remote stubs deliver signal
+# to the inferior while step over thread.  The program signal.c sends
+# signal SIGABRT to itself via kill syscall.  The test sets tracepoints
+# syscall instruction and the next one, and it is quite likely that
+# GDBserver gets the signal when it steps over thread and does the
+# collection.  If GDBserver doesn't deliver signal in thread step over,
+# one collection is got for one tracepoint hit.  Otherwise, there may
+# be two collections for one tracepoint hit, because tracepoint is
+# collected once before step over, the program goes into signal handler
+# (because signal is delivered in step over), and program goes back
+# to the tracepoint again (one more collection) after returns from
+# signal handler.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Step 1, find the syscall instruction address.
+
+set syscall_insn ""
+
+# Define the syscall instruction for each target.
+
+if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
+    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
+} elseif { [istarget "aarch64*-*-linux*"] || [istarget "arm*-*-linux*"] } {
+    set syscall_insn "\[ \t\](swi|svc)\[ \t\]"
+} else {
+    unsupported "unknown syscall instruction"
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_test "break kill" "Breakpoint $decimal at .*"
+gdb_test "handle SIGABRT nostop noprint pass" ".*" "pass SIGABRT"
+
+# Hit the breakpoint on $syscall for the first time.  In this time,
+# we will let PLT resolution done, and the number single steps we will
+# do later will be reduced.
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, (.* in |__libc_|)kill \\(\\).*" \
+    "continue to kill, 1st time"
+
+# Hit the breakpoint on $syscall for the second time.  In this time,
+# the address of syscall insn and next insn of syscall are recorded.
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, (.* in |__libc_|)kill \\(\\).*" \
+    "continue to kill, 2nd time"
+
+gdb_test "display/i \$pc" ".*"
+
+# Single step until we see a syscall insn or we reach the
+# upper bound of loop iterations.
+set msg "find syscall insn in kill"
+set steps 0
+set max_steps 1000
+gdb_test_multiple "stepi" $msg {
+    -re ".*$syscall_insn.*$gdb_prompt $" {
+	pass $msg
+    }
+    -re "x/i .*=>.*\r\n$gdb_prompt $" {
+	incr steps
+	if {$steps == $max_steps} {
+	    fail $msg
+	} else {
+	    send_gdb "stepi\n"
+	    exp_continue
+	}
+    }
+}
+
+if {$steps == $max_steps} {
+    return
+}
+
+# Remove the display
+gdb_test_no_output "delete display 1"
+
+set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0"]
+set syscall_insn_next 0
+set test "x/2i \$pc"
+gdb_test_multiple $test $test {
+    -re "$hex .*:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" {
+	set syscall_insn_next $expect_out(1,string)
+    }
+}
+
+delete_breakpoints
+gdb_test "break start" "Breakpoint $decimal at .*"
+gdb_continue_to_breakpoint "continue to start"
+
+gdb_assert { 0 == [get_integer_valueof "counter" "1"] } "counter is zero"
+
+delete_breakpoints
+
+# Step 2, set tracepoints on syscall instruction and the next one.
+# It is more likely to get signal on these two places when GDBserver
+# is doing step-over.
+gdb_test "trace *$syscall_insn_addr" "Tracepoint $decimal at .*" \
+    "tracepoint on syscall instruction"
+set tpnum [get_integer_valueof "\$bpnum" 0]
+gdb_test "trace *$syscall_insn_next" "Tracepoint $decimal at .*" \
+    "tracepoint on instruction following syscall instruction"
+
+gdb_test "break end" "Breakpoint $decimal at .*"
+
+gdb_test_no_output "tstart"
+gdb_test "continue" ".*Breakpoint.* end .*at.*$srcfile.*" \
+    "continue to end"
+gdb_test_no_output "tstop"
+
+set iterations [get_integer_valueof "iterations" "0"]
+
+gdb_assert { $iterations == [get_integer_valueof "counter" "0"] } \
+    "iterations equals to counter"
+
+# Record the hit times of each tracepoint in this array.
+array set tracepoint_hits { }
+for { set i $tpnum } { $i < [expr $tpnum + 2] } { incr i } {
+  set tracepoint_hits($i) 0
+}
+
+while { 1 } {
+   set test "tfind"
+   set idx 0
+   gdb_test_multiple $test $test {
+	-re "Found trace frame $decimal, tracepoint ($decimal).*\r\n$gdb_prompt $" {
+	    set idx [expr $expect_out(1,string)]
+	    incr tracepoint_hits($idx)
+	}
+       -re "Target failed to find requested trace frame\..*\r\n$gdb_prompt $" {
+	   set idx 0
+       }
+   }
+   if {$idx == 0} {
+	break
+   }
+}
+
+# Step 3, check the number of collections on each tracepoint.
+
+for { set i $tpnum } { $i < [expr $tpnum + 2] } { incr i } {
+    gdb_assert { $tracepoint_hits($i) == $iterations } \
+	"tracepoint $i hit $iterations times"
+}

Re: [PATCH 1/7] New test case gdb.trace/signal.exp

[Pedro Alves]

On 04/22/2016 11:52 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> +# Record the hit times of each tracepoint in this array.
>>> +array set tracepoint_hits { }
>>> +
>>> +set test "tfind 0"
>>> +gdb_test_multiple $test $test {
>>
>> Why do we need this separate "tfind 0" step?  I'd think the
>> "tfind" loop below would be sufficient?
> 
> I don't know the first "tfind" can find the first snapshot , and I think
> "tfind 0" should be the first one in all other "tfind X" commands.
> 
> The usage like this is not documented.

I didn't know it wasn't documented.  I think the testsuite
uses it extensively.

>  In current doc:
> 
> tfind
>    
>     No argument means find the next trace snapshot.
> 
> We probably need to change the doc to
> 
>     No argument means find the next trace snapshot or find the first
>     one if no trace snapshot is selected.

Yeah.

Thanks,
Pedro Alves

Re: [PATCH 1/7] New test case gdb.trace/signal.exp

[Pedro Alves]

BTW,

On 04/11/2016 09:40 AM, Yao Qi wrote:
> +#include <signal.h>
> +#include <string.h>
> +#include <stdio.h>
> +

> +      kill (getpid (), SIGABRT);

getpid needs unistd.h, and stdio.h doesn't look like needed.

Thanks,
Pedro Alves

[PATCH 5/7] [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-23  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 af01288..a85128e 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

Re: [PATCH 5/7] [GDBserver] Don't error in reinsert_raw_breakpoint if bp->inserted

[Pedro Alves]

OK.

Thanks,
Pedro Alves

[PATCH 3/7] 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-23  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.
	* gdbarch.sh (software_single_step): Update comments.
	* gdbarch.h: Regenerated.
---
 gdb/breakpoint.c | 9 ++++++++-
 gdb/gdbarch.h    | 5 ++++-
 gdb/gdbarch.sh   | 5 ++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f99a7ab..9ecfb07 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2219,9 +2219,16 @@ 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.  However, the instruction
+     won't be executed at all and it may break the semantics of the
+     instruction, for example, the instruction is a conditional
+     branch or updates some flags.  We can't fix it unless GDB is able
+     to emulate the instruction or switch to displaced stepping.  */
   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))
     {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 252fc4b..ba3d4ca 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -650,7 +650,10 @@ extern void set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch, gdbarch_addr_
    target can single step.  If not, then implement single step using breakpoints.
   
    A return value of 1 means that the software_single_step breakpoints
-   were inserted; 0 means they were not. */
+   were inserted; 0 means they were not.  Multiple breakpoints may be
+   inserted for some instructions such as conditional branch.  However,
+   each implementation must always evaluate the condition and only put
+   the breakpoint at the branch destination if the condition is true. */
 
 extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch);
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 37f59b7..8192370 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -609,7 +609,10 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core_addr_identity::0
 # target can single step.  If not, then implement single step using breakpoints.
 #
 # A return value of 1 means that the software_single_step breakpoints
-# were inserted; 0 means they were not.
+# were inserted; 0 means they were not.  Multiple breakpoints may be
+# inserted for some instructions such as conditional branch.  However,
+# each implementation must always evaluate the condition and only put
+# the breakpoint at the branch destination if the condition is true.
 F:int:software_single_step:struct frame_info *frame:frame
 
 # Return non-zero if the processor is executing a delay slot and a
-- 
1.9.1

Re: [PATCH 3/7] Force to insert software single step breakpoint

[Pedro Alves]

On 03/23/2016 04:09 PM, 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.
> 
> gdb:
> 
> 2016-03-23  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.
> 	* gdbarch.sh (software_single_step): Update comments.
> 	* gdbarch.h: Regenerated.
> ---
>  gdb/breakpoint.c | 9 ++++++++-
>  gdb/gdbarch.h    | 5 ++++-
>  gdb/gdbarch.sh   | 5 ++++-
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f99a7ab..9ecfb07 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2219,9 +2219,16 @@ 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.  However, the instruction
> +     won't be executed at all and it may break the semantics of the
> +     instruction, for example, the instruction is a conditional
> +     branch or updates some flags.  We can't fix it unless GDB is able
> +     to emulate the instruction or switch to displaced stepping.  */
>    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))

Another scenario occurred to me:

 - Thread A is software single-stepping.
 - Thread B hits single-step breakpoint of thread A.
 - We pause all threads and set thread B stepping past the
   single-step breakpoint of thread A.

But if the single-step breakpoint is for another thread, then
we won't actually manage to have thread B step past it, resulting
in spurious re-hits and no-guaranteed forward progress.  See
e.g., non-stop-fair-events.exp:

        # On software single-step targets that don't support displaced
        # stepping, threads keep hitting each others' single-step
        # breakpoints, and then GDB needs to pause all threads to step
        # past those.  The end result is that progress in the main
        # thread will be slower and it may take a bit longer for the
        # signal to be queued; bump the timeout.

Sounds like we may need to look at the single-step breakpoint's thread 
id, and only insert it if it is for the thread that is going to be 
doing the step-over?  We may need to record that in step_over_info and 
pass more info to stepping_past_instruction_at.

> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -609,7 +609,10 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core_addr_identity::0
>  # target can single step.  If not, then implement single step using breakpoints.
>  #
>  # A return value of 1 means that the software_single_step breakpoints
> -# were inserted; 0 means they were not.
> +# were inserted; 0 means they were not.  Multiple breakpoints may be
> +# inserted for some instructions such as conditional branch.  However,
> +# each implementation must always evaluate the condition and only put
> +# the breakpoint at the branch destination if the condition is true.

I'd add:

(...) condition is true, so that we ensure forward progress when 
stepping past a conditional branch to self.

This will help porters evaluate whether that's really necessary
for their ports.

Thanks,
Pedro Alves

Re: [PATCH 3/7] Force to insert software single step breakpoint

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Another scenario occurred to me:
>
>  - Thread A is software single-stepping.
>  - Thread B hits single-step breakpoint of thread A.
>  - We pause all threads and set thread B stepping past the
>    single-step breakpoint of thread A.
>
> But if the single-step breakpoint is for another thread, then
> we won't actually manage to have thread B step past it, resulting
> in spurious re-hits and no-guaranteed forward progress.  See
> e.g., non-stop-fair-events.exp:
>
>         # On software single-step targets that don't support displaced
>         # stepping, threads keep hitting each others' single-step
>         # breakpoints, and then GDB needs to pause all threads to step
>         # past those.  The end result is that progress in the main
>         # thread will be slower and it may take a bit longer for the
>         # signal to be queued; bump the timeout.

I finally managed to reproduce that thread id in step_over_info is
different from the thread id of the single-step breakpoint.

GDB now gives the high priority to finishing step over, to avoid
"threads keep hitting each others' single-step breakpoint".  With my
patch applied, single-step breakpoint (of threads other than we are
stepping over) is still inserted even we try to step past the location,
so the step-over can't be finished.

>
> Sounds like we may need to look at the single-step breakpoint's thread 
> id, and only insert it if it is for the thread that is going to be 
> doing the step-over?  We may need to record that in step_over_info and 
> pass more info to stepping_past_instruction_at.

Yes, after I added a new field 'struct thread_info *thread' in
'struct step_over_info', I realize that IWBN to convert 'step_over_info'
to 'the thread we are stepping over', so the fields 'aspace' and 'address'
can be replaced by 'thread', like this,

struct step_over_info
{
  struct thread_info *thread;
  int nonsteppable_watchpoint_p;
};

Is it a good idea?  If there is nothing obviously wrong, I'll post
patches to do this.

-- 
Yao (齐尧)

Re: [PATCH 3/7] Force to insert software single step breakpoint

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Another scenario occurred to me:
>
>  - Thread A is software single-stepping.
>  - Thread B hits single-step breakpoint of thread A.
>  - We pause all threads and set thread B stepping past the
>    single-step breakpoint of thread A.
>
> But if the single-step breakpoint is for another thread, then
> we won't actually manage to have thread B step past it, resulting
> in spurious re-hits and no-guaranteed forward progress.  See
> e.g., non-stop-fair-events.exp:
>
>         # On software single-step targets that don't support displaced
>         # stepping, threads keep hitting each others' single-step
>         # breakpoints, and then GDB needs to pause all threads to step
>         # past those.  The end result is that progress in the main
>         # thread will be slower and it may take a bit longer for the
>         # signal to be queued; bump the timeout.
>
> Sounds like we may need to look at the single-step breakpoint's thread 
> id, and only insert it if it is for the thread that is going to be 
> doing the step-over?  We may need to record that in step_over_info and 
> pass more info to stepping_past_instruction_at.

I think this is about any thread specific breakpoint, instead of
only single-step breakpoint (single-step breakpoint is thread specific
too).  If we are doing step-over for thread A, do we need to insert any
breakpoints specific to other threads?  (my answer is No).

-- 
Yao (齐尧)

Re: [PATCH 3/7] Force to insert software single step breakpoint

[Pedro Alves]

On 04/19/2016 03:54 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Another scenario occurred to me:
>>
>>  - Thread A is software single-stepping.
>>  - Thread B hits single-step breakpoint of thread A.
>>  - We pause all threads and set thread B stepping past the
>>    single-step breakpoint of thread A.
>>
>> But if the single-step breakpoint is for another thread, then
>> we won't actually manage to have thread B step past it, resulting
>> in spurious re-hits and no-guaranteed forward progress.  See
>> e.g., non-stop-fair-events.exp:
>>
>>         # On software single-step targets that don't support displaced
>>         # stepping, threads keep hitting each others' single-step
>>         # breakpoints, and then GDB needs to pause all threads to step
>>         # past those.  The end result is that progress in the main
>>         # thread will be slower and it may take a bit longer for the
>>         # signal to be queued; bump the timeout.
>>
>> Sounds like we may need to look at the single-step breakpoint's thread 
>> id, and only insert it if it is for the thread that is going to be 
>> doing the step-over?  We may need to record that in step_over_info and 
>> pass more info to stepping_past_instruction_at.
> 
> I think this is about any thread specific breakpoint, instead of
> only single-step breakpoint (single-step breakpoint is thread specific
> too).  If we are doing step-over for thread A, do we need to insert any
> breakpoints specific to other threads?  (my answer is No).

Right, we don't need to insert them, because other threads
will remain stopped while thread A is doing the step-over.

However, given that gdb does not remove/re-insert all breakpoints on
internal stops nowadays, removing thread-specific breakpoints of others
threads will be less efficient than leaving them be, I think.  I mean,
you'll get more z0/Z0 traffic than if you leave them inserted.

Thanks,
Pedro Alves

Re: [PATCH 3/7] Force to insert software single step breakpoint

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Sounds like we may need to look at the single-step breakpoint's thread 
> id, and only insert it if it is for the thread that is going to be 
> doing the step-over?  We may need to record that in step_over_info and 
> pass more info to stepping_past_instruction_at.

The patch below does this, however, I don't pass more info to
stepping_past_instruction_at, instead I add a new function
thread_is_being_stepped_over_p.

>
>> --- a/gdb/gdbarch.sh
>> +++ b/gdb/gdbarch.sh
>> @@ -609,7 +609,10 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR
>> addr:addr::core_addr_identity::0
>>  # target can single step.  If not, then implement single step using
>> breakpoints.
>>  #
>>  # A return value of 1 means that the software_single_step breakpoints
>> -# were inserted; 0 means they were not.
>> +# were inserted; 0 means they were not.  Multiple breakpoints may be
>> +# inserted for some instructions such as conditional branch.  However,
>> +# each implementation must always evaluate the condition and only put
>> +# the breakpoint at the branch destination if the condition is true.
>
> I'd add:
>
> (...) condition is true, so that we ensure forward progress when 
> stepping past a conditional branch to self.
>
> This will help porters evaluate whether that's really necessary
> for their ports.

Done.

-- 
Yao (齐尧)

Subject: [PATCH] Force to insert software single step breakpoint

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 skip the non-single-step breakpoints if they are inserted at
the place we are stepping over, however we don't want to skip
single-step breakpoint if its thread is the thread we are stepping
over, so in this patch, I add a thread num in 'struct step_over_info'
to record the thread we're stepping over.

gdb:

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

	* breakpoint.c (should_be_inserted): Return 0 if the location's
	owner is not single step breakpoint or single step brekapoint's
	owner isn't the thread we are stepping over.
	* gdbarch.sh (software_single_step): Update comments.
	* gdbarch.h: Regenerated.
	* infrun.c (struct step_over_info) <thread>: New field.
	(set_step_over_info): New argument 'thread'.  Callers updated.
	(clear_step_over_info): Set field thread to -1.
	(thread_is_being_stepped_over_p): New function.
	* infrun.h (thread_is_being_stepped_over_p): Declaration.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f99a7ab..64e97c6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2219,11 +2219,22 @@ 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 that the breakpoint is single-step breakpoint
+     and the single-step breakpoint's owner is the thread we're
+     stepping over.  */
   if ((bl->loc_type == bp_loc_software_breakpoint
        || bl->loc_type == bp_loc_hardware_breakpoint)
       && stepping_past_instruction_at (bl->pspace->aspace,
-				       bl->address))
+				       bl->address)
+      /* The single-step breakpoint may be inserted at the location
+	 we're trying to step if the instruction branches to itself.
+	 However, the instruction won't be executed at all and it may
+	 break the semantics of the instruction, for example, the
+	 instruction is a conditional branch or updates some flags.
+	 We can't fix it unless GDB is able to emulate the instruction
+	 or switch to displaced stepping.  */
+      && !(bl->owner->type == bp_single_step
+	   && thread_is_being_stepped_over_p (bl->owner->thread)))
     {
       if (debug_infrun)
 	{
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 252fc4b..859ba85 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -650,7 +650,12 @@ extern void set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch, gdbarch_addr_
    target can single step.  If not, then implement single step using breakpoints.
   
    A return value of 1 means that the software_single_step breakpoints
-   were inserted; 0 means they were not. */
+   were inserted; 0 means they were not.  Multiple breakpoints may be
+   inserted for some instructions such as conditional branch.  However,
+   each implementation must always evaluate the condition and only put
+   the breakpoint at the branch destination if the condition is true, so
+   that we ensure forward progress when stepping past a conditional
+   branch to self. */
 
 extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch);
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 37f59b7..c8787c2 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -609,7 +609,12 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core_addr_identity::0
 # target can single step.  If not, then implement single step using breakpoints.
 #
 # A return value of 1 means that the software_single_step breakpoints
-# were inserted; 0 means they were not.
+# were inserted; 0 means they were not.  Multiple breakpoints may be
+# inserted for some instructions such as conditional branch.  However,
+# each implementation must always evaluate the condition and only put
+# the breakpoint at the branch destination if the condition is true, so
+# that we ensure forward progress when stepping past a conditional
+# branch to self.
 F:int:software_single_step:struct frame_info *frame:frame
 
 # Return non-zero if the processor is executing a delay slot and a
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 696105d..1ccd648 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1296,6 +1296,9 @@ struct step_over_info
   /* The instruction being stepped over triggers a nonsteppable
      watchpoint.  If true, we'll skip inserting watchpoints.  */
   int nonsteppable_watchpoint_p;
+
+  /* The thread's global number.  */
+  int thread;
 };
 
 /* The step-over info of the location that is being stepped over.
@@ -1329,11 +1332,13 @@ static struct step_over_info step_over_info;
 
 static void
 set_step_over_info (struct address_space *aspace, CORE_ADDR address,
-		    int nonsteppable_watchpoint_p)
+		    int nonsteppable_watchpoint_p,
+		    int thread)
 {
   step_over_info.aspace = aspace;
   step_over_info.address = address;
   step_over_info.nonsteppable_watchpoint_p = nonsteppable_watchpoint_p;
+  step_over_info.thread = thread;
 }
 
 /* Called when we're not longer stepping over a breakpoint / an
@@ -1348,6 +1353,7 @@ clear_step_over_info (void)
   step_over_info.aspace = NULL;
   step_over_info.address = 0;
   step_over_info.nonsteppable_watchpoint_p = 0;
+  step_over_info.thread = -1;
 }
 
 /* See infrun.h.  */
@@ -1365,6 +1371,15 @@ stepping_past_instruction_at (struct address_space *aspace,
 /* See infrun.h.  */
 
 int
+thread_is_being_stepped_over_p (int thread)
+{
+  return (step_over_info.aspace != NULL
+	  && thread == step_over_info.thread);
+}
+
+/* See infrun.h.  */
+
+int
 stepping_past_nonsteppable_watchpoint (void)
 {
   return step_over_info.nonsteppable_watchpoint_p;
@@ -2579,7 +2594,7 @@ resume (enum gdb_signal sig)
 	    stop_all_threads ();
 
 	  set_step_over_info (get_regcache_aspace (regcache),
-			      regcache_read_pc (regcache), 0);
+			      regcache_read_pc (regcache), 0, tp->global_num);
 
 	  step = maybe_software_singlestep (gdbarch, pc);
 
@@ -7750,10 +7765,11 @@ keep_going_pass_signal (struct execution_control_state *ecs)
 	  && (remove_wps || !use_displaced_stepping (ecs->event_thread)))
 	{
 	  set_step_over_info (get_regcache_aspace (regcache),
-			      regcache_read_pc (regcache), remove_wps);
+			      regcache_read_pc (regcache), remove_wps,
+			      ecs->event_thread->global_num);
 	}
       else if (remove_wps)
-	set_step_over_info (NULL, 0, remove_wps);
+	set_step_over_info (NULL, 0, remove_wps, -1);
 
       /* If we now need to do an in-line step-over, we need to stop
 	 all other threads.  Note this must be done before
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 61d3b20..5f98414 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -133,6 +133,10 @@ extern void insert_step_resume_breakpoint_at_sal (struct gdbarch *,
 extern int stepping_past_instruction_at (struct address_space *aspace,
 					 CORE_ADDR address);
 
+/* Returns true if thread whose thread number is THREAD is being
+   stepped over.  */
+extern int thread_is_being_stepped_over_p (int thread);
+
 /* Returns true if we're trying to step past an instruction that
    triggers a non-steppable watchpoint.  */
 extern int stepping_past_nonsteppable_watchpoint (void);

Re: [PATCH 3/7] Force to insert software single step breakpoint

[Pedro Alves]

Looks OK to me now.  A couple nits below.

On 04/20/2016 08:49 AM, Yao Qi wrote:

> 
> 2016-04-20  Yao Qi  <yao.qi@linaro.org>
> 
> 	* breakpoint.c (should_be_inserted): Return 0 if the location's
> 	owner is not single step breakpoint or single step brekapoint's

type "brekapoint".

> 	owner isn't the thread we are stepping over.
> 	* gdbarch.sh (software_single_step): Update comments.
> 	* gdbarch.h: Regenerated.
> 	* infrun.c (struct step_over_info) <thread>: New field.
> 	(set_step_over_info): New argument 'thread'.  Callers updated.
> 	(clear_step_over_info): Set field thread to -1.
> 	(thread_is_being_stepped_over_p): New function.

We don't step over threads, but rather threads step over breakpoints.

I'd suggest:

 thread_is_stepping_over_breakpoint_p

(Personally. I don't see the need for a _p / predicate suffix
when the function is clearly a predicate, due to use of the
"is".  thread_being_stepped_over_p / thread_is_being_stepped_over).

> 	* infrun.h (thread_is_being_stepped_over_p): Declaration.
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f99a7ab..64e97c6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2219,11 +2219,22 @@ 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 that the breakpoint is single-step breakpoint
> +     and the single-step breakpoint's owner is the thread we're
> +     stepping over.  */

"breakpoint's owner" is kind of possible confusing
with "bp location owner", which is itself a breakpoint.

I'd find it clearer to copy&edit it to say:

   /* Don't insert a breakpoint if we're trying to step past its
      location, except if the breakpoint is a single-step breakpoint,
      and the breakpoint's thread is the thread that is stepping past
      a breakpoint.  */

>  /* See infrun.h.  */
> @@ -1365,6 +1371,15 @@ stepping_past_instruction_at (struct address_space *aspace,
>  /* See infrun.h.  */
>  
>  int
> +thread_is_being_stepped_over_p (int thread)
> +{
> +  return (step_over_info.aspace != NULL
> +	  && thread == step_over_info.thread);

Wouldn't:

  return (step_over_info.thread != -1
	  && thread == step_over_info.thread);

be a bit more to the point?  Using the aspace field makes me wonder whether
we're caring for a case where step_over_info.thread is set to some
thread, but aspace is NULL.

Thanks,
Pedro Alves

Re: [PATCH 3/7] Force to insert software single step breakpoint

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> type "brekapoint".
>

Fixed.

>> 	owner isn't the thread we are stepping over.
>> 	* gdbarch.sh (software_single_step): Update comments.
>> 	* gdbarch.h: Regenerated.
>> 	* infrun.c (struct step_over_info) <thread>: New field.
>> 	(set_step_over_info): New argument 'thread'.  Callers updated.
>> 	(clear_step_over_info): Set field thread to -1.
>> 	(thread_is_being_stepped_over_p): New function.
>
> We don't step over threads, but rather threads step over breakpoints.
>
> I'd suggest:
>
>  thread_is_stepping_over_breakpoint_p
>
> (Personally. I don't see the need for a _p / predicate suffix
> when the function is clearly a predicate, due to use of the
> "is".  thread_being_stepped_over_p / thread_is_being_stepped_over).

thread_is_stepping_over_breakpoint is used.

>
>> 	* infrun.h (thread_is_being_stepped_over_p): Declaration.
>> 
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index f99a7ab..64e97c6 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -2219,11 +2219,22 @@ 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 that the breakpoint is single-step breakpoint
>> +     and the single-step breakpoint's owner is the thread we're
>> +     stepping over.  */
>
> "breakpoint's owner" is kind of possible confusing
> with "bp location owner", which is itself a breakpoint.
>
> I'd find it clearer to copy&edit it to say:
>
>    /* Don't insert a breakpoint if we're trying to step past its
>       location, except if the breakpoint is a single-step breakpoint,
>       and the breakpoint's thread is the thread that is stepping past
>       a breakpoint.  */
>

Done.

>>  /* See infrun.h.  */
>> @@ -1365,6 +1371,15 @@ stepping_past_instruction_at (struct
>> address_space *aspace,
>>  /* See infrun.h.  */
>>  
>>  int
>> +thread_is_being_stepped_over_p (int thread)
>> +{
>> +  return (step_over_info.aspace != NULL
>> +	  && thread == step_over_info.thread);
>
> Wouldn't:
>
>   return (step_over_info.thread != -1
> 	  && thread == step_over_info.thread);
>
> be a bit more to the point?  Using the aspace field makes me wonder whether
> we're caring for a case where step_over_info.thread is set to some
> thread, but aspace is NULL.

I thought about the case you mentioned, but don't know how can it
happen.  I check aspace in order to align with the validity check of
step_over_info.  Checking .thread against -1 is fine to me, as well.

Patch below is what I pushed in.

-- 
Yao (齐尧)
From 21edc42f4e1ec6fe8cfce171232bab27ad4af372 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Mon, 25 Apr 2016 09:16:21 +0100
Subject: [PATCH] Force to insert software single step breakpoint

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 skip the non-single-step breakpoints if they are inserted at
the place we are stepping over, however we don't want to skip
single-step breakpoint if its thread is the thread we are stepping
over, so in this patch, I add a thread num in 'struct step_over_info'
to record the thread we're stepping over.

gdb:

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

	* breakpoint.c (should_be_inserted): Return 0 if the location's
	owner is not single step breakpoint or single step breakpoint's
	thread isn't the thread which is stepping past a breakpoint.
	* gdbarch.sh (software_single_step): Update comments.
	* gdbarch.h: Regenerated.
	* infrun.c (struct step_over_info) <thread>: New field.
	(set_step_over_info): New argument 'thread'.  Callers updated.
	(clear_step_over_info): Set field thread to -1.
	(thread_is_stepping_over_breakpoint): New function.
	* infrun.h (thread_is_stepping_over_breakpoint): Declaration.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 91db2e3..0b08605 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2016-04-25  Yao Qi  <yao.qi@linaro.org>
+
+	* breakpoint.c (should_be_inserted): Return 0 if the location's
+	owner is not single step breakpoint or single step breakpoint's
+	thread isn't the thread which is stepping past a breakpoint.
+	* gdbarch.sh (software_single_step): Update comments.
+	* gdbarch.h: Regenerated.
+	* infrun.c (struct step_over_info) <thread>: New field.
+	(set_step_over_info): New argument 'thread'.  Callers updated.
+	(clear_step_over_info): Set field thread to -1.
+	(thread_is_stepping_over_breakpoint): New function.
+	* infrun.h (thread_is_stepping_over_breakpoint): Declaration.
+
 2016-04-22  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
 
 	* ppc-linux-nat.c (ppc_linux_read_description): Use PPC_FEATURE_HAS_VSX
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f99a7ab..a39a15c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2219,11 +2219,22 @@ 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 if the breakpoint is a single-step breakpoint,
+     and the breakpoint's thread is the thread which is stepping past
+     a breakpoint.  */
   if ((bl->loc_type == bp_loc_software_breakpoint
        || bl->loc_type == bp_loc_hardware_breakpoint)
       && stepping_past_instruction_at (bl->pspace->aspace,
-				       bl->address))
+				       bl->address)
+      /* The single-step breakpoint may be inserted at the location
+	 we're trying to step if the instruction branches to itself.
+	 However, the instruction won't be executed at all and it may
+	 break the semantics of the instruction, for example, the
+	 instruction is a conditional branch or updates some flags.
+	 We can't fix it unless GDB is able to emulate the instruction
+	 or switch to displaced stepping.  */
+      && !(bl->owner->type == bp_single_step
+	   && thread_is_stepping_over_breakpoint (bl->owner->thread)))
     {
       if (debug_infrun)
 	{
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 252fc4b..859ba85 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -650,7 +650,12 @@ extern void set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch, gdbarch_addr_
    target can single step.  If not, then implement single step using breakpoints.
   
    A return value of 1 means that the software_single_step breakpoints
-   were inserted; 0 means they were not. */
+   were inserted; 0 means they were not.  Multiple breakpoints may be
+   inserted for some instructions such as conditional branch.  However,
+   each implementation must always evaluate the condition and only put
+   the breakpoint at the branch destination if the condition is true, so
+   that we ensure forward progress when stepping past a conditional
+   branch to self. */
 
 extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch);
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 37f59b7..c8787c2 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -609,7 +609,12 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core_addr_identity::0
 # target can single step.  If not, then implement single step using breakpoints.
 #
 # A return value of 1 means that the software_single_step breakpoints
-# were inserted; 0 means they were not.
+# were inserted; 0 means they were not.  Multiple breakpoints may be
+# inserted for some instructions such as conditional branch.  However,
+# each implementation must always evaluate the condition and only put
+# the breakpoint at the branch destination if the condition is true, so
+# that we ensure forward progress when stepping past a conditional
+# branch to self.
 F:int:software_single_step:struct frame_info *frame:frame
 
 # Return non-zero if the processor is executing a delay slot and a
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 696105d..cfb1d06 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1296,6 +1296,9 @@ struct step_over_info
   /* The instruction being stepped over triggers a nonsteppable
      watchpoint.  If true, we'll skip inserting watchpoints.  */
   int nonsteppable_watchpoint_p;
+
+  /* The thread's global number.  */
+  int thread;
 };
 
 /* The step-over info of the location that is being stepped over.
@@ -1329,11 +1332,13 @@ static struct step_over_info step_over_info;
 
 static void
 set_step_over_info (struct address_space *aspace, CORE_ADDR address,
-		    int nonsteppable_watchpoint_p)
+		    int nonsteppable_watchpoint_p,
+		    int thread)
 {
   step_over_info.aspace = aspace;
   step_over_info.address = address;
   step_over_info.nonsteppable_watchpoint_p = nonsteppable_watchpoint_p;
+  step_over_info.thread = thread;
 }
 
 /* Called when we're not longer stepping over a breakpoint / an
@@ -1348,6 +1353,7 @@ clear_step_over_info (void)
   step_over_info.aspace = NULL;
   step_over_info.address = 0;
   step_over_info.nonsteppable_watchpoint_p = 0;
+  step_over_info.thread = -1;
 }
 
 /* See infrun.h.  */
@@ -1365,6 +1371,15 @@ stepping_past_instruction_at (struct address_space *aspace,
 /* See infrun.h.  */
 
 int
+thread_is_stepping_over_breakpoint (int thread)
+{
+  return (step_over_info.thread != -1
+	  && thread == step_over_info.thread);
+}
+
+/* See infrun.h.  */
+
+int
 stepping_past_nonsteppable_watchpoint (void)
 {
   return step_over_info.nonsteppable_watchpoint_p;
@@ -2579,7 +2594,7 @@ resume (enum gdb_signal sig)
 	    stop_all_threads ();
 
 	  set_step_over_info (get_regcache_aspace (regcache),
-			      regcache_read_pc (regcache), 0);
+			      regcache_read_pc (regcache), 0, tp->global_num);
 
 	  step = maybe_software_singlestep (gdbarch, pc);
 
@@ -7750,10 +7765,11 @@ keep_going_pass_signal (struct execution_control_state *ecs)
 	  && (remove_wps || !use_displaced_stepping (ecs->event_thread)))
 	{
 	  set_step_over_info (get_regcache_aspace (regcache),
-			      regcache_read_pc (regcache), remove_wps);
+			      regcache_read_pc (regcache), remove_wps,
+			      ecs->event_thread->global_num);
 	}
       else if (remove_wps)
-	set_step_over_info (NULL, 0, remove_wps);
+	set_step_over_info (NULL, 0, remove_wps, -1);
 
       /* If we now need to do an in-line step-over, we need to stop
 	 all other threads.  Note this must be done before
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 61d3b20..e79bf2d 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -133,6 +133,10 @@ extern void insert_step_resume_breakpoint_at_sal (struct gdbarch *,
 extern int stepping_past_instruction_at (struct address_space *aspace,
 					 CORE_ADDR address);
 
+/* Returns true if thread whose thread number is THREAD is stepping
+   over a breakpoint.  */
+extern int thread_is_stepping_over_breakpoint (int thread);
+
 /* Returns true if we're trying to step past an instruction that
    triggers a non-steppable watchpoint.  */
 extern int stepping_past_nonsteppable_watchpoint (void);

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

[Yao Qi]

gdb/testsuite:

2016-03-23  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

Re: [PATCH 7/7] New test case gdb.base/branch-to-self.exp

[Pedro Alves]

On 03/23/2016 04:09 PM, Yao Qi wrote:

> +#if defined(__arm__) || defined(__aarch64__)
> +#define BRANCH_INSN "b"
> +#elif defined(__x86_64__) || defined(__i386__)
> +#define BRANCH_INSN "jmp"

It's nice when these are indented, like:

#if defined(__arm__) || defined(__aarch64__)
# define BRANCH_INSN "b"
#elif defined(__x86_64__) || defined(__i386__)
# define BRANCH_INSN "jmp"

> +#else

Did you mean to put something here?  #error perhaps?

I wonder whether we should just use

  while (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}"

Please avoid line numbers in test messages, as they'll change
as the test evolves in future.

> +    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"

Likewise.

> +
> +	gdb_continue_to_breakpoint "continue to break"
> +
> +	gdb_test "p counter" ".* = 5"
> +    }
> +}
> 

Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH 7/7] New test case gdb.base/branch-to-self.exp

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Did you mean to put something here?  #error perhaps?
>
> I wonder whether we should just use
>
>   while (1);
>

The source C code leading me looking at this issue is "for (;;);", we
can use it too.

>> +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}"
>
> Please avoid line numbers in test messages, as they'll change
> as the test evolves in future.

Fixed.  Patch below is pushed in.

-- 
Yao (齐尧)

From f3abeff575541dd80e1facd6d0f920e10f77fede Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Mon, 25 Apr 2016 09:53:51 +0100
Subject: [PATCH] New test case gdb.base/branch-to-self.exp

gdb/testsuite:

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

	* gdb.base/branch-to-self.c: New file.
	* gdb.base/branch-to-self.exp: New file.

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c7c9d58..c919c47 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2016-04-25  Yao Qi  <yao.qi@linaro.org>
+
+	* gdb.base/branch-to-self.c: New file.
+	* gdb.base/branch-to-self.exp: New file.
+
 2016-04-22  Yao Qi  <yao.qi@linaro.org>
 
 	* gdb.trace/unavailable.exp (gdb_collect_globals_test_1): Match
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..594214b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/branch-to-self.c
@@ -0,0 +1,40 @@
+/* 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>
+
+volatile int counter = 0;
+
+static void
+handler (int sig)
+{
+  counter = 5;
+}
+
+int
+main (void)
+{
+  signal (SIGALRM, handler);
+  alarm (5);
+
+  /* Compiler may generate a single instruction which branches to
+     itself.  */
+  for (;;); /* loop-line */
+  return 0;
+}
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..2fd3b38
--- /dev/null
+++ b/gdb/testsuite/gdb.base/branch-to-self.exp
@@ -0,0 +1,69 @@
+# 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}" "Breakpoint .*" \
+	"set breakpoint"
+    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" \
+	    "Breakpoint .*" "set breakpoint with condition"
+
+	gdb_continue_to_breakpoint "continue to break"
+
+	gdb_test "p counter" ".* = 5"
+    }
+}