From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58388 invoked by alias); 17 Sep 2018 00:52:34 -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 58370 invoked by uid 89); 17 Sep 2018 00:52:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=Let X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Sep 2018 00:52:32 +0000 Received: by simark.ca (Postfix, from userid 112) id 68FB81E5A4; Sun, 16 Sep 2018 20:52:30 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1537145550; bh=PKSZiZY15uVHrc00vmAiCn3sXAGmq08NZXdY4VTFMU8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=UXGn0vt5QNnRxQ5gZWm0lH22Qb00SAZmFB/Xi0aI7wiJjvmG6m/OxpLAdEMLA63tx mHZkzO3UAOHIrRZHejuuGcGmP/ORmsV+KOzsXobeHTz7VwsovLBelop9zlZTcbVzfK w4dqUsAB6jav9KcyP6/20352V7qODWwV1/2Brvt8= Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 9401B1E175; Sun, 16 Sep 2018 20:52:29 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=simark.ca; s=mail; t=1537145549; bh=PKSZiZY15uVHrc00vmAiCn3sXAGmq08NZXdY4VTFMU8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=tjPkylaKzeSlJD2+PCxOC4SJfiwPqueJ8AefInM+7ivQfdGFD8+Mg3PDFUu+JCcxw HAe2Mf/uaQb9BeUbuR9KdXJwfP0EgdkG51wvzImaQca+dvpHU+/WXxExRY6kTb0G34 pUHqLR1Y2QEwyKvUaoz79Exez99tvSVC+tfyeZZg= MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 17 Sep 2018 00:52:00 -0000 From: Simon Marchi To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference In-Reply-To: <875zz5pmg8.fsf@tromey.com> References: <20180913053007.11780-1-tom@tromey.com> <20180913053007.11780-4-tom@tromey.com> <38dedd0d-1f03-eb7d-84a6-15cdc29e8ea2@simark.ca> <875zz5pmg8.fsf@tromey.com> Message-ID: <03a3606359da188a8100201c7dfae823@simark.ca> X-Sender: simark@simark.ca User-Agent: Roundcube Webmail/1.3.6 X-SW-Source: 2018-09/txt/msg00553.txt.bz2 On 2018-09-16 10:05, Tom Tromey wrote: >>>>>> "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. Oh right. While working on this code I though adding a GDB_Py_REF_RETURN_NONE macro, which would be like Py_RETURN_NONE but returning a gdbpy_ref. Not sure it's really that useful, since what you wrote is explicit and clear. > 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 am not sure how these two mode of failure differ. Don't we expect the passed thread_info object to always be valid, and therefore both cases returning NULL would be an internal GDB logic error? > I think it is best to have a single style - returning NULL should also > set the Python exception. That makes sense, so the caller can just check for NULL and return NULL right away (as the snippet above does). We should expect inferior_to_inferior_object to set the exception on failure. > It seems to me that the second NULL return should just set an > exception. It's not a normal case. Right. Responding directly to your next message: > Maybe the gdbpy_ref (1-argument) constructor and release methods could > assert that the Python exception is set if the underlying pointer is > NULL. That would not get full checking but maybe it would catch some > problems. And maybe we should simply use gdbpy_ref in many more places > in the Python layer -- ideally, reserve raw pointers solely for > parameters which are borrowed references. Makes sense. > Let me know what you think of this. > > My apologies again for missing this review and pushing too eagerly. No problem, LGTM. Simon