Re: [PATCH] gdb/Python: Added ThreadExitedEvent

[Tom de Vries <tdevries@suse.de>]

On 4/10/22 16:47, Simon Farre via Gdb-patches wrote:
> Currently no event is emitted for a thread exit.
> 
> This adds this functionality by emitting a new gdb.ThreadExitedEvent.
> 

I can't comment on the functionality, I just have looked over the test-case.

> 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.
> 
> Tests have been added to test the event. However, due to my inexperience
> with the testsuite programs I do not know how to make the test suite
> "wait" for a result. Because of the test produces intermittent failures
> because it executes statements in the .exp file twice.
> 
> If someone wants to chime in on how to fix this error, that would be
> nice!
> 

You're using gdb_run_cmd, which has this peculiarity (see proc comment 
in lib/gdb.exp):
...
# N.B. This function does not wait for gdb to return to the prompt, 

# that is the caller's responsibility. 
                       ...

So, something like this will consume the prompt:
...
gdb_test "" "" "wait for prompt"
...
[ First "": issue no command, just wait.  Second "": expect no output in 
particular. You can try to make that last one more specific if you like. ]

Usage of gdb_run_cmd is somewhat atypical, you could also use instead 
the more boilerplate:
...
if { ![runto_main] } {
    return -1
}
...
and then:
...
gdb_breakpoint 52 "last of main"
gdb_continue_to_breakpoint "continue to breakpoint"
...

Some nits in the test-case below...

> Added info to docs & the NEWS file as well.
> 
> 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   | 53 +++++++++++++++++++
>   gdb/testsuite/gdb.python/py-thread-exited.exp | 47 ++++++++++++++++
>   gdb/testsuite/gdb.python/py-thread-exited.py  | 31 +++++++++++
>   10 files changed, 188 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..6151fe39000 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..ff0e1e81c7e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-thread-exited.c
> @@ -0,0 +1,53 @@
> +/* 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;
> +}
> +
> +void* break_one()
> +{
> +  return NULL;
> +}
> +
> +void* break_two()
> +{
> +  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);
> +  break_one();
> +  pthread_create (&thread3_id, NULL, do_thread, NULL);
> +  pthread_join (thread3_id, NULL);
> +  break_two();
> +  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..ab96dbb1281
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-thread-exited.exp
> @@ -0,0 +1,47 @@
> +# Copyright (C) 2010-2022 Free Software Foundation, Inc.
> +

Why the copyright starting at 2010 ?

> +# 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."
> +gdb_breakpoint 52 "last of main"
> +gdb_run_cmd
> +
> +gdb_test "python print(threadOneExit)" \
> +".*event type: thread-exited. global num: 2.*"

This is bad formatting, you should use indentation to indicate that the 
second line goes with the first (for me, tab in emacs DDRT)

> +gdb_test "python print(threadTwoExit)" \
> +".*event type: thread-exited. global num: 3.*"

Same.

> +gdb_exit

I don't think that's required at the end of the test-case, you can drop 
this.

> \ No newline at end of file

This should be fixed, just add the newline.  Same thing in .py file.

Thanks,
- Tom

> 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..2fc979fab1e
> --- /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()
> \ No newline at end of file
> 
> base-commit: bd1c798f0aef38493c5292917e47f76e1205f4e3