On Wed, 3 Mar 2021, David Malcolm wrote:
> On Wed, 2021-03-03 at 08:48 +0100, Richard Biener wrote:
> > On Tue, 2 Mar 2021, Martin Sebor wrote:
> >
> > > On 3/2/21 9:52 AM, Jeff Law via Gcc-patches wrote:
> > > >
> > > >
> > > > On 3/1/21 1:39 AM, Richard Biener wrote:
> > > > > The default diagnostic tree printer relies on dump_generic_node
> > > > > which
> > > > > for some reason manages to clobber the diagnostic pretty-
> > > > > printer state
> > > > > so we see garbled diagnostics like
> > > > >
> > > > > /home/rguenther/src/trunk/gcc/calls.c: In function
> > > > > 'expand_call':
> > > > > D.6750.coeffs[0]'/home/rguenther/src/trunk/gcc/dojump.c:118:28:
> > > > > warning:
> > > > > may be used uninitialized in this function [-Wmaybe-
> > > > > uninitialized]
> > > > >
> > > > > when the diagnostic is emitted by the LTO fronted. The
> > > > > following
> > > > > approach using a temporary pretty-printer for the
> > > > > dump_generic_node
> > > > > call fixes this for some unknown reason and we issue
> > > > >
> > > > > /home/rguenther/src/trunk/gcc/calls.c: In function
> > > > > 'expand_call':
> > > > > /home/rguenther/src/trunk/gcc/dojump.c:118:28: warning:
> > > > > 'MEM[(struct
> > > > > poly_int *)&save].D.6750.coeffs[0]' may be used uninitialized
> > > > > in this
> > > > > function [-Wmaybe-uninitialized]
> > > > >
> > > > > [LTO] Bootstrapped and tested on x86_64-unknown-linux-gnu, OK
> > > > > for trunk?
> > > > >
> > > > > Thanks,
> > > > > Richard.
> > > > >
> > > > > 2021-02-26 Richard Biener <[email protected]>
> > > > >
> > > > > PR middle-end/97855
> > > > > * tree-diagnostic.c (default_tree_printer): Use a temporary
> > > > > pretty-printer when formatting a tree via dump_generic_node.
> > > > It'd be good to know why this helps, but I trust your judgment
> > > > that this
> > > > is an improvement.
> > >
> > > I don't know if it's related but pr98492 tracks a problem in the
> > > C++
> > > front end caused by reinitializing the pretty printer in a number
> > > of
> > > functions in cp/error.c. When one of these functions is called
> > > while
> > > the pretty printer is formatting something, the effect of
> > > the reinitialization is to drop the already formatted contents
> > > of the printer's buffer.
> > >
> > > IIRC, I tripped over this when working on the MEM_REF formatting
> > > improvement for -Wuninitialized.
> >
> > I've poked quite a bit with breakpoints on suspicious pretty-printer
> > functions and watch points on the pp state but found nothing in the
> > case I was looking at (curiously also -Wuninitialized). But I also
> > wasn't able to understand why the caller should work at all. And
> > yes, the C/C++ tree printers also simply format to the passed
> > pretty-printer...
> >
> > Hoping that David could shed some light on how this should play
> > together.
>
> This looks very much like the issue I ran into in
> c46d057f55748520e819dcd8e04bca71be9902b2 (and, in retrospect, that
> commit may have just been papering over the problem).
>
> The issue there was that pp_printf is not reentrant - if a handler for
> a pp_printf format code ends up making a nested call to pp_printf, I
> got behavior that looks like what you're seeing.
>
> That said, I've been poring over the output in PR middle-end/97855 and
> comparing it to the various pretty-printer usage in the tree, and I'm
> not seeing anywhere where a pp_printf seems to be used when generating:
> MEM[(struct poly_int *)&save + 8B].D.6750.coeffs[0]
I think it's the D.6750 which is printed via
else if (TREE_CODE (node) == DEBUG_EXPR_DECL)
{
if (flags & TDF_NOUID)
pp_string (pp, "D#xxxx");
else
pp_printf (pp, "D#%i", DEBUG_TEMP_UID (node));
because this is a DECL_DEBUG_EXPR. One could experiment with
avoiding pp_printf in dump_decl_name.
> Is there a minimal reproducer (or a .i file?)
No, you need to do a LTO bootstrap, repeat the link step of
for example cc1 with -v -save-temps and pick an ltrans invocation
that exhibits the issue ...
I can poke at the above tomorrow again. I suppose we could
also add some checking-assert into the pp_printf code at
the problematic place (or is any recursion bogus?) to catch
the case with an ICE.
Richard.
> Dave
>
>
>
> > Most specifically
> >
> > pp_format (context->printer, &diagnostic->message);
> >
> > ^^^ this is the path affected by the patch
> >
> > (*diagnostic_starter (context)) (context, diagnostic);
> >
> > ^^^ this somehow messes things up, it does pp_set_prefix on
> > context->printer but also does some formatting
> >
> > pp_output_formatted_text (context->printer);
> >
> > and now we expect this to magically output the composed pieces.
> >
> > Note swapping the first two lines didn't have any effect (I don't
> > remember if it changed anything so details might have changed but
> > it was definitely still broken).
> >
> > That said, the only hint I have is that the diagnostic plus prefix
> > is quite long, but any problem in the generic code should eventually
> > affect non-LTO as well but the PR is reported for LTO only
> > (bogus diagnostics shown during LTO bootstrap). The patch fixes
> > all bogus diagnostics during LTO bootstrap.
> >
> > I originally thought there's maybe a pp_flush too much but maybe
> > there's a pp_flush missing ...
> >
> > I'll wait for Davids feedback but will eventually install the
> > patch to close the bug.
> >
> > Richard.
> >
>
>
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
