Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Pedro Alves <palves at redhat dot com>]

On 04/05/2017 10:15 PM, Yao Qi wrote:

> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -183,6 +183,27 @@ public:
>    explicit thread_info (inferior *inf, ptid_t ptid);
>    ~thread_info ();
>  
> +  bool deletable ()

Could be const:

  bool deletable () const

> +  {
> +    /* If this is the current thread, or there's code out there that
> +       relies on it existing (refcount > 0) we can't delete yet.  */
> +    return (refcount == 0 && !ptid_equal (ptid, inferior_ptid));
> +  }
> +
> +  /* Increase the refcount.  */
> +  void inc_refcount ()
> +  {
> +    gdb_assert (refcount >= 0);
> +    refcount++;
> +  }
> +
> +  /* Decrease the refcount.  */
> +  void dec_refcount ()
> +  {
> +    refcount--;
> +    gdb_assert (refcount >= 0);
> +  }

Nit: It's SO common to call this sort of methods "incref()" and
"decref()" that anything else looks a bit odd to me.

>    struct thread_info *step_over_prev = NULL;
>    struct thread_info *step_over_next = NULL;
> +
> +private:
> +
> +  /* If this is > 0, then it means there's code out there that relies
> +     on this thread being listed.  Don't delete it from the lists even
> +     if we detect it exiting.  */
> +  int refcount = 0;

Since this is now private, I think we should give it an "m_" prefix.

>  };
>  

>  
> +/* Set the TP's state as exited.  */
> +
> +static void
> +set_thread_exited (struct thread_info *tp, int silent)

Drop "struct" ?

>  static void
>  do_restore_current_thread_cleanup (void *arg)
>  {
> -  struct thread_info *tp;
>    struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg;
>  
> -  tp = find_thread_ptid (old->inferior_ptid);
> -
> -  /* If the previously selected thread belonged to a process that has
> -     in the mean time been deleted (due to normal exit, detach, etc.),
> -     then don't revert back to it, but instead simply drop back to no
> -     thread selected.  */

This comment still makes sense, with a small tweak -- saying "deleted"
has always been a bit misleading here:

  /* If the previously selected thread belonged to a process that has
     in the mean time exited (or killed, detached, etc.), then don't revert
     back to it, but instead simply drop back to no thread selected.  */

I'll be happy with restoring this comment alongside your new comment.

The patch will LGTM with the nits/comments up to here addressed.

The rest of the review comments below could be addressed separately
(by anyone, not necessarily you), I'm just putting them out as
I thought of them.  I do think we should do it to avoid
hard-to-debug corner cases around find_inferior_ptid finding
an unrelated process that reused the old inferior's pid.

> -  if (tp
> -      && find_inferior_ptid (tp->ptid) != NULL)
> -    restore_current_thread (old->inferior_ptid);
> +  /* If an entry of thread_info was previously selected, it won't be
> +     deleted because we've increased its refcount.  The thread represented
> +     by this thread_info entry may have already exited (due to normal exit,
> +     detach, etc), so the thread_info.state is THREAD_EXITED.  */
> +  if (old->thread != NULL
> +      && find_inferior_ptid (old->thread->ptid) != NULL)
> +    restore_current_thread (old->thread->ptid);

Note this was a look up by inferior ptid, not by (stable) inferior number,
so we can genuinely find no inferior, even though the old inferior _object_
will always be around when we get here, given that we mark it non-removable
while the cleanup is installed [1].  Quite similar to increasing the
thread's refcount, really.

So this predicate would probably be better as:

   if (old->thread != NULL
      && old->thread != THREAD_EXITED
      && find_inferior_id (old->inf_id)->pid != 0)

We could also store the inferior pointer in "old" directly,
instead of the id, sparing the inferior look up:

   if (old->thread != NULL
      && old->thread != THREAD_EXITED
      && old->inf->pid != 0)


[1] - We should probably have a test that does something like:

define kill-and-remove
  kill inferiors 2
  remove-inferiors 2
end

# Start one inferior.
start

# Start another inferior.
add-inferior 2
inferior 2
start

# Kill and remove inferior 1 while thread 2.1 / inferior 2 is selected.
thread apply 1.1 kill-and-remove

Thanks,
Pedro Alves