Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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
> > > > > > > >
> > > > > > > > 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 *) + 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#");
> > > 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
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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
> > > > > > >
> > > > > > > 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 *) + 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#");
> > 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
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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
> > > > > > >
> > > > > > > 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 *) + 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#");
> > 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
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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
> > > > > >
> > > > > > 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 *) + 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#");
> 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);
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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#");
> > 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
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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
> > > > > >
> > > > > > 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 *) + 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#");
> 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, >message);
> > >
> > > ^^^ this is the path affected by the patch
> > >
> > > (*diagnostic_starter (context)) (context, diagnostic);
> > >
> > > ^^^
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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#");
> 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
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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
> > > > >
> > > > > 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 *) + 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#");
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, >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,
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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 > > > > > > > > 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 *) + 8B].D.6750.coeffs[0] Is there a minimal reproducer (or a .i file?) Dave > Most specifically > > pp_format (context->printer, >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. >
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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 > >> > >> 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, >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.
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 *)].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 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
Re: [PATCH] middle-end/97855 - fix diagnostic with default pretty printer
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 > *)].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 > > 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
