From patchwork Fri Aug 24 19:44:13 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 29043 Received: (qmail 18169 invoked by alias); 24 Aug 2018 19:44:20 -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 18154 invoked by uid 89); 24 Aug 2018 19:44:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=blind X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Aug 2018 19:44:16 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1724E8780F; Fri, 24 Aug 2018 19:44:15 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5FDEA101041F; Fri, 24 Aug 2018 19:44:14 +0000 (UTC) Subject: [PATCH v2] Fix 8.2 regression in gdb.python/py-evthreads.exp w/ gdbserver (PR gdb/23379) To: Simon Marchi , gdb-patches@sourceware.org References: <20180824174616.3306-1-palves@redhat.com> <81f69245-7bad-a9f8-58ef-7f8ead16e71f@ericsson.com> From: Pedro Alves Message-ID: Date: Fri, 24 Aug 2018 20:44:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <81f69245-7bad-a9f8-58ef-7f8ead16e71f@ericsson.com> On 08/24/2018 07:52 PM, Simon Marchi wrote: > On 2018-08-24 01:46 PM, Pedro Alves wrote: > 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". Whoops, yes, blind copy&paste. Fixed. > 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. I like it! > >> } >> >> /* 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)); *see below* >> @@ -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). I guess you may have wanted to quote the other hunk above, since I didn't touch that line. Yeah, I definitely prefer copy-init when possible. In this case I'd prefer doing that as a separate patch though, because that's not something that the regression fix is adding, and also grepping around I see there's a lot of similar instances that can all get the same treatment. I'll post a patch for that as follow up. > Proposed changes: > Here's the two patches combined. Should I merge it? From aa0125e4c1fe4021aa4087cec5324ecc829c7fc3 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 24 Aug 2018 20:03:57 +0100 Subject: [PATCH] Fix 8.2 regression in gdb.python/py-evthreads.exp w/ gdbserver (PR gdb/23379) 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 Simon Marchi 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, make extern, add 'ptid' parameter and use it to find the thread. (create_thread_event_object): Assert that THREAD isn't null. Don't find the event thread here. --- gdb/python/py-continueevent.c | 19 ++++++++++++++++--- gdb/python/py-event.h | 12 +++++++++++- gdb/python/py-stopevent.c | 3 ++- gdb/python/py-threadevent.c | 36 ++++++++++++++---------------------- 4 files changed, 43 insertions(+), 27 deletions(-) diff --git a/gdb/python/py-continueevent.c b/gdb/python/py-continueevent.c index e5e922f0155..b047a83522b 100644 --- a/gdb/python/py-continueevent.c +++ b/gdb/python/py-continueevent.c @@ -20,11 +20,24 @@ #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 new reference is returned. */ 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 = py_get_event_thread (ptid); + + if (py_thr == nullptr) + return nullptr; + + return create_thread_event_object (&continue_event_object_type, py_thr); } /* Callback function which notifies observers when a continue event occurs. @@ -37,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 ()); + 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..56003e8785f 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 (ptid_t ptid) + 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..f6bd207f3b8 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); + 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 d6aa9467b9a..1bc2fb06d54 100644 --- a/gdb/python/py-threadevent.c +++ b/gdb/python/py-threadevent.c @@ -20,48 +20,40 @@ #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 (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<> create_thread_event_object (PyTypeObject *py_type, PyObject *thread) { + gdb_assert (thread != NULL); + gdbpy_ref<> thread_event_obj (create_event_object (py_type)); 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)