RE: [PATCH v7 5/5] gdb/infrun: handle already-exited threads when attempting to stop

Aktemur, Tankut Baris
2020-05-14T08:50:22Z

On Wednesday, May 13, 2020 11:15 PM, Pedro Alves wrote:
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index 167d50ff3ab..93169269553 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -4781,7 +4781,47 @@ stop_all_threads (void)
> > {
> > int need_wait = 0;
> >
> > - update_thread_list ();
> > + for (inferior *inf : all_non_exited_inferiors ())
> > + {
> > + update_thread_list ();
> > +
> > + /* After updating the thread list, it's possible to end
> > + up with pid != 0 but no threads, if the inf's process
> > + has exited but we have not processed that event yet.
> > + The exit event must be waiting somewhere in the queue
> > + to be processed. Silently add a thread so that we do
> > + a wait_one() below to pick the pending event. */
> > +
> > + bool has_threads = false;
> > + for (thread_info *tp ATTRIBUTE_UNUSED
> > + : inf->non_exited_threads ())
> > + {
> > + has_threads = true;
> > + break;
> > + }
> > +
> > + if (has_threads)
> > + continue;
> > +
> > + ptid_t ptid (inf->pid, inf->pid, 0);
> > +
>
> This here is what make me go think through all this. I don't really
> like it to have common code make up the ptid to use for the new thread.
> That should be the responsibility of the target backend. It may
> be that the target backend uses the tid field to store important
> information, for example.
>
> I also think that having to re-add a thread is not ideal.
> As we move GDB towards more multi-target support, and potentially other
> kinds of execution objects, I think that it's likely that we will always
> make it the responsibility of the target backend to create a thread, since
> it's going to the the target that knows the actual (C++) type of the thread
> Imagine target-specific classes that inherit from a common thread_info
> class, with virtual methods.
>
> After giving it some thought and experimentation, I think we should
> go back to your idea of not deleting the last thread of the process.
> But let's keep it simple without current_inferior() checks.
> When that solution was brought up before, I pointed at the code
> handle_no_resumed that handles the case of inferiors with no threads.
> I managed to reproduce that scenario, and confirm that we still
> handle it OK.
>
> I ended up squashing the remote and infrun changes in a single patch,
> as they're part of the same logical change.
>
> Finally, while playing with the testcases, I thought I'd make them
> spawn more inferiors, so that we're more sure we hit the race window.
> The way I've adjusted the testcases, it's simple to make them spawn
> any number of inferiors -- you just have to change one global.
>
> Please take a look and let me know what you think.

Thank you for checking and updating the broader impact! This revision looks
very good to me. I had minor comments about 3 patches and I emailed them.
I pushed the suggested changes to this branch for convenience:

https://github.com/barisaktemur/gdb/commits/thread-exit-in-stop-all-threads-v9

I repeated the regression testing with this branch, too.

Regards,
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928