[PATCH 0/2] Honour software single step in fallback of displaced stepping

[PATCH 0/2] Honour software single step in fallback of displaced stepping

[Yao Qi]

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

Hi,
This patch set fixes many fails I've seen in
gdb.threads/non-stop-fair-events.exp on arm-linux target.  I don't see
they are covered by Pedro's "All-stop on top of non-stop" V2, so I
post them out for the review.

Patch 2 is the real fix and the patch 1 is to make the debugging log
clear to identify the problem.

The two patches are tested on native and gdbserver {x86_64,arm}-linux.

*** BLURB HERE ***

Yao Qi (2):
  [gdbserver] assert on step if !can_hardware_single_step
  Honour software single step in fallback of displaced stepping

 gdb/gdbserver/linux-low.c | 3 +++
 gdb/infrun.c              | 8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 2/2] Honour software single step in fallback of displaced stepping

[Yao Qi]

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

Hi,
When I run gdb.threads/non-stop-fair-events.exp on arm-linux target,
I see the following error in the log,

  displaced: breakpoint is gone: Thread 22518, step(1)^M
  Sending packet: $vCont;s:p57f3.57f6#9d...
  gdb/gdbserver/linux-low.c:3686: A problem internal to GDBserver has been detected.^M
  linux_resume_one_lwp_throw: Assertion `step == 0' failed.

GDB sends vCont;s by mistake, and GDBserver fails on assert.  GDB
doesn't consider software single step in infrun.c:displaced_step_fixup,

	  /* Go back to what we were trying to do.  */
	  step = currently_stepping (tp);

	  if (debug_displaced)
	    fprintf_unfiltered (gdb_stdlog,
				"displaced: breakpoint is gone: %s, step(%d)\n",
				target_pid_to_str (tp->ptid), step);

	  target_resume (ptid, step, GDB_SIGNAL_0);

The patch is to let GDB consider software single step here.  It fixes
fails in gdb.threads/non-stop-fair-events.exp on arm.

gdb:

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

	* infrun.c (maybe_software_singlestep): Declare.
	(displaced_step_fixup): Call maybe_software_singlestep.
---
 gdb/infrun.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index f5faa0a..f4bbf67 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -100,6 +100,8 @@ static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
 
 static void insert_longjmp_resume_breakpoint (struct gdbarch *, CORE_ADDR);
 
+static int maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc);
+
 /* When set, stop the 'step' command if we enter a function which has
    no line number information.  The normal behavior is that we step
    over such function.  */
@@ -1847,6 +1849,7 @@ displaced_step_fixup (ptid_t event_ptid, enum gdb_signal signal)
       regcache = get_thread_regcache (ptid);
       actual_pc = regcache_read_pc (regcache);
       aspace = get_regcache_aspace (regcache);
+      gdbarch = get_regcache_arch (regcache);
 
       if (breakpoint_here_p (aspace, actual_pc))
 	{
@@ -1857,8 +1860,6 @@ displaced_step_fixup (ptid_t event_ptid, enum gdb_signal signal)
 
 	  displaced_step_prepare (ptid);
 
-	  gdbarch = get_regcache_arch (regcache);
-
 	  if (debug_displaced)
 	    {
 	      CORE_ADDR actual_pc = regcache_read_pc (regcache);
@@ -1891,6 +1892,9 @@ displaced_step_fixup (ptid_t event_ptid, enum gdb_signal signal)
 	  /* Go back to what we were trying to do.  */
 	  step = currently_stepping (tp);
 
+	  if (step)
+	    step = maybe_software_singlestep (gdbarch, actual_pc);
+
 	  if (debug_displaced)
 	    fprintf_unfiltered (gdb_stdlog,
 				"displaced: breakpoint is gone: %s, step(%d)\n",
-- 
1.9.1

[PATCH 1/2] [gdbserver] assert on step if !can_hardware_single_step

[Yao Qi]

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

GDB sends vCont;s by mistake to GDBserver on arm target which doesn't
have single step at all.  However, it is hard to find the problem from
the debugging log.  With this patch applied, the problem is easy to
identify, like:

(gdb) PASS: gdb.threads/non-stop-fair-events.exp: signal_thread=2: switch to thread 6 to step it
step&^M
(gdb) PASS: gdb.threads/non-stop-fair-events.exp: signal_thread=2: set 6 thread stepping
thread /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:3686: A problem internal to GDBserver has been detected.^M
linux_resume_one_lwp_throw: Assertion `step == 0' failed.

gdb/gdbserver:

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

	* linux-low.c (linux_resume_one_lwp_throw): Assert on step.
---
 gdb/gdbserver/linux-low.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e4c5420..bc6ab1ae 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3682,6 +3682,9 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
   if (the_low_target.prepare_to_resume != NULL)
     the_low_target.prepare_to_resume (lwp);
 
+  if (!can_hardware_single_step ())
+    gdb_assert (step == 0);
+
   regcache_invalidate_thread (thread);
   errno = 0;
   lwp->stepping = step;
-- 
1.9.1

Re: [PATCH 1/2] [gdbserver] assert on step if !can_hardware_single_step

[Pedro Alves]

On 04/07/2015 04:52 PM, Yao Qi wrote:
> From: Yao Qi <yao.qi@linaro.org>
> 
> GDB sends vCont;s by mistake to GDBserver on arm target which doesn't
> have single step at all.  However, it is hard to find the problem from
> the debugging log.  With this patch applied, the problem is easy to
> identify, like:
> 
> (gdb) PASS: gdb.threads/non-stop-fair-events.exp: signal_thread=2: switch to thread 6 to step it
> step&^M
> (gdb) PASS: gdb.threads/non-stop-fair-events.exp: signal_thread=2: set 6 thread stepping
> thread /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:3686: A problem internal to GDBserver has been detected.^M
> linux_resume_one_lwp_throw: Assertion `step == 0' failed.
> 
> gdb/gdbserver:
> 
> 2015-04-02  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (linux_resume_one_lwp_throw): Assert on step.
> ---
>  gdb/gdbserver/linux-low.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index e4c5420..bc6ab1ae 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -3682,6 +3682,9 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>    if (the_low_target.prepare_to_resume != NULL)
>      the_low_target.prepare_to_resume (lwp);
>  
> +  if (!can_hardware_single_step ())
> +    gdb_assert (step == 0);


Yeah, I have something like that on my x86 software single-step
branch, on the native side, and also in infrun.c.  See:

 https://github.com/palves/gdb/commits/palves/x86_software_single_step
 https://github.com/palves/gdb/commit/52940835548c124a80bd6f381f1a463eda9bab4c

( I just realized/recalled the top commit fixes the exact same as
your patch #2 :-) )

So I think your patch is a good idea.  :-)  But as you're suggesting
it for inclusion, I have to raise the bar a little.  I think gdbserver
crashing/exiting due to bad input from gdb isn't ideal.  This isn't
gdbserver's fault after all.  I think this should be an error instead.
Or perhaps even better, this could stay as an assert in the backend,
if server.c errors out earlier, even, while parsing the
vCont;s / s packets.

(
One nit, as step is a boolean, I think:

   gdb_assert (!step);

would read more naturally.
)

Thanks,
Pedro Alves

Re: [PATCH 0/2] Honour software single step in fallback of displaced stepping

[Pedro Alves]

On 04/07/2015 04:52 PM, Yao Qi wrote:
> From: Yao Qi <yao.qi@linaro.org>
> 
> Hi,
> This patch set fixes many fails I've seen in
> gdb.threads/non-stop-fair-events.exp on arm-linux target.  I don't see
> they are covered by Pedro's "All-stop on top of non-stop" V2, so I
> post them out for the review.

They actually are.  :-)  I tested v2 against x86 software single-step,
and it caught issues like that.

This patch:

 [PATCH v2 07/23] Embed the pending step-over chain in thread_info objects
 https://sourceware.org/ml/gdb-patches/2015-04/msg00218.html

splits that code you're touching to a separate "start_step_over_inferior"
function.

And then this patch:

 [PATCH v2 11/23] Use keep_going in proceed and start_step_over too
 https://sourceware.org/ml/gdb-patches/2015-04/msg00203.html

rewrites that whole function to defer to keep_going instead.  keep_going
already handles the case of the breakpoint disappearing (thread_still_needs_step_over
returns false).  And in case the breakpoint is still around, it ends
in 'resume' again, which is then the only place that knows
how to start a displaced step.

I don't mind if you push your patch in first.  I'll just
end up deleting that code again when I rebase it.

Thanks,
Pedro Alves

Re: [PATCH 0/2] Honour software single step in fallback of displaced stepping

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> They actually are.  :-)  I tested v2 against x86 software single-step,
> and it caught issues like that.

Yeah, I realised that when I apply your V2 on top of my patches
later yesterday for the testing.  I tested your V2 only, and fails in 
gdb.threads/non-stop-fair-events.exp go away!

> This patch:
>
>  [PATCH v2 07/23] Embed the pending step-over chain in thread_info objects
>  https://sourceware.org/ml/gdb-patches/2015-04/msg00218.html
>
> splits that code you're touching to a separate "start_step_over_inferior"
> function.
>
> And then this patch:
>
>  [PATCH v2 11/23] Use keep_going in proceed and start_step_over too
>  https://sourceware.org/ml/gdb-patches/2015-04/msg00203.html
>
> rewrites that whole function to defer to keep_going instead.  keep_going
> already handles the case of the breakpoint disappearing
> (thread_still_needs_step_over
> returns false).  And in case the breakpoint is still around, it ends
> in 'resume' again, which is then the only place that knows
> how to start a displaced step.

OK, I'll read the corresponding patches then.

>
> I don't mind if you push your patch in first.  I'll just
> end up deleting that code again when I rebase it.

I'll let your patches go in.

-- 
Yao (齐尧)

Re: [PATCH 0/2] Honour software single step in fallback of displaced stepping

[Yao Qi]

On 08/04/15 10:51, Yao Qi wrote:
>> I don't mind if you push your patch in first.  I'll just
>> >end up deleting that code again when I rebase it.
> I'll let your patches go in.

Hi Pedro,
Since this bug is still in FSF trunk, most of the tests in
gdb.threads/non-stop-fair-events.exp are timeout on arm-linux,
and takes much more time to run the whole testsuite.

Can I push patch 1/2 in to fix this problem?  assuming
your V3 "all stop on top of non-stop" series can't go in in a
a couple of days.

-- 
Yao (齐尧)

Re: [PATCH 0/2] Honour software single step in fallback of displaced stepping

[Pedro Alves]

On 04/16/2015 01:00 PM, Yao Qi wrote:
> On 08/04/15 10:51, Yao Qi wrote:
>>> I don't mind if you push your patch in first.  I'll just
>>>> end up deleting that code again when I rebase it.
>> I'll let your patches go in.
> 
> Hi Pedro,
> Since this bug is still in FSF trunk, most of the tests in
> gdb.threads/non-stop-fair-events.exp are timeout on arm-linux,
> and takes much more time to run the whole testsuite.
> 
> Can I push patch 1/2 in to fix this problem?  assuming
> your V3 "all stop on top of non-stop" series can't go in in a
> a couple of days.

Sure, go ahead.

Thanks,
Pedro Alves

Re: [PATCH 0/2] Honour software single step in fallback of displaced stepping

[Yao Qi]

On 16/04/15 13:02, Pedro Alves wrote:
> Sure, go ahead.

Thanks, Pedro.  Patch is pushed in.

-- 
Yao (齐尧)