This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] gdbserver: Set flag `suspended' properly for new lwp.
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 (éå)