From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42610 invoked by alias); 16 Sep 2018 14:05:16 -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 42600 invoked by uid 89); 16 Sep 2018 14:05:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: gateway24.websitewelcome.com Received: from gateway24.websitewelcome.com (HELO gateway24.websitewelcome.com) (192.185.51.202) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 16 Sep 2018 14:05:14 +0000 Received: from cm14.websitewelcome.com (cm14.websitewelcome.com [100.42.49.7]) by gateway24.websitewelcome.com (Postfix) with ESMTP id AD636C5A50 for ; Sun, 16 Sep 2018 09:05:12 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id 1Xfkg6ycEkBj61XfkgHKGy; Sun, 16 Sep 2018 09:05:12 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=KmiOmw0F7KjeXVemTpDI5DUnnkp8ZC24iuI3AN4MHNw=; b=UrQVnR4wx0bLmukAcSZ+9zhATU isDjNQPQ1ODGXlS0InbTLs8MNwv4Ipjl5wgsMNBuwxRwPbw0WcKkFZ83NfRdkJ4+a3y2sixLaH4iH s7tJVM7TL4A0OCuAvoOpV8c2H; Received: from 97-122-190-66.hlrn.qwest.net ([97.122.190.66]:46386 helo=bapiya) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1g1Xfk-004Iht-F3; Sun, 16 Sep 2018 09:05:12 -0500 From: Tom Tromey To: Simon Marchi Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference References: <20180913053007.11780-1-tom@tromey.com> <20180913053007.11780-4-tom@tromey.com> <38dedd0d-1f03-eb7d-84a6-15cdc29e8ea2@simark.ca> Date: Sun, 16 Sep 2018 14:05:00 -0000 In-Reply-To: <38dedd0d-1f03-eb7d-84a6-15cdc29e8ea2@simark.ca> (Simon Marchi's message of "Sat, 15 Sep 2018 22:11:06 -0400") Message-ID: <875zz5pmg8.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2018-09/txt/msg00543.txt.bz2 >>>>> "Simon" == Simon Marchi writes: >> + if (result == NULL) >> + result = gdbpy_ref<>::new_reference (Py_None); Simon> I would suggest using Py_RETURN_NONE, which we already use at many places. Simon> Instead of setting that result variable, is there something preventing you Simon> from returning directly above, like this? Simon> if (thread_info != NULL) Simon> return thread_to_thread_object (thread_info).release (); Simon> The bottom line could just be unconditionally Py_RETURN_NONE. I made this change. In the past you couldn't return from inside a try/catch but I think it is ok now. I couldn't use Py_RETURN_NONE in py_get_event_thread because it returns a gdbpy_ref<> -- and I think the latter is more important. I re-read all of this and I think thread_to_thread_object has a latent bug. It can return NULL early due to an error: gdbpy_ref inf_obj (inferior_to_inferior_object (thr->inf)); if (inf_obj == NULL) return NULL; or it can return NULL at the end meaning "thread not found". I think it is best to have a single style - returning NULL should also set the Python exception. It seems to me that the second NULL return should just set an exception. It's not a normal case. Let me know what you think of this. My apologies again for missing this review and pushing too eagerly. Tom commit 0a58a05fb6574c0e9c84c5160e99f5c922a6a0eb Author: Tom Tromey Date: Sun Sep 16 08:02:22 2018 -0600 Simplify uses of thread_to_thread_object An review by Simon of an earlier showed a few spots related to thread_to_thread_object that could be simplified. This also detected a latent bug, where thread_to_thread_object was inconsistent about setting the Python exception before a NULL return. Tested on x86-64 Fedora 28. gdb/ChangeLog 2018-09-16 Tom Tromey * python/py-threadevent.c (py_get_event_thread): Simplify. * python/py-inferior.c (infpy_thread_from_thread_handle): Return immediately after calling thread_to_thread_object. Use Py_RETURN_NONE. (thread_to_thread_object): Set the exception on a NULL return. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index d36f6cde426..492fcad4120 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,11 @@ +2018-09-16 Tom Tromey + + * python/py-threadevent.c (py_get_event_thread): Simplify. + * python/py-inferior.c (infpy_thread_from_thread_handle): + Return immediately after calling thread_to_thread_object. Use + Py_RETURN_NONE. + (thread_to_thread_object): Set the exception on a NULL return. + 2018-09-16 Tom Tromey * python/python-internal.h (CPYCHECKER_RETURNS_BORROWED_REF): diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c index 68c4c9d411e..145f53d14af 100644 --- a/gdb/python/py-inferior.c +++ b/gdb/python/py-inferior.c @@ -319,6 +319,8 @@ thread_to_thread_object (thread_info *thr) if (thread->thread_obj->thread == thr) return gdbpy_ref<>::new_reference ((PyObject *) thread->thread_obj); + PyErr_SetString (PyExc_SystemError, + _("could not find gdb thread object")); return NULL; } @@ -849,7 +851,6 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw) return NULL; } - gdbpy_ref<> result; TRY { struct thread_info *thread_info; @@ -857,7 +858,7 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw) thread_info = find_thread_by_handle (val, inf_obj->inferior); if (thread_info != NULL) - result = thread_to_thread_object (thread_info); + return thread_to_thread_object (thread_info).release (); } CATCH (except, RETURN_MASK_ALL) { @@ -865,10 +866,7 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw) } END_CATCH - if (result == NULL) - result = gdbpy_ref<>::new_reference (Py_None); - - return result.release (); + Py_RETURN_NONE; } /* Implement repr() for gdb.Inferior. */ diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c index 13af1c840ba..ea62540ff84 100644 --- a/gdb/python/py-threadevent.c +++ b/gdb/python/py-threadevent.c @@ -25,24 +25,15 @@ gdbpy_ref<> py_get_event_thread (ptid_t ptid) { - gdbpy_ref<> pythread; - if (non_stop) { thread_info *thread = find_thread_ptid (ptid); if (thread != nullptr) - pythread = thread_to_thread_object (thread); - } - else - pythread = gdbpy_ref<>::new_reference (Py_None); - - if (pythread == nullptr) - { + return thread_to_thread_object (thread); PyErr_SetString (PyExc_RuntimeError, "Could not find event thread"); return NULL; } - - return pythread; + return gdbpy_ref<>::new_reference (Py_None); } gdbpy_ref<>