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)}
--
Vlad
