Re: [PATCH] Add __repr__() implementation to a few Python types

[Andrew Burgess <aburgess@redhat.com>]

Matheus Branco Borella via Gdb-patches <gdb-patches@sourceware.org>
writes:

> Only a few types in the Python API currently have __repr__() implementations.
> This patch adds a few more of them. specifically: it adds __repr__()
> implementations to gdb.Symbol, gdb.Architecture, gdb.Block, gdb.Breakpoint,
> and gdb.Type.
>
> This makes it easier to play around the GDB Python API in the Python interpreter
> session invoked with the 'pi' command in GDB, giving more easily accessible tipe
> information to users.
>
> An example of how this would look like:
> ```
> (gdb) pi
>>> gdb.lookup_type("char")
> <gdb.Type code=TYPE_CODE_INT name=char>
>>> gdb.lookup_global_symbol("main")
> <gdb.Symbol print_name=main>
> ```

Thanks for working on this, I think this will be really useful.

>
> One thing to note about this patch is that it makes use of u8 string literals,
> so as to make sure we meet python's expectations of strings passed to it using
> PyUnicode_FromFormat being encoded in utf8. This should remove the chance of
> odd compilation environments spitting out strings Python would consider invalid
> for the function we're calling.

Sorry for being a little slow.  What does this actually mean?  When you
say "makes use of u8 string literals" - does this mean you have string
literals in this patch containing non ASCII characters?

I've trying to understand why this is different to any other part of GDB
that prints stuff via Python.

> ---
>  gdb/python/py-arch.c                       | 18 +++++++-
>  gdb/python/py-block.c                      | 30 ++++++++++++-
>  gdb/python/py-breakpoint.c                 | 49 +++++++++++++++++++++-
>  gdb/python/py-symbol.c                     | 16 ++++++-
>  gdb/python/py-type.c                       | 31 +++++++++++++-
>  gdb/testsuite/gdb.python/py-arch.exp       |  4 ++
>  gdb/testsuite/gdb.python/py-block.exp      |  4 +-
>  gdb/testsuite/gdb.python/py-breakpoint.exp | 26 +++++++-----
>  gdb/testsuite/gdb.python/py-symbol.exp     |  1 +
>  gdb/testsuite/gdb.python/py-type.exp       |  4 ++
>  10 files changed, 165 insertions(+), 18 deletions(-)
>
> diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
> index cf0978560f9..5384a0d0d0c 100644
> --- a/gdb/python/py-arch.c
> +++ b/gdb/python/py-arch.c
> @@ -319,6 +319,22 @@ archpy_integer_type (PyObject *self, PyObject *args, PyObject *kw)
>    return type_to_type_object (type);
>  }
>  
> +/* __repr__ implementation for gdb.Architecture.  */
> +
> +static PyObject *
> +archpy_repr (PyObject *self)
> +{
> +  const auto gdbarch = arch_object_to_gdbarch (self);
> +  if (gdbarch == nullptr)
> +    return PyUnicode_FromFormat
> +      ("<gdb.Architecture (invalid)>");
> +
> +  return PyUnicode_FromFormat
> +    ("<gdb.Architecture arch_name=%s printable_name=%s>",
> +     gdbarch_bfd_arch_info (gdbarch)->arch_name,
> +     gdbarch_bfd_arch_info (gdbarch)->printable_name);
> +}
> +
>  /* Implementation of gdb.architecture_names().  Return a list of all the
>     BFD architecture names that GDB understands.  */
>  
> @@ -391,7 +407,7 @@ PyTypeObject arch_object_type = {
>    0,                                  /* tp_getattr */
>    0,                                  /* tp_setattr */
>    0,                                  /* tp_compare */
> -  0,                                  /* tp_repr */
> +  archpy_repr,                        /* tp_repr */
>    0,                                  /* tp_as_number */
>    0,                                  /* tp_as_sequence */
>    0,                                  /* tp_as_mapping */
> diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
> index b9aea3aca69..1b8433d41e7 100644
> --- a/gdb/python/py-block.c
> +++ b/gdb/python/py-block.c
> @@ -23,6 +23,7 @@
>  #include "symtab.h"
>  #include "python-internal.h"
>  #include "objfiles.h"
> +#include <sstream>
>  
>  struct block_object {
>    PyObject_HEAD
> @@ -424,6 +425,33 @@ blpy_iter_is_valid (PyObject *self, PyObject *args)
>    Py_RETURN_TRUE;
>  }
>  
> +/* __repr__ implementation for gdb.Block.  */
> +
> +static PyObject *
> +blpy_repr (PyObject *self)
> +{
> +  const auto block = block_object_to_block (self);
> +  if (block == nullptr)
> +    return PyUnicode_FromFormat("<gdb.Block (invalid)>");

Missing space before '('.  I think Bruno pointed out more of these, so
I'll not both pointing out the others I see.

> +
> +  const auto name = block->function () ? block->function ()->print_name () : "<anonymous>";

This line is too long and needs to be wrapped to under 80 characters.

> +
> +  block_iterator iter;
> +  block_iterator_first (block, &iter);
> +
> +  std::stringstream ss;
> +  const struct symbol *symbol;
> +  while ((symbol = block_iterator_next (&iter)) != nullptr)
> +  {
> +    ss << std::endl;
> +    ss << symbol->print_name () << ",";
> +  }
> +  if(!ss.str ().empty ())
> +    ss << std::endl;

We don't make much use of std::stringstream in GDB, and unless there's a
compelling reason, then for consistency, I'd prefer to see this written
using std::string.  While playing with this I rewrote this as:

  std::string str;
  const struct symbol *symbol;
  while ((symbol = block_iterator_next (&iter)) != nullptr)
    str = (str + "\n") + symbol->print_name () + ",";
  if(!str.empty ())
    str += "\n";

  return PyUnicode_FromFormat("<gdb.Block %s {%s}>", name, str.c_str ());

which still seems to pass your tests.

> +
> +  return PyUnicode_FromFormat("<gdb.Block %s {%s}>", name, ss.str ().c_str ());
> +}
> +
>  int
>  gdbpy_initialize_blocks (void)
>  {
> @@ -486,7 +514,7 @@ PyTypeObject block_object_type = {
>    0,				  /*tp_getattr*/
>    0,				  /*tp_setattr*/
>    0,				  /*tp_compare*/
> -  0,				  /*tp_repr*/
> +  blpy_repr,                     /*tp_repr*/
>    0,				  /*tp_as_number*/
>    0,				  /*tp_as_sequence*/
>    &block_object_as_mapping,	  /*tp_as_mapping*/
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index de7b9f4266b..ce307f7be21 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -33,6 +33,7 @@
>  #include "location.h"
>  #include "py-event.h"
>  #include "linespec.h"
> +#include <sstream>
>  
>  extern PyTypeObject breakpoint_location_object_type
>      CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("breakpoint_location_object");
> @@ -967,6 +968,23 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>    return 0;
>  }
>  
> +/* __repr__ implementation for gdb.Breakpoint.  */
> +
> +static PyObject *
> +bppy_repr(PyObject *self)
> +{
> +  const auto bp = (struct gdbpy_breakpoint_object*) self;
> +  if (bp->bp == nullptr)
> +    return PyUnicode_FromFormat ("<gdb.Breakpoint (invalid)>");
> +
> +  return PyUnicode_FromFormat
> +    ("<gdb.Breakpoint number=%d thread=%d hits=%d enable_count=%d>",
> +     bp->bp->number,
> +     bp->bp->thread,
> +     bp->bp->hit_count,
> +     bp->bp->enable_count);

I think I'd rather see the fields that are included here added
dynamically based on their values.  For example you choose to include
'thread' but not 'task' which seems pretty arbitrary.

Of the fields given here 'number' and 'hits' seem like they should
always be included, but I'd then only include 'thread' if it's not -1,
and 'task' if it's not 0, similarly, the 'enabled_count' is probably
only worth including if it's greater than zero I guess.

This leaves the door open for adding more optional fields in the future.

> +}
> +
>  /* Append to LIST the breakpoint Python object associated to B.
>  
>     Return true on success.  Return false on failure, with the Python error
> @@ -1389,7 +1407,7 @@ PyTypeObject breakpoint_object_type =
>    0,				  /*tp_getattr*/
>    0,				  /*tp_setattr*/
>    0,				  /*tp_compare*/
> -  0,				  /*tp_repr*/
> +  bppy_repr,                     /*tp_repr*/
>    0,				  /*tp_as_number*/
>    0,				  /*tp_as_sequence*/
>    0,				  /*tp_as_mapping*/
> @@ -1604,6 +1622,33 @@ bplocpy_dealloc (PyObject *py_self)
>    Py_TYPE (py_self)->tp_free (py_self);
>  }
>  
> +/* __repr__ implementation for gdb.BreakpointLocation.  */
> +
> +static PyObject *
> +bplocpy_repr (PyObject *py_self)
> +{
> +  const auto self = (gdbpy_breakpoint_location_object *) py_self;
> +  if (self->owner == nullptr || self->owner->bp == nullptr || self->owner->bp != self->bp_loc->owner)
> +    return PyUnicode_FromFormat ("<gdb.BreakpointLocation (invalid)>");

That `if` condition line is too long.

> +
> +  const auto enabled = self->bp_loc->enabled ? "enabled" : "disabled";
> +
> +  std::stringstream ss;
> +  ss << std::endl << enabled << std::endl;
> +  ss << "requested_address=0x" << std::hex << self->bp_loc->requested_address << " ";
> +  ss << "address=0x" << self->bp_loc->address << " " << std::dec << std::endl;
> +  if (self->bp_loc->symtab != nullptr)
> +  {
> +    ss << self->bp_loc->symtab->filename << ":" << self->bp_loc->line_number << " " << std::endl;
> +  }
> +
> +  const auto fn_name = self->bp_loc->function_name.get ();
> +  if (fn_name != nullptr)
> +    ss << "in " << fn_name << " " << std::endl;

I think this can all be rewritten using std::string instead of
std::stringstream, maybe using string_printf to do some of the
formatting.  IMHO this would be more consistent with the rest of GDB.

> +
> +  return PyUnicode_FromFormat ("<gdb.BreakpointLocation %s>", ss.str ().c_str ());
> +}
> +
>  /* Attribute get/set Python definitions. */
>  
>  static gdb_PyGetSetDef bp_location_object_getset[] = {
> @@ -1635,7 +1680,7 @@ PyTypeObject breakpoint_location_object_type =
>    0,					/*tp_getattr*/
>    0,					/*tp_setattr*/
>    0,					/*tp_compare*/
> -  0,					/*tp_repr*/
> +  bplocpy_repr,                        /*tp_repr*/
>    0,					/*tp_as_number*/
>    0,					/*tp_as_sequence*/
>    0,					/*tp_as_mapping*/
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index 93c86964f3e..5a8149bbe66 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -375,6 +375,20 @@ sympy_dealloc (PyObject *obj)
>    Py_TYPE (obj)->tp_free (obj);
>  }
>  
> +/* __repr__ implementation for gdb.Symbol.  */
> +
> +static PyObject *
> +sympy_repr (PyObject *self)
> +{
> +  const auto symbol = symbol_object_to_symbol (self);
> +  if (symbol == nullptr)
> +    return PyUnicode_FromFormat ("<gdb.Symbol (invalid)>");
> +
> +  return PyUnicode_FromFormat
> +    ("<gdb.Symbol print_name=%s>",
> +     symbol->print_name ());
> +}
> +
>  /* Implementation of
>     gdb.lookup_symbol (name [, block] [, domain]) -> (symbol, is_field_of_this)
>     A tuple with 2 elements is always returned.  The first is the symbol
> @@ -732,7 +746,7 @@ PyTypeObject symbol_object_type = {
>    0,				  /*tp_getattr*/
>    0,				  /*tp_setattr*/
>    0,				  /*tp_compare*/
> -  0,				  /*tp_repr*/
> +  sympy_repr,                    /*tp_repr*/
>    0,				  /*tp_as_number*/
>    0,				  /*tp_as_sequence*/
>    0,				  /*tp_as_mapping*/
> diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> index 928efacfe8a..abe127eca76 100644
> --- a/gdb/python/py-type.c
> +++ b/gdb/python/py-type.c
> @@ -442,6 +442,7 @@ typy_is_signed (PyObject *self, void *closure)
>      Py_RETURN_TRUE;
>  }
>  
> +

Random extra blank line here.

>  /* Return the type, stripped of typedefs. */
>  static PyObject *
>  typy_strip_typedefs (PyObject *self, PyObject *args)
> @@ -1026,6 +1027,34 @@ typy_template_argument (PyObject *self, PyObject *args)
>    return value_to_value_object (val);
>  }
>  
> +/* __repr__ implementation for gdb.Type.  */
> +
> +static PyObject *
> +typy_repr (PyObject *self)
> +{
> +  const auto type = type_object_to_type (self);
> +  if (type == nullptr)
> +    return PyUnicode_FromFormat ("<gdb.Type (invalid)>");
> +
> +  const char *code = pyty_codes[type->code ()].name;
> +  string_file type_name;
> +  try
> +    {
> +      current_language->print_type (type, "",
> +				    &type_name, -1, 0,
> +				    &type_print_raw_options);
> +    }
> +  catch (const gdb_exception &except)
> +    {
> +      GDB_PY_HANDLE_EXCEPTION (except);
> +    }
> +  auto py_typename = PyUnicode_Decode
> +    (type_name.c_str (), type_name.size (),
> +		 host_charset (), NULL);
> +	
> +	return PyUnicode_FromFormat ("<gdb.Type code=%s name=%U>", code, py_typename);

Here's your whitespace error.  The return line is over indented, and the
preceding line has trailing whitespace.

> +}
> +
>  static PyObject *
>  typy_str (PyObject *self)
>  {
> @@ -1612,7 +1641,7 @@ PyTypeObject type_object_type =
>    0,				  /*tp_getattr*/
>    0,				  /*tp_setattr*/
>    0,				  /*tp_compare*/
> -  0,				  /*tp_repr*/
> +  typy_repr,                     /*tp_repr*/
>    &type_object_as_number,	  /*tp_as_number*/
>    0,				  /*tp_as_sequence*/
>    &typy_mapping,		  /*tp_as_mapping*/
> diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
> index 1fbbc47c872..a60b4a25cbb 100644
> --- a/gdb/testsuite/gdb.python/py-arch.exp
> +++ b/gdb/testsuite/gdb.python/py-arch.exp
> @@ -29,6 +29,8 @@ if ![runto_main] {
>  # Test python/15461.  Invalid architectures should not trigger an
>  # internal GDB assert.
>  gdb_py_test_silent_cmd "python empty = gdb.Architecture()" "get empty arch" 0
> +gdb_test "python print(repr (empty))" "<gdb\\.Architecture \\(invalid\\)>" \
> +    "Test empty achitecture __repr__ does not trigger an assert"

I guess I was surprised that so many of the new tests included an
explicit call to repr, given the premise of the change was that simply
'print(empty)' would now print something useful.

I guess maybe it doesn't hurt to _also_ include some explicit repr
calls, but I was expecting most tests to just be printing the object
directly.

Is there some reasoning here that I'm missing?

>  gdb_test "python print(empty.name())" ".*Architecture is invalid.*" \
>      "Test empty architecture.name does not trigger an assert"
>  gdb_test "python print(empty.disassemble())" ".*Architecture is invalid.*" \
> @@ -46,6 +48,8 @@ gdb_py_test_silent_cmd "python insn_list3 = arch.disassemble(pc, count=1)" \
>  gdb_py_test_silent_cmd "python insn_list4 = arch.disassemble(gdb.Value(pc))" \
>    "disassemble no end no count" 0
>  
> +gdb_test "python print (repr (arch))" "<gdb.Architecture arch_name=.* printable_name=.*>" "test __repr__ for architecture"

Over long line, please wrap a little.  There's other long lines in your
patch, I'll not point out each one.

> +
>  gdb_test "python print (len(insn_list1))" "1" "test number of instructions 1"
>  gdb_test "python print (len(insn_list2))" "1" "test number of instructions 2"
>  gdb_test "python print (len(insn_list3))" "1" "test number of instructions 3"
> diff --git a/gdb/testsuite/gdb.python/py-block.exp b/gdb/testsuite/gdb.python/py-block.exp
> index 0a88aec56a0..5e3d1c72d5e 100644
> --- a/gdb/testsuite/gdb.python/py-block.exp
> +++ b/gdb/testsuite/gdb.python/py-block.exp
> @@ -39,7 +39,7 @@ gdb_continue_to_breakpoint "Block break here."
>  gdb_py_test_silent_cmd "python frame = gdb.selected_frame()" "Get Frame" 0
>  gdb_py_test_silent_cmd "python block = frame.block()" \
>      "Get block, initial innermost block" 0
> -gdb_test "python print (block)" "<gdb.Block object at $hex>" "check block not None"
> +gdb_test "python print (block)" "<gdb.Block .* \{.*\}>" "check block not None"
>  gdb_test "python print (block.function)" "None" "first anonymous block"
>  gdb_test "python print (block.start)" "${decimal}" "check start not None"
>  gdb_test "python print (block.end)" "${decimal}" "check end not None"
> @@ -73,7 +73,7 @@ gdb_test "python print (block.function)" "block_func" \
>  gdb_test "up" ".*"
>  gdb_py_test_silent_cmd "python frame = gdb.selected_frame()" "Get Frame 2" 0
>  gdb_py_test_silent_cmd "python block = frame.block()" "Get Frame 2's block" 0
> -gdb_test "python print (block)" "<gdb.Block object at $hex>" \
> +gdb_test "python print (repr (block))" "<gdb.Block .* \{.*\}>" \
>           "Check Frame 2's block not None"
>  gdb_test "python print (block.function)" "main" "main block"
>  
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index e36e87dc291..0c904a12c90 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -50,11 +50,14 @@ proc_with_prefix test_bkpt_basic { } {
>  	return 0
>      }
>  
> +    set num_exp "-?\[0-9\]+"
> +    set repr_pattern "<gdb.Breakpoint number=$num_exp thread=$num_exp hits=$num_exp enable_count=$num_exp>"

We already have $decimal.  I think if you change py-breakpoint.c as I
suggest then you should be able to make use of that and remove num_exp.
This applies below too.

Thanks,
Andrew

> +
>      # Now there should be one breakpoint: main.
>      gdb_py_test_silent_cmd "python blist = gdb.breakpoints()" \
>  	"Get Breakpoint List" 0
> -    gdb_test "python print (blist\[0\])" \
> -	"<gdb.Breakpoint object at $hex>" "Check obj exists @main"
> +    gdb_test "python print (repr (blist\[0\]))" \
> +	"$repr_pattern" "Check obj exists @main"
>      gdb_test "python print (blist\[0\].location)" \
>  	"main." "Check breakpoint location @main"
>      gdb_test "python print (blist\[0\].pending)" "False" \
> @@ -71,12 +74,12 @@ proc_with_prefix test_bkpt_basic { } {
>  	"Get Breakpoint List" 0
>      gdb_test "python print (len(blist))" \
>  	"2" "Check for two breakpoints"
> -    gdb_test "python print (blist\[0\])" \
> -	"<gdb.Breakpoint object at $hex>" "Check obj exists @main 2"
> +    gdb_test "python print (repr (blist\[0\]))" \
> +	"$repr_pattern" "Check obj exists @main 2"
>      gdb_test "python print (blist\[0\].location)" \
>  	"main." "Check breakpoint location @main 2"
> -    gdb_test "python print (blist\[1\])" \
> -	"<gdb.Breakpoint object at $hex>" "Check obj exists @mult_line"
> +    gdb_test "python print (repr (blist\[1\]))" \
> +	"$repr_pattern" "Check obj exists @mult_line"
>  
>      gdb_test "python print (blist\[1\].location)" \
>  	"py-breakpoint\.c:${mult_line}*" \
> @@ -224,14 +227,17 @@ proc_with_prefix test_bkpt_invisible { } {
>  	return 0
>      }
>  
> +    set num_exp "-?\[0-9\]+"
> +    set repr_pattern "<gdb.Breakpoint number=$num_exp thread=$num_exp hits=$num_exp enable_count=$num_exp>"
> +
>      delete_breakpoints
>      set ibp_location [gdb_get_line_number "Break at multiply."]
>      gdb_py_test_silent_cmd  "python ibp = gdb.Breakpoint(\"$ibp_location\", internal=False)" \
>  	"Set invisible breakpoint" 0
>      gdb_py_test_silent_cmd "python ilist = gdb.breakpoints()" \
>  	"Get Breakpoint List" 0
> -    gdb_test "python print (ilist\[0\])" \
> -	"<gdb.Breakpoint object at $hex>" "Check invisible bp obj exists 1"
> +    gdb_test "python print (repr (ilist\[0\]))" \
> +	"$repr_pattern" "Check invisible bp obj exists 1"
>      gdb_test "python print (ilist\[0\].location)" \
>  	"py-breakpoint\.c:$ibp_location*" "Check breakpoint location 1"
>      gdb_test "python print (ilist\[0\].visible)" \
> @@ -243,8 +249,8 @@ proc_with_prefix test_bkpt_invisible { } {
>  	"Set invisible breakpoint" 0
>      gdb_py_test_silent_cmd "python ilist = gdb.breakpoints()" \
>  	"Get Breakpoint List" 0
> -    gdb_test "python print (ilist\[0\])" \
> -	"<gdb.Breakpoint object at $hex>" "Check invisible bp obj exists 2"
> +    gdb_test "python print (repr (ilist\[0\]))" \
> +	"$repr_pattern" "Check invisible bp obj exists 2"
>      gdb_test "python print (ilist\[0\].location)" \
>  	"py-breakpoint\.c:$ibp_location*" "Check breakpoint location 2"
>      gdb_test "python print (ilist\[0\].visible)" \
> diff --git a/gdb/testsuite/gdb.python/py-symbol.exp b/gdb/testsuite/gdb.python/py-symbol.exp
> index ad06b07c2c6..e0baed9b6d4 100644
> --- a/gdb/testsuite/gdb.python/py-symbol.exp
> +++ b/gdb/testsuite/gdb.python/py-symbol.exp
> @@ -44,6 +44,7 @@ clean_restart ${binfile}
>  # point where we don't have a current frame, and we don't want to
>  # require one.
>  gdb_py_test_silent_cmd "python main_func = gdb.lookup_global_symbol(\"main\")" "Lookup main" 1
> +gdb_test "python print (repr (main_func))" "<gdb.Symbol print_name=.*>" "test main_func.__repr__"
>  gdb_test "python print (main_func.is_function)" "True" "test main_func.is_function"
>  gdb_test "python print (gdb.lookup_global_symbol(\"junk\"))" "None" "test lookup_global_symbol(\"junk\")"
>  
> diff --git a/gdb/testsuite/gdb.python/py-type.exp b/gdb/testsuite/gdb.python/py-type.exp
> index 594c9749d8e..95cdfa54a6e 100644
> --- a/gdb/testsuite/gdb.python/py-type.exp
> +++ b/gdb/testsuite/gdb.python/py-type.exp
> @@ -393,3 +393,7 @@ if { [build_inferior "${binfile}-cxx" "c++"] == 0 } {
>        test_type_equality
>    }
>  }
> +
> +# Test __repr__()
> +gdb_test "python print (repr (gdb.lookup_type ('char')))" \
> +      "<gdb.Type code=TYPE_CODE_INT name=char>" "test __repr__()"
> -- 
> 2.37.3.windows.1