From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37744 invoked by alias); 10 Apr 2017 15:57: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 36346 invoked by uid 89); 10 Apr 2017 15:57:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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; Mon, 10 Apr 2017 15:57:28 +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 6B7C864D89; Mon, 10 Apr 2017 15:57:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6B7C864D89 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6B7C864D89 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 C7DABA9D25; Mon, 10 Apr 2017 15:57: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> <9c8ed015-2eb0-9b5c-affc-b5ba59179a7f@redhat.com> From: Pedro Alves Message-ID: <492d5199-df50-9516-3781-12c319e48233@redhat.com> Date: Mon, 10 Apr 2017 15:57: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: <9c8ed015-2eb0-9b5c-affc-b5ba59179a7f@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00247.txt.bz2 I'm taking a look at adding the test mentioned below, and managed to trigger an internal error: src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed. A problem internal to GDB has been detected,KFAIL: gdb.threads/threadapply.exp: kill_and_remove: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error) I'll poke a bit more at it. Thanks, Pedro Alves On 04/06/2017 11:18 AM, Pedro Alves wrote: > 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