Message ID | 66c5296d.170a0220.3bae0d.0cdc@mx.google.com |
---|---|
State | New |
Headers | show |
Series | [1/3] c++: Handle ABI for non-polymorphic dynamic classes | expand |
Ping. On Wed, Aug 21, 2024 at 09:40:25AM +1000, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > The ICE in the linked PR is caused because maybe_clone_decl is not > prepared to be called on a declaration that has already had clones > created; what happens otherwise is that start_preparsed_function early > exits and never sets up cfun, causing a segfault later on. > > To fix this we ensure that post_load_processing only calls > maybe_clone_decl if TREE_ASM_WRITTEN has not been marked on the > declaration yet, and (if maybe_clone_decls succeeds) marks this flag on > the decl so that it doesn't get called again later when finalising > deferred vague linkage declarations in c_parse_final_cleanups. > > As a bonus this now allows us to only keep the DECL_SAVED_TREE around in > expand_or_defer_fn_1 for modules which have CMIs, which will have > benefits for LTO performance in non-interface TUs. > > For clarity we also update the streaming code to do post_load_decls for > maybe in-charge cdtors rather than any DECL_ABSTRACT_P declaration, as > this is more accurate to the decls affected by maybe_clone_body. > > PR c++/115007 > > gcc/cp/ChangeLog: > > * module.cc (module_state::read_cluster): Replace > DECL_ABSTRACT_P with DECL_MAYBE_IN_CHARGE_CDTOR_P. > (post_load_processing): Check and mark TREE_ASM_WRITTEN. > * semantics.cc (expand_or_defer_fn_1): Use the more specific > module_maybe_has_cmi_p instead of modules_p. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/virt-6_a.C: New test. > * g++.dg/modules/virt-6_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 7 ++++--- > gcc/cp/semantics.cc | 2 +- > gcc/testsuite/g++.dg/modules/virt-6_a.C | 13 +++++++++++++ > gcc/testsuite/g++.dg/modules/virt-6_b.C | 6 ++++++ > 4 files changed, 24 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 7c42aea05ee..5cd4f313933 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -15525,7 +15525,7 @@ module_state::read_cluster (unsigned snum) > > if (abstract) > ; > - else if (DECL_ABSTRACT_P (decl)) > + else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) > vec_safe_push (post_load_decls, decl); > else > { > @@ -17947,10 +17947,11 @@ post_load_processing () > > dump () && dump ("Post-load processing of %N", decl); > > - gcc_checking_assert (DECL_ABSTRACT_P (decl)); > + gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)); > /* Cloning can cause loading -- specifically operator delete for > the deleting dtor. */ > - maybe_clone_body (decl); > + if (!TREE_ASM_WRITTEN (decl) && maybe_clone_body (decl)) > + TREE_ASM_WRITTEN (decl) = 1; > } > > cfun = old_cfun; > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 5ab2076b673..f7ae8e68dcf 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -5122,7 +5122,7 @@ expand_or_defer_fn_1 (tree fn) > demand, so we also need to keep the body. Otherwise we don't > need it anymore. */ > if (!DECL_DECLARED_CONSTEXPR_P (fn) > - && !(modules_p () && vague_linkage_p (fn))) > + && !(module_maybe_has_cmi_p () && vague_linkage_p (fn))) > DECL_SAVED_TREE (fn) = NULL_TREE; > return false; > } > diff --git a/gcc/testsuite/g++.dg/modules/virt-6_a.C b/gcc/testsuite/g++.dg/modules/virt-6_a.C > new file mode 100644 > index 00000000000..68e466ace3f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/virt-6_a.C > @@ -0,0 +1,13 @@ > +// PR c++/115007 > +// { dg-additional-options "-fmodules-ts -Wno-global-module" } > +// { dg-module-cmi M:a } > + > +module; > +struct S { > + virtual ~S() = default; > + virtual void f() = 0; > +}; > +module M:a; > +extern S* p; > +template <typename T> void format(T) { p->~S(); } > +template void format(int); > diff --git a/gcc/testsuite/g++.dg/modules/virt-6_b.C b/gcc/testsuite/g++.dg/modules/virt-6_b.C > new file mode 100644 > index 00000000000..c53f5fac742 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/virt-6_b.C > @@ -0,0 +1,6 @@ > +// PR c++/115007 > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi M } > + > +export module M; > +import :a; > -- > 2.43.2 >
Ping for https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660957.html On Wed, Aug 21, 2024 at 09:40:25AM +1000, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > The ICE in the linked PR is caused because maybe_clone_decl is not > prepared to be called on a declaration that has already had clones > created; what happens otherwise is that start_preparsed_function early > exits and never sets up cfun, causing a segfault later on. > > To fix this we ensure that post_load_processing only calls > maybe_clone_decl if TREE_ASM_WRITTEN has not been marked on the > declaration yet, and (if maybe_clone_decls succeeds) marks this flag on > the decl so that it doesn't get called again later when finalising > deferred vague linkage declarations in c_parse_final_cleanups. > > As a bonus this now allows us to only keep the DECL_SAVED_TREE around in > expand_or_defer_fn_1 for modules which have CMIs, which will have > benefits for LTO performance in non-interface TUs. > > For clarity we also update the streaming code to do post_load_decls for > maybe in-charge cdtors rather than any DECL_ABSTRACT_P declaration, as > this is more accurate to the decls affected by maybe_clone_body. > > PR c++/115007 > > gcc/cp/ChangeLog: > > * module.cc (module_state::read_cluster): Replace > DECL_ABSTRACT_P with DECL_MAYBE_IN_CHARGE_CDTOR_P. > (post_load_processing): Check and mark TREE_ASM_WRITTEN. > * semantics.cc (expand_or_defer_fn_1): Use the more specific > module_maybe_has_cmi_p instead of modules_p. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/virt-6_a.C: New test. > * g++.dg/modules/virt-6_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 7 ++++--- > gcc/cp/semantics.cc | 2 +- > gcc/testsuite/g++.dg/modules/virt-6_a.C | 13 +++++++++++++ > gcc/testsuite/g++.dg/modules/virt-6_b.C | 6 ++++++ > 4 files changed, 24 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 7c42aea05ee..5cd4f313933 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -15525,7 +15525,7 @@ module_state::read_cluster (unsigned snum) > > if (abstract) > ; > - else if (DECL_ABSTRACT_P (decl)) > + else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) > vec_safe_push (post_load_decls, decl); > else > { > @@ -17947,10 +17947,11 @@ post_load_processing () > > dump () && dump ("Post-load processing of %N", decl); > > - gcc_checking_assert (DECL_ABSTRACT_P (decl)); > + gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)); > /* Cloning can cause loading -- specifically operator delete for > the deleting dtor. */ > - maybe_clone_body (decl); > + if (!TREE_ASM_WRITTEN (decl) && maybe_clone_body (decl)) > + TREE_ASM_WRITTEN (decl) = 1; > } > > cfun = old_cfun; > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 5ab2076b673..f7ae8e68dcf 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -5122,7 +5122,7 @@ expand_or_defer_fn_1 (tree fn) > demand, so we also need to keep the body. Otherwise we don't > need it anymore. */ > if (!DECL_DECLARED_CONSTEXPR_P (fn) > - && !(modules_p () && vague_linkage_p (fn))) > + && !(module_maybe_has_cmi_p () && vague_linkage_p (fn))) > DECL_SAVED_TREE (fn) = NULL_TREE; > return false; > } > diff --git a/gcc/testsuite/g++.dg/modules/virt-6_a.C b/gcc/testsuite/g++.dg/modules/virt-6_a.C > new file mode 100644 > index 00000000000..68e466ace3f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/virt-6_a.C > @@ -0,0 +1,13 @@ > +// PR c++/115007 > +// { dg-additional-options "-fmodules-ts -Wno-global-module" } > +// { dg-module-cmi M:a } > + > +module; > +struct S { > + virtual ~S() = default; > + virtual void f() = 0; > +}; > +module M:a; > +extern S* p; > +template <typename T> void format(T) { p->~S(); } > +template void format(int); > diff --git a/gcc/testsuite/g++.dg/modules/virt-6_b.C b/gcc/testsuite/g++.dg/modules/virt-6_b.C > new file mode 100644 > index 00000000000..c53f5fac742 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/virt-6_b.C > @@ -0,0 +1,6 @@ > +// PR c++/115007 > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi M } > + > +export module M; > +import :a; > -- > 2.43.2 >
On 8/20/24 7:40 PM, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? OK. > -- >8 -- > > The ICE in the linked PR is caused because maybe_clone_decl is not > prepared to be called on a declaration that has already had clones > created; what happens otherwise is that start_preparsed_function early > exits and never sets up cfun, causing a segfault later on. > > To fix this we ensure that post_load_processing only calls > maybe_clone_decl if TREE_ASM_WRITTEN has not been marked on the > declaration yet, and (if maybe_clone_decls succeeds) marks this flag on > the decl so that it doesn't get called again later when finalising > deferred vague linkage declarations in c_parse_final_cleanups. > > As a bonus this now allows us to only keep the DECL_SAVED_TREE around in > expand_or_defer_fn_1 for modules which have CMIs, which will have > benefits for LTO performance in non-interface TUs. > > For clarity we also update the streaming code to do post_load_decls for > maybe in-charge cdtors rather than any DECL_ABSTRACT_P declaration, as > this is more accurate to the decls affected by maybe_clone_body. > > PR c++/115007 > > gcc/cp/ChangeLog: > > * module.cc (module_state::read_cluster): Replace > DECL_ABSTRACT_P with DECL_MAYBE_IN_CHARGE_CDTOR_P. > (post_load_processing): Check and mark TREE_ASM_WRITTEN. > * semantics.cc (expand_or_defer_fn_1): Use the more specific > module_maybe_has_cmi_p instead of modules_p. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/virt-6_a.C: New test. > * g++.dg/modules/virt-6_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 7 ++++--- > gcc/cp/semantics.cc | 2 +- > gcc/testsuite/g++.dg/modules/virt-6_a.C | 13 +++++++++++++ > gcc/testsuite/g++.dg/modules/virt-6_b.C | 6 ++++++ > 4 files changed, 24 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 7c42aea05ee..5cd4f313933 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -15525,7 +15525,7 @@ module_state::read_cluster (unsigned snum) > > if (abstract) > ; > - else if (DECL_ABSTRACT_P (decl)) > + else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) > vec_safe_push (post_load_decls, decl); > else > { > @@ -17947,10 +17947,11 @@ post_load_processing () > > dump () && dump ("Post-load processing of %N", decl); > > - gcc_checking_assert (DECL_ABSTRACT_P (decl)); > + gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)); > /* Cloning can cause loading -- specifically operator delete for > the deleting dtor. */ > - maybe_clone_body (decl); > + if (!TREE_ASM_WRITTEN (decl) && maybe_clone_body (decl)) > + TREE_ASM_WRITTEN (decl) = 1; > } > > cfun = old_cfun; > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 5ab2076b673..f7ae8e68dcf 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -5122,7 +5122,7 @@ expand_or_defer_fn_1 (tree fn) > demand, so we also need to keep the body. Otherwise we don't > need it anymore. */ > if (!DECL_DECLARED_CONSTEXPR_P (fn) > - && !(modules_p () && vague_linkage_p (fn))) > + && !(module_maybe_has_cmi_p () && vague_linkage_p (fn))) > DECL_SAVED_TREE (fn) = NULL_TREE; > return false; > } > diff --git a/gcc/testsuite/g++.dg/modules/virt-6_a.C b/gcc/testsuite/g++.dg/modules/virt-6_a.C > new file mode 100644 > index 00000000000..68e466ace3f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/virt-6_a.C > @@ -0,0 +1,13 @@ > +// PR c++/115007 > +// { dg-additional-options "-fmodules-ts -Wno-global-module" } > +// { dg-module-cmi M:a } > + > +module; > +struct S { > + virtual ~S() = default; > + virtual void f() = 0; > +}; > +module M:a; > +extern S* p; > +template <typename T> void format(T) { p->~S(); } > +template void format(int); > diff --git a/gcc/testsuite/g++.dg/modules/virt-6_b.C b/gcc/testsuite/g++.dg/modules/virt-6_b.C > new file mode 100644 > index 00000000000..c53f5fac742 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/virt-6_b.C > @@ -0,0 +1,6 @@ > +// PR c++/115007 > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi M } > + > +export module M; > +import :a;
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 7c42aea05ee..5cd4f313933 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -15525,7 +15525,7 @@ module_state::read_cluster (unsigned snum) if (abstract) ; - else if (DECL_ABSTRACT_P (decl)) + else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) vec_safe_push (post_load_decls, decl); else { @@ -17947,10 +17947,11 @@ post_load_processing () dump () && dump ("Post-load processing of %N", decl); - gcc_checking_assert (DECL_ABSTRACT_P (decl)); + gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)); /* Cloning can cause loading -- specifically operator delete for the deleting dtor. */ - maybe_clone_body (decl); + if (!TREE_ASM_WRITTEN (decl) && maybe_clone_body (decl)) + TREE_ASM_WRITTEN (decl) = 1; } cfun = old_cfun; diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 5ab2076b673..f7ae8e68dcf 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -5122,7 +5122,7 @@ expand_or_defer_fn_1 (tree fn) demand, so we also need to keep the body. Otherwise we don't need it anymore. */ if (!DECL_DECLARED_CONSTEXPR_P (fn) - && !(modules_p () && vague_linkage_p (fn))) + && !(module_maybe_has_cmi_p () && vague_linkage_p (fn))) DECL_SAVED_TREE (fn) = NULL_TREE; return false; } diff --git a/gcc/testsuite/g++.dg/modules/virt-6_a.C b/gcc/testsuite/g++.dg/modules/virt-6_a.C new file mode 100644 index 00000000000..68e466ace3f --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/virt-6_a.C @@ -0,0 +1,13 @@ +// PR c++/115007 +// { dg-additional-options "-fmodules-ts -Wno-global-module" } +// { dg-module-cmi M:a } + +module; +struct S { + virtual ~S() = default; + virtual void f() = 0; +}; +module M:a; +extern S* p; +template <typename T> void format(T) { p->~S(); } +template void format(int); diff --git a/gcc/testsuite/g++.dg/modules/virt-6_b.C b/gcc/testsuite/g++.dg/modules/virt-6_b.C new file mode 100644 index 00000000000..c53f5fac742 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/virt-6_b.C @@ -0,0 +1,6 @@ +// PR c++/115007 +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi M } + +export module M; +import :a;
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- The ICE in the linked PR is caused because maybe_clone_decl is not prepared to be called on a declaration that has already had clones created; what happens otherwise is that start_preparsed_function early exits and never sets up cfun, causing a segfault later on. To fix this we ensure that post_load_processing only calls maybe_clone_decl if TREE_ASM_WRITTEN has not been marked on the declaration yet, and (if maybe_clone_decls succeeds) marks this flag on the decl so that it doesn't get called again later when finalising deferred vague linkage declarations in c_parse_final_cleanups. As a bonus this now allows us to only keep the DECL_SAVED_TREE around in expand_or_defer_fn_1 for modules which have CMIs, which will have benefits for LTO performance in non-interface TUs. For clarity we also update the streaming code to do post_load_decls for maybe in-charge cdtors rather than any DECL_ABSTRACT_P declaration, as this is more accurate to the decls affected by maybe_clone_body. PR c++/115007 gcc/cp/ChangeLog: * module.cc (module_state::read_cluster): Replace DECL_ABSTRACT_P with DECL_MAYBE_IN_CHARGE_CDTOR_P. (post_load_processing): Check and mark TREE_ASM_WRITTEN. * semantics.cc (expand_or_defer_fn_1): Use the more specific module_maybe_has_cmi_p instead of modules_p. gcc/testsuite/ChangeLog: * g++.dg/modules/virt-6_a.C: New test. * g++.dg/modules/virt-6_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 7 ++++--- gcc/cp/semantics.cc | 2 +- gcc/testsuite/g++.dg/modules/virt-6_a.C | 13 +++++++++++++ gcc/testsuite/g++.dg/modules/virt-6_b.C | 6 ++++++ 4 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_a.C create mode 100644 gcc/testsuite/g++.dg/modules/virt-6_b.C