Re: [pushed] gdbserver: redo stepping over breakpoint that was on top of a permanent breakpoint

[Pedro Alves <palves@redhat.com>]

On 02/24/2015 12:25 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> +      unsigned int increment_pc;
>> +
>> +      if (the_low_target.breakpoint_len > the_low_target.decr_pc_after_break)
>> +	increment_pc = the_low_target.breakpoint_len;
>> +      else
>> +	increment_pc = the_low_target.decr_pc_after_break;
> 
> AFAICS, the_low_target.breakpoint_len is greater or equal to
> the_low_target.decr_pc_after_break.  Why don't we use .breakpoint_len
> instead?  The code is correct, however, the comparison here leads me to
> think under what situation .breakpoint_len is less than .decr_pc_after_break.

Good point.  The history is that I started out by always incrementing
by decr_pc_after_break.  Later, I recalled that that's not always
the same as breakpoint length, and thinking that maybe archs that
set decr_pc_after_break may not set breakpoint_len, wrote
"increment = max (decr_pc_after_break, breakpoint_len);".  That didn't
work (we don't define max anywhere), so I expanded that into the "if",
and never thought about it again.  :-)  It's hard to tell whether
there are such ports, but likely not.  If there are, bp-permanent.exp
should catch them failing, I think.  :-)  So I went ahead and pushed
the patch below.

Thanks.

---
[PATCH] gdbserver/Linux: Simplify stepping past program breakpoint a
 little

.decr_pc_after_break is never higher than .breakpoint_len, so use
.breakpoint_len directly.  Based on idea from Yao here:
https://sourceware.org/ml/gdb-patches/2015-02/msg00689.html

gdb/gdbserver/ChangeLog:
2015-02-26  Pedro Alves  <palves@redhat.com>

	* linux-low.c (linux_wait_1): When incrementing the PC past a
	program breakpoint always use the_low_target.breakpoint_len as
	increment, rather than the maximum between that and
	the_low_target.decr_pc_after_break.
---
 gdb/gdbserver/ChangeLog   | 7 +++++++
 gdb/gdbserver/linux-low.c | 7 +------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index a130aab..6bb8950 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,10 @@
+2015-02-26  Pedro Alves  <palves@redhat.com>
+
+	* linux-low.c (linux_wait_1): When incrementing the PC past a
+	program breakpoint always use the_low_target.breakpoint_len as
+	increment, rather than the maximum between that and
+	the_low_target.decr_pc_after_break.
+
 2015-02-23  Pedro Alves  <palves@redhat.com>
 
 	* linux-low.c (check_stopped_by_breakpoint): Don't check if the
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 1c66985..c0d3b0d 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2555,12 +2555,7 @@ linux_wait_1 (ptid_t ptid,
   if (!ptid_equal (step_over_bkpt, null_ptid)
       && event_child->stop_reason == LWP_STOPPED_BY_SW_BREAKPOINT)
     {
-      unsigned int increment_pc;
-
-      if (the_low_target.breakpoint_len > the_low_target.decr_pc_after_break)
-	increment_pc = the_low_target.breakpoint_len;
-      else
-	increment_pc = the_low_target.decr_pc_after_break;
+      unsigned int increment_pc = the_low_target.breakpoint_len;
 
       if (debug_threads)
 	{
-- 
1.9.3