Vladislav Ivanishin <v...@ispras.ru> writes:
> David Malcolm <dmalc...@redhat.com> writes:
>
>> On Tue, 2019-07-02 at 14:29 +0300, Vladislav Ivanishin wrote:
>>> David Malcolm <dmalc...@redhat.com> writes:
>>>
>>> > On Mon, 2019-07-01 at 12:50 +0300, Vladislav Ivanishin wrote:
>>> > > Hi!
>>> > >
>>> > > GDB's Python API provides strip_typedefs method that can be
>>> > > instrumental for writing DRY code. Using it at least partially
>>> > > obviates the need for the add_printer_for_types method we have in
>>> > > gdbhooks.py (it takes a list of typenames to match and is usually
>>> > > used to deal with typedefs).
>>> > >
>>> > > I think, the code can be simplified further (there's still
>>> > > ['tree_node *', 'const tree_node *'], which is a little awkward)
>>> > > if we deal with pointer types in a uniform fashion (I'll touch on
>>> > > this in the description of the second patch). But that can be
>>> > > improved separately.
>>> > >
>>> > > The gimple_statement_cond, etc. part has been dysfunctional for a
>>> > > while (namely since gimple-classes-v2-option-3 branch was
>>> > > merged). I updated it to use the new classes' names. That seems
>>> > > to work (though it doesn't output much info anyway: pretty
>>> > > <gimple_phi 0x7ffff78c0200> vs.
>>> > > (gphi *) 0x7ffff78c0200
>>> > > in the raw version).
>>> > >
>>> > > I changed the name passed to pp.add_printer_for_types() for
>>> > > scalar_mode and friends -- so they all share the same name now --
>>> > > but I don't believe the name is used in any context where it
>>> > > would matter.
>>> >
>>> > IIRC there's a gdb command to tell you what the registered pretty-
>>> > printers are;
>>>
>>> Yeah, it's (gdb) info pretty-printer
>>>
>>> > I think the name is only ever used in that context (or maybe for
>>> > fine-grained control of pretty-printing) -
>>>
>>> From the gdb info manual:
>>> 'disable pretty-printer [OBJECT-REGEXP [NAME-REGEXP]]'
>>> Disable pretty-printers matching OBJECT-REGEXP and NAME-REGEXP.
>>>
>>> In our case, this is how to do it:
>>> (gdb) disable pretty-printer global gcc;scalar.*
>>>
>>> So, that would be a regression, if different modes were given
>>> different names on purpose (starts with r251467). Looks
>>> unintentional though, and the subprinter is very simple.
>>>
>>> I think, these scenarios exhaust the uses for the name attribute of a
>>> pretty printer.
>>>
>>> > and I don't know of anyone who uses that. I don't think changing
>>> > the name matters.
>>>
>>> Good. Neither do I :) Except after writing this I started using this
>>> feature myself. It provides a convenient way to isolate a faulty
>>> pretty-printer, or continue debugging without it. The manual says
>>> that "The consequences of a broken pretty-printer are severe enough
>>> that GDB provides support for enabling and disabling individual
>>> printers". Oh, were they right ... (see below).
>>
>> In any case, I have no objection to the change-of-names part of the
>> patch.
>>
>>> > > This is just a clean up of gdbhooks.py. OK to commit?
>>> >
>>> > The only area I wasn't sure about were the removal hunks relating
>>> > to machine modes: is that all covered now by the looking-through-
>>> > typedefs?
>>>
>>> Yes, I checked (using debug output) that all the modes for which I
>>> removed explicit handling are correctly expanded to either
>>> opt_mode<>, or pod_mode<>.
>>>
>>> > Otherwise, looks good to me. I assume you've tested the patch by
>>> > debugging with it.
>>>
>>> Yeah, I thought my debugging with the patch should have given it
>>> reasonable coverage.
>>>
>>> However, I had not previously actually tested debugging code
>>> involving modes, so I went ahead and did it. And I am seeing a
>>> segfault with RtxPrinter now (-ex "b reg_nonzero_bits_for_combine"
>>> -ex "r" -ex "bt"). Delving in...
>>>
>>> The rtx type (for which the unqualified form is also 'rtx') was not
>>> being matched by any of the printers. The typedef-stripped form is
>>> 'rtx_def *'. It matches now and uncovers a bug in the subprinter(?)
>>>
>>> I dug into it and the impression I have so far is that the
>>> print_inline_rtx call upsets gdb's frame unwinders... Upon the "bt"
>>> command, GDB itself produces more than 157000 frames before hitting
>>> the segfault.
>>
>> Sounds like an infinite recursion (possibly a mutual recursion
>> involving gdb repeatedly injecting new frames into the inferior cc1, or
>> could just be an infinite recursion in the inferior).
>
> It was an infinite recursion, all inside gdb.
>
>>> Have you seen anything like that before? It would be nice to
>>> understand the root cause of this bug. I am going to spend some more
>>> time on it, but I've no prior experience in debugging gdb internals.
>>>
>>> As a workaround, I'd propose perhaps removing the kludge, but the
>>> output looks nice, while the alternative implementation (after
>>> return) is not as informative.
>>
>> Yeah, the rtx-printing is a kludge.
>>
>> The gdbhooks.py code isn't well tested. I wonder if what you're seeing
>> is a regression, or you're simply uncovering an existing bug.
>
> Talking to Alex made me realize that pretty printing function arguments
> in a backtrace is fundamentally problematic. Consider a function that
> is passed a pointer whose pointed-to value gets clobbered in a callee of
> that function. The pretty-printer would access garbage through that
> pointer and that would lead to incorrect results at best and to screwing
> up the whole process at worst (I think, this is the root cause of the
> bug I described).
>
> GDB seems to have some provision for that:
>
> Function: pretty_printer.to_string (self)
> [...]
> Also, depending on the print settings (see Print Settings), the CLI
> may print just the result of to_string in a stack trace, omitting
> the result of children.
>
> And in 'Print Settings' I learned that there's the `set print
> frame-arguments all|scalars|none` command. The default setting is
> "scalars", and that makes sense in our situation, but the deal is
> pointers - 'rtx_def *' in particular - are (rightfully) considered
> scalars.
>
> I tried moving the call to print_inline_rtx from RtxPrinter.to_string()
> to RtxPrinter.children(), but whenever the former is called, the latter
> gets called, too (regardless of display_hint).
>
> So I think, the right direction forward here is to always handle the
> pointed-to types in pretty printers and only do simple forwarding to
> them for pointers. That way we stay aligned with GDB's printing only
> scalar types in frame infos and keep backtraces safe (at the cost of
> perhaps making them less informative).
>
> Hope I make sense.
>
> Anyway, I wrote up a patch that does that. Do you like this approach?
>
> The patch is against trunk, but it requires the
> - type_ = gdbval.type.unqualified()
> + type_ = gdbval.type.unqualified().strip_typedefs()
> change to reliably show visible effect.
>
> diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
> index a7e665cadb9..15f9738aeee 100644
> --- a/gcc/gdbhooks.py
> +++ b/gcc/gdbhooks.py
> @@ -411,8 +411,8 @@ class RtxPrinter:
> string for gdb
> """
> # We use print_inline_rtx to avoid a trailing newline
> - gdb.execute('call print_inline_rtx (stderr, (const_rtx) %s, 0)'
> - % intptr(self.gdbval))
> + my_debug_rtx =
> gdb.lookup_global_symbol('my_debug_inline_rtx').value()
> + my_debug_rtx(self.gdbval)
> return ''
>
> # or by hand; based on gcc/print-rtl.c:print_rtx
> @@ -428,6 +428,27 @@ class RtxPrinter:
>
> ######################################################################
>
> +class PtrPrinter:
> + def __init__(self, gdbval):
> + self.gdbval = gdbval
> +
> + def to_string (self):
> + """
> + Print the pointer value. We forward to the pretty-printer for
> + the pointed-to value in children().
> + """
> + return '0x%x' % intptr(self.gdbval)
> +
> + def display_hint (self):
> + # The choice of displayhint is based purely on aesthetics (it is not
> + # ideal, but the options are limited).
> + return 'array'
> +
> + def children (self):
> + yield ('pointed-to', self.gdbval.dereference())
> +
> +######################################################################
> +
> class PassPrinter:
> def __init__(self, gdbval):
> self.gdbval = gdbval
> @@ -590,7 +611,8 @@ def build_pretty_printer():
> pp.add_printer_for_types(['edge_def *'],
> 'edge',
> CfgEdgePrinter)
> - pp.add_printer_for_types(['rtx_def *'], 'rtx_def', RtxPrinter)
> + pp.add_printer_for_types(['rtx_def *'], 'rtx_def-forwarder', PtrPrinter)
> + pp.add_printer_for_types(['rtx_def'], 'rtx_def', RtxPrinter)
> pp.add_printer_for_types(['opt_pass *'], 'opt_pass', PassPrinter)
>
> pp.add_printer_for_regex(r'vec<.*> \*',
> diff --git a/gcc/print-rtl.c b/gcc/print-rtl.c
> index fbb108568b3..2783e921a75 100644
> --- a/gcc/print-rtl.c
> +++ b/gcc/print-rtl.c
> @@ -1023,6 +1023,15 @@ debug (const rtx_def &ref)
> debug_rtx (&ref);
> }
>
> +/* Copy of the above, inlined and w/o a newline, under a different name. */
> +
> +DEBUG_FUNCTION void
> +my_debug_inline_rtx (const rtx_def &ref)
> +{
> + rtx_writer w (stderr, 0, false, false, NULL);
> + w.print_rtx (&ref);
> +}
> +
> DEBUG_FUNCTION void
> debug (const rtx_def *ptr)
> {
>
>
> We can also get 'const rtx_def *' to be pretty printed with e.g.
>
> diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
> index 4f0f9688b35..98c6e13197a 100644
> --- a/gcc/gdbhooks.py
> +++ b/gcc/gdbhooks.py
> @@ -600,7 +626,7 @@ def build_pretty_printer():
> pp.add_printer_for_types(['edge', 'edge_def *'],
> 'edge',
> CfgEdgePrinter)
> - pp.add_printer_for_regex(r'.* rtx_def *', 'rtx_def-forwarder',
> PtrPrinter)
> + pp.add_printer_for_regex(r'.*rtx_def \*', 'rtx_def-forwarder',
> PtrPrinter)
> pp.add_printer_for_types(['rtx_def'], 'rtx_def', RtxPrinter)
> pp.add_printer_for_types(['opt_pass *'], 'opt_pass', PassPrinter)
>
> Here's a debugging session and output I got with the patch.
>
> $ gdb -ex "b reg_nonzero_bits_for_combine" \
> -ex "r" \
> -ex "f 4" \
> -ex "p x" \
> --args ./cc1 -O /mnt/co/gcc/gcc/testsuite/gcc.dg/Wunused-var-3.c
>
> Breakpoint 5, reg_nonzero_bits_for_combine (x=0x7ffff78cae70 = {...},
> xmode=DImode, mode=DImode, nonzero=0x7fffffffdcf0) at
> /mnt/co/gcc/gcc/combine.c:10271
> 10271 {
> #4 set_nonzero_bits_and_sign_copies (x=0x7ffff78d2018 = {...},
> set=<optimized out>, data=<optimized out>) at /mnt/co/gcc/gcc/combine.c:1743
> 1743 set_nonzero_bits_and_sign_copies (rtx x, const_rtx set, void *data)
> $1 = 0x7ffff78d2018 = {(reg:DI 86)}
Hi David,
I'd like to ping the patch in the previous message of this thread
(https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00188.html).
The issue it solves (infinite recursion and crash in gdb upon "bt")
blocks the original 1/2 patch. And the 2/2 (vec support) - which you
also reviewed <https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00063.html>
(thank you), but I haven't yet updated - depends on that.
--
Vlad
