[PATCH v2] gdb/Python: Added ThreadExitedEvent

[PATCH v2] gdb/Python: Added ThreadExitedEvent

[Simon Farre]

Currently no event is emitted for a thread exit.

This adds this functionality by emitting a new gdb.ThreadExitedEvent.

It currently provides three attributes, the LWP id, the TID and
the GLOBAL NUM. A case could be made to also add the per-inferior
number, but, I wasn't sure if it is actually any useful.

Added info to docs & the NEWS file as well.

Added test to test suite.

Fixed formatting.

Feedback wanted and appreciated.
---
 gdb/NEWS                                      |  3 ++
 gdb/doc/python.texi                           | 16 ++++++
 gdb/python/py-all-events.def                  |  1 +
 gdb/python/py-event-types.def                 |  5 ++
 gdb/python/py-event.h                         |  3 ++
 gdb/python/py-inferior.c                      |  7 +++
 gdb/python/py-threadevent.c                   | 22 ++++++++
 gdb/testsuite/gdb.python/py-thread-exited.c   | 41 +++++++++++++++
 gdb/testsuite/gdb.python/py-thread-exited.exp | 52 +++++++++++++++++++
 gdb/testsuite/gdb.python/py-thread-exited.py  | 31 +++++++++++
 10 files changed, 181 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-thread-exited.c
 create mode 100644 gdb/testsuite/gdb.python/py-thread-exited.exp
 create mode 100644 gdb/testsuite/gdb.python/py-thread-exited.py

diff --git a/gdb/NEWS b/gdb/NEWS
index 760cb2b7abc..985121d5818 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -38,6 +38,9 @@ maintenance info line-table
      This is the same format that GDB uses when printing address, symbol,
      and offset information from the disassembler.
 
+  ** gdb.ThreadExitedEvent added. Emits LWP ID, TID and GLOBAL NUM of the
+     exiting thread.
+
 *** Changes in GDB 12
 
 * DBX mode is deprecated, and will be removed in GDB 13
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 7c414b01d70..9a410dc18f8 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3568,6 +3568,22 @@ This has a single attribute:
 The new thread.
 @end defvar
 
+@item events.thread_exited
+This is emitted when @value{GDBN} notices a thread has exited.  The event
+is of type @code{gdb.ThreadExitedEvent}.  This has three attributes:
+
+@defvar ThreadExitedEvent.num
+Global thread number.
+@end defvar
+
+@defvar ThreadExitedEvent.lwp
+The light weight process ID.
+@end defvar
+
+@defvar ThreadExitedEvent.tid
+The thread's thread id
+@end defvar
+
 @item events.gdb_exiting
 This is emitted when @value{GDBN} exits.  This event is not emitted if
 @value{GDBN} exits as a result of an internal error, or after an
diff --git a/gdb/python/py-all-events.def b/gdb/python/py-all-events.def
index 7db8efa1390..0dd1a295135 100644
--- a/gdb/python/py-all-events.def
+++ b/gdb/python/py-all-events.def
@@ -31,6 +31,7 @@ GDB_PY_DEFINE_EVENT(clear_objfiles)
 GDB_PY_DEFINE_EVENT(new_inferior)
 GDB_PY_DEFINE_EVENT(inferior_deleted)
 GDB_PY_DEFINE_EVENT(new_thread)
+GDB_PY_DEFINE_EVENT(thread_exited)
 GDB_PY_DEFINE_EVENT(inferior_call)
 GDB_PY_DEFINE_EVENT(memory_changed)
 GDB_PY_DEFINE_EVENT(register_changed)
diff --git a/gdb/python/py-event-types.def b/gdb/python/py-event-types.def
index 596e68a852a..4529ddc7caa 100644
--- a/gdb/python/py-event-types.def
+++ b/gdb/python/py-event-types.def
@@ -51,6 +51,11 @@ GDB_PY_DEFINE_EVENT_TYPE (new_thread,
 			  "GDB new thread event object",
 			  thread_event_object_type);
 
+GDB_PY_DEFINE_EVENT_TYPE (thread_exited,
+			  "ThreadExitedEvent",
+			  "GDB thread exited event object",
+			  event_object_type);
+
 GDB_PY_DEFINE_EVENT_TYPE (new_inferior,
 			  "NewInferiorEvent",
 			  "GDB new inferior event object",
diff --git a/gdb/python/py-event.h b/gdb/python/py-event.h
index 56e53b7a1e7..2274c63b0b8 100644
--- a/gdb/python/py-event.h
+++ b/gdb/python/py-event.h
@@ -61,6 +61,9 @@ extern int emit_memory_changed_event (CORE_ADDR addr, ssize_t len);
 extern int evpy_emit_event (PyObject *event,
 			    eventregistry_object *registry);
 
+/* Emits a thread exit event for thread with PTID and GLOBAL_NUM */
+extern int emit_thread_exit_event (ptid_t ptid, int global_num);
+
 extern gdbpy_ref<> create_event_object (PyTypeObject *py_type);
 
 /* thread events can either be thread specific or process wide.  If gdb is
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index ebcd5b0a70f..407e1bae4a3 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -363,6 +363,13 @@ delete_thread_object (struct thread_info *tp, int ignore)
     return;
 
   tmp = *entry;
+  if (!evregpy_no_listeners_p (gdb_py_events.thread_exited))
+    {
+      ptid_t ptid = tmp->thread_obj->thread->ptid;
+      int global_num = tmp->thread_obj->thread->global_num;
+      if (emit_thread_exit_event (ptid, global_num) < 0)
+	gdbpy_print_stack();
+    }
   tmp->thread_obj->thread = NULL;
 
   *entry = (*entry)->next;
diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c
index 0a5d30087fb..8cb1a187f12 100644
--- a/gdb/python/py-threadevent.c
+++ b/gdb/python/py-threadevent.c
@@ -54,3 +54,25 @@ create_thread_event_object (PyTypeObject *py_type, PyObject *thread)
 
   return thread_event_obj;
 }
+
+int
+emit_thread_exit_event (ptid_t ptid, int global_num)
+{
+  gdbpy_ref<> thread_event_obj = create_event_object (&thread_exited_event_object_type);
+  if (thread_event_obj == NULL)
+    return -1;
+
+  if (evpy_add_attribute (thread_event_obj.get (), "num",
+			  PyLong_FromLong (global_num)) < 0)
+    return -1;
+  long lwp = ptid.lwp();
+  if (evpy_add_attribute (thread_event_obj.get (), "lwp",
+			  PyLong_FromLong (lwp)) < 0)
+    return -1;
+  int tid = ptid.tid ();
+  if (evpy_add_attribute (thread_event_obj.get (),
+			  "tid",
+			  PyLong_FromLong (tid)) < 0)
+    return -1;
+  return evpy_emit_event (thread_event_obj.get (), gdb_py_events.thread_exited);
+}
diff --git a/gdb/testsuite/gdb.python/py-thread-exited.c b/gdb/testsuite/gdb.python/py-thread-exited.c
new file mode 100644
index 00000000000..4a12e584f9c
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-thread-exited.c
@@ -0,0 +1,41 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010-2022 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see  <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <signal.h>
+
+pthread_t thread2_id;
+pthread_t thread3_id;
+
+void* do_thread (void* d)
+{
+  return NULL;
+}
+
+int main (void)
+{
+  /* Use single line to not to race whether `thread2' breakpoint or `next' over
+     pthread_create will stop first.  */
+
+  pthread_create (&thread2_id, NULL, do_thread, NULL);
+  pthread_join (thread2_id, NULL);
+  pthread_create (&thread3_id, NULL, do_thread, NULL);
+  pthread_join (thread3_id, NULL);
+  return 12;
+}
diff --git a/gdb/testsuite/gdb.python/py-thread-exited.exp b/gdb/testsuite/gdb.python/py-thread-exited.exp
new file mode 100644
index 00000000000..90f07e8b3b5
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-thread-exited.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2022-2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if { ![support_displaced_stepping] } {
+    unsupported "displaced stepping"
+    return -1
+}
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    return -1
+}
+
+save_vars { GDBFLAGS } {
+    append GDBFLAGS " -ex \"set non-stop on\""
+    clean_restart $testfile
+}
+
+if { [skip_python_tests] } { continue }
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/py-thread-exited.py]
+gdb_test_no_output "source ${pyfile}" "load python file"
+
+gdb_test "test-events" "Event testers registered."
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint 40 "last of main"
+
+gdb_continue_to_breakpoint "continue to breakpoint"
+
+gdb_test "python print(threadOneExit)" \
+	 ".*event type: thread-exited. global num: 2.*"
+gdb_test "python print(threadTwoExit)" \
+	 ".*event type: thread-exited. global num: 3.*"
diff --git a/gdb/testsuite/gdb.python/py-thread-exited.py b/gdb/testsuite/gdb.python/py-thread-exited.py
new file mode 100644
index 00000000000..ebd6532b158
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-thread-exited.py
@@ -0,0 +1,31 @@
+import gdb
+
+threadOneExit = ""
+threadTwoExit = ""
+# we don't want to overwrite the 2nd thread's exit event, thus
+# store it here. we don't care about it though.
+mainThreadExit = ""
+
+def thread_exited_handler(event):
+    global threadOneExit, threadTwoExit, mainThreadExit
+    print("{}".format(event))
+    assert isinstance(event, gdb.ThreadExitedEvent)
+    if threadOneExit == "":
+        threadOneExit = "event type: thread-exited. global num: %s" % event.num
+    else:
+        if threadTwoExit == "":
+            threadTwoExit = "event type: thread-exited. global num: %s" % event.num
+        else:
+            mainThreadExit = "event type: thread-exited. global num: %s" % event.num
+
+class test_events(gdb.Command):
+    """Test events."""
+
+    def __init__(self):
+        gdb.Command.__init__(self, "test-events", gdb.COMMAND_STACK)
+
+    def invoke(self, arg, from_tty):
+        gdb.events.thread_exited.connect(thread_exited_handler)
+        print("Event testers registered.")
+
+test_events()

base-commit: bd1c798f0aef38493c5292917e47f76e1205f4e3
-- 
2.32.0

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Currently no event is emitted for a thread exit.
Simon> This adds this functionality by emitting a new gdb.ThreadExitedEvent.

Simon> It currently provides three attributes, the LWP id, the TID and
Simon> the GLOBAL NUM. A case could be made to also add the per-inferior
Simon> number, but, I wasn't sure if it is actually any useful.

Simon> Added info to docs & the NEWS file as well.

Eli will have to review the docs, unless he already has.

Simon> +  if (!evregpy_no_listeners_p (gdb_py_events.thread_exited))
Simon> +    {
Simon> +      ptid_t ptid = tmp->thread_obj->thread->ptid;
Simon> +      int global_num = tmp->thread_obj->thread->global_num;
Simon> +      if (emit_thread_exit_event (ptid, global_num) < 0)
Simon> +	gdbpy_print_stack();

Space before open paren here.

Simon> +emit_thread_exit_event (ptid_t ptid, int global_num)
Simon> +{
Simon> +  gdbpy_ref<> thread_event_obj = create_event_object (&thread_exited_event_object_type);
Simon> +  if (thread_event_obj == NULL)

We're using 'nullptr' in new code.

I'm curious why the event has various attributes of the thread object
but not a reference to the thread object itself.  That seems a little
odd.

Simon> +    append GDBFLAGS " -ex \"set non-stop on\""

Is this really needed?

Otherwise, this patch looks good.  Thank you for doing this.

Tom

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Simon Farre]

Tom> Space before open paren here.
I'll go over and fix white spacing and formatting in the next patch version
(finally getting
the hang of it more or less).

Tom> I'm curious why the event has various attributes of the thread object
Tom> but not a reference to the thread object itself.  That seems a little
Tom> odd.

So my reasoning for this was; since the thread is exiting it is dying, and
from
a Python perspective I thought it proper not to hold on to any resources
that
GDB otherwise would dispose of. I'm open to suggestions for whether or not
this logic is sound. So if we are to return the thread object itself,
I would probably have to change the function from where the event is
emitted, or perhaps
some other change.

It's currently emitted inside `delete_thread_object` which is a function
that is attached
to the observer `gdb::observers::thread_exit` in py-inferior.c. So if we
are to return
a reference to the thread object, the function attached here should
probably be removed
entirely and replaced with just emitting the newly introduced "thread exit
event" (and handle
the de-allocation of the thread object inside the thread exit event type
tp_dealloc function).

Thanks for taking your time to review!
Simon F.

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Tom Tromey]

>>>>> Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

> So my reasoning for this was; since the thread is exiting it is dying,
> and from a Python perspective I thought it proper not to hold on to
> any resources that GDB otherwise would dispose of. I'm open to
> suggestions for whether or not this logic is sound.

gdb uses a maybe-weird approach to lifetime management.

Normally, a Python object isn't allowed to keep some underlying gdb
object alive.  So, if an inferior thread exits, the gdb data structures
will be cleaned up.  However, the InferiorThread wrapper object may live
on -- but now it is in a special invalid date.

This approach is why a bunch of types, like InferiorThread, have an
'is_valid' method.

So, it's fine to emit this event with the InferiorThread object.  If the
Python code capture it, that will be fine.  (Though of course this
probably won't normally happen anyway.)

gdb could probably be better about notifying Python about these
transitions to the invalid state.  I feel like there's some Python
protocol in this area that gdb should participate in but does not.
Like, it may be useful for using these objects as keys in a weak map.

> It's currently emitted inside `delete_thread_object` which is a
> function that is attached to the observer
> `gdb::observers::thread_exit` in py-inferior.c. So if we are to return
> a reference to the thread object, the function attached here should
> probably be removed entirely and replaced with just emitting the newly
> introduced "thread exit event" (and handle the de-allocation of the
> thread object inside the thread exit event type tp_dealloc function).

I think the current placement of the code is correct.  I'm not sure I
understand the tp_dealloc comment, but I don't think a change there is
needed.

Tom

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Eli Zaretskii]

> Date: Tue, 12 Apr 2022 11:03:15 +0200
> From: Simon Farre via Gdb-patches <gdb-patches@sourceware.org>
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 760cb2b7abc..985121d5818 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -38,6 +38,9 @@ maintenance info line-table
>       This is the same format that GDB uses when printing address, symbol,
>       and offset information from the disassembler.
>  
> +  ** gdb.ThreadExitedEvent added. Emits LWP ID, TID and GLOBAL NUM of the
> +     exiting thread.
> +
>  *** Changes in GDB 12

This part is OK.

> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 7c414b01d70..9a410dc18f8 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3568,6 +3568,22 @@ This has a single attribute:
>  The new thread.
>  @end defvar
>  
> +@item events.thread_exited
> +This is emitted when @value{GDBN} notices a thread has exited.  The event
> +is of type @code{gdb.ThreadExitedEvent}.  This has three attributes:
> +
> +@defvar ThreadExitedEvent.num
> +Global thread number.
> +@end defvar
> +
> +@defvar ThreadExitedEvent.lwp
> +The light weight process ID.
> +@end defvar
> +
> +@defvar ThreadExitedEvent.tid
> +The thread's thread id
> +@end defvar

This is also OK, but I wonder whether we should say something about
the lwp part that it isn't available on all platforms?

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Simon Farre]

I was unaware of the LWP not being cross-platform, perhaps this field
should be removed then?
Or is there some relevant/similar info on other platforms that can be
returned here (and by that
logic, also change the attribute to a name that logically could explain the
attribute in a cross-platform
manner?)

On Fri, Apr 15, 2022 at 9:46 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Tue, 12 Apr 2022 11:03:15 +0200
> > From: Simon Farre via Gdb-patches <gdb-patches@sourceware.org>
> >
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 760cb2b7abc..985121d5818 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -38,6 +38,9 @@ maintenance info line-table
> >       This is the same format that GDB uses when printing address,
> symbol,
> >       and offset information from the disassembler.
> >
> > +  ** gdb.ThreadExitedEvent added. Emits LWP ID, TID and GLOBAL NUM of
> the
> > +     exiting thread.
> > +
> >  *** Changes in GDB 12
>
> This part is OK.
>
> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > index 7c414b01d70..9a410dc18f8 100644
> > --- a/gdb/doc/python.texi
> > +++ b/gdb/doc/python.texi
> > @@ -3568,6 +3568,22 @@ This has a single attribute:
> >  The new thread.
> >  @end defvar
> >
> > +@item events.thread_exited
> > +This is emitted when @value{GDBN} notices a thread has exited.  The
> event
> > +is of type @code{gdb.ThreadExitedEvent}.  This has three attributes:
> > +
> > +@defvar ThreadExitedEvent.num
> > +Global thread number.
> > +@end defvar
> > +
> > +@defvar ThreadExitedEvent.lwp
> > +The light weight process ID.
> > +@end defvar
> > +
> > +@defvar ThreadExitedEvent.tid
> > +The thread's thread id
> > +@end defvar
>
> This is also OK, but I wonder whether we should say something about
> the lwp part that it isn't available on all platforms?
>

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Eli Zaretskii]

> From: Simon Farre <simon.farre.cx@gmail.com>
> Date: Mon, 18 Apr 2022 11:38:14 +0200
> Cc: Simon Farre via Gdb-patches <gdb-patches@sourceware.org>
> 
> I was unaware of the LWP not being cross-platform, perhaps this field should be removed then?
> Or is there some relevant/similar info on other platforms that can be returned here (and by that
> logic, also change the attribute to a name that logically could explain the attribute in a cross-platform
> manner?)

Sorry, I don't know.  I hope someone else will be able to answer your
questions.

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> I was unaware of the LWP not being cross-platform, perhaps this
Simon> field should be removed then?  Or is there some relevant/similar
Simon> info on other platforms that can be returned here (and by that
Simon> logic, also change the attribute to a name that logically could
Simon> explain the attribute in a cross-platform manner?)

Which fields in a ptid are used depends on the target.
There's been some attempt to regularize this across targets, but TBH I
don't know if that's been completed or not.  (And despite having done
some of this, I also forgot what the rule is :-)

Anyway I wouldn't worry about it.  Emitting the thread object alone
would probably be better.  And, if someone references the LWP and it
isn't set, it will just be 0.

Tom

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Pedro Alves]

On 2022-04-12 10:03, Simon Farre via Gdb-patches wrote:
>  
> +@item events.thread_exited
> +This is emitted when @value{GDBN} notices a thread has exited.  The event
> +is of type @code{gdb.ThreadExitedEvent}.  This has three attributes:
> +
> +@defvar ThreadExitedEvent.num
> +Global thread number.
> +@end defvar
> +
> +@defvar ThreadExitedEvent.lwp
> +The light weight process ID.
> +@end defvar
> +
> +@defvar ThreadExitedEvent.tid
> +The thread's thread id
> +@end defvar

Do other thread events have these same attributes?  It seems odd to print lwp and tid, but not pid.

But why not a ptid object instead?  InferiorThread has:


~~~
Variable: InferiorThread.ptid
ID of the thread, as assigned by the operating system. This attribute is a tuple containing three integers.
The first is the Process ID (PID); the second is the Lightweight Process ID (LWPID), and the third is the Thread ID (TID). 
Either the LWPID or TID may be 0, which indicates that the operating system does not use that identifier.
~~~

And other events use it too, like:

~~~
 Variable: InferiorCallPreEvent.ptid
 The thread in which the call will be run.

 gdb.InferiorCallPostEvent
 Indicates that a function in the inferior has just been called.

 Variable: InferiorCallPostEvent.ptid
~~~

I think not using ptid requires a strong justification.

But I also think that it might be useful to contrast the attributes included in the Python event to
what MI events include.

Re: [PATCH v2] gdb/Python: Added ThreadExitedEvent

[Simon Farre]

Pedro> Do other thread events have these same attributes?  It seems odd to
print lwp and tid, but not pid.
Pedro> But why not a ptid object instead?  InferiorThread has:

In the most recent patch I came to the same conclusion. It now returns the
same attributes
that InferiorThread holds (global_num, num, ptid and name - details was
left out of the v3 patch
as I could really get the reasoning for keeping it, but I am open for tips
& pointers)