* [PATCH] middle-end/97855 - fix diagnostic with default pretty printer @ 2021-03-01 8:39 Richard Biener 2021-03-02 16:52 ` Jeff Law 0 siblings, 1 reply; 13+ messages in thread From: Richard Biener @ 2021-03-01 8:39 UTC (permalink / raw) To: gcc-patches; +Cc: dmalcolm, jakub 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 <rguenther@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. --- gcc/tree-diagnostic.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c index 95b8ef30070..f124e627aad 100644 --- a/gcc/tree-diagnostic.c +++ b/gcc/tree-diagnostic.c @@ -300,7 +300,11 @@ default_tree_printer (pretty_printer *pp, text_info *text, const char *spec, pp_string (pp, n); } else - dump_generic_node (pp, t, 0, TDF_SLIM, 0); + { + pretty_printer buffer; + dump_generic_node (&buffer, t, 0, TDF_SLIM, 0); + pp_string (pp, pp_formatted_text (&buffer)); + } return true; } -- 2.26.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-01 8:39 [PATCH] middle-end/97855 - fix diagnostic with default pretty printer Richard Biener @ 2021-03-02 16:52 ` Jeff Law 2021-03-02 20:36 ` Martin Sebor 0 siblings, 1 reply; 13+ messages in thread From: Jeff Law @ 2021-03-02 16:52 UTC (permalink / raw) To: Richard Biener, gcc-patches; +Cc: jakub 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 <rguenther@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. OK jeff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-02 16:52 ` Jeff Law @ 2021-03-02 20:36 ` Martin Sebor 2021-03-03 7:48 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: Martin Sebor @ 2021-03-02 20:36 UTC (permalink / raw) To: Jeff Law, Richard Biener, gcc-patches; +Cc: jakub 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 <rguenther@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. Martin > > OK > > jeff > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-02 20:36 ` Martin Sebor @ 2021-03-03 7:48 ` Richard Biener 2021-03-03 15:00 ` David Malcolm 0 siblings, 1 reply; 13+ messages in thread From: Richard Biener @ 2021-03-03 7:48 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, gcc-patches, Jakub Jelinek, dmalcolm 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 <rguenther@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. 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-03 7:48 ` Richard Biener @ 2021-03-03 15:00 ` David Malcolm 2021-03-03 15:23 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: David Malcolm @ 2021-03-03 15:00 UTC (permalink / raw) To: Richard Biener, Martin Sebor; +Cc: Jeff Law, gcc-patches, Jakub Jelinek 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 <rguenther@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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-03 15:00 ` David Malcolm @ 2021-03-03 15:23 ` Richard Biener 2021-03-03 15:52 ` Jakub Jelinek ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Richard Biener @ 2021-03-03 15:23 UTC (permalink / raw) To: David Malcolm; +Cc: Martin Sebor, Jeff Law, gcc-patches, Jakub Jelinek 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 <rguenther@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] 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 <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-03 15:23 ` Richard Biener @ 2021-03-03 15:52 ` Jakub Jelinek 2021-03-03 17:45 ` David Malcolm 2021-03-04 8:39 ` Richard Biener 2 siblings, 0 replies; 13+ messages in thread From: Jakub Jelinek @ 2021-03-03 15:52 UTC (permalink / raw) To: Richard Biener; +Cc: David Malcolm, Martin Sebor, Jeff Law, gcc-patches On Wed, Mar 03, 2021 at 04:23:59PM +0100, Richard Biener wrote: > 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. Sure, { pp_string (pp, "D#"); pp_decimal_int (pp, DEBUG_TEMP_UID (node)); } (or pp_wide_int) looks like the way to go. But dump_decl_name has several such pp_printf calls. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-03 15:23 ` Richard Biener 2021-03-03 15:52 ` Jakub Jelinek @ 2021-03-03 17:45 ` David Malcolm 2021-03-03 18:02 ` Jakub Jelinek 2021-03-04 8:39 ` Richard Biener 2 siblings, 1 reply; 13+ messages in thread From: David Malcolm @ 2021-03-03 17:45 UTC (permalink / raw) To: Richard Biener; +Cc: Martin Sebor, Jeff Law, gcc-patches, Jakub Jelinek On Wed, 2021-03-03 at 16:23 +0100, Richard Biener wrote: > 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 <rguenther@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] > > 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. Wouldn't that print D#6750 rather than D.6750 ? > 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. > > > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-03 17:45 ` David Malcolm @ 2021-03-03 18:02 ` Jakub Jelinek 0 siblings, 0 replies; 13+ messages in thread From: Jakub Jelinek @ 2021-03-03 18:02 UTC (permalink / raw) To: David Malcolm; +Cc: Richard Biener, Martin Sebor, Jeff Law, gcc-patches On Wed, Mar 03, 2021 at 12:45:54PM -0500, David Malcolm wrote: > > 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. > > Wouldn't that print > D#6750 > rather than > D.6750 Sure. The D.6750 case is a few lines later: pp_printf (pp, "%c%c%u", c, uid_sep, DECL_UID (node)); so we'd use pp_character (pp, c); pp_character (pp, uid_sep); pp_decimal_int (pp, DECL_UID (node)); for that one. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-03 15:23 ` Richard Biener 2021-03-03 15:52 ` Jakub Jelinek 2021-03-03 17:45 ` David Malcolm @ 2021-03-04 8:39 ` Richard Biener 2021-03-04 12:24 ` Richard Biener 2021-03-04 14:20 ` David Malcolm 2 siblings, 2 replies; 13+ messages in thread From: Richard Biener @ 2021-03-04 8:39 UTC (permalink / raw) To: David Malcolm; +Cc: Martin Sebor, Jeff Law, gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 5397 bytes --] On Wed, 3 Mar 2021, Richard Biener wrote: > 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 <rguenther@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] > > 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. It ICEs _a_ _lot_. diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c index ade1933f86a..7755157a7d7 100644 --- a/gcc/pretty-print.c +++ b/gcc/pretty-print.c @@ -1069,6 +1069,11 @@ static const char *get_end_url_string (pretty_printer *); void pp_format (pretty_printer *pp, text_info *text) { + /* pp_format is not reentrant. */ + static bool in_pp_format; + gcc_checking_assert (!in_pp_format); + in_pp_format = true; + output_buffer *buffer = pp_buffer (pp); const char *p; const char **args; @@ -1500,6 +1505,8 @@ pp_format (pretty_printer *pp, text_info *text) buffer->line_length = old_line_length; pp_wrapping_mode (pp) = old_wrapping_mode; pp_clear_state (pp); + + in_pp_format = false; } /* Format of a message pointed to by TEXT. */ testresult summary attached (but it passes bootstrap). Richard. [-- Attachment #2: test summary --] [-- Type: application/x-xz, Size: 48936 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-04 8:39 ` Richard Biener @ 2021-03-04 12:24 ` Richard Biener 2021-03-04 14:20 ` David Malcolm 1 sibling, 0 replies; 13+ messages in thread From: Richard Biener @ 2021-03-04 12:24 UTC (permalink / raw) To: David Malcolm; +Cc: Martin Sebor, Jeff Law, gcc-patches, Jakub Jelinek On Thu, 4 Mar 2021, Richard Biener wrote: > On Wed, 3 Mar 2021, Richard Biener wrote: > > > 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 <rguenther@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] > > > > 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. > > It ICEs _a_ _lot_. > > diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c > index ade1933f86a..7755157a7d7 100644 > --- a/gcc/pretty-print.c > +++ b/gcc/pretty-print.c > @@ -1069,6 +1069,11 @@ static const char *get_end_url_string > (pretty_printer *); > void > pp_format (pretty_printer *pp, text_info *text) > { > + /* pp_format is not reentrant. */ > + static bool in_pp_format; > + gcc_checking_assert (!in_pp_format); > + in_pp_format = true; > + > output_buffer *buffer = pp_buffer (pp); > const char *p; > const char **args; > @@ -1500,6 +1505,8 @@ pp_format (pretty_printer *pp, text_info *text) > buffer->line_length = old_line_length; > pp_wrapping_mode (pp) = old_wrapping_mode; > pp_clear_state (pp); > + > + in_pp_format = false; > } > > /* Format of a message pointed to by TEXT. */ > > testresult summary attached (but it passes bootstrap). OK, and after the patch and better recursion checking for the above (use pp->in_pp_format) the only ICE that remains is for FAIL: gcc.dg/noncompile/pr79758.c -O0 (test for warnings, line 5) FAIL: gcc.dg/noncompile/pr79758.c -O0 1 blank line(s) in output FAIL: gcc.dg/noncompile/pr79758.c -O0 (internal compiler error) FAIL: gcc.dg/noncompile/pr79758.c -O0 (test for excess errors) /home/rguenther/src/trunk/gcc/testsuite/gcc.dg/noncompile/pr79758.c:4:15: error: 'a' undeclared here (not in a function)^M /home/rguenther/src/trunk/gcc/testsuite/gcc.dg/noncompile/pr79758.c:5:6: error: redefinition of 'fn1'^M 'void(<type-error>^M ^M Internal compiler error: Error reporting routines re-entered.^M 0x807e00 pp_format(pretty_printer*, text_info*)^M /home/rguenther/src/trunk/gcc/pretty-print.c:1073^M 0x19a6556 diagnostic_report_diagnostic(diagnostic_context*, diagnostic_info*)^M /home/rguenther/src/trunk/gcc/diagnostic.c:1244^M 0x19a6ace diagnostic_impl^M /home/rguenther/src/trunk/gcc/diagnostic.c:1406^M 0x19a6e17 internal_error(char const*, ...)^M /home/rguenther/src/trunk/gcc/diagnostic.c:1808^M 0x807315 fancy_abort(char const*, int, char const*)^M /home/rguenther/src/trunk/gcc/diagnostic.c:1907^M 0x807e00 pp_format(pretty_printer*, text_info*)^M /home/rguenther/src/trunk/gcc/pretty-print.c:1073^M 0x19c4ae0 pp_format_verbatim(pretty_printer*, text_info*)^M /home/rguenther/src/trunk/gcc/pretty-print.c:1542^M 0x19c4ae0 pp_verbatim(pretty_printer*, char const*, ...)^M /home/rguenther/src/trunk/gcc/pretty-print.c:1798^M 0x8f5be1 pp_c_parameter_type_list(c_pretty_printer*, tree_node*)^M /home/rguenther/src/trunk/gcc/c-family/c-pretty-print.c:544^M 0x8f7df7 c_pretty_printer::direct_abstract_declarator(tree_node*)^M /home/rguenther/src/trunk/gcc/c-family/c-pretty-print.c:591^M 0x860c62 print_type^M /home/rguenther/src/trunk/gcc/c/c-objc-common.c:198^M 0x860fd5 c_tree_printer^M /home/rguenther/src/trunk/gcc/c/c-objc-common.c:310^M 0x860fd5 c_tree_printer^M /home/rguenther/src/trunk/gcc/c/c-objc-common.c:254^M 0x19c2a6c pp_format(pretty_printer*, text_info*)^M /home/rguenther/src/trunk/gcc/pretty-print.c:1479^M 0x19a6556 diagnostic_report_diagnostic(diagnostic_context*, diagnostic_info*)^M /home/rguenther/src/trunk/gcc/diagnostic.c:1244^M 0x19a9337 diagnostic_impl^M /home/rguenther/src/trunk/gcc/diagnostic.c:1406^M 0x19a9337 inform(unsigned int, char const*, ...)^M /home/rguenther/src/trunk/gcc/diagnostic.c:1485^M 0x816eee diagnose_mismatched_decls^M /home/rguenther/src/trunk/gcc/c/c-decl.c:2282^M 0x818624 duplicate_decls^M /home/rguenther/src/trunk/gcc/c/c-decl.c:2950^M 0x81afe9 pushdecl(tree_node*)^M /home/rguenther/src/trunk/gcc/c/c-decl.c:3143^M Please submit a full bug report,^M with preprocessed source if appropriate.^M Please include the complete backtrace with any bug report.^M See <https://gcc.gnu.org/bugs/> for instructions.^M which happens because #define pp_unsupported_tree(PP, T) \ pp_verbatim (PP, "%qs not supported by %s", \ get_tree_code_name (TREE_CODE (T)), __FUNCTION__) going to fix that. Richard ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-04 8:39 ` Richard Biener 2021-03-04 12:24 ` Richard Biener @ 2021-03-04 14:20 ` David Malcolm 2021-03-04 14:21 ` David Malcolm 1 sibling, 1 reply; 13+ messages in thread From: David Malcolm @ 2021-03-04 14:20 UTC (permalink / raw) To: Richard Biener; +Cc: Martin Sebor, Jeff Law, gcc-patches, Jakub Jelinek On Thu, 2021-03-04 at 09:39 +0100, Richard Biener wrote: > On Wed, 3 Mar 2021, Richard Biener wrote: > > > 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:1 > > > > > > > 18: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 <rguenther@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] > > > > 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. > > It ICEs _a_ _lot_. > > diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c > index ade1933f86a..7755157a7d7 100644 > --- a/gcc/pretty-print.c > +++ b/gcc/pretty-print.c > @@ -1069,6 +1069,11 @@ static const char *get_end_url_string > (pretty_printer *); > void > pp_format (pretty_printer *pp, text_info *text) > { > + /* pp_format is not reentrant. */ > + static bool in_pp_format; > + gcc_checking_assert (!in_pp_format); > + in_pp_format = true; > + The issue happens when pp_printf is re-entered on the same pretty_printer instance. I think it's safe (although not particularly efficient) to have pp_printf calls leading to another pretty_printer being instantiated, printing with that, then pp_string (first_pp, pp_formatted_text (second_pp)); for example (like in your earlier patch), and the analyzer has code that does that (when generating messages for events, IIRC). So I think the flag would need to be a field in the pretty_printer rather than static in the function (and thus effectively global state). Dave > output_buffer *buffer = pp_buffer (pp); > const char *p; > const char **args; > @@ -1500,6 +1505,8 @@ pp_format (pretty_printer *pp, text_info *text) > buffer->line_length = old_line_length; > pp_wrapping_mode (pp) = old_wrapping_mode; > pp_clear_state (pp); > + > + in_pp_format = false; > } > > /* Format of a message pointed to by TEXT. */ > > testresult summary attached (but it passes bootstrap). > > Richard. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer 2021-03-04 14:20 ` David Malcolm @ 2021-03-04 14:21 ` David Malcolm 0 siblings, 0 replies; 13+ messages in thread From: David Malcolm @ 2021-03-04 14:21 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches On Thu, 2021-03-04 at 09:20 -0500, David Malcolm via Gcc-patches wrote: > On Thu, 2021-03-04 at 09:39 +0100, Richard Biener wrote: > > On Wed, 3 Mar 2021, Richard Biener wrote: > > > > > 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 > > > > > > > > :1 > > > > > > > > 18: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 <rguenther@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] > > > > > > 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. > > > > It ICEs _a_ _lot_. > > > > diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c > > index ade1933f86a..7755157a7d7 100644 > > --- a/gcc/pretty-print.c > > +++ b/gcc/pretty-print.c > > @@ -1069,6 +1069,11 @@ static const char *get_end_url_string > > (pretty_printer *); > > void > > pp_format (pretty_printer *pp, text_info *text) > > { > > + /* pp_format is not reentrant. */ > > + static bool in_pp_format; > > + gcc_checking_assert (!in_pp_format); > > + in_pp_format = true; > > + > > The issue happens when pp_printf is re-entered on the same > pretty_printer instance. I think it's safe (although not > particularly > efficient) to have pp_printf calls leading to another pretty_printer > being instantiated, printing with that, then > pp_string (first_pp, pp_formatted_text (second_pp)); > for example (like in your earlier patch), and the analyzer has code > that does that (when generating messages for events, IIRC). > > So I think the flag would need to be a field in the pretty_printer > rather than static in the function (and thus effectively global > state). ...and I see that you did that in a followup; sorry for the noise. > Dave > > > output_buffer *buffer = pp_buffer (pp); > > const char *p; > > const char **args; > > @@ -1500,6 +1505,8 @@ pp_format (pretty_printer *pp, text_info > > *text) > > buffer->line_length = old_line_length; > > pp_wrapping_mode (pp) = old_wrapping_mode; > > pp_clear_state (pp); > > + > > + in_pp_format = false; > > } > > > > /* Format of a message pointed to by TEXT. */ > > > > testresult summary attached (but it passes bootstrap). > > > > Richard. > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-03-04 14:21 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-01 8:39 [PATCH] middle-end/97855 - fix diagnostic with default pretty printer Richard Biener 2021-03-02 16:52 ` Jeff Law 2021-03-02 20:36 ` Martin Sebor 2021-03-03 7:48 ` Richard Biener 2021-03-03 15:00 ` David Malcolm 2021-03-03 15:23 ` Richard Biener 2021-03-03 15:52 ` Jakub Jelinek 2021-03-03 17:45 ` David Malcolm 2021-03-03 18:02 ` Jakub Jelinek 2021-03-04 8:39 ` Richard Biener 2021-03-04 12:24 ` Richard Biener 2021-03-04 14:20 ` David Malcolm 2021-03-04 14:21 ` David Malcolm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).