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 <nathanielosh...@gmail.com> > > > > Reviewed-by: Jason Merrill <ja...@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