From patchwork Fri Aug 24 18:52:59 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 29041 Received: (qmail 125610 invoked by alias); 24 Aug 2018 18:53:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 125600 invoked by uid 89); 24 Aug 2018 18:53:36 -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, SPF_PASS autolearn=ham version=3.3.2 spammy=borrow X-HELO: sessmg23.ericsson.net Received: from sessmg23.ericsson.net (HELO sessmg23.ericsson.net) (193.180.251.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Aug 2018 18:53:33 +0000 DKIM-Signature: v=1; a=rsa-sha256; d=ericsson.com; s=mailgw201801; c=relaxed/simple; q=dns/txt; i=@ericsson.com; t=1535136810; h=From:Sender:Reply-To:Subject:Date:Message-ID:To:Cc:MIME-Version:Content-Type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=GxrRB+jxKh00oUbMG7mr6lnnjduEmB9XL6fTxKevfho=; b=fPuJSyzBjNJrEmMSoGuwOa9si/0pqXMRvbQZaqedAMHkeLN5USlmVxz/7vSxM8Kg bKxq+aWpkgrwl9uSNU14S6ZQB8gKiWiCMtcl00NQR7utbCUVwGmogKhM3kbUoxkL p5E6YlBuanL9jGYN3jDd4J2jXURXjOaNy+U0Hgtq8NA=; Received: from ESESSMB505.ericsson.se (Unknown_Domain [153.88.183.123]) by sessmg23.ericsson.net (Symantec Mail Security) with SMTP id 99.22.22015.A24508B5; Fri, 24 Aug 2018 20:53:30 +0200 (CEST) Received: from ESESSMB501.ericsson.se (153.88.183.162) by ESESSMB505.ericsson.se (153.88.183.166) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Fri, 24 Aug 2018 20:53:21 +0200 Received: from NAM04-CO1-obe.outbound.protection.outlook.com (153.88.183.157) by ESESSMB501.ericsson.se (153.88.183.162) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3 via Frontend Transport; Fri, 24 Aug 2018 20:53:21 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ericsson.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=sJxojBoYFlEEvf/j75jLY7GpRoohw5wt8dt7+3nirM4=; b=gRyKTbndP1Ffz+YVsTyl1Mp+P5LJQvyX6m7Go417wCkGKMs+7HVN8gLOgqT8dFCsNcu5EAKl7RF5Mg0D1NAFVxjUZV5foUODCxHhanLEamnbkx2H98h8eoSdOB+mq5jwes+gShfkkEAapeKRcmyQHrXJ4KZqo399sfWAqUbhe04= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=simon.marchi@ericsson.com; Received: from [142.133.48.188] (192.75.88.130) by DM6PR15MB2396.namprd15.prod.outlook.com (2603:10b6:5:8d::30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1080.16; Fri, 24 Aug 2018 18:53:17 +0000 Subject: Re: [PATCH] Fix 8.2 regression in gdb.python/py-evthreads.exp w/ gdbserver (PR gdb/23379) To: Pedro Alves , References: <20180824174616.3306-1-palves@redhat.com> From: Simon Marchi Message-ID: <81f69245-7bad-a9f8-58ef-7f8ead16e71f@ericsson.com> Date: Fri, 24 Aug 2018 14:52:59 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180824174616.3306-1-palves@redhat.com> Return-Path: simon.marchi@ericsson.com Received-SPF: None (protection.outlook.com: ericsson.com does not designate permitted sender hosts) X-IsSubscribed: yes On 2018-08-24 01:46 PM, Pedro Alves wrote: > This commit fixes a 8.1->8.2 regression exposed by > gdb.python/py-evthreads.exp when testing with > --target_board=native-gdbserver. > > gdb.log shows: > > src/gdb/thread.c:93: internal-error: thread_info* inferior_thread(): Assertion `tp' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) FAIL: gdb.python/py-evthreads.exp: run to breakpoint 1 (GDB internal error) > > A backtrace shows (frames #2 and #10 highlighted) that the assertion > fails when GDB is setting up the connection to the remote target, in > non-stop mode: > > #0 0x0000000000622ff0 in internal_error(char const*, int, char const*, ...) (file=0xc1ad98 "src/gdb/thread.c", line=93, fmt=0xc1ad20 "%s: Assertion `%s' failed.") at src/gdb/common/errors.c:54 > #1 0x000000000089567e in inferior_thread() () at src/gdb/thread.c:93 > = #2 0x00000000004da91d in get_event_thread() () at src/gdb/python/py-threadevent.c:38 > #3 0x00000000004da9b7 in create_thread_event_object(_typeobject*, _object*) (py_type=0x11574c0 , thread=0x0) > at src/gdb/python/py-threadevent.c:60 > #4 0x00000000004bf6fe in create_continue_event_object() () at src/gdb/python/py-continueevent.c:27 > #5 0x00000000004bf738 in emit_continue_event(ptid_t) (ptid=...) at src/gdb/python/py-continueevent.c:40 > #6 0x00000000004c7d47 in python_on_resume(ptid_t) (ptid=...) at src/gdb/python/py-inferior.c:108 > #7 0x0000000000485bfb in std::_Function_handler::_M_invoke(std::_Any_data const&, ptid_t&&) (__functor=..., __args#0=...) at /usr/include/c++/7/bits/std_function.h:316 > #8 0x000000000089b416 in std::function::operator()(ptid_t) const (this=0x12aa600, __args#0=...) > at /usr/include/c++/7/bits/std_function.h:706 > #9 0x000000000089aa0e in gdb::observers::observable::notify(ptid_t) const (this=0x118a7a0 , args#0=...) > at src/gdb/common/observable.h:106 > = #10 0x0000000000896fbe in set_running(ptid_t, int) (ptid=..., running=1) at src/gdb/thread.c:880 > #11 0x00000000007f750f in remote_target::remote_add_thread(ptid_t, bool, bool) (this=0x12c5440, ptid=..., running=true, executing=true) at src/gdb/remote.c:2434 > #12 0x00000000007f779d in remote_target::remote_notice_new_inferior(ptid_t, int) (this=0x12c5440, currthread=..., executing=1) > at src/gdb/remote.c:2515 > #13 0x00000000007f9c44 in remote_target::update_thread_list() (this=0x12c5440) at src/gdb/remote.c:3831 > #14 0x00000000007fb922 in remote_target::start_remote(int, int) (this=0x12c5440, from_tty=0, extended_p=0) > at src/gdb/remote.c:4655 > #15 0x00000000007fd102 in remote_target::open_1(char const*, int, int) (name=0x1a4f45e "localhost:2346", from_tty=0, extended_p=0) > at src/gdb/remote.c:5638 > #16 0x00000000007fbec1 in remote_target::open(char const*, int) (name=0x1a4f45e "localhost:2346", from_tty=0) > at src/gdb/remote.c:4862 > > So on frame #10, we're marking a newly-discovered thread as running, > and that causes the Python API to emit a gdb.ContinueEvent. > gdb.ContinueEvent is a gdb.ThreadEvent, and as such includes the event > thread as the "inferior_thread" attribute. The problem is that when > we get to frame #3/#4, we lost all references to the thread that is > being marked as running. create_continue_event_object assumes that it > is the current thread, which is not true in this case. > > Fix this by passing down the right thread in > create_continue_event_object. Also remove > create_thread_event_object's default argument and have the only other > caller left pass down the right thread explicitly too. > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves > > PR gdb/23379 > * python/py-continueevent.c: Include "gdbthread.h". > (create_continue_event_object): Add intro comment. Add 'ptid' > parameter. Use it to find thread to pass to > create_thread_event_object. > (emit_continue_event): Pass PTID down to > create_continue_event_object. > * python/py-event.h (py_get_event_thread): Declare. > (create_thread_event_object): Remove default from 'thread' > parameter. > * python/py-stopevent.c (create_stop_event_object): Use > py_get_event_thread. > * python/py-threadevent.c (get_event_thread): Rename to ... > (py_get_event_thread): ... this, and make extern. > (create_thread_event_object): Assert that THREAD isn't null. > Don't find the event thread here. > --- > gdb/python/py-continueevent.c | 31 ++++++++++++++++++++++++++++--- > gdb/python/py-event.h | 12 +++++++++++- > gdb/python/py-stopevent.c | 2 +- > gdb/python/py-threadevent.c | 22 +++++----------------- > 4 files changed, 45 insertions(+), 22 deletions(-) > > diff --git a/gdb/python/py-continueevent.c b/gdb/python/py-continueevent.c > index e5e922f0155..08d7da3cf2f 100644 > --- a/gdb/python/py-continueevent.c > +++ b/gdb/python/py-continueevent.c > @@ -20,11 +20,36 @@ > #include "defs.h" > #include "py-event.h" > #include "py-ref.h" > +#include "gdbthread.h" > + > +/* Create a gdb.ContinueEvent event. gdb.ContinueEvent is-a > + gdb.ThreadEvent, and thread events can either be thread specific or > + process wide. If gdb is running in non-stop mode then the event is > + thread specific (in which case the PTID thread is included in the > + event), otherwise it is process wide (in which case PTID is > + ignored). In either case a borrowed reference is returned. */ I think the last sentence is wrong. If we are returning a gdbpy_ref, doesn't mean we are returning an owning reference? From what I understand, a borrowed reference would be a naked pointer that you don't need to decref. py_get_event_thread, from which this seems to have been copied from, does indeed return a borrowed reference. So it should say "a new reference is returned". Ref: https://docs.python.org/3.7/c-api/intro.html, ctrl-f "borrow". > static gdbpy_ref<> > -create_continue_event_object (void) > +create_continue_event_object (ptid_t ptid) > { > - return create_thread_event_object (&continue_event_object_type); > + PyObject *py_thr = nullptr; > + > + if (non_stop) > + { > + thread_info *thr = find_thread_ptid (ptid); > + if (thr != nullptr) > + py_thr = (PyObject *) thread_to_thread_object (thr); > + } > + else > + py_thr = Py_None; > + > + if (py_thr == nullptr) > + { > + PyErr_SetString (PyExc_RuntimeError, _("Could not find event thread")); > + return nullptr; > + } > + > + return create_thread_event_object (&continue_event_object_type, py_thr); This and py_get_thread_event look very similar, could we use py_get_thread_event from here? See the patch at the end of the message. > } > > /* Callback function which notifies observers when a continue event occurs. > @@ -37,7 +62,7 @@ emit_continue_event (ptid_t ptid) > if (evregpy_no_listeners_p (gdb_py_events.cont)) > return 0; > > - gdbpy_ref<> event (create_continue_event_object ()); > + gdbpy_ref<> event (create_continue_event_object (ptid)); > if (event != NULL) > return evpy_emit_event (event.get (), gdb_py_events.cont); > return -1; > diff --git a/gdb/python/py-event.h b/gdb/python/py-event.h > index 22bab108ed1..7a9a51ddb1c 100644 > --- a/gdb/python/py-event.h > +++ b/gdb/python/py-event.h > @@ -63,8 +63,18 @@ extern int evpy_emit_event (PyObject *event, > eventregistry_object *registry); > > extern gdbpy_ref<> create_event_object (PyTypeObject *py_type); > + > +/* thread events can either be thread specific or process wide. If gdb is > + running in non-stop mode then the event is thread specific, otherwise > + it is process wide. > + This function returns the currently stopped thread in non-stop mode and > + Py_None otherwise. In each case it returns a borrowed reference. */ > +extern PyObject *py_get_event_thread () > + CPYCHECKER_RETURNS_BORROWED_REF; > + > extern gdbpy_ref<> create_thread_event_object (PyTypeObject *py_type, > - PyObject *thread = nullptr); > + PyObject *thread); > + > extern int emit_new_objfile_event (struct objfile *objfile); > extern int emit_clear_objfiles_event (void); > > diff --git a/gdb/python/py-stopevent.c b/gdb/python/py-stopevent.c > index 884e6e3ef8f..33b65f99b80 100644 > --- a/gdb/python/py-stopevent.c > +++ b/gdb/python/py-stopevent.c > @@ -23,7 +23,7 @@ > gdbpy_ref<> > create_stop_event_object (PyTypeObject *py_type) > { > - return create_thread_event_object (py_type); > + return create_thread_event_object (py_type, py_get_event_thread ()); > } > > /* Callback observers when a stop event occurs. This function will create a > diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c > index d6aa9467b9a..1a115bad801 100644 > --- a/gdb/python/py-threadevent.c > +++ b/gdb/python/py-threadevent.c > @@ -20,17 +20,10 @@ > #include "infrun.h" > #include "gdbthread.h" > > -/* thread events can either be thread specific or process wide. If gdb is > - running in non-stop mode then the event is thread specific, otherwise > - it is process wide. > - This function returns the currently stopped thread in non-stop mode and > - Py_None otherwise. In each case it returns a borrowed reference. */ > +/* See py-event.h. */ > > -static PyObject *get_event_thread (void) > - CPYCHECKER_RETURNS_BORROWED_REF; > - > -static PyObject * > -get_event_thread (void) > +PyObject * > +py_get_event_thread () > { > PyObject *thread; > > @@ -51,17 +44,12 @@ get_event_thread (void) > gdbpy_ref<> > create_thread_event_object (PyTypeObject *py_type, PyObject *thread) > { > + gdb_assert (thread != NULL); > + > gdbpy_ref<> thread_event_obj (create_event_object (py_type)); You mentioned at some point that you preferred gdbpy_ref<> thread_event_obj = create_event_object (py_type); over gdbpy_ref<> thread_event_obj (create_event_object (py_type)); (which I agree with). > if (thread_event_obj == NULL) > return NULL; > > - if (thread == NULL) > - { > - thread = get_event_thread (); > - if (!thread) > - return NULL; > - } > - > if (evpy_add_attribute (thread_event_obj.get (), > "inferior_thread", > thread) < 0) > Proposed changes: From 6b789cb151b5160fca3e3c90d47fb921e3d7697e Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Fri, 24 Aug 2018 14:26:23 -0400 Subject: [PATCH] Use py_get_event_thread in create_continue_event_object --- gdb/python/py-continueevent.c | 18 +++--------------- gdb/python/py-event.h | 2 +- gdb/python/py-stopevent.c | 3 ++- gdb/python/py-threadevent.c | 16 ++++++++++------ 4 files changed, 16 insertions(+), 23 deletions(-) diff --git a/gdb/python/py-continueevent.c b/gdb/python/py-continueevent.c index 08d7da3..c011b71 100644 --- a/gdb/python/py-continueevent.c +++ b/gdb/python/py-continueevent.c @@ -32,22 +32,10 @@ static gdbpy_ref<> create_continue_event_object (ptid_t ptid) { - PyObject *py_thr = nullptr; - - if (non_stop) - { - thread_info *thr = find_thread_ptid (ptid); - if (thr != nullptr) - py_thr = (PyObject *) thread_to_thread_object (thr); - } - else - py_thr = Py_None; + PyObject *py_thr = py_get_event_thread (ptid); if (py_thr == nullptr) - { - PyErr_SetString (PyExc_RuntimeError, _("Could not find event thread")); - return nullptr; - } + return nullptr; return create_thread_event_object (&continue_event_object_type, py_thr); } @@ -62,7 +50,7 @@ emit_continue_event (ptid_t ptid) if (evregpy_no_listeners_p (gdb_py_events.cont)) return 0; - gdbpy_ref<> event (create_continue_event_object (ptid)); + gdbpy_ref<> event = create_continue_event_object (ptid); if (event != NULL) return evpy_emit_event (event.get (), gdb_py_events.cont); return -1; diff --git a/gdb/python/py-event.h b/gdb/python/py-event.h index 7a9a51d..56003e8 100644 --- a/gdb/python/py-event.h +++ b/gdb/python/py-event.h @@ -69,7 +69,7 @@ extern gdbpy_ref<> create_event_object (PyTypeObject *py_type); it is process wide. This function returns the currently stopped thread in non-stop mode and Py_None otherwise. In each case it returns a borrowed reference. */ -extern PyObject *py_get_event_thread () +extern PyObject *py_get_event_thread (ptid_t ptid) CPYCHECKER_RETURNS_BORROWED_REF; extern gdbpy_ref<> create_thread_event_object (PyTypeObject *py_type, diff --git a/gdb/python/py-stopevent.c b/gdb/python/py-stopevent.c index 33b65f9..f6bd207 100644 --- a/gdb/python/py-stopevent.c +++ b/gdb/python/py-stopevent.c @@ -23,7 +23,8 @@ gdbpy_ref<> create_stop_event_object (PyTypeObject *py_type) { - return create_thread_event_object (py_type, py_get_event_thread ()); + return create_thread_event_object (py_type, + py_get_event_thread (inferior_ptid)); } /* Callback observers when a stop event occurs. This function will create a diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c index 1a115ba..1bc2fb0 100644 --- a/gdb/python/py-threadevent.c +++ b/gdb/python/py-threadevent.c @@ -23,22 +23,26 @@ /* See py-event.h. */ PyObject * -py_get_event_thread () +py_get_event_thread (ptid_t ptid) { - PyObject *thread; + PyObject *pythread; if (non_stop) - thread = (PyObject *) thread_to_thread_object (inferior_thread ()); + { + thread_info *thread = find_thread_ptid (ptid); + if (thread != nullptr) + pythread = (PyObject *) thread_to_thread_object (thread); + } else - thread = Py_None; + pythread = Py_None; - if (!thread) + if (!pythread) { PyErr_SetString (PyExc_RuntimeError, "Could not find event thread"); return NULL; } - return thread; + return pythread; } gdbpy_ref<>