diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]

  Currently, the ipa-free-lang-data pass resets most of the frontend's
diagnostic customizations, such as the diagnostic_finalizer that prints macro
expansion information, which is the subject of the two PRs. In most cases,
however, there is no need to reset these customizations; they still work just
fine after the language-specific data has been freed. (Macro tracking
information, for instance, only depends on the line_maps instance and does not
use the tree data structures at all.)

Add an interface whereby frontends can convey which of their customizations
should be preserved by ipa-free-lang-data. Only the macro tracking behavior is
changed for now.  Subsequent patches will add further configurations for each
frontend.

gcc/ChangeLog:

	PR lto/106274
	PR middle-end/101551
	* diagnostic.h (struct diagnostic_context): Add new customization
	point diagnostic_context::preserve_on_reset.
	* diagnostic.cc (diagnostic_initialize): Initialize it.
	* tree-diagnostic.h (tree_diagnostics_defaults): Add new optional
	argument enable_preserve.
	* tree-diagnostic.cc (tree_diagnostics_defaults): Implement it.
	* ipa-free-lang-data.cc (free_lang_data): Use it.

gcc/c-family/ChangeLog:

        PR lto/106274
        PR middle-end/101551
	* c-opts.cc (c_common_diagnostics_set_defaults): Preserve
	diagnostic finalizer for the middle end.

libgomp/ChangeLog:

        PR lto/106274
        PR middle-end/101551
	* testsuite/libgomp.oacc-c-c++-common/reduction-5.c: Remove
	now-unnecessary workaround.
	* testsuite/libgomp.oacc-c-c++-common/vred2d-128.c: Likewise.

gcc/testsuite/ChangeLog:

        PR lto/106274
        PR middle-end/101551
	* c-c++-common/diag-after-fld-1.c: New test.
---

Notes:
    Hello-
    
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101551
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106274
    
    PR101551 complains that when compiling with offloading enabled, diagnostics
    output changes in several different ways. PR106274 notes the same thing (which
    has the same cause) when compiling with -flto, for the specific case of macro
    expansion tracking, which is no longer output for middle-end diagnostics when
    -flto is present. Restoring the macro tracking information, at least, can be
    done simply, which is the attached patch. This is straightforward because the
    printing of macro tracking information is not really reliant on the sort of
    language-specific data structures that are freed by ipa-free-lang-data... it
    just needs the line-maps API, which is common to all languages and which is
    not impacted by the work done by ipa-free-lang-data.
    
    To be clear, this is not about diagnostics issued *by* the lto frontend. This
    is just about diagnostics issued by the language frontends, in case they were
    asked to stream the IL for later use by the lto frontend. I think from the
    user's perspective, compiling with or without -flto should not change the
    quality of diagnostics, since on the face of it, -flto seems to be just a
    request for the compiler to do something extra (write out the IL in addition
    to all the other stuff it does), and it's not clear why this should change the
    way diagnostics look. (I understand there must be some good reasons, why it's
    not the case.)  So anyway I was focusing on how to keep the diagnostics as
    close as possible to their normal form after ipa-free-lang-data is done.
    
    The approach I took was to add a new variable "preserve_on_reset" in the
    diagnostic_context, allowing a frontend to specify whether its diagnostic
    context customizations are safe to leave in place for the middle end. There
    are currently four customizations for which preservation can be enabled:
    
        * diagnostic starter
        * diagnostic finalizer
        * format decoder
        * decl_printable_name langhook
    
    Preserving the diagnostic finalizer is sufficient to restore the output of
    macro tracking information, and that's what I did in this patch. But it seems
    that it's possible to go a bit farther than this as well. Here are some
    examples:
    
    1. Fortran:
        Consider the existing testcase
        gfortran.dg/allocatable_uninitialized_1.f90. When compiled without -flto,
        it outputs:
    
        ===========================
        allocatable_uninitialized_1.f90:6:47:
    
            6 |    a(1)=2*b(1) ! { dg-warning "uninitialized" }
              |                                               ^
        Warning: 'b.offset' is used uninitialized [-Wuninitialized]
        allocatable_uninitialized_1.f90:4:30:
    
            4 |   real,allocatable:: a(:),b(:)
              |                              ^
        note: 'b' declared here
        allocatable_uninitialized_1.f90:6:47:
    
            6 |    a(1)=2*b(1) ! { dg-warning "uninitialized" }
              |                                               ^
        Warning: 'a.offset' is used uninitialized [-Wuninitialized]
        allocatable_uninitialized_1.f90:4:25:
    
            4 |   real,allocatable:: a(:),b(:)
              |                         ^
        note: 'a' declared here
        ===========================
    
        When compiled with -flto, it outputs instead:
        ===========================
        allocatable_uninitialized_1.f90: In function 'MAIN__':
        allocatable_uninitialized_1.f90:6:47: warning: 'b.offset' is used uninitialized in this function [-Wuninitialized]
            6 |    a(1)=2*b(1) ! { dg-warning "uninitialized" }
              |                                               ^
        allocatable_uninitialized_1.f90:6:47: warning: 'a.offset' is used uninitialized in this function [-Wuninitialized]
        ===========================
    
        So the informative notes are missing, and a rather relevant source line is
        omitted, plus the internal name 'MAIN__' ends up in the output as well.
    
        Assuming the current patch goes in, then this incremental patch (not
        proposing it yet) would make the diagnostics the same with or without
        -flto:
    
    --- a/gcc/fortran/error.cc
    +++ b/gcc/fortran/error.cc
    @@ -1641,6 +1641,12 @@ gfc_diagnostics_init (void)
          management when using gfc_push/pop_error. */
       pp_error_buffer = &(error_buffer.buffer);
       pp_error_buffer->flush_p = false;
    +
    +  /* Our diagnostic customizations are safe for the middle end.  */
    +  auto &p = global_dc->preserve_on_reset;
    +  p[diagnostic_context::PRESERVE_STARTER] = true;
    +  p[diagnostic_context::PRESERVE_FINALIZER] = true;
    +  p[diagnostic_context::PRESERVE_FORMAT_DECODER] = true;
    
        I have a testcase for it, but I'm a bit hesistant to suggest this change
        because I worry about the fragility that it introduces. Setting
        preserve_on_reset is a guarantee that the diagnostic customizations won't be
        broken by ipa-free-lang-data, but there's no real mechanism to insure the
        guarantee is honored (although to the best I can tell, it is currently), and
        most of the testsuite does not run with -flto so it's not covered well that
        way.
    
    2. C++:
    
        Another major way that diagnostics change with -flto is due to the
        resetting of the decl_printable_name langhook. For example, a template
        function normally described as:
    
        int f(void*) [with T = void]
    
        becomes instead:
    
        f<void>(void*)int
    
        The nicer output can be restored by preserving more customizations. I have
        a patch for that with a long testcase too, which I can send if there is
        interest, however I have all the same reservations about that which I
        mentioned above, except moreso because the patch needs to do more -- just
        out of the box, the C++ customizations don't work after
        ipa-free-lang-data. They need a small tweak to understand that after the
        pass, member functions other than destructors no longer satisfy
        DECL_CLASS_SCOPE_P, and so they need to be printed by falling back to
        gimple_decl_printable_name instead. Here I worry it's more likely that it
        could silently break in the future, and it wouldn't be noticed right away.
    
    3. ObjC:
    
       ObjC customizes decl_printable_name so that a member function can be
       printed e.g. as '[T f]' instead of '_c_T__f'.  In this case it seems pretty
       clear that preserving the decl_printable_name would be fine, but the upside
       is not that large either.  I have a patch + testcase for that if there is
       interest.
    
    Bootstrap + regtest all languages looks good on x86-64 Linux. There turned out
    not to be any tests in the testsuite relying on the presence or lack of macro
    tracking information with -flto. I added some coverage for that. A few other
    tests for libgomp were explicitly working around the current behavior with
    offloading enabled, and I have also removed those workarounds that are now
    unnecessary.
    
    Thanks very much to whoever may have time to take a look at it. (If you've
    read this far, the actual patch is much shorter :)). I would appreciate some
    guidance as to whether this patch looks OK, and whether there is any interest
    in the other two which take it further. Thanks!
    
    -Lewis

 gcc/c-family/c-opts.cc                        |  5 +++
 gcc/diagnostic.cc                             |  2 +
 gcc/diagnostic.h                              | 20 +++++++++
 gcc/ipa-free-lang-data.cc                     |  6 ++-
 gcc/testsuite/c-c++-common/diag-after-fld-1.c | 25 +++++++++++
 gcc/tree-diagnostic.cc                        | 12 ++++--
 gcc/tree-diagnostic.h                         |  3 +-
 .../libgomp.oacc-c-c++-common/reduction-5.c   |  5 +--
 .../libgomp.oacc-c-c++-common/vred2d-128.c    | 41 +++++++++----------
 9 files changed, 88 insertions(+), 31 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/diag-after-fld-1.c
  

On Thu, Oct 20, 2022 at 1:09 AM Lewis Hyatt via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Currently, the ipa-free-lang-data pass resets most of the frontend's
> diagnostic customizations, such as the diagnostic_finalizer that prints macro
> expansion information, which is the subject of the two PRs. In most cases,
> however, there is no need to reset these customizations; they still work just
> fine after the language-specific data has been freed. (Macro tracking
> information, for instance, only depends on the line_maps instance and does not
> use the tree data structures at all.)
>
> Add an interface whereby frontends can convey which of their customizations
> should be preserved by ipa-free-lang-data. Only the macro tracking behavior is
> changed for now.  Subsequent patches will add further configurations for each
> frontend.

One point of the resetting of the hooks is to avoid crashes due to us zapping
many of the lang specific data structures.  If the hooks were more resilent
that wouldn't be an issue.

Now - as for macro tracking, how difficult is it to replicate that in the
default hook implementation?  Basically we have similar issues for
late diagnostics of the LTO compile step where only the LTO (aka default)
variant of the hooks are present - it would be nice to improve that as well.

Note free_lang_data exists to "simplify" the LTO bytecode output - things
freed do not need to be output.  Of course the "freeing" logic could be
wired into the LTO bytecode output machinery directly - simply do not
output what we'd free.  That way all info would prevail for the non-LTO
compile and the hooks could continue to work as they do without any
LTO streaming done.

Richard.

>
> gcc/ChangeLog:
>
>         PR lto/106274
>         PR middle-end/101551
>         * diagnostic.h (struct diagnostic_context): Add new customization
>         point diagnostic_context::preserve_on_reset.
>         * diagnostic.cc (diagnostic_initialize): Initialize it.
>         * tree-diagnostic.h (tree_diagnostics_defaults): Add new optional
>         argument enable_preserve.
>         * tree-diagnostic.cc (tree_diagnostics_defaults): Implement it.
>         * ipa-free-lang-data.cc (free_lang_data): Use it.
>
> gcc/c-family/ChangeLog:
>
>         PR lto/106274
>         PR middle-end/101551
>         * c-opts.cc (c_common_diagnostics_set_defaults): Preserve
>         diagnostic finalizer for the middle end.
>
> libgomp/ChangeLog:
>
>         PR lto/106274
>         PR middle-end/101551
>         * testsuite/libgomp.oacc-c-c++-common/reduction-5.c: Remove
>         now-unnecessary workaround.
>         * testsuite/libgomp.oacc-c-c++-common/vred2d-128.c: Likewise.
>
> gcc/testsuite/ChangeLog:
>
>         PR lto/106274
>         PR middle-end/101551
>         * c-c++-common/diag-after-fld-1.c: New test.
> ---
>
> Notes:
>     Hello-
>
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101551
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106274
>
>     PR101551 complains that when compiling with offloading enabled, diagnostics
>     output changes in several different ways. PR106274 notes the same thing (which
>     has the same cause) when compiling with -flto, for the specific case of macro
>     expansion tracking, which is no longer output for middle-end diagnostics when
>     -flto is present. Restoring the macro tracking information, at least, can be
>     done simply, which is the attached patch. This is straightforward because the
>     printing of macro tracking information is not really reliant on the sort of
>     language-specific data structures that are freed by ipa-free-lang-data... it
>     just needs the line-maps API, which is common to all languages and which is
>     not impacted by the work done by ipa-free-lang-data.
>
>     To be clear, this is not about diagnostics issued *by* the lto frontend. This
>     is just about diagnostics issued by the language frontends, in case they were
>     asked to stream the IL for later use by the lto frontend. I think from the
>     user's perspective, compiling with or without -flto should not change the
>     quality of diagnostics, since on the face of it, -flto seems to be just a
>     request for the compiler to do something extra (write out the IL in addition
>     to all the other stuff it does), and it's not clear why this should change the
>     way diagnostics look. (I understand there must be some good reasons, why it's
>     not the case.)  So anyway I was focusing on how to keep the diagnostics as
>     close as possible to their normal form after ipa-free-lang-data is done.
>
>     The approach I took was to add a new variable "preserve_on_reset" in the
>     diagnostic_context, allowing a frontend to specify whether its diagnostic
>     context customizations are safe to leave in place for the middle end. There
>     are currently four customizations for which preservation can be enabled:
>
>         * diagnostic starter
>         * diagnostic finalizer
>         * format decoder
>         * decl_printable_name langhook
>
>     Preserving the diagnostic finalizer is sufficient to restore the output of
>     macro tracking information, and that's what I did in this patch. But it seems
>     that it's possible to go a bit farther than this as well. Here are some
>     examples:
>
>     1. Fortran:
>         Consider the existing testcase
>         gfortran.dg/allocatable_uninitialized_1.f90. When compiled without -flto,
>         it outputs:
>
>         ===========================
>         allocatable_uninitialized_1.f90:6:47:
>
>             6 |    a(1)=2*b(1) ! { dg-warning "uninitialized" }
>               |                                               ^
>         Warning: 'b.offset' is used uninitialized [-Wuninitialized]
>         allocatable_uninitialized_1.f90:4:30:
>
>             4 |   real,allocatable:: a(:),b(:)
>               |                              ^
>         note: 'b' declared here
>         allocatable_uninitialized_1.f90:6:47:
>
>             6 |    a(1)=2*b(1) ! { dg-warning "uninitialized" }
>               |                                               ^
>         Warning: 'a.offset' is used uninitialized [-Wuninitialized]
>         allocatable_uninitialized_1.f90:4:25:
>
>             4 |   real,allocatable:: a(:),b(:)
>               |                         ^
>         note: 'a' declared here
>         ===========================
>
>         When compiled with -flto, it outputs instead:
>         ===========================
>         allocatable_uninitialized_1.f90: In function 'MAIN__':
>         allocatable_uninitialized_1.f90:6:47: warning: 'b.offset' is used uninitialized in this function [-Wuninitialized]
>             6 |    a(1)=2*b(1) ! { dg-warning "uninitialized" }
>               |                                               ^
>         allocatable_uninitialized_1.f90:6:47: warning: 'a.offset' is used uninitialized in this function [-Wuninitialized]
>         ===========================
>
>         So the informative notes are missing, and a rather relevant source line is
>         omitted, plus the internal name 'MAIN__' ends up in the output as well.
>
>         Assuming the current patch goes in, then this incremental patch (not
>         proposing it yet) would make the diagnostics the same with or without
>         -flto:
>
>     --- a/gcc/fortran/error.cc
>     +++ b/gcc/fortran/error.cc
>     @@ -1641,6 +1641,12 @@ gfc_diagnostics_init (void)
>           management when using gfc_push/pop_error. */
>        pp_error_buffer = &(error_buffer.buffer);
>        pp_error_buffer->flush_p = false;
>     +
>     +  /* Our diagnostic customizations are safe for the middle end.  */
>     +  auto &p = global_dc->preserve_on_reset;
>     +  p[diagnostic_context::PRESERVE_STARTER] = true;
>     +  p[diagnostic_context::PRESERVE_FINALIZER] = true;
>     +  p[diagnostic_context::PRESERVE_FORMAT_DECODER] = true;
>
>         I have a testcase for it, but I'm a bit hesistant to suggest this change
>         because I worry about the fragility that it introduces. Setting
>         preserve_on_reset is a guarantee that the diagnostic customizations won't be
>         broken by ipa-free-lang-data, but there's no real mechanism to insure the
>         guarantee is honored (although to the best I can tell, it is currently), and
>         most of the testsuite does not run with -flto so it's not covered well that
>         way.
>
>     2. C++:
>
>         Another major way that diagnostics change with -flto is due to the
>         resetting of the decl_printable_name langhook. For example, a template
>         function normally described as:
>
>         int f(void*) [with T = void]
>
>         becomes instead:
>
>         f<void>(void*)int
>
>         The nicer output can be restored by preserving more customizations. I have
>         a patch for that with a long testcase too, which I can send if there is
>         interest, however I have all the same reservations about that which I
>         mentioned above, except moreso because the patch needs to do more -- just
>         out of the box, the C++ customizations don't work after
>         ipa-free-lang-data. They need a small tweak to understand that after the
>         pass, member functions other than destructors no longer satisfy
>         DECL_CLASS_SCOPE_P, and so they need to be printed by falling back to
>         gimple_decl_printable_name instead. Here I worry it's more likely that it
>         could silently break in the future, and it wouldn't be noticed right away.
>
>     3. ObjC:
>
>        ObjC customizes decl_printable_name so that a member function can be
>        printed e.g. as '[T f]' instead of '_c_T__f'.  In this case it seems pretty
>        clear that preserving the decl_printable_name would be fine, but the upside
>        is not that large either.  I have a patch + testcase for that if there is
>        interest.
>
>     Bootstrap + regtest all languages looks good on x86-64 Linux. There turned out
>     not to be any tests in the testsuite relying on the presence or lack of macro
>     tracking information with -flto. I added some coverage for that. A few other
>     tests for libgomp were explicitly working around the current behavior with
>     offloading enabled, and I have also removed those workarounds that are now
>     unnecessary.
>
>     Thanks very much to whoever may have time to take a look at it. (If you've
>     read this far, the actual patch is much shorter :)). I would appreciate some
>     guidance as to whether this patch looks OK, and whether there is any interest
>     in the other two which take it further. Thanks!
>
>     -Lewis
>
>  gcc/c-family/c-opts.cc                        |  5 +++
>  gcc/diagnostic.cc                             |  2 +
>  gcc/diagnostic.h                              | 20 +++++++++
>  gcc/ipa-free-lang-data.cc                     |  6 ++-
>  gcc/testsuite/c-c++-common/diag-after-fld-1.c | 25 +++++++++++
>  gcc/tree-diagnostic.cc                        | 12 ++++--
>  gcc/tree-diagnostic.h                         |  3 +-
>  .../libgomp.oacc-c-c++-common/reduction-5.c   |  5 +--
>  .../libgomp.oacc-c-c++-common/vred2d-128.c    | 41 +++++++++----------
>  9 files changed, 88 insertions(+), 31 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/diag-after-fld-1.c
>
> diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
> index 32b929e3ece..e6880c23444 100644
> --- a/gcc/c-family/c-opts.cc
> +++ b/gcc/c-family/c-opts.cc
> @@ -182,6 +182,11 @@ void
>  c_common_diagnostics_set_defaults (diagnostic_context *context)
>  {
>    diagnostic_finalizer (context) = c_diagnostic_finalizer;
> +
> +  /* Our finalizer is the same as the default one, except it also outputs macro
> +     tracking information.  This is safe to do in the middle end.  */
> +  context->preserve_on_reset[diagnostic_context::PRESERVE_FINALIZER] = true;
> +
>    context->opt_permissive = OPT_fpermissive;
>  }
>
> diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
> index 22f7b0b6d6e..3fe4fdb42ca 100644
> --- a/gcc/diagnostic.cc
> +++ b/gcc/diagnostic.cc
> @@ -243,6 +243,8 @@ diagnostic_initialize (diagnostic_context *context, int n_opts)
>    context->final_cb = default_diagnostic_final_cb;
>    context->includes_seen = NULL;
>    context->m_client_data_hooks = NULL;
> +  memset (context->preserve_on_reset, 0,
> +         ARRAY_SIZE (context->preserve_on_reset));
>  }
>
>  /* Maybe initialize the color support. We require clients to do this
> diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
> index ae6f2dfb7f4..a429ad4846f 100644
> --- a/gcc/diagnostic.h
> +++ b/gcc/diagnostic.h
> @@ -414,6 +414,26 @@ struct diagnostic_context
>       Used by SARIF output to give metadata about the client that's
>       producing diagnostics.  */
>    diagnostic_client_data_hooks *m_client_data_hooks;
> +
> +  /* When transitioning from the frontend to the middle end, the
> +     ipa-free-lang-data pass will reset the diagnostic context to the default
> +     configuration so that frontend customizations, which may no longer have
> +     access to the data structures they expect to exist, will not cause any
> +     problems with subsequent diagnostics.  Setting these variables to true
> +     allows a frontend to communicate that its customizations are safe to run
> +     post ipa-free-lang-data and need not be reset.  */
> +  enum {
> +    PRESERVE_STARTER = 0,
> +    PRESERVE_FINALIZER,
> +    PRESERVE_FORMAT_DECODER,
> +
> +    /* This is a langhook rather than a member of the DC, but it is still
> +       convenient to handle it here.  */
> +    PRESERVE_DECL_PRINTABLE_NAME,
> +
> +    PRESERVE_NUM
> +  };
> +  bool preserve_on_reset[PRESERVE_NUM];
>  };
>
>  static inline void
> diff --git a/gcc/ipa-free-lang-data.cc b/gcc/ipa-free-lang-data.cc
> index ccdbf849c25..fbdeab8f641 100644
> --- a/gcc/ipa-free-lang-data.cc
> +++ b/gcc/ipa-free-lang-data.cc
> @@ -1115,7 +1115,9 @@ free_lang_data (void)
>    /* Reset some langhooks.  Do not reset types_compatible_p, it may
>       still be used indirectly via the get_alias_set langhook.  */
>    lang_hooks.dwarf_name = lhd_dwarf_name;
> -  lang_hooks.decl_printable_name = gimple_decl_printable_name;
> +  if (!global_dc->preserve_on_reset[diagnostic_context::
> +                                   PRESERVE_DECL_PRINTABLE_NAME])
> +    lang_hooks.decl_printable_name = gimple_decl_printable_name;
>    lang_hooks.gimplify_expr = lhd_gimplify_expr;
>    lang_hooks.overwrite_decl_assembler_name = lhd_overwrite_decl_assembler_name;
>    lang_hooks.print_xnode = lhd_print_tree_nothing;
> @@ -1142,7 +1144,7 @@ free_lang_data (void)
>       devise a separate, middle-end private scheme for it.  */
>
>    /* Reset diagnostic machinery.  */
> -  tree_diagnostics_defaults (global_dc);
> +  tree_diagnostics_defaults (global_dc, /* enable_preserve= */ true);
>
>    rebuild_type_inheritance_graph ();
>
> diff --git a/gcc/testsuite/c-c++-common/diag-after-fld-1.c b/gcc/testsuite/c-c++-common/diag-after-fld-1.c
> new file mode 100644
> index 00000000000..c1fc87a03f3
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/diag-after-fld-1.c
> @@ -0,0 +1,25 @@
> +/* Make sure that post-ipa-free-lang-data diagnostics expand macros as
> +   expected.  */
> +
> +/* { dg-do compile } */
> +/* { dg-require-effective-target lto } */
> +/* { dg-options "-flto -O2 -Wnonnull-compare" } */
> +
> +#define X(p) p == 0 /* { dg-warning {-Wnonnull-compare} } */
> +int f (void *) __attribute__((nonnull));
> +int f (void *p)
> +{
> +  return X (p); /* { dg-note {in expansion of macro 'X'} } */
> +}
> +
> +#define X2(p) p == 0 /* { dg-warning {-Wnonnull-compare} } */
> +#define Y2(p) X2(p) /* { dg-note {in expansion of macro 'X2'} } */
> +
> +#define MAKE_F2 \
> +  int f2 (void *) __attribute__((nonnull)); \
> +  int f2 (void *p) \
> +  { \
> +    return Y2 (p); /* { dg-note {in expansion of macro 'Y2'} } */ \
> +  }
> +
> +MAKE_F2 /* { dg-note {in expansion of macro 'MAKE_F2'} } */
> diff --git a/gcc/tree-diagnostic.cc b/gcc/tree-diagnostic.cc
> index 0d79fe3c3c1..cbf6d0678b7 100644
> --- a/gcc/tree-diagnostic.cc
> +++ b/gcc/tree-diagnostic.cc
> @@ -366,11 +366,15 @@ set_inlining_locations (diagnostic_context *,
>
>  /* Sets CONTEXT to use language independent diagnostics.  */
>  void
> -tree_diagnostics_defaults (diagnostic_context *context)
> +tree_diagnostics_defaults (diagnostic_context *context, bool enable_preserve)
>  {
> -  diagnostic_starter (context) = default_tree_diagnostic_starter;
> -  diagnostic_finalizer (context) = default_diagnostic_finalizer;
> -  diagnostic_format_decoder (context) = default_tree_printer;
> +  const auto &p = context->preserve_on_reset;
> +  if (!(enable_preserve && p[diagnostic_context::PRESERVE_STARTER]))
> +    diagnostic_starter (context) = default_tree_diagnostic_starter;
> +  if (!(enable_preserve && p[diagnostic_context::PRESERVE_FINALIZER]))
> +    diagnostic_finalizer (context) = default_diagnostic_finalizer;
> +  if (!(enable_preserve && p[diagnostic_context::PRESERVE_FORMAT_DECODER]))
> +    diagnostic_format_decoder (context) = default_tree_printer;
>    context->print_path = default_tree_diagnostic_path_printer;
>    context->make_json_for_path = default_tree_make_json_for_path;
>    context->set_locations_cb = set_inlining_locations;
> diff --git a/gcc/tree-diagnostic.h b/gcc/tree-diagnostic.h
> index f60f2320e95..f7c1f08105d 100644
> --- a/gcc/tree-diagnostic.h
> +++ b/gcc/tree-diagnostic.h
> @@ -53,7 +53,8 @@ void diagnostic_report_current_function (diagnostic_context *,
>  void virt_loc_aware_diagnostic_finalizer (diagnostic_context *,
>                                           diagnostic_info *);
>
> -void tree_diagnostics_defaults (diagnostic_context *context);
> +void tree_diagnostics_defaults (diagnostic_context *context,
> +                               bool enable_preserve = false);
>  bool default_tree_printer (pretty_printer *, text_info *, const char *,
>                            int, bool, bool, bool, bool *, const char **);
>
> diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
> index ddccfe89e73..a3f3f758e9d 100644
> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
> @@ -47,9 +47,8 @@ main (void)
>       gangs.  */
>    check_reduction (num_workers (nw) vector_length (vl), worker); /* { dg-line check_reduction_loc } */
>    /* { dg-warning "22:region is vector partitioned but does not contain vector partitioned code" "" { target *-*-* } pragma_loc }
> -     { dg-note "1:in expansion of macro 'DO_PRAGMA'" "" { target *-*-* xfail offloading_enabled } DO_PRAGMA_loc }
> -     { dg-note "3:in expansion of macro 'check_reduction'" "" { target *-*-* xfail offloading_enabled } check_reduction_loc }
> -     TODO See PR101551 for 'offloading_enabled' XFAILs.  */
> +     { dg-note "1:in expansion of macro 'DO_PRAGMA'" "" { target *-*-* } DO_PRAGMA_loc }
> +     { dg-note "3:in expansion of macro 'check_reduction'" "" { target *-*-* } check_reduction_loc } */
>    check_reduction (vector_length (vl), vector);
>    check_reduction (num_gangs (ng) num_workers (nw) vector_length (vl), gang
>                    worker vector);
> diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c
> index 84e6d51670b..614b527409a 100644
> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c
> @@ -42,46 +42,45 @@ gentest (test1, "acc parallel loop gang vector_length (128) firstprivate (t1, t2
>          "acc loop vector reduction(+:t1) reduction(-:t2)")
>  /* { dg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
>     { dg-note {'t1' was declared here} {} { target *-*-* } vars }
> -   { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> -/* { dg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
> +   { dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-4 }
> +
> +   { dg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
>     { dg-note {'t2' was declared here} {} { target *-*-* } vars }
> -   { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> +   { DUP_dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-8 }
> +*/
>
>  gentest (test2, "acc parallel loop gang vector_length (128) firstprivate (t1, t2)",
>          "acc loop worker vector reduction(+:t1) reduction(-:t2)")
>  /* { DUPdg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
>     { DUP_dg-note {'t1' was declared here} {} { target *-*-* } vars }
> -   { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> -/* { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
> +   { dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-4 }
> +
> +   { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
>     { DUP_dg-note {'t2' was declared here} {} { target *-*-* } vars }
> -   { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> +   { DUP_dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-8 }
> +*/
>
>  gentest (test3, "acc parallel loop gang worker vector_length (128) firstprivate (t1, t2)",
>          "acc loop vector reduction(+:t1) reduction(-:t2)")
>  /* { DUPdg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
>     { DUP_dg-note {'t1' was declared here} {} { target *-*-* } vars }
> -   { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> -/* { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
> +   { dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-4 }
> +
> +   { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
>     { DUP_dg-note {'t2' was declared here} {} { target *-*-* } vars }
> -   { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> +   { DUP_dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-8 }
> +*/
>
>  gentest (test4, "acc parallel loop firstprivate (t1, t2)",
>          "acc loop reduction(+:t1) reduction(-:t2)")
>  /* { DUPdg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
>     { DUP_dg-note {'t1' was declared here} {} { target *-*-* } vars }
> -   { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> -/* { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
> -   { DUP_dg-note {'t2' was declared here} {} { target *-*-* } vars }
> -   { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
> -     TODO See PR101551 for 'offloading_enabled' differences.  */
> +   { dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-4 }
>
> +   { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
> +   { DUP_dg-note {'t2' was declared here} {} { target *-*-* } vars }
> +   { DUP_dg-note {in expansion of macro 'gentest'} {} { target *-*-* } .-8 }
> +*/
>
>  int
>  main ()
  

On Tue, Oct 25, 2022 at 7:35 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Oct 20, 2022 at 1:09 AM Lewis Hyatt via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Currently, the ipa-free-lang-data pass resets most of the frontend's
> > diagnostic customizations, such as the diagnostic_finalizer that prints macro
> > expansion information, which is the subject of the two PRs. In most cases,
> > however, there is no need to reset these customizations; they still work just
> > fine after the language-specific data has been freed. (Macro tracking
> > information, for instance, only depends on the line_maps instance and does not
> > use the tree data structures at all.)
> >
> > Add an interface whereby frontends can convey which of their customizations
> > should be preserved by ipa-free-lang-data. Only the macro tracking behavior is
> > changed for now.  Subsequent patches will add further configurations for each
> > frontend.
>
> One point of the resetting of the hooks is to avoid crashes due to us zapping
> many of the lang specific data structures.  If the hooks were more resilent
> that wouldn't be an issue.
>

Right. The patch I have for C++ (not sent yet) makes the C++ versions
of decl_printable_name and and the diagnostic starter able to work
after free_lang_data runs.  I just worry that future changes to the
C++ hooks would need to preserve this property, which could be error
prone since issues are not immediately apparent, and most of the
testsuite does not use -flto.

> Now - as for macro tracking, how difficult is it to replicate that in the
> default hook implementation?  Basically we have similar issues for
> late diagnostics of the LTO compile step where only the LTO (aka default)
> variant of the hooks are present - it would be nice to improve that as well.
>

It is easy enough to make the default diagnostic finalizer print the
macro tracking information stored in the global line_table. (It just
needs to check if the global line_table is set, in which case call
virt_loc_aware_diagnostic_finalizer()). This would remove the need for
C-family frontends to override that callback. Fortran would still do
so, since it does other things in its finalizer. However, this would
not help with the LTO frontend because the line_table is not part of
what gets streamed out. Rather the line_table is rebuilt from scratch
when reading the data back in, but the macro tracking information is
not available at that time, just the basic location info (filename and
source location). I am not that familiar with the LTO streaming
process but I feel like streaming the entire line_table would not mesh
well with it (especially since multiple of them from different
translation units would need to be combined back together).

> Note free_lang_data exists to "simplify" the LTO bytecode output - things
> freed do not need to be output.  Of course the "freeing" logic could be
> wired into the LTO bytecode output machinery directly - simply do not
> output what we'd free.  That way all info would prevail for the non-LTO
> compile and the hooks could continue to work as they do without any
> LTO streaming done.
>

Naively (emphasis on the naive, as I don't have any experience with
this part of GCC), that is how I would have guessed it worked. But I
understood there are some benefits to freeing the lang data earlier
(e.g. reduced resource usage), and even a hope to start doing it in
non-LTO builds as well, so I thought some incremental changes as in
this patch to make diagnostics better after free_lang_data could
perhaps be useful. Thanks for taking a look at it!

-Lewis
  

On Tue, Oct 25, 2022 at 11:05 PM Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> On Tue, Oct 25, 2022 at 7:35 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 1:09 AM Lewis Hyatt via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > Currently, the ipa-free-lang-data pass resets most of the frontend's
> > > diagnostic customizations, such as the diagnostic_finalizer that prints macro
> > > expansion information, which is the subject of the two PRs. In most cases,
> > > however, there is no need to reset these customizations; they still work just
> > > fine after the language-specific data has been freed. (Macro tracking
> > > information, for instance, only depends on the line_maps instance and does not
> > > use the tree data structures at all.)
> > >
> > > Add an interface whereby frontends can convey which of their customizations
> > > should be preserved by ipa-free-lang-data. Only the macro tracking behavior is
> > > changed for now.  Subsequent patches will add further configurations for each
> > > frontend.
> >
> > One point of the resetting of the hooks is to avoid crashes due to us zapping
> > many of the lang specific data structures.  If the hooks were more resilent
> > that wouldn't be an issue.
> >
>
> Right. The patch I have for C++ (not sent yet) makes the C++ versions
> of decl_printable_name and and the diagnostic starter able to work
> after free_lang_data runs.  I just worry that future changes to the
> C++ hooks would need to preserve this property, which could be error
> prone since issues are not immediately apparent, and most of the
> testsuite does not use -flto.
>
> > Now - as for macro tracking, how difficult is it to replicate that in the
> > default hook implementation?  Basically we have similar issues for
> > late diagnostics of the LTO compile step where only the LTO (aka default)
> > variant of the hooks are present - it would be nice to improve that as well.
> >
>
> It is easy enough to make the default diagnostic finalizer print the
> macro tracking information stored in the global line_table. (It just
> needs to check if the global line_table is set, in which case call
> virt_loc_aware_diagnostic_finalizer()). This would remove the need for
> C-family frontends to override that callback. Fortran would still do
> so, since it does other things in its finalizer. However, this would
> not help with the LTO frontend because the line_table is not part of
> what gets streamed out. Rather the line_table is rebuilt from scratch
> when reading the data back in, but the macro tracking information is
> not available at that time, just the basic location info (filename and
> source location). I am not that familiar with the LTO streaming
> process but I feel like streaming the entire line_table would not mesh
> well with it (especially since multiple of them from different
> translation units would need to be combined back together).
>
> > Note free_lang_data exists to "simplify" the LTO bytecode output - things
> > freed do not need to be output.  Of course the "freeing" logic could be
> > wired into the LTO bytecode output machinery directly - simply do not
> > output what we'd free.  That way all info would prevail for the non-LTO
> > compile and the hooks could continue to work as they do without any
> > LTO streaming done.
> >
>
> Naively (emphasis on the naive, as I don't have any experience with
> this part of GCC), that is how I would have guessed it worked. But I
> understood there are some benefits to freeing the lang data earlier
> (e.g. reduced resource usage), and even a hope to start doing it in
> non-LTO builds as well, so I thought some incremental changes as in
> this patch to make diagnostics better after free_lang_data could
> perhaps be useful. Thanks for taking a look at it!

Yes, the idea was also to free up memory but then that part never
really materialized - the idea was to always run free-lang-data, not
just when later outputting LTO bytecode.  The reason is probably
mainly the diagnostic regressions you observe.

Maybe a better strathegy than your patch would be to work towards
that goal but reduce the number of "freeings", instead adjusting the
LTO streamer to properly ignore frontend specific bits where clearing
conflicts with the intent to preserve accurate diagnostics throughout
the compilation.

If you see bits that when not freed would fix some of the observed
issues we can see to replicate the freeing in the LTO output machinery.

Richard.

>
> -Lewis
  

On Fri, Oct 28, 2022 at 10:28:21AM +0200, Richard Biener wrote:
> Yes, the idea was also to free up memory but then that part never
> really materialized - the idea was to always run free-lang-data, not
> just when later outputting LTO bytecode.  The reason is probably
> mainly the diagnostic regressions you observe.
> 
> Maybe a better strathegy than your patch would be to work towards
> that goal but reduce the number of "freeings", instead adjusting the
> LTO streamer to properly ignore frontend specific bits where clearing
> conflicts with the intent to preserve accurate diagnostics throughout
> the compilation.
> 
> If you see bits that when not freed would fix some of the observed
> issues we can see to replicate the freeing in the LTO output machinery.
> 
> Richard.

Thanks again for the suggestions. I took a look and it seems pretty doable to
just stop resetting all the diagnostics hooks in free-lang-data. Once that's
done, the only problematic part that I have been able to identify is here in
ipa-free-lang-data.c around line 674:

====
  /* We need to keep field decls associated with their trees. Otherwise tree
     merging may merge some fields and keep others disjoint which in turn will
     not do well with TREE_CHAIN pointers linking them.

     Also do not drop containing types for virtual methods and tables because
     these are needed by devirtualization.
     C++ destructors are special because C++ frontends sometimes produces
     virtual destructor as an alias of non-virtual destructor.  In
     devirutalization code we always walk through aliases and we need
     context to be preserved too.  See PR89335  */
  if (TREE_CODE (decl) != FIELD_DECL
      && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL)
          || (!DECL_VIRTUAL_P (decl)
	      && (TREE_CODE (decl) != FUNCTION_DECL
		  || !DECL_CXX_DESTRUCTOR_P (decl)))))
    DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl));
====

The C++ implementations of the decl_printable_name langhook and the diagnostic
starter callback do not work as-is when the DECL_CONTEXT for class member
functions disappears.  So I did have a patch that changes the C++
implementations to work in this case, but attached here is a new one along the
lines of what you suggested, rather changing the above part of free-lang-data
so it doesn't activate as often. The patch is pretty complete (other than
missing a commit message) and bootstrap + regtest all languages looks good
with no regressions. I tried the same with BUILD_CONFIG=bootstrap-lto as well,
and that also looked good when it eventually finished. I added testcases for
several frontends to verify that the diagnostics still work with -flto. I am
not sure what are the implications for LTO itself, of changing this part of
the pass, so I would have to ask you to weigh in on that aspect please. Thanks!

-Lewis
[PATCH] middle-end: Preserve frontend diagnostics in free-lang-data [PR101551, PR106274]

gcc/ChangeLog:

	PR lto/106274
        PR middle-end/101551
	* ipa-free-lang-data.cc (free_lang_data_in_decl): Preserve
	DECL_CONTEXT for class member functions.
	(free_lang_data): Do not reset frontend diagnostics customizations.

gcc/testsuite/ChangeLog:

	PR lto/106274
        PR middle-end/101551
	* c-c++-common/diag-after-fld-1.c: New test.
	* g++.dg/diag-after-fld-1.C: New test.
	* g++.dg/diag-after-fld-2.C: New test.
	* gfortran.dg/allocatable_uninitialized_2.f90: New test.
	* objc.dg/diag-after-fld-1.m: New test.

diff --git a/gcc/ipa-free-lang-data.cc b/gcc/ipa-free-lang-data.cc
index ccdbf849c25..391b7689639 100644
--- a/gcc/ipa-free-lang-data.cc
+++ b/gcc/ipa-free-lang-data.cc
@@ -682,10 +682,8 @@ free_lang_data_in_decl (tree decl, class free_lang_data_d *fld)
      devirutalization code we always walk through aliases and we need
      context to be preserved too.  See PR89335  */
   if (TREE_CODE (decl) != FIELD_DECL
-      && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL)
-          || (!DECL_VIRTUAL_P (decl)
-	      && (TREE_CODE (decl) != FUNCTION_DECL
-		  || !DECL_CXX_DESTRUCTOR_P (decl)))))
+      && TREE_CODE (decl) != VAR_DECL
+      && TREE_CODE (decl) != FUNCTION_DECL)
     DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl));
 }
 
@@ -1115,7 +1113,6 @@ free_lang_data (void)
   /* Reset some langhooks.  Do not reset types_compatible_p, it may
      still be used indirectly via the get_alias_set langhook.  */
   lang_hooks.dwarf_name = lhd_dwarf_name;
-  lang_hooks.decl_printable_name = gimple_decl_printable_name;
   lang_hooks.gimplify_expr = lhd_gimplify_expr;
   lang_hooks.overwrite_decl_assembler_name = lhd_overwrite_decl_assembler_name;
   lang_hooks.print_xnode = lhd_print_tree_nothing;
@@ -1141,9 +1138,6 @@ free_lang_data (void)
      make sure we never call decl_assembler_name on local symbols and
      devise a separate, middle-end private scheme for it.  */
 
-  /* Reset diagnostic machinery.  */
-  tree_diagnostics_defaults (global_dc);
-
   rebuild_type_inheritance_graph ();
 
   delete fld_incomplete_types;
diff --git a/gcc/testsuite/c-c++-common/diag-after-fld-1.c b/gcc/testsuite/c-c++-common/diag-after-fld-1.c
new file mode 100644
index 00000000000..c1fc87a03f3
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/diag-after-fld-1.c
@@ -0,0 +1,25 @@
+/* Make sure that post-ipa-free-lang-data diagnostics expand macros as
+   expected.  */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target lto } */
+/* { dg-options "-flto -O2 -Wnonnull-compare" } */
+
+#define X(p) p == 0 /* { dg-warning {-Wnonnull-compare} } */
+int f (void *) __attribute__((nonnull));
+int f (void *p)
+{
+  return X (p); /* { dg-note {in expansion of macro 'X'} } */
+}
+
+#define X2(p) p == 0 /* { dg-warning {-Wnonnull-compare} } */
+#define Y2(p) X2(p) /* { dg-note {in expansion of macro 'X2'} } */
+
+#define MAKE_F2 \
+  int f2 (void *) __attribute__((nonnull)); \
+  int f2 (void *p) \
+  { \
+    return Y2 (p); /* { dg-note {in expansion of macro 'Y2'} } */ \
+  }
+
+MAKE_F2 /* { dg-note {in expansion of macro 'MAKE_F2'} } */
diff --git a/gcc/testsuite/g++.dg/diag-after-fld-1.C b/gcc/testsuite/g++.dg/diag-after-fld-1.C
new file mode 100644
index 00000000000..0f8d10b7848
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diag-after-fld-1.C
@@ -0,0 +1,470 @@
+/* Based on 20090121-1.C; verify these middle-end diagnostics work fine after
+   ipa-free-lang data pass.  Try to exercise most code paths in cp/error.cc:
+   dump_function_decl(), i.e. various combinations of templates and member
+   functions.  */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target lto } */
+/* { dg-options "-flto -Wuninitialized -O2" } */
+
+int y;
+
+int w1 (int)
+{
+  struct S
+  {
+    S () /* { dg-regexp {.*: In constructor 'w1\(int\)::S::S\(\)':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S(), y;
+}
+
+template<typename T>
+int w2 (T)
+{
+  struct S
+  {
+    S () /* { dg-regexp {.*: In constructor 'w2\(T\)::S::S\(\) \[with T = char\]':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S (), y;
+}
+int w2a ()
+{
+  return w2('\0');
+}
+
+int w3 (int)
+{
+  struct S
+  {
+    void f2 () /* { dg-regexp {.*: In member function 'void w3\(int\)::S::f2\(\)':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S().f2 (), y;
+}
+
+template<typename T>
+int w4 (T)
+{
+  struct S
+  {
+    void f2 () /* { dg-regexp {.*: In member function 'void w4\(T\)::S::f2\(\) \[with T = char\]':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S().f2 (), y;
+}
+int w4a ()
+{
+  return w4 ('\0');
+}
+
+struct A1
+{
+  A1 () /* { dg-regexp {.*: In constructor 'A1::A1\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  void f2 () /* { dg-regexp {.*: In member function 'void A1::f2\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename T>
+  void f3 () /* { dg-regexp {.*: In member function 'void A1::f3\(\) \[with T = char\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  static int f4() /* { dg-regexp {.*: In static member function 'static int A1::f4\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  operator int () /* { dg-regexp {.*: In member function 'A1::operator int\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename T>
+  operator T* () /* { dg-regexp {.*: In member function 'A1::operator T\*\(\) \[with T = char\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+    return 0;
+  }
+};
+
+int f1 ()
+{
+  return A1 (), y;
+}
+
+int f2 ()
+{
+  return A1 ().f2 (), y;
+}
+
+int f3 ()
+{
+  return A1 ().f3<char> (), y;
+}
+
+int f4 ()
+{
+  return A1 ().f4 ();
+}
+
+int f5 ()
+{
+  return A1 ();
+}
+
+int f6 ()
+{
+  char *s = A1 ();
+  return y;
+}
+
+template<typename T>
+struct A2
+{
+  A2 () /* { dg-regexp {.*: In constructor 'A2<T>::A2\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  void f2 () /* { dg-regexp {.*: In member function 'void A2<T>::f2\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename U>
+  void f3 () /* { dg-regexp {.*: In member function 'void A2<T>::f3\(\) \[with U = char; T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  static int f4() /* { dg-regexp {.*: In static member function 'static int A2<T>::f4\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  operator int () /* { dg-regexp {.*: In member function 'A2<T>::operator int\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename U>
+  operator U* () /* { dg-regexp {.*: In member function 'A2<T>::operator U\*\(\) \[with U = char; T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+    return 0;
+  }
+};
+
+int g1 ()
+{
+  return A2<void> (), y;
+}
+
+int g2 ()
+{
+  return A2<void> ().f2 (), y;
+}
+
+int g3 ()
+{
+  return A2<void> ().f3<char> (), y;
+}
+
+int g4 ()
+{
+  return A2<void> ().f4 ();
+}
+
+int g5 ()
+{
+  return A2<void> ();
+}
+
+int g6 ()
+{
+  char *s = A2<void> ();
+  return y;
+}
+
+struct A3
+{
+  ~A3 () /* { dg-regexp {.*: In destructor 'A3::~A3\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+};
+
+int h1 ()
+{
+  {
+    A3 a;
+  }
+  return y;
+}
+
+/* Let's do it all again inside a namespace too.  */
+namespace N {
+
+int w1 (int)
+{
+  struct S
+  {
+    S () /* { dg-regexp {.*: In constructor 'N::w1\(int\)::S::S\(\)':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S(), y;
+}
+
+template<typename T>
+int w2 (T)
+{
+  struct S
+  {
+    S () /* { dg-regexp {.*: In constructor 'N::w2\(T\)::S::S\(\) \[with T = char\]':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S (), y;
+}
+int w2a ()
+{
+  return w2('\0');
+}
+
+int w3 (int)
+{
+  struct S
+  {
+    void f2 () /* { dg-regexp {.*: In member function 'void N::w3\(int\)::S::f2\(\)':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S().f2 (), y;
+}
+
+template<typename T>
+int w4 (T)
+{
+  struct S
+  {
+    void f2 () /* { dg-regexp {.*: In member function 'void N::w4\(T\)::S::f2\(\) \[with T = char\]':} } */
+    {
+      int x; /* { dg-note {'x' was declared here} } */
+      y = x; /* { dg-warning {'x' is used uninitialized} } */
+    }
+  };
+  return S().f2 (), y;
+}
+int w4a ()
+{
+  return w4 ('\0');
+}
+
+struct A1
+{
+  A1 () /* { dg-regexp {.*: In constructor 'N::A1::A1\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  void f2 () /* { dg-regexp {.*: In member function 'void N::A1::f2\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename T>
+  void f3 () /* { dg-regexp {.*: In member function 'void N::A1::f3\(\) \[with T = char\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  static int f4() /* { dg-regexp {.*: In static member function 'static int N::A1::f4\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  operator int () /* { dg-regexp {.*: In member function 'N::A1::operator int\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename T>
+  operator T* () /* { dg-regexp {.*: In member function 'N::A1::operator T\*\(\) \[with T = char\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+    return 0;
+  }
+
+};
+
+int f1 ()
+{
+  return A1 (), y;
+}
+
+int f2 ()
+{
+  return A1 ().f2 (), y;
+}
+
+int f3 ()
+{
+  return A1 ().f3<char> (), y;
+}
+
+int f4 ()
+{
+  return A1 ().f4 ();
+}
+
+int f5 ()
+{
+  return A1 ();
+}
+
+int f6 ()
+{
+  char *s = A1 ();
+  return y;
+}
+
+template<typename T>
+struct A2
+{
+  A2 () /* { dg-regexp {.*: In constructor 'N::A2<T>::A2\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  void f2 () /* { dg-regexp {.*: In member function 'void N::A2<T>::f2\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename U>
+  void f3 () /* { dg-regexp {.*: In member function 'void N::A2<T>::f3\(\) \[with U = char; T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  static int f4() /* { dg-regexp {.*: In static member function 'static int N::A2<T>::f4\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  operator int () /* { dg-regexp {.*: In member function 'N::A2<T>::operator int\(\) \[with T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+
+  template<typename U>
+  operator U* () /* { dg-regexp {.*: In member function 'N::A2<T>::operator U\*\(\) \[with U = char; T = void\]':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+    return 0;
+  }
+
+};
+
+int g1 ()
+{
+  return A2<void> (), y;
+}
+
+int g2 ()
+{
+  return A2<void> ().f2 (), y;
+}
+
+int g3 ()
+{
+  return A2<void> ().f3<char> (), y;
+}
+
+int g4 ()
+{
+  return A2<void> ().f4 ();
+}
+
+int g5 ()
+{
+  return A2<void> ();
+}
+
+int g6 ()
+{
+  char *s = A2<void> ();
+  return y;
+}
+
+struct A3
+{
+  ~A3 () /* { dg-regexp {.*: In destructor 'N::A3::~A3\(\)':} } */
+  {
+    int x; /* { dg-note {'x' was declared here} } */
+    y = x; /* { dg-warning {'x' is used uninitialized} } */
+  }
+};
+
+int h1 ()
+{
+  {
+    A3 a;
+  }
+  return y;
+}
+
+
+} /* namespace N */
diff --git a/gcc/testsuite/g++.dg/diag-after-fld-2.C b/gcc/testsuite/g++.dg/diag-after-fld-2.C
new file mode 100644
index 00000000000..ffc5ae5c635
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diag-after-fld-2.C
@@ -0,0 +1,27 @@
+/* Continuation of diag-after-fld-1.C for C++11 features.  */
+/* { dg-do compile { target c++11 } } */
+/* { dg-require-effective-target lto } */
+/* { dg-options "-flto -Wuninitialized -O2" } */
+
+int y;
+
+int f1 (int)
+{
+  return [] () { /* { dg-regexp {.*: In lambda function:} } */
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  } (); 
+}
+
+template<typename T>
+int f2 (T)
+{
+  return [] () { /* { dg-regexp {.*: In lambda function:} } */
+    int x; /* { dg-note {'x' was declared here} } */
+    return y = x; /* { dg-warning {'x' is used uninitialized} } */
+  } (); 
+}
+int f2a ()
+{
+  return f2('\0');
+}
diff --git a/gcc/testsuite/gfortran.dg/allocatable_uninitialized_2.f90 b/gcc/testsuite/gfortran.dg/allocatable_uninitialized_2.f90
new file mode 100644
index 00000000000..9664ad63ba9
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/allocatable_uninitialized_2.f90
@@ -0,0 +1,17 @@
+! { dg-do compile }
+! { dg-require-effective-target lto }
+! { dg-options "-O -Wall -flto" }
+! { dg-additional-options "-fdiagnostics-show-caret -fdiagnostics-show-line-numbers" }
+
+program main
+  real,allocatable:: a(:),b(:)
+
+   a(1)=2*b(1)
+
+end
+
+! { dg-allow-blank-lines-in-output 1 }
+! { dg-regexp {.*allocatable_uninitialized_2\.f90:9:14:\n\n    9 \|    a\(1\)=2\*b\(1\)\n      \|              \^\nWarning: 'b.offset' is used uninitialized \[-Wuninitialized\]\n} }
+! { dg-regexp {.*allocatable_uninitialized_2\.f90:7:30:\n\n    7 \|   real,allocatable:: a\(:\),b\(:\)\n      \|                              \^\nnote: 'b' declared here\n} }
+! { dg-regexp {.*allocatable_uninitialized_2\.f90:9:14:\n\n    9 \|    a\(1\)=2\*b\(1\)\n      \|              \^\nWarning: 'a.offset' is used uninitialized \[-Wuninitialized\]\n} }
+! { dg-regexp {.*allocatable_uninitialized_2\.f90:7:25:\n\n    7 \|   real,allocatable:: a\(:\),b\(:\)\n      \|                         \^\nnote: 'a' declared here\n} }
diff --git a/gcc/testsuite/objc.dg/diag-after-fld-1.m b/gcc/testsuite/objc.dg/diag-after-fld-1.m
new file mode 100644
index 00000000000..3bc4aa18997
--- /dev/null
+++ b/gcc/testsuite/objc.dg/diag-after-fld-1.m
@@ -0,0 +1,24 @@
+/* Make sure that post-ipa-free-lang-data diagnostics call objc_printable_name
+   as expected.  */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target lto } */
+/* { dg-options "-flto -O2 -Wuninitialized" } */
+
+#include "../objc-obj-c++-shared/TestsuiteObject.m"
+
+@interface T : TestsuiteObject
++ (void) f;
+@end
+
+int y;
+
+@implementation T
+/* This dg-regexp is the main test; prior to this patch, we output the mangled
+   name '_c_T__f' here.  */
++ (void) f /* { dg-regexp {.*: In function '\+\[T f\]':} } */
+{
+  int x; /* { dg-note {'x' was declared here} } */
+  y = x; /* { dg-warning {-Wuninitialized} } */
+}
+@end
  

On Thu, Nov 3, 2022 at 9:07 PM Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> On Fri, Oct 28, 2022 at 10:28:21AM +0200, Richard Biener wrote:
> > Yes, the idea was also to free up memory but then that part never
> > really materialized - the idea was to always run free-lang-data, not
> > just when later outputting LTO bytecode.  The reason is probably
> > mainly the diagnostic regressions you observe.
> >
> > Maybe a better strathegy than your patch would be to work towards
> > that goal but reduce the number of "freeings", instead adjusting the
> > LTO streamer to properly ignore frontend specific bits where clearing
> > conflicts with the intent to preserve accurate diagnostics throughout
> > the compilation.
> >
> > If you see bits that when not freed would fix some of the observed
> > issues we can see to replicate the freeing in the LTO output machinery.
> >
> > Richard.
>
> Thanks again for the suggestions. I took a look and it seems pretty doable to
> just stop resetting all the diagnostics hooks in free-lang-data. Once that's
> done, the only problematic part that I have been able to identify is here in
> ipa-free-lang-data.c around line 674:
>
> ====
>   /* We need to keep field decls associated with their trees. Otherwise tree
>      merging may merge some fields and keep others disjoint which in turn will
>      not do well with TREE_CHAIN pointers linking them.
>
>      Also do not drop containing types for virtual methods and tables because
>      these are needed by devirtualization.
>      C++ destructors are special because C++ frontends sometimes produces
>      virtual destructor as an alias of non-virtual destructor.  In
>      devirutalization code we always walk through aliases and we need
>      context to be preserved too.  See PR89335  */
>   if (TREE_CODE (decl) != FIELD_DECL
>       && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL)
>           || (!DECL_VIRTUAL_P (decl)
>               && (TREE_CODE (decl) != FUNCTION_DECL
>                   || !DECL_CXX_DESTRUCTOR_P (decl)))))
>     DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl));
> ====
>
> The C++ implementations of the decl_printable_name langhook and the diagnostic
> starter callback do not work as-is when the DECL_CONTEXT for class member
> functions disappears.  So I did have a patch that changes the C++
> implementations to work in this case, but attached here is a new one along the
> lines of what you suggested, rather changing the above part of free-lang-data
> so it doesn't activate as often. The patch is pretty complete (other than
> missing a commit message) and bootstrap + regtest all languages looks good
> with no regressions. I tried the same with BUILD_CONFIG=bootstrap-lto as well,
> and that also looked good when it eventually finished. I added testcases for
> several frontends to verify that the diagnostics still work with -flto. I am
> not sure what are the implications for LTO itself, of changing this part of
> the pass, so I would have to ask you to weigh in on that aspect please. Thanks!

First of all sorry for the delay and thanks for trying.  The effect on LTO is an
increase in the amount of streamed IL since we follow the DECL_CONTEXT
edge when streaming the tree graph.  So my solution for this would be to
reflect the case you remove in free-lang-data in both
lto-streamer-out.cc:DFS::DFS_write_tree_body where we do

      if (TREE_CODE (expr) != TRANSLATION_UNIT_DECL
          && ! DECL_CONTEXT (expr))
        DFS_follow_tree_edge ((*all_translation_units)[0]);
      else
        DFS_follow_tree_edge (DECL_CONTEXT (expr));

and in tree-streamer-out.cc:write_ts_decl_minimal_tree_pointers which
does

  if (TREE_CODE (expr) != TRANSLATION_UNIT_DECL
      && ! DECL_CONTEXT (expr))
    stream_write_tree_ref (ob, (*all_translation_units)[0]);
  else
    stream_write_tree_ref (ob, DECL_CONTEXT (expr));

that possibly boils down to "just" doing

   tree ctx = DECL_CONTEXT (..);
   if (TREE_CODE (..) == VAR_DECL || TREE_CODE (..) == FUNCTION_DECL)
     ctx = fld_decl_context (ctx);

and using 'ctx' for DECL_CONTEXT in those two places (and exporting the
fld_decl_context function).

As said the idea for this is that we want to avoid streaming type trees when
not necessary.  When doing an LTO bootstrap with your patch you should
see (slightly) larger object files.

Richard.

>
> -Lewis