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 <rguent...@suse.de> > > > > > > > > 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] Is there a minimal reproducer (or a .i file?) 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. >
