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>
> ```
>
>> 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.
>
> I forgot to take that out of the commit message, my bad. Originally, I'd 
> intended for the string literals in the patch that get handed to Python to be
> all u8 literals so that I could guarantee it wouldn't break in an environment
> that doesn't output regular string literals in an ASCII-compatible encoding,
> as Python expects all strings handed to it to be encoded in UTF-8. But seeing
> as all of the rest of the Python interface code uses regular string literals, 
> I figured it wouldn't make much of difference having them in anyway.

Thanks, that makes sense.

>
>> 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.
> As blarsen@ also pointed out, `print`-ing an object directly that has an 
> implmentation of __str__() will print whatever its __str__() functions returns, 
> regardless of whether it implements __repr__() or not, which is not what we want 
> here. __repr__() is always preferred in the REPL, though, so it's understandable 
> it might not be clear at first why I'm calling `repr()` explicitly.

Again, thanks (and to Bruno too) for the explanation.  I understand
__str__ and __repr__ better now :)  I agree that what you're doing makes
sense now I understand it.

>
>> Over long line, please wrap a little.  There's other long lines in your
>> patch, I'll not point out each one.
>
> Should be all fixed now (hopefully I didn't miss any), with the exception of the
> `repr_pattern` strings in `py-breakpoint.exp`, which I couldn't for the life of 
> me get to match properly with the output were they not on a single line.
>
> ---
>  gdb/python/py-arch.c                       | 18 +++++-
>  gdb/python/py-block.c                      | 27 ++++++++-
>  gdb/python/py-breakpoint.c                 | 68 +++++++++++++++++++++-
>  gdb/python/py-symbol.c                     | 16 ++++-
>  gdb/python/py-type.c                       | 30 +++++++++-
>  gdb/testsuite/gdb.python/py-arch.exp       |  6 ++
>  gdb/testsuite/gdb.python/py-block.exp      |  4 +-
>  gdb/testsuite/gdb.python/py-breakpoint.exp | 24 ++++----
>  gdb/testsuite/gdb.python/py-symbol.exp     |  2 +
>  gdb/testsuite/gdb.python/py-type.exp       |  4 ++
>  10 files changed, 181 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..b4c55add765 100644
> --- a/gdb/python/py-block.c
> +++ b/gdb/python/py-block.c
> @@ -424,6 +424,31 @@ 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)>");
> +
> +  const auto name = block->function () ?
> +    block->function ()->print_name () : "<anonymous>";
> +
> +  block_iterator iter;
> +  block_iterator_first (block, &iter);
> +
> +  std::string str;
> +  const struct symbol *symbol;
> +  while ((symbol = block_iterator_next (&iter)) != nullptr)

You should be using ALL_BLOCK_SYMBOLS here rather than calling the
block_iterator_* functions directly.  As it's currently written this
code will crash on a block with no symbols.

I've included a fix-up patch at the end of this email that both fixes
this issue, and extended the existing test to check this case.  Please
feel free to merge this with your work.

> +    str = (str + "\n") + symbol->print_name () + ",";

I don't object to including all the symbol names as you've done.  But
did you consider that some blocks could have a _lot_ of symbols?

As an alternative did you consider just counting the symbols, and
including the count?

This isn't a requirement, if you feel the symbol list is going to be
more useful then I don't feel strongly enough, but I just wanted to
ask.

> +  if(!str.empty ())
> +    str += "\n";
> +
> +  return PyUnicode_FromFormat ("<gdb.Block %s {%s}>", name, str.c_str ());
> +}
> +
>  int
>  gdbpy_initialize_blocks (void)
>  {
> @@ -486,7 +511,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..d68a205330c 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 "gdbsupport/common-utils.h"
>  
>  extern PyTypeObject breakpoint_location_object_type
>      CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("breakpoint_location_object");
> @@ -967,6 +968,31 @@ 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)>");
> +
> +  std::string str = " ";
> +  if (bp->bp->thread != -1)
> +    str += string_printf ("thread=%d ", bp->bp->thread);
> +  if (bp->bp->task > 0)
> +    str += string_printf ("task=%d ", bp->bp->task);
> +  if (bp->bp->enable_count > 0)
> +    str += string_printf ("enable_count=%d ", bp->bp->enable_count);
> +  str.pop_back ();
> +
> +  return PyUnicode_FromFormat
> +    ("<gdb.Breakpoint number=%d hits=%d%s>",
> +     bp->bp->number,
> +     bp->bp->hit_count,
> +     str.c_str ());
> +}
> +
>  /* Append to LIST the breakpoint Python object associated to B.
>  
>     Return true on success.  Return false on failure, with the Python error
> @@ -1389,7 +1415,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 +1630,44 @@ 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)>");
> +
> +  const auto enabled = self->bp_loc->enabled ? "enabled" : "disabled";
> +
> +  std::string str(enabled);
> +
> +  str += " requested_address=0x";
> +  str += string_printf ("%lx", self->bp_loc->requested_address);
> +
> +  str += " address=0x";
> +  str += string_printf ("%lx", self->bp_loc->address);

I'd suggest placing 'address' first, and only include
'requested_address' if it's different to 'address'.  In almost all cases
these two fields will be the same, so this would cut down on some
clutter.

Additionally, you should use core_addr_to_string_nz to format the
CORE_ADDR, like this:

  str += " address=";
  str += string_printf ("%s", core_addr_to_string_nz (self->bp_loc->address));


> +
> +  if (self->bp_loc->symtab != nullptr)
> +  {
> +    str += " source=";
> +    str += self->bp_loc->symtab->filename;
> +    str += ":";
> +    str += string_printf ("%d", self->bp_loc->line_number);
> +  }

The GNU style is to indent the opening and closing curly braces by an
additional 2 spaces, and then everything within the block gets 2 extra
spaces beyond that.

> +
> +  const auto fn_name = self->bp_loc->function_name.get ();
> +  if (fn_name != nullptr)
> +  {
> +    str += " in ";
> +    str += fn_name;
> +  }

Indentation again here.

> +
> +  return PyUnicode_FromFormat ("<gdb.BreakpointLocation %s>", str.c_str ());
> +}
> +
>  /* Attribute get/set Python definitions. */
>  
>  static gdb_PyGetSetDef bp_location_object_getset[] = {
> @@ -1635,7 +1699,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..eb11ef029ca 100644
> --- a/gdb/python/py-type.c
> +++ b/gdb/python/py-type.c
> @@ -1026,6 +1026,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);

I think there's enough space to start the arguments on the
PyUnicode_Decode line (i.e. no line break before '(' char).  The
'host_charset ()' would still be the start of a second line, and should
align under 'type_name':

  auto py_typename = PyUnicode_Decode (type_name.c_str (), type_name.size (),
				       host_charset (), NULL);

> +	

You still have some trailing whitespace on this ^^^ line.

> +  return PyUnicode_FromFormat ("<gdb.Type code=%s name=%U>", code, py_typename);
> +}
> +
>  static PyObject *
>  typy_str (PyObject *self)
>  {
> @@ -1612,7 +1640,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..d436c957e25 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"
>  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,10 @@ 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"
> +
>  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"

I think I'd be tempted to at least replace the first '.*' with the
function name we expect to see.  Might as well make the test as accurate
as possible.

>  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 .* \{.*\}>" \

Same again where with the function name.

>           "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..4da46431a3a 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -50,11 +50,13 @@ proc_with_prefix test_bkpt_basic { } {
>  	return 0
>      }
>  
> +    set repr_pattern "<gdb.Breakpoint number=-?$decimal hits=-?$decimal\( thread=$decimal\)?\( task=$decimal\)?\( enable_count=$decimal\)?>"
> +
>      # 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 +73,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 +226,16 @@ proc_with_prefix test_bkpt_invisible { } {
>  	return 0
>      }
>  
> +    set repr_pattern "<gdb.Breakpoint number=-?$decimal hits=-?$decimal\( thread=$decimal\)?\( task=$decimal\)?\( enable_count=$decimal\)?>"
> +
>      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 +247,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..979b7dfb8fb 100644
> --- a/gdb/testsuite/gdb.python/py-symbol.exp
> +++ b/gdb/testsuite/gdb.python/py-symbol.exp
> @@ -44,6 +44,8 @@ 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=.*>" \

I think replace '.*' with 'main' here.

> +    "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__()

Missing '.' at the end of the comment.

> +gdb_test "python print (repr (gdb.lookup_type ('char')))" \
> +      "<gdb.Type code=TYPE_CODE_INT name=char>" "test __repr__()"

Thanks
Andrew

---

diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index a9d45b75dc9..c642be4208d 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -437,11 +437,10 @@ blpy_repr (PyObject *self)
     block->function ()->print_name () : "<anonymous>";
 
   block_iterator iter;
-  block_iterator_first (block, &iter);
-
-  std::string str;
   const struct symbol *symbol;
-  while ((symbol = block_iterator_next (&iter)) != nullptr)
+  std::string str;
+
+  ALL_BLOCK_SYMBOLS (block, iter, symbol)
     str = (str + "\n") + symbol->print_name () + ",";
   if(!str.empty ())
     str += "\n";
diff --git a/gdb/testsuite/gdb.python/py-block.c b/gdb/testsuite/gdb.python/py-block.c
index a0c6e165605..4c503827347 100644
--- a/gdb/testsuite/gdb.python/py-block.c
+++ b/gdb/testsuite/gdb.python/py-block.c
@@ -30,9 +30,14 @@ int block_func (void)
   }
 }
 
+int
+no_locals_func (void)
+{
+  return block_func ();
+}
 
 int main (int argc, char *argv[])
 {
-  block_func ();
+  no_locals_func ();
   return 0; /* Break at end. */
 }
diff --git a/gdb/testsuite/gdb.python/py-block.exp b/gdb/testsuite/gdb.python/py-block.exp
index 5cca798eeb3..3ac8c97dc69 100644
--- a/gdb/testsuite/gdb.python/py-block.exp
+++ b/gdb/testsuite/gdb.python/py-block.exp
@@ -68,15 +68,22 @@ gdb_test_no_output "python block = block.superblock" "get superblock 2"
 gdb_test "python print (block.function)" "block_func" \
          "Print superblock 2 function"
 
+gdb_test "up" ".*" "up to no_locals_func"
+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 (repr (block))" "<gdb.Block no_locals_func \{\}>" \
+    "Check block in empty_frame"
+gdb_test "python print (block.function)" "no_locals_func" \
+    "no_locals_func block"
+
 # Switch frames, then test for main block.
-gdb_test "up" ".*"
+gdb_test "up" ".*" "up to main"
 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 (repr (block))" "<gdb.Block .* \{.*\}>" \
          "Check Frame 2's block not None"
 gdb_test "python print (block.function)" "main" "main block"
 
-
 # Test Block is_valid.  This must always be the last test in this
 # testcase as it unloads the object file.
 delete_breakpoints