| Message ID | nycvar.YFH.7.76.2103010938020.5511@elmra.sevgm.obk |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end/97855 - fix diagnostic with default pretty printer | expand |
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
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 >
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.
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. >
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. > > > > >
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
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. > > > > > > > > > >
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
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.
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
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.
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. > >
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; }