[MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[Vladimir Prus]

My MI non-stop tests (coming later in the series) revealed a bug in displaced
stepping -- we fail to discard cleanup on an early exit path, which caused a
call to normal_stop at very unfortunate time -- when a thread was running.

OK?

- Volodya

	* infrun.c (resume): Discard cleanups on early exit path.
---
 gdb/infrun.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index cf23c18..e803d1b 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -964,10 +964,13 @@ a command like `return' or `jump' to continue execution."));
       && sig == TARGET_SIGNAL_0)
     {
       if (!displaced_step_prepare (inferior_ptid))
-	/* Got placed in displaced stepping queue.  Will be resumed
-	   later when all the currently queued displaced stepping
-	   requests finish.  */
-	return;
+	{
+	  /* Got placed in displaced stepping queue.  Will be resumed
+	     later when all the currently queued displaced stepping
+	     requests finish.  */
+	  discard_cleanups (old_cleanups);
+	  return;
+	}
     }
 
   if (step && gdbarch_software_single_step_p (gdbarch))
-- 
1.5.3.5

Re: [MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[Daniel Jacobowitz]

On Sat, Jun 28, 2008 at 08:39:52PM +0400, Vladimir Prus wrote:
> 
> My MI non-stop tests (coming later in the series) revealed a bug in displaced
> stepping -- we fail to discard cleanup on an early exit path, which caused a
> call to normal_stop at very unfortunate time -- when a thread was running.
> 
> OK?

Yes, thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[Pedro Alves]

A Saturday 28 June 2008 17:39:52, Vladimir Prus wrote:
> My MI non-stop tests (coming later in the series) revealed a bug in
> displaced stepping -- we fail to discard cleanup on an early exit path,
> which caused a call to normal_stop at very unfortunate time -- when a
> thread was running.
>

Ooops, my bad.  Thanks for catching this!

I think it would make sense to tag the thread as running at that point.
In the point of view of the frontend, it is running, but internally,
it is known to not be executing, until its turn in the displaced stepping
request queue arrives.  That is, the frontend should not allow inspecting
this thread as if it was running already, and interrupting it should make
it stop as soon as it tries to execute, not skip over it like ends up
happening currently.

To be clear, the user told the thread to resume, it just didn't resume
immediatelly due to an internal implementation detail.  (is_running (ptid)
&& !is_executing (ptid)) should be true at that early return.  Not a
serious issue to run to fix it (may need care if starting the
displaced step on it ends up failing), but,

Anyone disagrees in principle?

> OK?
>
> - Volodya
>
> 	* infrun.c (resume): Discard cleanups on early exit path.
> ---
>  gdb/infrun.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index cf23c18..e803d1b 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -964,10 +964,13 @@ a command like `return' or `jump' to continue
> execution.")); && sig == TARGET_SIGNAL_0)
>      {
>        if (!displaced_step_prepare (inferior_ptid))
> -	/* Got placed in displaced stepping queue.  Will be resumed
> -	   later when all the currently queued displaced stepping
> -	   requests finish.  */
> -	return;
> +	{
> +	  /* Got placed in displaced stepping queue.  Will be resumed
> +	     later when all the currently queued displaced stepping
> +	     requests finish.  */
> +	  discard_cleanups (old_cleanups);
> +	  return;
> +	}
>      }
>
>    if (step && gdbarch_software_single_step_p (gdbarch))



-- 
Pedro Alves

Re: [MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[Vladimir Prus]

On Friday 11 July 2008 17:10:58 Pedro Alves wrote:
> A Saturday 28 June 2008 17:39:52, Vladimir Prus wrote:
> > My MI non-stop tests (coming later in the series) revealed a bug in
> > displaced stepping -- we fail to discard cleanup on an early exit path,
> > which caused a call to normal_stop at very unfortunate time -- when a
> > thread was running.
> >
> 
> Ooops, my bad.  Thanks for catching this!
> 
> I think it would make sense to tag the thread as running at that point.
> In the point of view of the frontend, it is running, but internally,
> it is known to not be executing, until its turn in the displaced stepping
> request queue arrives.  That is, the frontend should not allow inspecting
> this thread as if it was running already, and interrupting it should make
> it stop as soon as it tries to execute, not skip over it like ends up
> happening currently.
> 
> To be clear, the user told the thread to resume, it just didn't resume
> immediatelly due to an internal implementation detail.  (is_running (ptid)
> && !is_executing (ptid)) should be true at that early return.  Not a
> serious issue to run to fix it (may need care if starting the
> displaced step on it ends up failing), but,
> 
> Anyone disagrees in principle?

Seems right to me. Should that be a separate patch, or should I adjust
this one?

- Volodya

Re: [MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[Daniel Jacobowitz]

On Fri, Jul 11, 2008 at 05:21:24PM +0400, Vladimir Prus wrote:
> Seems right to me. Should that be a separate patch, or should I adjust
> this one?

Separate, please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[Pedro Alves]

A Friday 11 July 2008 14:21:24, Vladimir Prus wrote:

> Seems right to me. Should that be a separate patch, or should I adjust
> this one?

Oh great, I wasn't asking you to do it, but if you want :-).  I'd
prefer separate.

-- 
Pedro Alves

Re: [MI non-stop 03/11, RFA] Discard cleanup when deferring displaced step

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Fri, Jul 11, 2008 at 05:21:24PM +0400, Vladimir Prus wrote:
>> Seems right to me. Should that be a separate patch, or should I adjust
>> this one?
> 
> Separate, please.

I've checked in the patch without changes. I'll post this separate tweak
later today.

- Volodya