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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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