RE: [PATCH v5 3/5] gdb/remote: do not delete a thread if it has a pending event

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

On Thursday, April 16, 2020 6:24 PM, Pedro Alves wrote:
> On 4/6/20 4:45 PM, Tankut Baris Aktemur via Gdb-patches wrote:
> > gdb/ChangeLog:
> > 2020-04-06  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> >
> > 	* remote.c (remote_target::update_thread_list): Do not delete
> > 	a thread if it has a pending event.
> > ---
> >  gdb/remote.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/gdb/remote.c b/gdb/remote.c
> > index bfbc0bc21d3..12ac7cb9862 100644
> > --- a/gdb/remote.c
> > +++ b/gdb/remote.c
> > @@ -3821,6 +3821,9 @@ remote_target::update_thread_list ()
> >  	  if (tp->inf->process_target () != this)
> >  	    continue;
> >
> > +	  if (tp->suspend.waitstatus_pending_p)
> > +	    continue;
> > +
> 
> This patch makes me nervous.  :-/
> 
> Why doesn't prune_threads, via remote_target::thread_alive end
> up removing the thread anyway?

As far as I see, prune_threads is called only if gdbserver does not
send a thread list.  The comment in the function suggests that this
could be the case for older servers.  I'm not sure how old it should
be, though, for me to be able to test it.
 
> I think it'd be better to extend the check to also check for
> TARGET_WAITKIND_EXITED/TARGET_WAITKIND_SIGNALLED.  Let threads
> with other kinds of events pending be deleted.
> 
> Other targets are likely to need something like this, but I'm
> OK with going with remote-specific test to start with, so we
> can all this in, including the testcase, and then work on
> something more generic if we find a need.
> 
> In any case, please add some comments.

First I did this change, but when I started working on Patch #5, interesting
things have come up.

Having both of the inferiors on a single extended-remote target is supposed
to trigger the same stop-all-threads scenario that this series attempts to fix.
But it turned out to be slightly different.

The 'update_thread_list' at the beginning of the stop_all_threads pass
removes all the threads because the remote target sends an empty list of threads
(well both inferiors exited and no thread remained).  So, before we even attempt
to stop threads, they are already gone when the list is updated.  The problem is,
inferior 2 (i.e. the inferior whose thread we would attempt to stop but would
receive an EXITED waitstatus for) is then left in a seemingly live state (pid != 0),
but with no threads.  Hence, we cannot even switch to it after inferior 1's
exit event is shown.

Therefore I updated this patch as below.  Although no regression was detected,
I don't feel much safe with this new version.  Comments welcome.

Thanks,
-Baris

From f9c9a7c1bb15cb99925744e25026bd2b09b74194 Mon Sep 17 00:00:00 2001
From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Date: Mon, 6 Apr 2020 13:54:35 +0200
Subject: [PATCH 3/5] gdb/remote: do not delete a thread if this could leave
 its inferior empty

gdb/ChangeLog:
2020-04-06  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

        * remote.c (remote_target::update_thread_list): Do not delete
        a thread if this could leave its inferior empty.
---
 gdb/remote.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/gdb/remote.c b/gdb/remote.c
index 5db406e045c..681957c012b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3822,6 +3822,18 @@ remote_target::update_thread_list ()
          if (tp->inf->process_target () != this)
            continue;

+         /* Do not delete the thread if it belongs to another inferior
+            that has only one non-exited thread, because otherwise we
+            would end up with a seemingly live inferior with no threads.  */
+         if (tp->inf != current_inferior ())
+           {
+             int num_threads = 0;
+             for (thread_info *t ATTRIBUTE_UNUSED : tp->inf->non_exited_threads ())
+               num_threads++;
+             if (num_threads == 1)
+               continue;
+           }
+
          if (!context.contains_thread (tp->ptid))
            {
              /* Not found.  */
--
2.17.1

 


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