Re: [PATCH 2/2] Fix gdb.threads/multiple-step-overs.exp fails on arm

[Pedro Alves <palves at redhat dot com>]

On 10/29/2015 03:01 PM, Yao Qi wrote:
> Hi,
> Some tests in gdb.threads/multiple-step-overs.exp fail on arm target
> when the displaced stepping on, but they pass when displaced stepping
> is off.
> 
>  FAIL: gdb.threads/multiple-step-overs.exp: displaced=on: step: step
>  FAIL: gdb.threads/multiple-step-overs.exp: displaced=on: next: next
>  FAIL: gdb.threads/multiple-step-overs.exp: displaced=on: continue: continue
>  FAIL: gdb.threads/multiple-step-overs.exp: displaced=on: signal thr1: continue to sigusr1_handler
> 
> when displaced stepping is on,
> 
> Sending packet: $vCont;c#a8...infrun: infrun_async(1)^M <--- [1]
> infrun: prepare_to_wait^M
> infrun: target_wait (-1.0.0, status) =^M
> infrun:   -1.0.0 [Thread 0],^M
> infrun:   status->kind = ignore^M
> infrun: TARGET_WAITKIND_IGNORE^M
> infrun: prepare_to_wait^M
> Packet received: T05swbreak:;0b:f8faffbe;0d:409ee7b6;0f:d0880000;thread:p635.636;core:0;^M
> infrun: target_wait (-1.0.0, status) =^M
> infrun:   1589.1590.0 [Thread 1590],^M
> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP^M
> infrun: TARGET_WAITKIND_STOPPED^M
> infrun: stop_pc = 0x88d0^M
> infrun: context switch^M
> infrun: Switching context from Thread 1591 to Thread 1590^
> 
> GDB resumes the whole process (all threads) rather than the specific
> thread it wants to single step (as shown in [1]).  That is wrong.

(I understand this, but I think it'd make it clearer to explicitly
state _why_ that is wrong.)

> 
> when displaced stepping is off, GDB behaves correctly, only resumes
> the specific thread (as shown in [2]).
> 
> Sending packet: $vCont;c:p611.613#b2...infrun: infrun_async(1)^M <-- [2]
> infrun: prepare_to_wait^M
> infrun: target_wait (-1.0.0, status) =^M
> infrun:   -1.0.0 [Thread 0],^M
> infrun:   status->kind = ignore^M
> infrun: TARGET_WAITKIND_IGNORE^M
> infrun: prepare_to_wait^M
> Packet received: T05swbreak:;0b:f8faffbe;0d:409e67b6;0f:48880000;thread:p611.613;core:1;^M
> infrun: target_wait (-1.0.0, status) =^M
> infrun:   1553.1555.0 [Thread 1555],^M
> infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP^M
> infrun: TARGET_WAITKIND_STOPPED^M
> infrun: clear_step_over_info^M
> infrun: stop_pc = 0x8848
> 
> The current logic in GDB on deciding the set of threads to resume is:
> 
>   /* Decide the set of threads to ask the target to resume.  */
>   if ((step || thread_has_single_step_breakpoints_set (tp))
>       && tp->control.trap_expected)
>     {
>       /* We're allowing a thread to run past a breakpoint it has
> 	 hit, by single-stepping the thread with the breakpoint
> 	 removed.  In which case, we need to single-step only this
> 	 thread, and keep others stopped, as they can miss this
> 	 breakpoint if allowed to run.  */
>       resume_ptid = inferior_ptid;
>     }
>   else
>     resume_ptid = internal_resume_ptid (user_step);
> 
> it doesn't handle the case correctly that GDB continue (instead of
> single step) the thread for displaced stepping.
> 
> I also update the comment below to reflect the code.  I remove the
> "with the breakpoint removed" comment, because GDB doesn't remove
> breakpoints in displaced stepping, so we don't have to worry that
> other threads may miss the breakpoint.
> 
> Patch is regression tested on both x86_64-linux and arm-linux.
> 
> gdb:
> 
> 2015-10-28  Yao Qi  <yao.qi@linaro.org>
> 
> 	* infrun.c (resume): Check displaced_step_in_progress_thread
> 	when deciding the set of threads to resume.
> ---
>  gdb/infrun.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 0265d35..c619b61 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2631,14 +2631,12 @@ resume (enum gdb_signal sig)
>    gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step));
>  
>    /* Decide the set of threads to ask the target to resume.  */
> -  if ((step || thread_has_single_step_breakpoints_set (tp))
> +  if ((step || thread_has_single_step_breakpoints_set (tp)
> +       || displaced_step_in_progress_thread (tp->ptid))
>        && tp->control.trap_expected)


I wonder, can't we just remove the "step" check, like:

  if (tp->control.trap_expected)
    {



>      {
>        /* We're allowing a thread to run past a breakpoint it has
> -	 hit, by single-stepping the thread with the breakpoint
> -	 removed.  In which case, we need to single-step only this
> -	 thread, and keep others stopped, as they can miss this
> -	 breakpoint if allowed to run.  */
> +	 hit, by single-stepping (in-line or out-of-line) the thread.  */

The change looks good to me, though I think we should clarify
the comment here.  How about:

       /* We're allowing a thread to run past a breakpoint it has
	 hit, either by single-stepping the thread with the breakpoint
	 removed, or by displaced stepping, with the breakpoint inserted.
         In the former case, we need to single-step only this thread, and keep
         others stopped, as they can miss this breakpoint if allowed to run.
         That's not really a problem for displaced stepping, but, we still keep
         other threads stopped, in case another thread is also stopped for a
         breakpoint waiting for its turn in the displaced stepping queue.  */

I think we could optimize this by checking for
thread_step_over_chain_next (tp) == NULL, because if no other thread
is waiting for a step-over, then we could resume all threads, but
that's maybe not worth it.

>        resume_ptid = inferior_ptid;
>      }
>    else

Thanks,
Pedro Alves