diff mbox

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

Message ID 1446130862-12824-3-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi Oct. 29, 2015, 3:01 p.m. UTC 
  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.

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(-)

Comments

Pedro Alves Nov. 4, 2015, 5:19 p.m. UTC | #1 
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
diff mbox

Patch

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)
     {
       /* 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.  */
       resume_ptid = inferior_ptid;
     }
   else