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

[Lewis Hyatt <lhyatt@gmail.com>]

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

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 ()