Re: [PATCH v3 05/17] Embed the pending step-over chain in thread_info objects

[Pedro Alves <palves at redhat dot com>]

On 04/21/2015 09:28 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> This patch looks good to me, some questions below.
> 
>> 	(displaced_step_prepare): Assert that trap_expected is set.  Use
>> 	thread_step_over_chain_enqueue.  Split starting a new displaced
>> 	step to ...
>> 	(start_step_over): ... this new function.
> 
> If I read this patch correctly,  start_step_over is moved from
> displaced_step_fixup.  That is why we call start_step_over after each
> displaced_step_fixup.

Correct.

> 
>> v3:
>>
>> 	More comments.  The step-over chain is now a global instead of
>> 	being per-inferior.  Previous versions had actually broken
>> 	multiple-processes displaced stepping at the same time.  Added new
> 
> How does per-inferior step-over chain (or displaced stepping queue)
> break multi-process displaced stepping?

v1 and v2 put the head of the step-over chain in the inferior
structure.  start_step_over would look up the inferior structure of
the thread that just finished the step over, and try to
start a step-over of another thread of that inferior.

And if we had just finished an in-line step over, and the step-over that we
could start now is a displaced-step of _another_ inferior, in v2,
we wouldn't start it (because start_step_over_inferior wouldn't see that
thread).

And if we could start a displaced-step for a thread of the event
inferior, start_step_over would return immediately, instead
of trying to start a displaced step in another inferior too.

Even in-line step-overs were broken.  E.g., say you have two inferiors,
each with one thread.  Everything is stopped at a breakpoint that
must be stepped over. sched-multi is on, and the user does "continue"
to continue both inferiors.  We'd start an in-line step-over for
inferior 1.  Once that is done, we'd try starting a new step over
in the same inferior, and we'd miss that the other inferior
has a thread to step over too.

I went a bit in circles a trying to address that.  The fact that for
in-line step overs we must stop all threads currently (we should be
able to stop only threads sharing the stepped thread's address space
instead, but we're not there yet), but not for displaced stepping
started making it too complicated.  Related I also considered that we
could have more that one displaced step scratch pad slot per
inferior (e.g., "reserve" a few more bytes around the entry point).

Another thing I realized is that per-inferior queue breaks the
forward-progress-guarantee ordering, as we'd be giving priority
to start step overs on threads of the same inferior that had
just finished a step over, potentially starving threads of other
inferiors.  The ordering issue is that there was no ordering
between the step-over chains of the multiple inferiors, so if we
left the chain per-inferior, we wouldn't know which inferior's
chain had the thread that was waiting for a step-over for the
longest time.

All in all, I realized that a single list is simpler and
more flexible.

> 
>> @@ -2972,35 +2983,17 @@ infrun_thread_stop_requested_callback (struct thread_info *info, void *arg)
>>  static void
>>  infrun_thread_stop_requested (ptid_t ptid)
>>  {
>> -  struct displaced_step_inferior_state *displaced;
>> -
>> -  /* PTID was requested to stop.  Remove it from the displaced
>> -     stepping queue, so we don't try to resume it automatically.  */
>> -
>> -  for (displaced = displaced_step_inferior_states;
>> -       displaced;
>> -       displaced = displaced->next)
>> -    {
>> -      struct displaced_step_request *it, **prev_next_p;
>> -
>> -      it = displaced->step_request_queue;
>> -      prev_next_p = &displaced->step_request_queue;
>> -      while (it)
>> -	{
>> -	  if (ptid_match (it->ptid, ptid))
>> -	    {
>> -	      *prev_next_p = it->next;
>> -	      it->next = NULL;
>> -	      xfree (it);
>> -	    }
>> -	  else
>> -	    {
>> -	      prev_next_p = &it->next;
>> -	    }
>> +  struct thread_info *tp;
>>  
>> -	  it = *prev_next_p;
>> -	}
>> -    }
>> +  /* PTID was requested to stop.  Remove matching threads from the
>> +     step-over queue, so we don't try to resume them
>> +     automatically.  */
> 
> I can understand the code below, except the comment "we don't try to
> resume them automatically".  It looks not necessary here.

By "resumed automatically" I meant that if thread A is left in
the step-over chain (or currently in mainline in the displaced step queue)
it ends up stepped when all others threads in the step over queue
are done with their steps, even if thread A is supposed to be stopped.
But I agree it's not really necessary.  I'll remove it.

> 
>> +  ALL_NON_EXITED_THREADS (tp)
>> +    if (ptid_match (tp->ptid, ptid))
>> +      {
>> +	if (thread_is_in_step_over_chain (tp))
>> +	  thread_step_over_chain_remove (tp);
>> +      }
>>  
>>    iterate_over_threads (infrun_thread_stop_requested_callback, &ptid);
>>  }
>> @@ -4051,6 +4044,9 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
>>  	       that this operation also cleans up the child process for vfork,
>>  	       because their pages are shared.  */
>>  	    displaced_step_fixup (ecs->ptid, GDB_SIGNAL_TRAP);
>> +	    /* Start a new step-over in another thread if there's one
>> +	       that needs it.  */
>> +	    start_step_over ();
> 
> The comment is confusing to me, especially the "one" and the "it".  Do
> you mean "in another thread if there is one thread that needs step-over"?
> 
>> @@ -323,6 +403,10 @@ delete_thread_1 (ptid_t ptid, int silent)
>>    if (!tp)
>>      return;
>>  
>> +  /* Dead threads don't need to step-over.  Remove from queue.  */
>> +  if (tp->step_over_next != NULL)
>> +    thread_step_over_chain_remove (tp);
>> +
> 
> I am wondering how this can happen?  A thread needs step-over becomes dead?

It can happen if the process exits or disappears (target is closed, etc.)
while the thread was waiting for its turn.

Thanks,
Pedro Alves