Re: [PATCH 2/2] gdbhooks.py: extend vec support in pretty printers

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2019-07-01 at 13:07 +0300, Vladislav Ivanishin wrote:
> This change is threefold:
> - enable pretty printing of vec<>, not just vec<>*
> - generalize 'vec<(\S+), (\S+), (\S+)>' regex, which is limiting
> - extend to work for vl_ptr layout (only vl_embed was supported)
> 
> The motivating example for all three is a vector of vectors in
> tree-ssa-uninit.c:
> 
>     typedef vec<pred_info, va_heap, vl_ptr> pred_chain;
>     typedef vec<pred_chain, va_heap, vl_ptr> pred_chain_union;
> 
> (This predictably expands to
>       vec<vec<pred_info, va_heap, vl_ptr>, va_heap, vl_ptr>
> in gdb, if we use strip_typedefs() on it -- otherwise it's just
> pred_chain_union.  The first patch of the series takes care of that.)
> 
> This example features "direct" vecs themselves rather than pointers
> to
> vecs, which is not a rare case; in fact, a quick grep showed that the
> number of declarations of direct vecs is about the same as that of
> pointers to vec.
> 
> The GDB's Python API seems to do dereferencing automatically (i.e. it
> detects whether to use '->', or '.' for field access), allowing to
> use
> the same code to access fields from a struct directly as well as
> through
> a pointer.
> 
> This makes it possible to reuse VecPrinter (meant for vec<> *) for
> plain
> vec<>, but perhaps it would be beneficial to handle all pointers in a
> generic way e.g. print the address and then print the dereferenced
> object, letting the appropriate pretty printer to pick it up and do
> its
> thing, if one is provided for the type.
> 
> The output I have for direct vecs now:
> 
>     (gdb) p *bb->succs
>     $94 = @0x7ffff76569b0 = {<edge 0x7ffff7654870 (2 -> 5)>, <edge
> 0x7ffff76548a0 (2 -> 3)>}
> 
> Compare that to
> 
>     (gdb) p bb->succs
>     $70 = 0x7ffff76569b0 = {<edge 0x7ffff7654870 (2 -> 5)>, <edge
> 0x7ffff76548a0 (2 -> 3)>}
> 
> Hope the choice of the '@' sign to represent the address of an object
> taken by the pretty printer is not confusing?  (GDB itself uses this
> notation for references, like so: (edge_def *&) @0x7ffff7656c38.)

Thanks for the patch.

I like the notation idea.

[...]

> diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
> index b036704b86a..a7e665cadb9 100644
> --- a/gcc/gdbhooks.py
> +++ b/gcc/gdbhooks.py
> @@ -128,12 +128,18 @@ and here's a length 1 vec:
>    (gdb) p bb->succs
>    $19 = 0x7ffff0428bb8 = {<edge 0x7ffff044d3f0 (5 -> EXIT)>}
>  
> -You cannot yet use array notation [] to access the elements within the
> -vector: attempting to do so instead gives you the vec itself (for vec[0]),
> -or a (probably) invalid cast to vec<> for the memory after the vec (for
> -vec[1] onwards).
> +Direct instances of vec<> are also supported (their addresses are
> +prefixed with the '@' sign).
>  
> -Instead (for now) you must access m_vecdata:
> +In case you have a vec<> pointer, to use array notation [] to access the
> +elements within the vector you must first dereference the pointer, just
> +as you do in C:
> +  (gdb) p (*bb->succs)[0]
> +  $50 = (edge_def *&) @0x7ffff7656c38: <edge 0x7ffff7654ab0 (9 -> 10)>
> +  (gdb) p bb->succs[0][0]
> +  $51 = (edge_def *&) @0x7ffff7656c38: <edge 0x7ffff7654ab0 (9 -> 10)>

That's a nice improvement.

If the user erroneously attempts, say:

  (gdb) p bb->succs[2]

are they still accessing whatever's in memory after the vector pointer?
If so, I think the comment needs to retain some kind of warning about
this, as it's a nasty "gotcha".

> +Another option is to access m_vecdata:
>    (gdb) p bb->preds->m_vecdata[0]
>    $20 = <edge 0x7ffff044d380 (3 -> 5)>
>    (gdb) p bb->preds->m_vecdata[1]

[...]

> @@ -441,6 +447,10 @@ class VecPrinter:
>      #    -ex "up" -ex "p bb->preds"
>      def __init__(self, gdbval):
>          self.gdbval = gdbval
> +        if self.gdbval.type.code == gdb.TYPE_CODE_PTR:
> +            self.ptr = intptr(self.gdbval)
> +        else:
> +            self.ptr = None

I think this could use a comment.

If we have a NULL "vec *", then presumably self.ptr will be 0...

>      def display_hint (self):
>          return 'array'
> @@ -448,14 +458,25 @@ class VecPrinter:
>      def to_string (self):
>          # A trivial implementation; prettyprinting the contents is done
>          # by gdb calling the "children" method below.
> -        return '0x%x' % intptr(self.gdbval)
> +        if self.ptr:
> +            return '0x%x' % self.ptr
> +        else:
> +            return '@0x%x' % intptr(self.gdbval.address)

...and if we have a NULL vec *, with self.ptr as 0, it will take the
"else" path here, which I think is meant for the non-pointer case.

Should the condition be "if self.ptr is not None"?

>      def children (self):
> -        if intptr(self.gdbval) == 0:
> +        if self.ptr == 0:
>              return
> -        m_vecpfx = self.gdbval['m_vecpfx']
> +        m_vec = self.gdbval
> +        # There are 2 kinds of vec layouts: vl_embed, and vl_ptr.  The latter
> +        # is implemented with the former stored in the m_vec field.  Sadly,
> +        # template_argument(2) won't work: `gdb.error: No type named vl_embed`.
> +        try:
> +            m_vec = m_vec['m_vec']
> +        except:
> +            pass

I don't like the use of naked "except"; I always worry it might
silently swallow a SyntaxError at some later revision of the code.  Can
the code be more specific about what exception class(es) to
swallow?  (probably just gdb.Error)

[...]

FWIW I'm wishing we had automated test coverage for gdbhooks.py, but
that's probably overkill, I'm not sure how to go about doing it.

Dave