Re: [PATCH] gdbserver: Set flag `suspended' properly for new lwp.

[Yao Qi <yao at codesourcery dot com>]

On 05/11/2012 04:18 AM, Pedro Alves wrote:
> This looks correct, but I had some trouble groking the comment.
> 
> I think doing it in this form makes it harder to understand and justify.
> 

Yeah, the comment is hard to read, and the comment you saw is the
version 3, version 1-2 are even worse :).

> I went to so many different strategies for this step-over/suspending
> mechanism, that I sometimes confuse myself on the properties of the
> version that I ended up with, but I think that nested stop_all_lwps
> where the inner stop_all_lwps starts before the outer stop_all_lwps
> finishes stopping all threads _can't_ happen nowadays.  (I had
> at one point tracepoints be collected immediately if a thread would
> hit a tracepoint while stop_all_threads was trying to stop it,
> leading to such scenarios, and it was hard to impossible to
> get right.)  In that case, checking event_child->suspended would be
> wrong, and we'd need to set new_lwp->suspended to the current
> nested level of suspension.  Ugh.

Do you think we need an assertion "stopping_threads ==
NOT_STOPPING_THREADS" at the beginning of stop_all_lwps?  This can
remind us if stop_all_lwps is called in a nested manner in the future.

> 
> But if nothing leads to a stop_all_lwps call while another is
> already in effect, then it is sufficient to know whether we're presently
> suspending threads, and if yes, it is right to set the new lwp's
> suspend count to 1 (instead of for example the suspend count of the
> event_child).
> 
> That is effectively what you have, but in a roundabout way, I think.

Yes, that is correct.

> 
> There's a simpler way.  We're already checking the `stopping_threads'
> global.  `stopping_threads' is exclusively set by stop_all_lwps.  So we
> _already_ use global state to let leaf code know what stop_all_lwps
> is going.  Which leads me to the variant of the patch below.
> 
> WDYT?

I like this approach.  I had a similar thought when working on the
patch, but discouraged by the comment on `stopping_threads',

  /* FIXME this is a bit of a hack, and could be removed.  */

so I give up on that way.

> @@ -3072,14 +3089,16 @@ lwp_running (struct inferior_list_entry *entry, void *data)
>  static void
>  stop_all_lwps (int suspend, struct lwp_info *except)
>  {
As I said above, do we need an assertion here,

     /* stop_all_lwps should not be called recursively.  */
     gdb_assert (stopping_threads == NOT_STOPPING_THREADS);

-- 
Yao (éå)