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

  Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

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

I am not sure how to make the description clearer, how about this?

GDB resumes the whole process (all threads) rather than the specific
thread for which GDB wants to step over the breakpoint (as shown in [1]).
That is wrong.


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

Yes, it works! as the patch below,

>
>
>>      {
>>        /* 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.  */
>

They are copied into the patch.

> 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.

'thread_step_over_chain_next (tp) == NULL' doesn't work on arm-linux,
however, I didn't investigate why it doesn't.

Patch below is regression tested on x86_64-linux and arm-linux.
  

On 11/09/2015 11:23 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> 
>>> 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.)
> 
> I am not sure how to make the description clearer, how about this?
> 
> GDB resumes the whole process (all threads) rather than the specific
> thread for which GDB wants to step over the breakpoint (as shown in [1]).
> That is wrong.

I think the _why_ is that wrong is still missing.  I was thinking
of something around this:

The reason we resume a single thread and leave others stopped when doing a
normal step over where we temporarily remove the breakpoint, single-step,
reinsert the breakpoint, is that if we let other threads run in the period
while the breakpoint is removed, then these other threads could miss
the breakpoint.  Since with displaced stepping, we don't ever remove the
breakpoint, it should be fine to let other threads run.  However, there's another
reason that we should not let other threads run: that is the case where some of
those threads are also stopped for a breakpoint that itself needs to be
stepped over.  If we just let those threads run, then they immediately re-trap
their breakpoint again.

> 
> Patch below is regression tested on x86_64-linux and arm-linux.
> 

LGTM.

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> The reason we resume a single thread and leave others stopped when doing a
> normal step over where we temporarily remove the breakpoint, single-step,
> reinsert the breakpoint, is that if we let other threads run in the period
> while the breakpoint is removed, then these other threads could miss
> the breakpoint.  Since with displaced stepping, we don't ever remove the
> breakpoint, it should be fine to let other threads run.  However,
> there's another
> reason that we should not let other threads run: that is the case where some of
> those threads are also stopped for a breakpoint that itself needs to be
> stepped over.  If we just let those threads run, then they immediately re-trap
> their breakpoint again.
>

OK, copy them into the commit log.

>> 
>> Patch below is regression tested on x86_64-linux and arm-linux.
>> 
>
> LGTM.

Patch is pushed in.