Re: [PATCH v1] gdb/DAP Introduce new methods to to Pretty Printers

[Tom Tromey <tromey@adacore.com>]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Sorry about the delay on replying to this.

I have a few comments to make, most of them not about the patch but
rather about how we ought to proceed.

Simon> This patch introduces the `num_children` method to the NoOpArray-pretty
Simon> printer as well as fixing a bug where some types accidentally uses
Simon> NoOpScalar pretty printers. It also cleans up the retrieval of variables
Simon> during a "variables" request (or an "evaluate" request) reducing the
Simon> complexity.

I am not really sure we can/should do this.

The reason stems from a mistake we made early on when adding Python to
gdb -- when adding pretty-printers, we weren't very careful about
letting people know that we might add more methods to the printers.

This means existing code is free to use any name on the printer objects,
except the few we documented.  I'm not sure num_children or
children_range are used anywhere -- but they might well be.

Of course it's not very hard to fix this in printers.  However, it's bad
to ship a new gdb that breaks people's setup.  We've tried pretty hard
to avoid this.

Maybe one way out would be to define a new way to do pretty-printers,
with a compatibility shim.

Simon> Caching of variables would be fine if we
Simon> actually kept variables around during step/continues that stops inside
Simon> the same frame we're currently at

I don't see how to do this, since any aspect of them can change even
within the same frame.  Preserving the gdb.Value won't really work,
since it won't update.


Another issue in this same area is how to handle types that have special
cases when printing.

An example here is an Ada unconstrained array, or a Rust slice -- these
are both array-like objects that are implemented as a structure holding
a pointer and a length (more or less, Ada can be complicated).

These are exposed to Python as struct types, but we'd like to present
them as array types over DAP -- which is what is done for the CLI (via
special cases in the printing code) and also for MI (via varobj, though
I haven't actually implemented this for Rust yet).

I had a couple of ideas here.

One idea is to rewrite the DAP layer to simply use varobj via
gdb.execute_mi.  There are some downsides here, though.

Mainly, your proposed new methods wouldn't work automatically -- they'd
require hacking on the unfortunately-super-complicated varobj code to
make use of them.

Maybe adding watchpoint support here would also be difficult.  We would
no longer be seeing gdb.Values at all.


Another idea would be to try to fix this in the core, by adding some new
kind of array-like typecode and/or type attribute.  The idea here would
be to try to remove the special cases so that Python could see that
these things are "really" arrays.

I like this idea, since it seems more principled, but I fear it might be
pretty difficult.


There's a couple other kind-of-related issues.

One is that Rust tuples have field names like "__5" rather than "5".
Planning to ignore this one, probably.  This could be fixed in the DWARF
reader but the fix would have some downsides (in the obscure case of
"set lang c" and then trying to poke at the underlying representation).

Another is that an Ada string is just an array of character; this also
has a special case in printing, but I think changing NoOpArrayPrinter to
understand this seems basically reasonable.


Simon> +def is_structured_type(type):
Simon> +    """Attempts to find out if TYPE points to something that's structured,
Simon> +    like a struct or a class.  Unfortunately type codes are not represented
Simon> +    as bitfields.  This has the unfortunate side effect of e.g.
Simon> +    TYPE_CODE_TYPEDEF not being the same as TYPE_CODE_STRUCT, even if that
Simon> +    typedef resolves to a struct."""

I don't fully understand the text here, and it probably doesn't really
belong in the documentation for this function.

Normally what you'd do here instead is strip_typedefs:

Simon> @@ -331,7 +365,7 @@ def make_visualizer(value):
Simon>          result = gdb.printing.NoOpArrayPrinter(value)
Simon>          (low, high) = value.type.range()
Simon>          result.n_children = high - low + 1
Simon> -    elif value.type.code in (gdb.TYPE_CODE_STRUCT, gdb.TYPE_CODE_UNION):
Simon> +    elif is_structured_type(value.type):
Simon>          result = gdb.printing.NoOpStructPrinter(value)

... this code changed on master due to exactly this bug, and now does:

        ty = value.type.strip_typedefs()
...
        elif ty.code in (gdb.TYPE_CODE_STRUCT, gdb.TYPE_CODE_UNION):


Tom