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 <rguent...@suse.de>
> > > > > > > >
> > > > > > > > PR middle-end/97855
> > > > > > > > * tree-diagnostic.c (default_tree_printer): Use a
> > > > > > > > temporary
> > > > > > > > pretty-printer when formatting a tree via
> > > > > > > > dump_generic_node.
> > > > > > > It'd be good to know why this helps, but I trust your
> > > > > > > judgment
> > > > > > > that this
> > > > > > > is an improvement.
> > > > > >
> > > > > > I don't know if it's related but pr98492 tracks a problem
> > > > > > in
> > > > > > the
> > > > > > C++
> > > > > > front end caused by reinitializing the pretty printer in a
> > > > > > number
> > > > > > of
> > > > > > functions in cp/error.c. When one of these functions is
> > > > > > called
> > > > > > while
> > > > > > the pretty printer is formatting something, the effect of
> > > > > > the reinitialization is to drop the already formatted
> > > > > > contents
> > > > > > of the printer's buffer.
> > > > > >
> > > > > > IIRC, I tripped over this when working on the MEM_REF
> > > > > > formatting
> > > > > > improvement for -Wuninitialized.
> > > > >
> > > > > I've poked quite a bit with breakpoints on suspicious pretty-
> > > > > printer
> > > > > functions and watch points on the pp state but found nothing
> > > > > in
> > > > > the
> > > > > case I was looking at (curiously also -Wuninitialized). But
> > > > > I
> > > > > also
> > > > > wasn't able to understand why the caller should work at all.
> > > > > And
> > > > > yes, the C/C++ tree printers also simply format to the passed
> > > > > pretty-printer...
> > > > >
> > > > > Hoping that David could shed some light on how this should
> > > > > play
> > > > > together.
> > > >
> > > > This looks very much like the issue I ran into in
> > > > c46d057f55748520e819dcd8e04bca71be9902b2 (and, in retrospect,
> > > > that
> > > > commit may have just been papering over the problem).
> > > >
> > > > The issue there was that pp_printf is not reentrant - if a
> > > > handler for
> > > > a pp_printf format code ends up making a nested call to
> > > > pp_printf, I
> > > > got behavior that looks like what you're seeing.
> > > >
> > > > That said, I've been poring over the output in PR middle-
> > > > end/97855 and
> > > > comparing it to the various pretty-printer usage in the tree,
> > > > and
> > > > I'm
> > > > not seeing anywhere where a pp_printf seems to be used when
> > > > generating:
> > > > MEM[(struct poly_int *)&save + 8B].D.6750.coeffs[0]
> > >
> > > 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.
>
>
