This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PATCH v2] Fix 8.2 regression in gdb.python/py-evthreads.exp w/ gdbserver (PR gdb/23379)
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 <palves@redhat.com>
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 <continue_event_object_type>, 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<void (ptid_t), void (*)(ptid_t)>::_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<void (ptid_t)>::operator()(ptid_t) const (this=0x12aa600, __args#0=...)
at /usr/include/c++/7/bits/std_function.h:706
#9 0x000000000089aa0e in gdb::observers::observable<ptid_t>::notify(ptid_t) const (this=0x118a7a0 <gdb::observers::target_resumed>, 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 <palves@redhat.com>
Simon Marchi <simon.marchi@ericsson.com>
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)
--
2.14.4