Re: [PATCHv3 1/3] gdb/python: introduce gdb.TargetConnection object type

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-10-19 06:17, Andrew Burgess wrote:
> This commit adds a new object type gdb.TargetConnection.  This new
> type represents a connection within GDB (a connection as displayed by
> 'info connections').
> 
> There's three ways to find a gdb.TargetConnection, there's a new
> 'gdb.connections()' function, which returns a list of all currently
> active connections.
> 
> Or you can read the new 'connection' property on the gdb.Inferior
> object type, this contains the connection for that inferior (or None
> if the inferior has no connection, for example, it is exited).
> 
> Finally, there's a new gdb.events.connection_removed event registry,
> this emits a new gdb.ConnectionEvent whenever a connection is removed
> from GDB (this happens when all inferiors using a connection exit).
> The gdb.ConnectionEvent has a 'connection' property, which is the
> gdb.TargetConnection being removed from GDB.
> 
> The gdb.TargetConnection has a 'is_valid()' method.  A connection
> object becomes invalid when the underlying connection is removed from
> GDB (all inferiors using the connection exit).
> 
> The gdb.TargetConnection has the following read-only properties:
> 
>   'num': The number for this connection,
> 
>   'type': e.g. 'native', 'remove', 'sim', etc
> 
>   'description': The longer description as seen in the 'info
>                  connections' command output.
> 
>   'details': A string or None.  Extra details for the connection, for
>              example, a remote connection's details might be
>              'hostname:port'.
> ---
>  gdb/Makefile.in                               |   1 +
>  gdb/NEWS                                      |  16 +
>  gdb/doc/python.texi                           |  92 ++++-
>  gdb/observable.c                              |   1 +
>  gdb/observable.h                              |   3 +
>  gdb/python/py-all-events.def                  |   1 +
>  gdb/python/py-connection.c                    | 366 ++++++++++++++++++
>  gdb/python/py-event-types.def                 |   5 +
>  gdb/python/py-inferior.c                      |  16 +
>  gdb/python/python-internal.h                  |   6 +
>  gdb/python/python.c                           |   5 +
>  gdb/target-connection.c                       |   4 +
>  .../gdb.multi/multi-target-info-inferiors.exp |  38 ++
>  .../gdb.multi/multi-target-info-inferiors.py  |  63 +++
>  gdb/testsuite/gdb.python/py-connection.c      |  22 ++
>  gdb/testsuite/gdb.python/py-connection.exp    |  63 +++
>  gdb/testsuite/gdb.python/py-inferior.exp      |  20 +-
>  17 files changed, 718 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/python/py-connection.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-target-info-inferiors.py
>  create mode 100644 gdb/testsuite/gdb.python/py-connection.c
>  create mode 100644 gdb/testsuite/gdb.python/py-connection.exp
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 4201f65e68d..5ec58aba439 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -392,6 +392,7 @@ SUBDIR_PYTHON_SRCS = \
>  	python/py-breakpoint.c \
>  	python/py-cmd.c \
>  	python/py-continueevent.c \
> +	python/py-connection.c \

This is not in alphabetical order :).

>  	python/py-event.c \
>  	python/py-evtregistry.c \
>  	python/py-evts.c \
> diff --git a/gdb/NEWS b/gdb/NEWS
> index bd26d2b1ec2..43b81df526d 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -45,6 +45,22 @@ maint show internal-warning backtrace
>       event is triggered once GDB decides it is going to exit, but
>       before GDB starts to clean up its internal state.
>  
> +  ** New gdb.TargetConnection object type that represents a connection
> +     (as displayed by the 'info connections' command).
> +
> +  ** The gdb.Inferior type now has a 'connection' property which is an
> +     instance of gdb.TargetConnection, the connection used by this
> +     inferior.  This can be None if the inferior has no connection
> +     (for example, when exited).

"when exited" is not completely accurate (see below, I wrote that other
comment before).

> +A @code{gdb.TargetConnection} has the following method:
> +
> +@defun TargetConnection.is_valid ()
> +Return @code{True} if the @code{gdb.TargetConnection} object is valid,
> +@code{False} if not.  A @code{gdb.TargetConnection} will become
> +invalid if the connection no longer exists within @value{GDBN}, this
> +will happen when all the inferiors using that connection exit.

I don't think the last part is completely accurate.  In some cases,
an inferior will keep its process target (and therefore keep the
connection alive) after exiting.  For instance, if you push the native
target explicitly, or if you connect using the extended-remote protocol.
You could say "this will happen when no inferior use the connection" or
something along those lines.

> +gdbpy_ref<>
> +target_to_connection_object (process_stratum_target *target)
> +{
> +  if (target == nullptr)
> +    return gdbpy_ref<>::new_reference (Py_None);
> +
> +  gdbpy_ref <connection_object> conn_obj = all_connection_objects[target];
> +  if (conn_obj == nullptr)
> +    {
> +      conn_obj.reset (PyObject_New (connection_object,
> +				    &connection_object_type));
> +      if (conn_obj == nullptr)
> +	return nullptr;
> +
> +      conn_obj->target = target;
> +
> +      /* PyObject_New initializes the new object with a refcount of 1.  This
> +	 counts for the reference we are keeping in the target_ops data.  */
> +      all_connection_objects[target] = conn_obj;

I find this comment more confusing than helpful.  The strong reference
created by PyObject_New belongs to conn_obj, which is then returned by
this function.  The strong reference belonging to the map (what I
presume you mean by "target_ops data") is created when copying conn_obj
into the map.  In the end all references are the same, I just find it a
bit confusing.

> +/* Callback for the connection_removed observer.  */
> +
> +static void
> +connpy_connection_removed (process_stratum_target *target)
> +{
> +  if (!gdb_python_initialized)
> +    return;
> +
> +  gdbpy_enter enter_py (get_current_arch (), current_language);
> +
> +  if (!evregpy_no_listeners_p (gdb_py_events.connection_removed))
> +    if (emit_connection_event (target, gdb_py_events.connection_removed) < 0)
> +      gdbpy_print_stack ();
> +
> +  gdbpy_ref <connection_object> conn_obj = all_connection_objects[target];
> +  if (conn_obj != nullptr)
> +    {
> +      conn_obj->target = nullptr;
> +      all_connection_objects.erase (target);
> +    }

Watch out with using operator[], that will insert an item for `target`
with value nullptr in the map, if there isn't one already.  Maybe it
doesn't matter in the end, but it brings the situation where a target
without a corresponding Python object can exist in two different states:
no entry in the map, and an entry with value nullptr.  I think it would
be easier to understand / less error prone if all entries in the map
had non-nullptr values, and targets without corresponding Python object
had no entries in the map.

> diff --git a/gdb/testsuite/gdb.python/py-connection.exp b/gdb/testsuite/gdb.python/py-connection.exp
> new file mode 100644
> index 00000000000..b994cec01eb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-connection.exp
> @@ -0,0 +1,63 @@
> +# Copyright (C) 2021 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/>.
> +
> +# This file is for testing the gdb.TargetConnection API.  This API is
> +# already tested in gdb.multi/multi-target-info-inferiors.exp and
> +# gdb.python/py-inferior.exp, this file just covers some edge cases
> +# that are not tested in other places.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +if ![runto_main] then {
> +    fail "can't run to main"
> +    return 0
> +}

Since 4dfef5be6812 ("gdb/testsuite: make runto_main not pass no-message
to runto"), you don't have to make an explicit fail when
runto_main fails.

> +
> +# Create a gdb.TargetConnection object and check it becomes invalid
> +# once the connection has gone away.
> +gdb_test_no_output "python conn = gdb.selected_inferior().connection"
> +gdb_test "python print(conn)" \
> +    "<gdb.TargetConnection num=1, what=\"\[^\"\]+\">" \
> +    "print gdb.TargetConnection while it is still valid"
> +gdb_test "python print(conn.is_valid())" "True" "is_valid returns True"
> +gdb_test "info connections" "\r\n\\* 1 .*" \
> +    "info connections while the connection is still around"
> +gdb_test "kill" "" "kill the inferior" \
> +    "Kill the program being debugged.*y or n. $" "y"
> +gdb_test "info connections" "No connections\\." \
> +    "info connections now all the connections have gone"

The above will fail with the native-extended-gdbserver board, as the
connection will not be gone.  You could maybe use "disconnect", which
should pop the process target, whatever it is.

Simon