From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53541 invoked by alias); 7 Apr 2017 10:29:35 -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 53512 invoked by uid 89); 7 Apr 2017 10:29:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=no version=3.3.2 spammy=gotten X-HELO: mail-wr0-f181.google.com Received: from mail-wr0-f181.google.com (HELO mail-wr0-f181.google.com) (209.85.128.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Apr 2017 10:29:33 +0000 Received: by mail-wr0-f181.google.com with SMTP id g19so57162243wrb.0 for ; Fri, 07 Apr 2017 03:29:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=eHlBfbPik9yJvIUQIdiptRq7SmqMT+FuexcceWN360g=; b=d9P35rrpag1RAMqIxf9tA+psvoM+cVBniP7kaCDizJhk1h4fE1M+Bii42PEOsT9zOn T/L0LOPD9PPhKCsBpMBp2hfjp+rnCD8t8RWRoQCjaeoTMGvmsYpsGmOTBMs7pRFSipOI L3/bcW6K4yKjQiniZS1UgiheprbwDHn0TgnaHAGM8qBZzaBbXQO9gyHV53nRU/V1XxHv 8or5sgt1i1czLojWvtXDoB3zs4uiXC4Kf6j8ww0GIxdUY2+z/TfAWHyG+csi82W143ou 8lcgK/Ncf6q4QKFm2JF6hlVSVKxq+vHsNUTwwcItHdkU0xKAEgrKLI0bCj34g5uFyKjv gr0w== X-Gm-Message-State: AFeK/H0ARHP/KjQUnWzRCBjiyufS2VKOmsLi8o1Zx6HTXt5PIsIe6qy7aAx4FeOTqRXKsVgQ X-Received: by 10.28.9.15 with SMTP id 15mr29430281wmj.83.1491560972379; Fri, 07 Apr 2017 03:29:32 -0700 (PDT) Received: from [192.168.0.101] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id s82sm3225282wmb.33.2017.04.07.03.29.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 07 Apr 2017 03:29:31 -0700 (PDT) Subject: Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero To: Yao Qi 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> <86a87sk1wz.fsf@gmail.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <228668fb-05be-3f9b-9290-c12059a25cab@redhat.com> Date: Fri, 07 Apr 2017 10:29: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: <86a87sk1wz.fsf@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00164.txt.bz2 On 04/07/2017 10:22 AM, Yao Qi wrote: > Pedro Alves writes: > >>> - 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 comments describe something different from what the code does. It doesn't. See below. > With > my patch, in make_cleanup_restore_current_thread, if we can find > thread_info by inferior_ptid, saved it in old->thread and increase the > refcount, so that it won't be deleted, but it can be marked as exited. > Then, in do_restore_current_thread_cleanup, > > + if (old->thread != NULL > + && find_inferior_ptid (old->thread->ptid) != NULL) > + restore_current_thread (old->thread->ptid); > else > { > restore_current_thread (null_ptid); > > the first line means we previously selected one existing thread, we > still switch to that thread, which may be already marked as exited. It > is nothing wrong as far as I can see. I'm not sure what you mean by "wrong". I'm saying that the old comment still makes sense. That comment is talking about the "find_inferior_ptid (old->thread->ptid) != NULL" line, which is a lookup for an _inferior_ not a thread. Both the inferior and thread_info object are still around, but the process may have exited/been detached meanwhile, and consequently the inferior's "pid" field is now zero. And in that case, we don't restore back the selected thread. E.g., with: start thread apply all detach #1 - the current thread (thread 1), has its refcount incremented. #2 - detach gets rid of all threads of the process. The "thread 1" object is left around, marked exited, because it was not deletable. #3 - when we try to restore the previous thread, its _inferior_ has meanwhile gotten its "pid" field zeroed, so a look up for an inferior with the pid of the previous thread's "ptid" (which just looks at the pid) won't find any inferior. (Unless it finds the wrong inferior that happened to reuse the pid meanwhile.) Hopefully that makes the rest of my comments in my previous message clearer. Thanks, Pedro Alves