Re: [PATCHv2 2/2] Use method children instead of to_string in pretty printers

[Joel Brobecker <brobecker@adacore.com>]

Hi Hannes,

On Fri, Jun 04, 2021 at 06:24:00PM +0200, Hannes Domani via Gdb-patches wrote:
> With '-pretty on' the output is basically the same, but like this the
> pointer members can be expanded in the TUI variable windows.
> 
> gdb/ChangeLog:
> 
> 2021-06-04  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* gdb-gdb.py.in: Use method children in pretty printers.

It looks like no one has reviewed this patch, yet, so I took a look.

Overall, this looks good to me, except for one area:

> ---
> v2:
> - Added ChangeLog.
> - Reformatted with black.
> ---
>  gdb/gdb-gdb.py.in | 175 ++++++++++++++++++++--------------------------
>  1 file changed, 77 insertions(+), 98 deletions(-)
> 
> diff --git a/gdb/gdb-gdb.py.in b/gdb/gdb-gdb.py.in
> index af9fcfedc2f..1432d7bbcf8 100644
> --- a/gdb/gdb-gdb.py.in
> +++ b/gdb/gdb-gdb.py.in
> @@ -107,17 +107,13 @@ class StructTypePrettyPrinter:
>      def __init__(self, val):
>          self.val = val
>  
> -    def to_string(self):
> -        fields = []
> -        fields.append("pointer_type = %s" % self.val["pointer_type"])
> -        fields.append("reference_type = %s" % self.val["reference_type"])
> -        fields.append("chain = %s" % self.val["reference_type"])
> -        fields.append(
> -            "instance_flags = %s" % TypeFlagsPrinter(self.val["m_instance_flags"])
> -        )
> -        fields.append("length = %d" % self.val["length"])
> -        fields.append("main_type = %s" % self.val["main_type"])
> -        return "\n{" + ",\n ".join(fields) + "}"
> +    def children(self):
> +        for f in self.val.type.fields():
> +            name = f.name
> +            val = self.val[f]
> +            if name == "m_instance_flags":
> +                val = str(TypeFlagsPrinter(val))
> +            yield name, val
>  
>  
>  class StructMainTypePrettyPrinter:
> @@ -142,46 +138,6 @@ class StructMainTypePrettyPrinter:
>          ]
>          return "|".join(fields)
>  
> -    def owner_to_string(self):
> -        """Return an image of component "owner"."""
> -        if self.val["m_flag_objfile_owned"] != 0:
> -            return "%s (objfile)" % self.val["m_owner"]["objfile"]
> -        else:
> -            return "%s (gdbarch)" % self.val["m_owner"]["gdbarch"]
> -
> -    def struct_field_location_img(self, field_val):
> -        """Return an image of the loc component inside the given field
> -        gdb.Value.
> -        """
> -        loc_val = field_val["loc"]
> -        loc_kind = str(field_val["loc_kind"])
> -        if loc_kind == "FIELD_LOC_KIND_BITPOS":
> -            return "bitpos = %d" % loc_val["bitpos"]
> -        elif loc_kind == "FIELD_LOC_KIND_ENUMVAL":
> -            return "enumval = %d" % loc_val["enumval"]
> -        elif loc_kind == "FIELD_LOC_KIND_PHYSADDR":
> -            return "physaddr = 0x%x" % loc_val["physaddr"]
> -        elif loc_kind == "FIELD_LOC_KIND_PHYSNAME":
> -            return "physname = %s" % loc_val["physname"]
> -        elif loc_kind == "FIELD_LOC_KIND_DWARF_BLOCK":
> -            return "dwarf_block = %s" % loc_val["dwarf_block"]
> -        else:
> -            return "loc = ??? (unsupported loc_kind value)"
> -
> -    def struct_field_img(self, fieldno):
> -        """Return an image of the main_type field number FIELDNO."""
> -        f = self.val["flds_bnds"]["fields"][fieldno]
> -        label = "flds_bnds.fields[%d]:" % fieldno
> -        if f["artificial"]:
> -            label += " (artificial)"
> -        fields = []
> -        fields.append("name = %s" % f["name"])
> -        fields.append("type = %s" % f["m_type"])
> -        fields.append("loc_kind = %s" % f["loc_kind"])
> -        fields.append("bitsize = %d" % f["bitsize"])
> -        fields.append(self.struct_field_location_img(f))
> -        return label + "\n" + "  {" + ",\n   ".join(fields) + "}"
> -
>      def bound_img(self, bound_name):
>          """Return an image of the given main_type's bound."""
>          bounds = self.val["flds_bnds"]["bounds"].dereference()
> @@ -197,17 +153,6 @@ class StructMainTypePrettyPrinter:
>                  info.append("upper_bound_is_count")
>              return "{} ({})".format(str(b["m_data"]["baton"]), ",".join(info))
>  
> -    def bounds_img(self):
> -        """Return an image of the main_type bounds."""
> -        b = self.val["flds_bnds"]["bounds"].dereference()
> -        low = self.bound_img("low")
> -        high = self.bound_img("high")
> -
> -        img = "flds_bnds.bounds = {%s, %s}" % (low, high)
> -        if b["flag_bound_evaluated"]:
> -            img += " [evaluated]"
> -        return img
> -
>      def type_specific_img(self):
>          """Return a string image of the main_type type_specific union.
>          Only the relevant component of that union is printed (based on
> @@ -216,54 +161,86 @@ class StructMainTypePrettyPrinter:
>          type_specific_kind = str(self.val["type_specific_field"])
>          type_specific = self.val["type_specific"]
>          if type_specific_kind == "TYPE_SPECIFIC_NONE":
> -            img = "type_specific_field = %s" % type_specific_kind
> +            return "type_specific_field", type_specific_kind
>          elif type_specific_kind == "TYPE_SPECIFIC_CPLUS_STUFF":
> -            img = "cplus_stuff = %s" % type_specific["cplus_stuff"]
> +            return "cplus_stuff", type_specific["cplus_stuff"]
>          elif type_specific_kind == "TYPE_SPECIFIC_GNAT_STUFF":
> -            img = (
> -                "gnat_stuff = {descriptive_type = %s}"
> -                % type_specific["gnat_stuff"]["descriptive_type"]
> +            return (
> +                "gnat_stuff.descriptive_type",
> +                type_specific["gnat_stuff"]["descriptive_type"],
>              )
>          elif type_specific_kind == "TYPE_SPECIFIC_FLOATFORMAT":
> -            img = "floatformat[0..1] = %s" % type_specific["floatformat"]
> +            return "floatformat[0..1]", type_specific["floatformat"]
>          elif type_specific_kind == "TYPE_SPECIFIC_FUNC":
> -            img = (
> -                "calling_convention = %d"
> -                % type_specific["func_stuff"]["calling_convention"]
> +            return (
> +                "calling_convention",
> +                type_specific["func_stuff"]["calling_convention"],
>              )
>              # tail_call_list is not printed.
>          elif type_specific_kind == "TYPE_SPECIFIC_SELF_TYPE":
> -            img = "self_type = %s" % type_specific["self_type"]
> -        elif type_specific_kind == "TYPE_SPECIFIC_FIXED_POINT":
> -            # The scaling factor is an opaque structure, so we cannot
> -            # decode its value from Python (not without insider knowledge).
> -            img = (
> -                "scaling_factor: <opaque> (call __gmpz_dump with "
> -                " _mp_num and _mp_den fields if needed)"
> -            )
> -        else:
> -            img = (
> -                "type_specific = ??? (unknown type_secific_kind: %s)"
> -                % type_specific_kind
> -            )
> -        return img
> +            return "self_type", type_specific["self_type"]
> +        return (
> +            "type_specific",
> +            "??? (unknown type_secific_kind: %s)" % type_specific_kind,
> +        )

It looks to me like you collapsed the case of TYPE_SPECIFIC_FIXED_POINT
with the "else:" part. While we are unable to print the actual data
in both cases, I still think it's worth showing preserving the current
behavior, because it confirms the type-specific kind, and also shows
to the user that the value is opaque, with a tipe on how to get it
in practice.

The rest looked good to me.

>  
> -    def to_string(self):
> -        """Return a pretty-printed image of our main_type."""
> -        fields = []
> -        fields.append("name = %s" % self.val["name"])
> -        fields.append("code = %s" % self.val["code"])
> -        fields.append("flags = [%s]" % self.flags_to_string())
> -        fields.append("owner = %s" % self.owner_to_string())
> -        fields.append("target_type = %s" % self.val["target_type"])
> +    def children(self):
> +        yield "name", self.val["name"].format_string(symbols=False, address=False)
> +        yield "code", self.val["code"]
> +        yield "flags", "[%s]" % self.flags_to_string()
> +        if self.val["m_flag_objfile_owned"] != 0:
> +            yield "owner.objfile", self.val["m_owner"]["objfile"]
> +        else:
> +            yield "owner.gdbarch", self.val["m_owner"]["gdbarch"]
> +        yield "target_type", self.val["target_type"]
>          if self.val["nfields"] > 0:
> -            for fieldno in range(self.val["nfields"]):
> -                fields.append(self.struct_field_img(fieldno))
> +            fields = self.val["flds_bnds"]["fields"]
> +            field_array_type = (
> +                fields.type.target().array(self.val["nfields"] - 1).pointer()
> +            )
> +            yield "flds_bnds.fields", fields.cast(field_array_type).dereference()
>          if self.val["code"] == gdb.TYPE_CODE_RANGE:
> -            fields.append(self.bounds_img())
> -        fields.append(self.type_specific_img())
> +            b = self.val["flds_bnds"]["bounds"].dereference()
> +            low = self.bound_img("low")
> +            high = self.bound_img("high")
> +
> +            img = "{%s, %s}" % (low, high)
> +            if b["flag_bound_evaluated"]:
> +                img += " [evaluated]"
> +            yield "flds_bnds.bounds", img
> +        yield self.type_specific_img()
> +
> +
> +class StructFieldPrettyPrinter:
> +    """Pretty-print an objet of type field"""
> +
> +    def __init__(self, val):
> +        self.val = val
> +
> +    def children(self):
> +        if self.val["artificial"]:
> +            yield "artificial", self.val["artificial"]
> +        yield "name", self.val["name"].format_string(symbols=False, address=False)
> +        yield "type", self.val["m_type"]
> +        yield "loc_kind", self.val["loc_kind"]
> +        yield "bitsize", self.val["bitsize"]
>  
> -        return "\n{" + ",\n ".join(fields) + "}"
> +        loc_val = self.val["loc"]
> +        loc_kind = str(self.val["loc_kind"])
> +        if loc_kind == "FIELD_LOC_KIND_BITPOS":
> +            yield "bitpos", loc_val["bitpos"]
> +        elif loc_kind == "FIELD_LOC_KIND_ENUMVAL":
> +            yield "enumval", loc_val["enumval"]
> +        elif loc_kind == "FIELD_LOC_KIND_PHYSADDR":
> +            yield "physaddr", loc_val["physaddr"]
> +        elif loc_kind == "FIELD_LOC_KIND_PHYSNAME":
> +            yield "physname", loc_val["physname"].format_string(
> +                symbols=False, address=False
> +            )
> +        elif loc_kind == "FIELD_LOC_KIND_DWARF_BLOCK":
> +            yield "dwarf_block", loc_val["dwarf_block"]
> +        else:
> +            yield "loc", "???"
>  
>  
>  class CoreAddrPrettyPrinter:
> @@ -284,6 +261,8 @@ def type_lookup_function(val):
>          return StructTypePrettyPrinter(val)
>      elif val.type.tag == "main_type":
>          return StructMainTypePrettyPrinter(val)
> +    elif val.type.tag == "field":
> +        return StructFieldPrettyPrinter(val)
>      elif val.type.name == "CORE_ADDR":
>          return CoreAddrPrettyPrinter(val)
>      return None
> -- 
> 2.31.1
> 

-- 
Joel