This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, gdb-patches at sourceware dot org
- Date: Thu, 6 Apr 2017 11:18:27 +0100
- Subject: Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 310007D0C7
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 310007D0C7
- References: <1f525e52-f547-63ac-0a31-e92686c9caf8@redhat.com> <1491426942-6306-1-git-send-email-yao.qi@linaro.org> <1491426942-6306-3-git-send-email-yao.qi@linaro.org>
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