* [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] @ 2024-03-29 3:21 Nathaniel Shead 2024-03-29 12:33 ` Patrick Palka 2024-04-02 17:18 ` Jason Merrill 0 siblings, 2 replies; 10+ messages in thread From: Nathaniel Shead @ 2024-03-29 3:21 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell 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): Also keep DECL_SAVED_TREE for template instantiations. 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> --- gcc/cp/semantics.cc | 7 +++++-- gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++ gcc/testsuite/g++.dg/modules/pr104040_b.C | 8 ++++++++ 3 files changed, 27 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..84e9901509a 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -5033,9 +5033,12 @@ 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 () + && (DECL_DECLARED_INLINE_P (fn) + || DECL_TEMPLATE_INSTANTIATION (fn)))) DECL_SAVED_TREE (fn) = NULL_TREE; return false; } diff --git a/gcc/testsuite/g++.dg/modules/pr104040_a.C b/gcc/testsuite/g++.dg/modules/pr104040_a.C new file mode 100644 index 00000000000..ea36ce0a798 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr104040_a.C @@ -0,0 +1,14 @@ +// PR c++/104040 +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi test } + +export module test; + +export template <typename T> +struct test { + ~test() {} +}; + +test<bool> use() { + return {}; +} diff --git a/gcc/testsuite/g++.dg/modules/pr104040_b.C b/gcc/testsuite/g++.dg/modules/pr104040_b.C new file mode 100644 index 00000000000..efe014673fb --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr104040_b.C @@ -0,0 +1,8 @@ +// PR c++/104040 +// { dg-additional-options "-fmodules-ts" } + +import test; + +int main() { + test<bool> t{}; +} -- 2.43.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-03-29 3:21 [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] Nathaniel Shead @ 2024-03-29 12:33 ` Patrick Palka 2024-04-02 17:18 ` Jason Merrill 1 sibling, 0 replies; 10+ messages in thread From: Patrick Palka @ 2024-03-29 12:33 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Jason Merrill, Nathan Sidwell On Fri, 29 Mar 2024, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? LGTM > > -- >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): Also keep DECL_SAVED_TREE > for template instantiations. > > 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> > --- > gcc/cp/semantics.cc | 7 +++++-- > gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++ > gcc/testsuite/g++.dg/modules/pr104040_b.C | 8 ++++++++ > 3 files changed, 27 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..84e9901509a 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -5033,9 +5033,12 @@ 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 () > + && (DECL_DECLARED_INLINE_P (fn) > + || DECL_TEMPLATE_INSTANTIATION (fn)))) > DECL_SAVED_TREE (fn) = NULL_TREE; > return false; > } > diff --git a/gcc/testsuite/g++.dg/modules/pr104040_a.C b/gcc/testsuite/g++.dg/modules/pr104040_a.C > new file mode 100644 > index 00000000000..ea36ce0a798 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr104040_a.C > @@ -0,0 +1,14 @@ > +// PR c++/104040 > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi test } > + > +export module test; > + > +export template <typename T> > +struct test { > + ~test() {} > +}; > + > +test<bool> use() { > + return {}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/pr104040_b.C b/gcc/testsuite/g++.dg/modules/pr104040_b.C > new file mode 100644 > index 00000000000..efe014673fb > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr104040_b.C > @@ -0,0 +1,8 @@ > +// PR c++/104040 > +// { dg-additional-options "-fmodules-ts" } > + > +import test; > + > +int main() { > + test<bool> t{}; > +} > -- > 2.43.2 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-03-29 3:21 [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] Nathaniel Shead 2024-03-29 12:33 ` Patrick Palka @ 2024-04-02 17:18 ` Jason Merrill 2024-04-03 0:57 ` Nathaniel Shead 1 sibling, 1 reply; 10+ messages in thread From: Jason Merrill @ 2024-04-02 17:18 UTC (permalink / raw) To: Nathaniel Shead, gcc-patches; +Cc: Nathan Sidwell On 3/28/24 23:21, Nathaniel Shead wrote: > 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): Also keep DECL_SAVED_TREE > for template instantiations. > > 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> > --- > gcc/cp/semantics.cc | 7 +++++-- > gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++ > gcc/testsuite/g++.dg/modules/pr104040_b.C | 8 ++++++++ > 3 files changed, 27 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..84e9901509a 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -5033,9 +5033,12 @@ 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 () > + && (DECL_DECLARED_INLINE_P (fn) > + || DECL_TEMPLATE_INSTANTIATION (fn)))) How about using vague_linkage_p? > DECL_SAVED_TREE (fn) = NULL_TREE; > return false; > } > diff --git a/gcc/testsuite/g++.dg/modules/pr104040_a.C b/gcc/testsuite/g++.dg/modules/pr104040_a.C > new file mode 100644 > index 00000000000..ea36ce0a798 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr104040_a.C > @@ -0,0 +1,14 @@ > +// PR c++/104040 > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi test } > + > +export module test; > + > +export template <typename T> > +struct test { > + ~test() {} > +}; > + > +test<bool> use() { > + return {}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/pr104040_b.C b/gcc/testsuite/g++.dg/modules/pr104040_b.C > new file mode 100644 > index 00000000000..efe014673fb > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr104040_b.C > @@ -0,0 +1,8 @@ > +// PR c++/104040 > +// { dg-additional-options "-fmodules-ts" } > + > +import test; > + > +int main() { > + test<bool> t{}; > +} ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-04-02 17:18 ` Jason Merrill @ 2024-04-03 0:57 ` Nathaniel Shead 2024-04-03 15:18 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Nathaniel Shead @ 2024-04-03 0:57 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches, Nathan Sidwell On Tue, Apr 02, 2024 at 01:18:17PM -0400, Jason Merrill wrote: > On 3/28/24 23:21, Nathaniel Shead wrote: > > 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): Also keep DECL_SAVED_TREE > > for template instantiations. > > > > 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> > > --- > > gcc/cp/semantics.cc | 7 +++++-- > > gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++ > > gcc/testsuite/g++.dg/modules/pr104040_b.C | 8 ++++++++ > > 3 files changed, 27 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..84e9901509a 100644 > > --- a/gcc/cp/semantics.cc > > +++ b/gcc/cp/semantics.cc > > @@ -5033,9 +5033,12 @@ 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 () > > + && (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))) DECL_SAVED_TREE (fn) = NULL_TREE; return false; } diff --git a/gcc/testsuite/g++.dg/modules/pr104040_a.C b/gcc/testsuite/g++.dg/modules/pr104040_a.C new file mode 100644 index 00000000000..ea36ce0a798 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr104040_a.C @@ -0,0 +1,14 @@ +// PR c++/104040 +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi test } + +export module test; + +export template <typename T> +struct test { + ~test() {} +}; + +test<bool> use() { + return {}; +} diff --git a/gcc/testsuite/g++.dg/modules/pr104040_b.C b/gcc/testsuite/g++.dg/modules/pr104040_b.C new file mode 100644 index 00000000000..efe014673fb --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr104040_b.C @@ -0,0 +1,8 @@ +// PR c++/104040 +// { dg-additional-options "-fmodules-ts" } + +import test; + +int main() { + test<bool> t{}; +} -- 2.43.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-04-03 0:57 ` Nathaniel Shead @ 2024-04-03 15:18 ` Jason Merrill 2024-04-04 11:27 ` Nathaniel Shead 0 siblings, 1 reply; 10+ messages in thread From: Jason Merrill @ 2024-04-03 15:18 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Nathan Sidwell 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. Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-04-03 15:18 ` Jason Merrill @ 2024-04-04 11:27 ` Nathaniel Shead 2024-04-09 3:17 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Nathaniel Shead @ 2024-04-04 11:27 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches, Nathan Sidwell 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. > > Jason > 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? 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. 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-04-04 11:27 ` Nathaniel Shead @ 2024-04-09 3:17 ` Jason Merrill 2024-04-09 13:36 ` Nathaniel Shead 0 siblings, 1 reply; 10+ messages in thread From: Jason Merrill @ 2024-04-09 3:17 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Nathan Sidwell 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. > 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? > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-04-09 3:17 ` Jason Merrill @ 2024-04-09 13:36 ` Nathaniel Shead 2024-04-09 14:28 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Nathaniel Shead @ 2024-04-09 13:36 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-04-09 13:36 ` Nathaniel Shead @ 2024-04-09 14:28 ` Jason Merrill 2024-04-10 1:42 ` Nathaniel Shead 0 siblings, 1 reply; 10+ messages in thread From: Jason Merrill @ 2024-04-09 14:28 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Nathan Sidwell On 4/9/24 09:36, Nathaniel Shead wrote: > 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. Looking at the testcase, I guess the problem is that we parse the header, clone the constructor, throw away the cloned body, then import the same cloned constructor, merge it with the one with the body discarded, try to clone the result, and fail. Perhaps we want to avoid trying to clone after merging? For now, the last patch is OK with the comment adjustment mentioned below. >>> 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? > > 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 Aha, please add that to the comment. Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] 2024-04-09 14:28 ` Jason Merrill @ 2024-04-10 1:42 ` Nathaniel Shead 0 siblings, 0 replies; 10+ messages in thread From: Nathaniel Shead @ 2024-04-10 1:42 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches, Nathan Sidwell On Tue, Apr 09, 2024 at 10:28:01AM -0400, Jason Merrill wrote: > On 4/9/24 09:36, Nathaniel Shead wrote: > > 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. > > Looking at the testcase, I guess the problem is that we parse the header, > clone the constructor, throw away the cloned body, then import the same > cloned constructor, merge it with the one with the body discarded, try to > clone the result, and fail. > > Perhaps we want to avoid trying to clone after merging? For now, the last > patch is OK with the comment adjustment mentioned below. > Makes sense, thanks. > > > > 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? > > > > 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 > > Aha, please add that to the comment. > > Jason > Done; here's what I've pushed: -- >8 -- Subject: [PATCH] c++: Keep DECL_SAVED_TREE of cdtor instantiations in modules [PR104040] 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. This is true even for maybe-in-charge functions, because we don't currently stream the clones directly but instead regenerate them from this function. PR c++/104040 gcc/cp/ChangeLog: * semantics.cc (expand_or_defer_fn_1): Keep DECL_SAVED_TREE for all vague linkage cdtors with modules. 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 | 8 ++++++-- gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++ gcc/testsuite/g++.dg/modules/pr104040_b.C | 8 ++++++++ 3 files changed, 28 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 a43ff6e2ab2..329c524a509 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -5029,9 +5029,13 @@ 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 we still need the body to be + able to evaluate it. Similarly, with modules we only stream + the maybe-in-charge cdtor and regenerate the clones from it on + demand, so we also need to keep the body. Otherwise we don't + need it anymore. */ if (!DECL_DECLARED_CONSTEXPR_P (fn) - && !(modules_p () && DECL_DECLARED_INLINE_P (fn))) + && !(modules_p () && vague_linkage_p (fn))) DECL_SAVED_TREE (fn) = NULL_TREE; return false; } diff --git a/gcc/testsuite/g++.dg/modules/pr104040_a.C b/gcc/testsuite/g++.dg/modules/pr104040_a.C new file mode 100644 index 00000000000..ea36ce0a798 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr104040_a.C @@ -0,0 +1,14 @@ +// PR c++/104040 +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi test } + +export module test; + +export template <typename T> +struct test { + ~test() {} +}; + +test<bool> use() { + return {}; +} diff --git a/gcc/testsuite/g++.dg/modules/pr104040_b.C b/gcc/testsuite/g++.dg/modules/pr104040_b.C new file mode 100644 index 00000000000..efe014673fb --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr104040_b.C @@ -0,0 +1,8 @@ +// PR c++/104040 +// { dg-additional-options "-fmodules-ts" } + +import test; + +int main() { + test<bool> t{}; +} -- 2.43.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-10 1:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-29 3:21 [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040] Nathaniel Shead 2024-03-29 12:33 ` Patrick Palka 2024-04-02 17:18 ` Jason Merrill 2024-04-03 0:57 ` Nathaniel Shead 2024-04-03 15:18 ` Jason Merrill 2024-04-04 11:27 ` Nathaniel Shead 2024-04-09 3:17 ` Jason Merrill 2024-04-09 13:36 ` Nathaniel Shead 2024-04-09 14:28 ` Jason Merrill 2024-04-10 1:42 ` Nathaniel Shead
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).