Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040]

[Nathaniel Shead <nathanieloshead@gmail.com>]

On Mon, Apr 08, 2024 at 11:17:27PM -0400, Jason Merrill wrote:
> On 4/4/24 07:27, Nathaniel Shead wrote:
> > On Wed, Apr 03, 2024 at 11:18:01AM -0400, Jason Merrill wrote:
> > > On 4/2/24 20:57, Nathaniel Shead wrote:
> > > > On Tue, Apr 02, 2024 at 01:18:17PM -0400, Jason Merrill wrote:
> > > > > On 3/28/24 23:21, Nathaniel Shead wrote:
> > > > > > -	  && !(modules_p () && DECL_DECLARED_INLINE_P (fn)))
> > > > > > +	  && !(modules_p ()
> > > > > > +	       && (DECL_DECLARED_INLINE_P (fn)
> > > > > > +		   || DECL_TEMPLATE_INSTANTIATION (fn))))
> > > > > 
> > > > > How about using vague_linkage_p?
> > > > > 
> > > > 
> > > > Right, of course.  How about this?
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > 
> > > > -- >8 --
> > > > 
> > > > A template instantiation still needs to have its DECL_SAVED_TREE so that
> > > > its definition is emitted into the CMI. This way it can be emitted in
> > > > the object file of any importers that use it, in case it doesn't end up
> > > > getting emitted in this TU.
> > > > 
> > > > 	PR c++/104040
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	* semantics.cc (expand_or_defer_fn_1): Keep DECL_SAVED_TREE for
> > > > 	all vague linkage functions.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 
> > > > 	* g++.dg/modules/pr104040_a.C: New test.
> > > > 	* g++.dg/modules/pr104040_b.C: New test.
> > > > 
> > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > > > Reviewed-by: Jason Merrill <jason@redhat.com>
> > > > ---
> > > >    gcc/cp/semantics.cc                       |  5 +++--
> > > >    gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++
> > > >    gcc/testsuite/g++.dg/modules/pr104040_b.C |  8 ++++++++
> > > >    3 files changed, 25 insertions(+), 2 deletions(-)
> > > >    create mode 100644 gcc/testsuite/g++.dg/modules/pr104040_a.C
> > > >    create mode 100644 gcc/testsuite/g++.dg/modules/pr104040_b.C
> > > > 
> > > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> > > > index adb1ba48d29..03800a20b26 100644
> > > > --- a/gcc/cp/semantics.cc
> > > > +++ b/gcc/cp/semantics.cc
> > > > @@ -5033,9 +5033,10 @@ expand_or_defer_fn_1 (tree fn)
> > > >          /* We don't want to process FN again, so pretend we've written
> > > >    	 it out, even though we haven't.  */
> > > >          TREE_ASM_WRITTEN (fn) = 1;
> > > > -      /* If this is a constexpr function, keep DECL_SAVED_TREE.  */
> > > > +      /* If this is a constexpr function, or the body might need to be
> > > > +	 exported from a module CMI, keep DECL_SAVED_TREE.  */
> > > >          if (!DECL_DECLARED_CONSTEXPR_P (fn)
> > > > -	  && !(modules_p () && DECL_DECLARED_INLINE_P (fn)))
> > > > +	  && !(modules_p () && vague_linkage_p (fn)))
> > > 
> > > Also, how about module_maybe_has_cmi_p?  OK with that change.
> > 
> > Using 'module_maybe_has_cmi_p' doesn't seem to work.  This is for two
> > reasons, one of them fixable and one of them not (easily):
> > 
> > - It seems that header modules don't count for 'module_maybe_has_cmi_p';
> >    I didn't notice this initially, and maybe they should for the
> >    no-linkage decls too?
> 
> I think so; they could similarly be referred to by an importer.
> 

I'll investigate further and make a patch and test for this when I get a
chance then.

> >  But even accounting for this,
> > 
> > - For some reason only clearing it if the module might have a CMI causes
> >    crashes in importers for some testcases.  I'm not 100% sure why yet,
> >    but I suspect it might be some duplicate-decls thing where the type
> >    inconsistently has DECL_SAVED_TREE applied, since this is also called
> >    on streamed-in declarations.
> 
> Clearing if the module might have a CMI sounds backwards, I'd expect that to
> be the case where we want to leave it alone.  Is that the problem, or just a
> typo?
> 

Sorry typo, yes. I've tried the following incremental patch:

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 5a862a3ee5f..3341ade4e33 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -5036,7 +5036,8 @@ expand_or_defer_fn_1 (tree fn)
       /* If this is a constexpr function, or the body might need to be
         exported from a module CMI, keep DECL_SAVED_TREE.  */
       if (!DECL_DECLARED_CONSTEXPR_P (fn)
-         && !(modules_p () && vague_linkage_p (fn)))
+         && !((module_maybe_has_cmi_p () || header_module_p ())
+              && vague_linkage_p (fn)))
        DECL_SAVED_TREE (fn) = NULL_TREE;
       return false;
     }

and this causes ICEs with e.g. testsuite/g++.dg/modules/concept-6_b.C,
where maybe_clone_body is called with a NULL cfun.  I think one of the
post-load processing loops might have cleared cfun before it got called?
Not sure, haven't looked too hard; I can dig in further later if you
would like.

> > Out of interest, what was the reason that it was cleared at all in the
> > first place?  I wasn't able to find anything with git blame; is it just
> > for performance reasons in avoiding excess lowering later?
> 
> That change goes back to the LTO merge, I believe it was to reduce
> unnecessary LTO streaming.
> 
> But now that I think about it some more, I don't see why handling modules
> specially here is necessary at all; the point of this code is that after we
> build the destructor clones, the DECL_SAVED_TREE of the cloned function is
> no longer useful.  Why would modules care about the maybe-in-charge
> function?
> 
> Jason
> 

The current modules implementation doesn't stream the clones: instead
it always just streams the maybe-in-charge functions (including its
tree) and recreates the clones on import.  I believe Nathan said that
there were issues with streaming the clones directly, see
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635882.html