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