From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73204 invoked by alias); 6 Apr 2017 10:18:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 73190 invoked by uid 89); 6 Apr 2017 10:18:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=genuinely, increasing, misleading X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Apr 2017 10:18:27 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 310007D0C7; Thu, 6 Apr 2017 10:18:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 310007D0C7 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@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 310007D0C7 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8C81881886; Thu, 6 Apr 2017 10:18:27 +0000 (UTC) Subject: Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero To: Yao Qi , gdb-patches@sourceware.org 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> From: Pedro Alves Message-ID: <9c8ed015-2eb0-9b5c-affc-b5ba59179a7f@redhat.com> Date: Thu, 06 Apr 2017 10:18:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1491426942-6306-3-git-send-email-yao.qi@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00137.txt.bz2 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