* [PATCH] c++/modules: Prevent treating suppressed debug info as extern template [PR112820] @ 2023-12-03 12:46 Nathaniel Shead 2024-01-02 22:52 ` Nathaniel Shead 2024-01-04 20:39 ` Patrick Palka 0 siblings, 2 replies; 11+ messages in thread From: Nathaniel Shead @ 2023-12-03 12:46 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same underlying bit. This is causing confusion when attempting to determine the interface for a streamed-in class type, since the modules code currently assumes that all DECL_EXTERNAL types are extern templates. However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence DECL_EXTERNAL) is marked on various other kinds of declarations, such as vtables, which causes them to never be emitted. This patch constrains the checks for DECL_EXTERNAL for this to only consider template instantiations, thus avoiding the issue. PR c++/102607 PR c++/112820 gcc/cp/ChangeLog: * module.cc (trees_in::read_class_def): Only set interface for template instantiations. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 4 +++- gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 +++++++++ gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 +++++++++ gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 +++++++++ 6 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 33fcf396875..257f39421d0 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -12041,7 +12041,9 @@ trees_in::read_class_def (tree defn, tree maybe_template) bool installing = maybe_dup && !TYPE_SIZE (type); if (installing) { - if (DECL_EXTERNAL (defn) && TYPE_LANG_SPECIFIC (type)) + if (DECL_EXTERNAL (defn) + && TYPE_LANG_SPECIFIC (type) + && CLASSTYPE_TEMPLATE_INSTANTIATION (type)) { /* We don't deal with not-really-extern, because, for a module you want the import to be the interface, and for a diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C new file mode 100644 index 00000000000..eed0905542b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi io } + +export module io; + +export struct error { + virtual const char* what() const noexcept; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C new file mode 100644 index 00000000000..fc9afbc02e0 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C @@ -0,0 +1,8 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } + +module io; + +const char* error::what() const noexcept { + return "bla"; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C new file mode 100644 index 00000000000..37117f69dcd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import io; + +int main() { + error{}; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C new file mode 100644 index 00000000000..9e33d8260fd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C @@ -0,0 +1,8 @@ +// PR c++/102607 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi mod } + +export module mod; +export struct B { + virtual ~B() = default; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C new file mode 100644 index 00000000000..03c78b71b5d --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C @@ -0,0 +1,9 @@ +// PR c++/102607 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import mod; +int main() { + struct D : B {}; + (void)D{}; +} -- 2.42.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] c++/modules: Prevent treating suppressed debug info as extern template [PR112820] 2023-12-03 12:46 [PATCH] c++/modules: Prevent treating suppressed debug info as extern template [PR112820] Nathaniel Shead @ 2024-01-02 22:52 ` Nathaniel Shead 2024-01-04 20:39 ` Patrick Palka 1 sibling, 0 replies; 11+ messages in thread From: Nathaniel Shead @ 2024-01-02 22:52 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639082.html. On Sun, Dec 03, 2023 at 11:46:36PM +1100, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same > underlying bit. This is causing confusion when attempting to determine > the interface for a streamed-in class type, since the modules code > currently assumes that all DECL_EXTERNAL types are extern templates. > However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence > DECL_EXTERNAL) is marked on various other kinds of declarations, such as > vtables, which causes them to never be emitted. > > This patch constrains the checks for DECL_EXTERNAL for this to only > consider template instantiations, thus avoiding the issue. > > PR c++/102607 > PR c++/112820 > > gcc/cp/ChangeLog: > > * module.cc (trees_in::read_class_def): Only set interface for > template instantiations. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/debug-2_a.C: New test. > * g++.dg/modules/debug-2_b.C: New test. > * g++.dg/modules/debug-2_c.C: New test. > * g++.dg/modules/debug-3_a.C: New test. > * g++.dg/modules/debug-3_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 4 +++- > gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 +++++++++ > 6 files changed, 46 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 33fcf396875..257f39421d0 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -12041,7 +12041,9 @@ trees_in::read_class_def (tree defn, tree maybe_template) > bool installing = maybe_dup && !TYPE_SIZE (type); > if (installing) > { > - if (DECL_EXTERNAL (defn) && TYPE_LANG_SPECIFIC (type)) > + if (DECL_EXTERNAL (defn) > + && TYPE_LANG_SPECIFIC (type) > + && CLASSTYPE_TEMPLATE_INSTANTIATION (type)) > { > /* We don't deal with not-really-extern, because, for a > module you want the import to be the interface, and for a > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C > new file mode 100644 > index 00000000000..eed0905542b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi io } > + > +export module io; > + > +export struct error { > + virtual const char* what() const noexcept; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C > new file mode 100644 > index 00000000000..fc9afbc02e0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C > @@ -0,0 +1,8 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > + > +module io; > + > +const char* error::what() const noexcept { > + return "bla"; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C > new file mode 100644 > index 00000000000..37117f69dcd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import io; > + > +int main() { > + error{}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C > new file mode 100644 > index 00000000000..9e33d8260fd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C > @@ -0,0 +1,8 @@ > +// PR c++/102607 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi mod } > + > +export module mod; > +export struct B { > + virtual ~B() = default; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C > new file mode 100644 > index 00000000000..03c78b71b5d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C > @@ -0,0 +1,9 @@ > +// PR c++/102607 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import mod; > +int main() { > + struct D : B {}; > + (void)D{}; > +} > -- > 2.42.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] c++/modules: Prevent treating suppressed debug info as extern template [PR112820] 2023-12-03 12:46 [PATCH] c++/modules: Prevent treating suppressed debug info as extern template [PR112820] Nathaniel Shead 2024-01-02 22:52 ` Nathaniel Shead @ 2024-01-04 20:39 ` Patrick Palka 2024-01-08 9:57 ` [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] Nathaniel Shead 1 sibling, 1 reply; 11+ messages in thread From: Patrick Palka @ 2024-01-04 20:39 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Jason Merrill, Nathan Sidwell On Sun, 3 Dec 2023, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same > underlying bit. This is causing confusion when attempting to determine > the interface for a streamed-in class type, since the modules code > currently assumes that all DECL_EXTERNAL types are extern templates. > However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence > DECL_EXTERNAL) is marked on various other kinds of declarations, such as > vtables, which causes them to never be emitted. Good catch.. Maybe we should use different bits for these flags? I wouldn't be surprised if this bit sharing causes issues elsewhere in the compiler. The documentation in tree.h / tree-core.h says DECL_EXTERNAL is only valid for VAR_DECL and FUNCTION_DECL, so at one point it was safe to share the same bit but that's not true anymore it seems. Looking at tree-core.h:tree_decl_common luckily we have plenty of spare bits. We could also e.g. make TYPE_DECL_SUPPRESS_DEBUG use the decl_not_flexarray bit which is otherwise only used for FIELD_DECL. > > This patch constrains the checks for DECL_EXTERNAL for this to only > consider template instantiations, thus avoiding the issue. > > PR c++/102607 > PR c++/112820 > > gcc/cp/ChangeLog: > > * module.cc (trees_in::read_class_def): Only set interface for > template instantiations. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/debug-2_a.C: New test. > * g++.dg/modules/debug-2_b.C: New test. > * g++.dg/modules/debug-2_c.C: New test. > * g++.dg/modules/debug-3_a.C: New test. > * g++.dg/modules/debug-3_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 4 +++- > gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 +++++++++ > 6 files changed, 46 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 33fcf396875..257f39421d0 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -12041,7 +12041,9 @@ trees_in::read_class_def (tree defn, tree maybe_template) > bool installing = maybe_dup && !TYPE_SIZE (type); > if (installing) > { > - if (DECL_EXTERNAL (defn) && TYPE_LANG_SPECIFIC (type)) > + if (DECL_EXTERNAL (defn) > + && TYPE_LANG_SPECIFIC (type) > + && CLASSTYPE_TEMPLATE_INSTANTIATION (type)) > { > /* We don't deal with not-really-extern, because, for a > module you want the import to be the interface, and for a > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C > new file mode 100644 > index 00000000000..eed0905542b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi io } > + > +export module io; > + > +export struct error { > + virtual const char* what() const noexcept; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C > new file mode 100644 > index 00000000000..fc9afbc02e0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C > @@ -0,0 +1,8 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > + > +module io; > + > +const char* error::what() const noexcept { > + return "bla"; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C > new file mode 100644 > index 00000000000..37117f69dcd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import io; > + > +int main() { > + error{}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C > new file mode 100644 > index 00000000000..9e33d8260fd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C > @@ -0,0 +1,8 @@ > +// PR c++/102607 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi mod } > + > +export module mod; > +export struct B { > + virtual ~B() = default; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C > new file mode 100644 > index 00000000000..03c78b71b5d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C > @@ -0,0 +1,9 @@ > +// PR c++/102607 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import mod; > +int main() { > + struct D : B {}; > + (void)D{}; > +} > -- > 2.42.0 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] 2024-01-04 20:39 ` Patrick Palka @ 2024-01-08 9:57 ` Nathaniel Shead 2024-01-08 11:50 ` Richard Biener 2024-01-08 15:27 ` Patrick Palka 0 siblings, 2 replies; 11+ messages in thread From: Nathaniel Shead @ 2024-01-08 9:57 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, Jason Merrill, Nathan Sidwell On Thu, Jan 04, 2024 at 03:39:15PM -0500, Patrick Palka wrote: > On Sun, 3 Dec 2023, Nathaniel Shead wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > -- >8 -- > > > > The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same > > underlying bit. This is causing confusion when attempting to determine > > the interface for a streamed-in class type, since the modules code > > currently assumes that all DECL_EXTERNAL types are extern templates. > > However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence > > DECL_EXTERNAL) is marked on various other kinds of declarations, such as > > vtables, which causes them to never be emitted. > > Good catch.. Maybe we should use different bits for these flags? I wouldn't be > surprised if this bit sharing causes issues elsewhere in the compiler. The > documentation in tree.h / tree-core.h says DECL_EXTERNAL is only valid for > VAR_DECL and FUNCTION_DECL, so at one point it was safe to share the same bit > but that's not true anymore it seems. > > Looking at tree-core.h:tree_decl_common luckily we have plenty of spare bits. > We could also e.g. make TYPE_DECL_SUPPRESS_DEBUG use the decl_not_flexarray bit > which is otherwise only used for FIELD_DECL. > That seems like a good idea, thanks. How does this look? Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- Currently, DECL_EXTERNAL and TYPE_DECL_SUPPRESS_DEBUG share a bit. This causes issues with module code, which then incorrectly assumes that anything with suppressed debug info (such as vtables when '-g' is specified) is an extern template and thus prevents their emission. This patch splits the two flags up; extern templates continue to use the DECL_EXTERNAL flag (and the documentation is updated to indicate this), but TYPE_DECL_SUPPRESS_DEBUG now uses the 'decl_not_flexarray' flag, which currently is only used by FIELD_DECLs. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * pt.cc (mark_class_instantiated): Set DECL_EXTERNAL explicitly. gcc/ChangeLog: * tree-core.h (struct tree_decl_common): Update comments. * tree.h (DECL_EXTERNAL): Update comments. (TYPE_DECL_SUPPRESS_DEBUG): Use 'decl_not_flexarray' instead. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/pt.cc | 1 + gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 +++++++++ gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 +++++++++ gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 +++++++++ gcc/tree-core.h | 6 +++--- gcc/tree.h | 8 ++++---- 8 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index e38e7a773f0..7839745035b 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -26256,6 +26256,7 @@ mark_class_instantiated (tree t, int extern_p) SET_CLASSTYPE_EXPLICIT_INSTANTIATION (t); SET_CLASSTYPE_INTERFACE_KNOWN (t); CLASSTYPE_INTERFACE_ONLY (t) = extern_p; + DECL_EXTERNAL (TYPE_NAME (t)) = extern_p; TYPE_DECL_SUPPRESS_DEBUG (TYPE_NAME (t)) = extern_p; if (! extern_p) { diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C new file mode 100644 index 00000000000..eed0905542b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi io } + +export module io; + +export struct error { + virtual const char* what() const noexcept; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C new file mode 100644 index 00000000000..fc9afbc02e0 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C @@ -0,0 +1,8 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } + +module io; + +const char* error::what() const noexcept { + return "bla"; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C new file mode 100644 index 00000000000..37117f69dcd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import io; + +int main() { + error{}; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C new file mode 100644 index 00000000000..9e33d8260fd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C @@ -0,0 +1,8 @@ +// PR c++/102607 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi mod } + +export module mod; +export struct B { + virtual ~B() = default; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C new file mode 100644 index 00000000000..03c78b71b5d --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C @@ -0,0 +1,9 @@ +// PR c++/102607 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import mod; +int main() { + struct D : B {}; + (void)D{}; +} diff --git a/gcc/tree-core.h b/gcc/tree-core.h index 8a89462bd7e..1ca4d4c07bd 100644 --- a/gcc/tree-core.h +++ b/gcc/tree-core.h @@ -1814,8 +1814,7 @@ struct GTY(()) tree_decl_common { In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */ unsigned decl_flag_0 : 1; /* In FIELD_DECL, this is DECL_BIT_FIELD - In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL. - In TYPE_DECL, this is TYPE_DECL_SUPPRESS_DEBUG. */ + In VAR_DECL, FUNCTION_DECL, and TYPE_DECL, this is DECL_EXTERNAL. */ unsigned decl_flag_1 : 1; /* In FIELD_DECL, this is DECL_NONADDRESSABLE_P In VAR_DECL, PARM_DECL and RESULT_DECL, this is @@ -1845,7 +1844,8 @@ struct GTY(()) tree_decl_common { TYPE_WARN_IF_NOT_ALIGN. */ unsigned int warn_if_not_align : 6; - /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY. */ + /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY. + In TYPE_DECL, this is TYPE_DECL_SUPPRESS_DEBUG. */ unsigned int decl_not_flexarray : 1; /* 5 bits unused. */ diff --git a/gcc/tree.h b/gcc/tree.h index 972a067a1f7..8bdc471ad4e 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -2848,9 +2848,9 @@ extern tree vector_element_bits_tree (const_tree); #define DECL_LANG_SPECIFIC(NODE) \ (DECL_COMMON_CHECK (NODE)->decl_common.lang_specific) -/* In a VAR_DECL or FUNCTION_DECL, nonzero means external reference: - do not allocate storage, and refer to a definition elsewhere. Note that - this does not necessarily imply the entity represented by NODE +/* In a VAR_DECL, FUNCTION_DECL, or TYPE_DECL, nonzero means external + reference: do not allocate storage, and refer to a definition elsewhere. + Note that this does not necessarily imply the entity represented by NODE has no program source-level definition in this translation unit. For example, for a FUNCTION_DECL, DECL_SAVED_TREE may be non-NULL and DECL_EXTERNAL may be true simultaneously; that can be the case for @@ -3553,7 +3553,7 @@ extern vec<tree, va_gc> **decl_debug_args_insert (tree); into stabs. Instead it will generate cross reference ('x') of names. This uses the same flag as DECL_EXTERNAL. */ #define TYPE_DECL_SUPPRESS_DEBUG(NODE) \ - (TYPE_DECL_CHECK (NODE)->decl_common.decl_flag_1) + (TYPE_DECL_CHECK (NODE)->decl_common.decl_not_flexarray) /* Getter of the imported declaration associated to the IMPORTED_DECL node. */ -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] 2024-01-08 9:57 ` [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] Nathaniel Shead @ 2024-01-08 11:50 ` Richard Biener 2024-01-08 15:27 ` Patrick Palka 1 sibling, 0 replies; 11+ messages in thread From: Richard Biener @ 2024-01-08 11:50 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Patrick Palka, gcc-patches, Jason Merrill, Nathan Sidwell On Mon, Jan 8, 2024 at 10:58 AM Nathaniel Shead <nathanieloshead@gmail.com> wrote: > > On Thu, Jan 04, 2024 at 03:39:15PM -0500, Patrick Palka wrote: > > On Sun, 3 Dec 2023, Nathaniel Shead wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > > > -- >8 -- > > > > > > The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same > > > underlying bit. This is causing confusion when attempting to determine > > > the interface for a streamed-in class type, since the modules code > > > currently assumes that all DECL_EXTERNAL types are extern templates. > > > However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence > > > DECL_EXTERNAL) is marked on various other kinds of declarations, such as > > > vtables, which causes them to never be emitted. > > > > Good catch.. Maybe we should use different bits for these flags? I wouldn't be > > surprised if this bit sharing causes issues elsewhere in the compiler. The > > documentation in tree.h / tree-core.h says DECL_EXTERNAL is only valid for > > VAR_DECL and FUNCTION_DECL, so at one point it was safe to share the same bit > > but that's not true anymore it seems. > > > > Looking at tree-core.h:tree_decl_common luckily we have plenty of spare bits. > > We could also e.g. make TYPE_DECL_SUPPRESS_DEBUG use the decl_not_flexarray bit > > which is otherwise only used for FIELD_DECL. > > > > That seems like a good idea, thanks. How does this look? > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? OK if C++ folks are fine. Richard. > -- >8 -- > > Currently, DECL_EXTERNAL and TYPE_DECL_SUPPRESS_DEBUG share a bit. This > causes issues with module code, which then incorrectly assumes that > anything with suppressed debug info (such as vtables when '-g' is > specified) is an extern template and thus prevents their emission. > > This patch splits the two flags up; extern templates continue to use the > DECL_EXTERNAL flag (and the documentation is updated to indicate this), > but TYPE_DECL_SUPPRESS_DEBUG now uses the 'decl_not_flexarray' flag, > which currently is only used by FIELD_DECLs. > > PR c++/112820 > PR c++/102607 > > gcc/cp/ChangeLog: > > * pt.cc (mark_class_instantiated): Set DECL_EXTERNAL explicitly. > > gcc/ChangeLog: > > * tree-core.h (struct tree_decl_common): Update comments. > * tree.h (DECL_EXTERNAL): Update comments. > (TYPE_DECL_SUPPRESS_DEBUG): Use 'decl_not_flexarray' instead. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/debug-2_a.C: New test. > * g++.dg/modules/debug-2_b.C: New test. > * g++.dg/modules/debug-2_c.C: New test. > * g++.dg/modules/debug-3_a.C: New test. > * g++.dg/modules/debug-3_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/pt.cc | 1 + > gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 +++++++++ > gcc/tree-core.h | 6 +++--- > gcc/tree.h | 8 ++++---- > 8 files changed, 51 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index e38e7a773f0..7839745035b 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -26256,6 +26256,7 @@ mark_class_instantiated (tree t, int extern_p) > SET_CLASSTYPE_EXPLICIT_INSTANTIATION (t); > SET_CLASSTYPE_INTERFACE_KNOWN (t); > CLASSTYPE_INTERFACE_ONLY (t) = extern_p; > + DECL_EXTERNAL (TYPE_NAME (t)) = extern_p; > TYPE_DECL_SUPPRESS_DEBUG (TYPE_NAME (t)) = extern_p; > if (! extern_p) > { > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C > new file mode 100644 > index 00000000000..eed0905542b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi io } > + > +export module io; > + > +export struct error { > + virtual const char* what() const noexcept; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C > new file mode 100644 > index 00000000000..fc9afbc02e0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C > @@ -0,0 +1,8 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > + > +module io; > + > +const char* error::what() const noexcept { > + return "bla"; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C > new file mode 100644 > index 00000000000..37117f69dcd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import io; > + > +int main() { > + error{}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C > new file mode 100644 > index 00000000000..9e33d8260fd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C > @@ -0,0 +1,8 @@ > +// PR c++/102607 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi mod } > + > +export module mod; > +export struct B { > + virtual ~B() = default; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C > new file mode 100644 > index 00000000000..03c78b71b5d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C > @@ -0,0 +1,9 @@ > +// PR c++/102607 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import mod; > +int main() { > + struct D : B {}; > + (void)D{}; > +} > diff --git a/gcc/tree-core.h b/gcc/tree-core.h > index 8a89462bd7e..1ca4d4c07bd 100644 > --- a/gcc/tree-core.h > +++ b/gcc/tree-core.h > @@ -1814,8 +1814,7 @@ struct GTY(()) tree_decl_common { > In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */ > unsigned decl_flag_0 : 1; > /* In FIELD_DECL, this is DECL_BIT_FIELD > - In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL. > - In TYPE_DECL, this is TYPE_DECL_SUPPRESS_DEBUG. */ > + In VAR_DECL, FUNCTION_DECL, and TYPE_DECL, this is DECL_EXTERNAL. */ > unsigned decl_flag_1 : 1; > /* In FIELD_DECL, this is DECL_NONADDRESSABLE_P > In VAR_DECL, PARM_DECL and RESULT_DECL, this is > @@ -1845,7 +1844,8 @@ struct GTY(()) tree_decl_common { > TYPE_WARN_IF_NOT_ALIGN. */ > unsigned int warn_if_not_align : 6; > > - /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY. */ > + /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY. > + In TYPE_DECL, this is TYPE_DECL_SUPPRESS_DEBUG. */ > unsigned int decl_not_flexarray : 1; > > /* 5 bits unused. */ > diff --git a/gcc/tree.h b/gcc/tree.h > index 972a067a1f7..8bdc471ad4e 100644 > --- a/gcc/tree.h > +++ b/gcc/tree.h > @@ -2848,9 +2848,9 @@ extern tree vector_element_bits_tree (const_tree); > #define DECL_LANG_SPECIFIC(NODE) \ > (DECL_COMMON_CHECK (NODE)->decl_common.lang_specific) > > -/* In a VAR_DECL or FUNCTION_DECL, nonzero means external reference: > - do not allocate storage, and refer to a definition elsewhere. Note that > - this does not necessarily imply the entity represented by NODE > +/* In a VAR_DECL, FUNCTION_DECL, or TYPE_DECL, nonzero means external > + reference: do not allocate storage, and refer to a definition elsewhere. > + Note that this does not necessarily imply the entity represented by NODE > has no program source-level definition in this translation unit. For > example, for a FUNCTION_DECL, DECL_SAVED_TREE may be non-NULL and > DECL_EXTERNAL may be true simultaneously; that can be the case for > @@ -3553,7 +3553,7 @@ extern vec<tree, va_gc> **decl_debug_args_insert (tree); > into stabs. Instead it will generate cross reference ('x') of names. > This uses the same flag as DECL_EXTERNAL. */ > #define TYPE_DECL_SUPPRESS_DEBUG(NODE) \ > - (TYPE_DECL_CHECK (NODE)->decl_common.decl_flag_1) > + (TYPE_DECL_CHECK (NODE)->decl_common.decl_not_flexarray) > > /* Getter of the imported declaration associated to the > IMPORTED_DECL node. */ > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] 2024-01-08 9:57 ` [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] Nathaniel Shead 2024-01-08 11:50 ` Richard Biener @ 2024-01-08 15:27 ` Patrick Palka 2024-01-15 23:10 ` Jason Merrill 1 sibling, 1 reply; 11+ messages in thread From: Patrick Palka @ 2024-01-08 15:27 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Patrick Palka, gcc-patches, Jason Merrill, Nathan Sidwell On Mon, 8 Jan 2024, Nathaniel Shead wrote: > On Thu, Jan 04, 2024 at 03:39:15PM -0500, Patrick Palka wrote: > > On Sun, 3 Dec 2023, Nathaniel Shead wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > > > -- >8 -- > > > > > > The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same > > > underlying bit. This is causing confusion when attempting to determine > > > the interface for a streamed-in class type, since the modules code > > > currently assumes that all DECL_EXTERNAL types are extern templates. > > > However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence > > > DECL_EXTERNAL) is marked on various other kinds of declarations, such as > > > vtables, which causes them to never be emitted. > > > > Good catch.. Maybe we should use different bits for these flags? I wouldn't be > > surprised if this bit sharing causes issues elsewhere in the compiler. The > > documentation in tree.h / tree-core.h says DECL_EXTERNAL is only valid for > > VAR_DECL and FUNCTION_DECL, so at one point it was safe to share the same bit > > but that's not true anymore it seems. > > > > Looking at tree-core.h:tree_decl_common luckily we have plenty of spare bits. > > We could also e.g. make TYPE_DECL_SUPPRESS_DEBUG use the decl_not_flexarray bit > > which is otherwise only used for FIELD_DECL. > > > > That seems like a good idea, thanks. How does this look? > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > Currently, DECL_EXTERNAL and TYPE_DECL_SUPPRESS_DEBUG share a bit. This > causes issues with module code, which then incorrectly assumes that > anything with suppressed debug info (such as vtables when '-g' is > specified) is an extern template and thus prevents their emission. > > This patch splits the two flags up; extern templates continue to use the > DECL_EXTERNAL flag (and the documentation is updated to indicate this), > but TYPE_DECL_SUPPRESS_DEBUG now uses the 'decl_not_flexarray' flag, > which currently is only used by FIELD_DECLs. > > PR c++/112820 > PR c++/102607 > > gcc/cp/ChangeLog: > > * pt.cc (mark_class_instantiated): Set DECL_EXTERNAL explicitly. > > gcc/ChangeLog: > > * tree-core.h (struct tree_decl_common): Update comments. > * tree.h (DECL_EXTERNAL): Update comments. > (TYPE_DECL_SUPPRESS_DEBUG): Use 'decl_not_flexarray' instead. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/debug-2_a.C: New test. > * g++.dg/modules/debug-2_b.C: New test. > * g++.dg/modules/debug-2_c.C: New test. > * g++.dg/modules/debug-3_a.C: New test. > * g++.dg/modules/debug-3_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/pt.cc | 1 + > gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 +++++++++ > gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++++ > gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 +++++++++ > gcc/tree-core.h | 6 +++--- > gcc/tree.h | 8 ++++---- > 8 files changed, 51 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index e38e7a773f0..7839745035b 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -26256,6 +26256,7 @@ mark_class_instantiated (tree t, int extern_p) > SET_CLASSTYPE_EXPLICIT_INSTANTIATION (t); > SET_CLASSTYPE_INTERFACE_KNOWN (t); > CLASSTYPE_INTERFACE_ONLY (t) = extern_p; > + DECL_EXTERNAL (TYPE_NAME (t)) = extern_p; > TYPE_DECL_SUPPRESS_DEBUG (TYPE_NAME (t)) = extern_p; > if (! extern_p) > { > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C > new file mode 100644 > index 00000000000..eed0905542b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi io } > + > +export module io; > + > +export struct error { > + virtual const char* what() const noexcept; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C > new file mode 100644 > index 00000000000..fc9afbc02e0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C > @@ -0,0 +1,8 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > + > +module io; > + > +const char* error::what() const noexcept { > + return "bla"; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C > new file mode 100644 > index 00000000000..37117f69dcd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import io; > + > +int main() { > + error{}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C > new file mode 100644 > index 00000000000..9e33d8260fd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C > @@ -0,0 +1,8 @@ > +// PR c++/102607 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi mod } > + > +export module mod; > +export struct B { > + virtual ~B() = default; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C > new file mode 100644 > index 00000000000..03c78b71b5d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C > @@ -0,0 +1,9 @@ > +// PR c++/102607 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import mod; > +int main() { > + struct D : B {}; > + (void)D{}; > +} > diff --git a/gcc/tree-core.h b/gcc/tree-core.h > index 8a89462bd7e..1ca4d4c07bd 100644 > --- a/gcc/tree-core.h > +++ b/gcc/tree-core.h > @@ -1814,8 +1814,7 @@ struct GTY(()) tree_decl_common { > In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */ > unsigned decl_flag_0 : 1; > /* In FIELD_DECL, this is DECL_BIT_FIELD > - In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL. > - In TYPE_DECL, this is TYPE_DECL_SUPPRESS_DEBUG. */ > + In VAR_DECL, FUNCTION_DECL, and TYPE_DECL, this is DECL_EXTERNAL. */ > unsigned decl_flag_1 : 1; > /* In FIELD_DECL, this is DECL_NONADDRESSABLE_P > In VAR_DECL, PARM_DECL and RESULT_DECL, this is > @@ -1845,7 +1844,8 @@ struct GTY(()) tree_decl_common { > TYPE_WARN_IF_NOT_ALIGN. */ > unsigned int warn_if_not_align : 6; > > - /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY. */ > + /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY. > + In TYPE_DECL, this is TYPE_DECL_SUPPRESS_DEBUG. */ > unsigned int decl_not_flexarray : 1; > > /* 5 bits unused. */ > diff --git a/gcc/tree.h b/gcc/tree.h > index 972a067a1f7..8bdc471ad4e 100644 > --- a/gcc/tree.h > +++ b/gcc/tree.h > @@ -2848,9 +2848,9 @@ extern tree vector_element_bits_tree (const_tree); > #define DECL_LANG_SPECIFIC(NODE) \ > (DECL_COMMON_CHECK (NODE)->decl_common.lang_specific) > > -/* In a VAR_DECL or FUNCTION_DECL, nonzero means external reference: > - do not allocate storage, and refer to a definition elsewhere. Note that > - this does not necessarily imply the entity represented by NODE > +/* In a VAR_DECL, FUNCTION_DECL, or TYPE_DECL, nonzero means external > + reference: do not allocate storage, and refer to a definition elsewhere. > + Note that this does not necessarily imply the entity represented by NODE > has no program source-level definition in this translation unit. For > example, for a FUNCTION_DECL, DECL_SAVED_TREE may be non-NULL and > DECL_EXTERNAL may be true simultaneously; that can be the case for > @@ -3553,7 +3553,7 @@ extern vec<tree, va_gc> **decl_debug_args_insert (tree); > into stabs. Instead it will generate cross reference ('x') of names. > This uses the same flag as DECL_EXTERNAL. */ I guess this last sentence should be removed since it's no longer true, LGTM otherwise, thanks! > #define TYPE_DECL_SUPPRESS_DEBUG(NODE) \ > - (TYPE_DECL_CHECK (NODE)->decl_common.decl_flag_1) > + (TYPE_DECL_CHECK (NODE)->decl_common.decl_not_flexarray) > > /* Getter of the imported declaration associated to the > IMPORTED_DECL node. */ > -- > 2.43.0 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] 2024-01-08 15:27 ` Patrick Palka @ 2024-01-15 23:10 ` Jason Merrill 2024-01-16 12:14 ` Nathaniel Shead 2024-01-17 6:33 ` [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820] Nathaniel Shead 0 siblings, 2 replies; 11+ messages in thread From: Jason Merrill @ 2024-01-15 23:10 UTC (permalink / raw) To: Patrick Palka, Nathaniel Shead; +Cc: gcc-patches, Nathan Sidwell On 1/8/24 10:27, Patrick Palka wrote: > On Mon, 8 Jan 2024, Nathaniel Shead wrote: >> On Thu, Jan 04, 2024 at 03:39:15PM -0500, Patrick Palka wrote: >>> On Sun, 3 Dec 2023, Nathaniel Shead wrote: >>>> >>>> The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same >>>> underlying bit. This is causing confusion when attempting to determine >>>> the interface for a streamed-in class type, since the modules code >>>> currently assumes that all DECL_EXTERNAL types are extern templates. >>>> However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence >>>> DECL_EXTERNAL) is marked on various other kinds of declarations, such as >>>> vtables, which causes them to never be emitted. But a vtable isn't a TYPE_DECL? I suspect what you mean is that maybe_suppress_debug_info is setting TYPE_DECL_SUPPRESS_DEBUG to try to avoid duplication of debug info for classes with vtables, and then the modules code is wrongly assuming that you can check DECL_EXTERNAL for TYPE_DECL, and that it's set only if CLASSTYPE_INTERFACE_ONLY is also set, which is wrong in this case, so we avoid emitting the vtable or anything else for that class. It seems unnecessary to start setting DECL_EXTERNAL on the TYPE_DECL to mean the exact same thing as CLASSTYPE_INTERFACE_ONLY. Rather, the modules code should stop trying to check DECL_EXTERNAL on a TYPE_DECL. Under what circumstances does it make sense for CLASSTYPE_INTERFACE_ONLY to be set in the context of modules, anyway? We probably want to propagate it for things in the global module so that various libstdc++ explicit instantiations work the same with import std. For an class imported from a named module, this ties into the earlier discussion about vtables and inlines that hasn't resolved yet in the ABI committee. But it's certainly significantly interface-like. And I would expect maybe_suppress_debug_info to suppress the debug info for such a class on the assumption that the module unit has the needed debug info. Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] 2024-01-15 23:10 ` Jason Merrill @ 2024-01-16 12:14 ` Nathaniel Shead 2024-01-17 6:33 ` [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820] Nathaniel Shead 1 sibling, 0 replies; 11+ messages in thread From: Nathaniel Shead @ 2024-01-16 12:14 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, Nathan Sidwell On Mon, Jan 15, 2024 at 06:10:55PM -0500, Jason Merrill wrote: > On 1/8/24 10:27, Patrick Palka wrote: > > On Mon, 8 Jan 2024, Nathaniel Shead wrote: > > > On Thu, Jan 04, 2024 at 03:39:15PM -0500, Patrick Palka wrote: > > > > On Sun, 3 Dec 2023, Nathaniel Shead wrote: > > > > > > > > > > The TYPE_DECL_SUPPRESS_DEBUG and DECL_EXTERNAL flags use the same > > > > > underlying bit. This is causing confusion when attempting to determine > > > > > the interface for a streamed-in class type, since the modules code > > > > > currently assumes that all DECL_EXTERNAL types are extern templates. > > > > > However, when -g is specified then TYPE_DECL_SUPPRESS_DEBUG (and hence > > > > > DECL_EXTERNAL) is marked on various other kinds of declarations, such as > > > > > vtables, which causes them to never be emitted. > > But a vtable isn't a TYPE_DECL? > > I suspect what you mean is that maybe_suppress_debug_info is setting > TYPE_DECL_SUPPRESS_DEBUG to try to avoid duplication of debug info for > classes with vtables, and then the modules code is wrongly assuming that you > can check DECL_EXTERNAL for TYPE_DECL, and that it's set only if > CLASSTYPE_INTERFACE_ONLY is also set, which is wrong in this case, so we > avoid emitting the vtable or anything else for that class. > Ah, right; I definitely misread what was going on. > It seems unnecessary to start setting DECL_EXTERNAL on the TYPE_DECL to mean > the exact same thing as CLASSTYPE_INTERFACE_ONLY. Rather, the modules code > should stop trying to check DECL_EXTERNAL on a TYPE_DECL. > > Under what circumstances does it make sense for CLASSTYPE_INTERFACE_ONLY to > be set in the context of modules, anyway? We probably want to propagate it > for things in the global module so that various libstdc++ explicit > instantiations work the same with import std. > > For an class imported from a named module, this ties into the earlier > discussion about vtables and inlines that hasn't resolved yet in the ABI > committee. But it's certainly significantly interface-like. And I would > expect maybe_suppress_debug_info to suppress the debug info for such a class > on the assumption that the module unit has the needed debug info. > > Jason > I didn't consider messing with 'CLASSTYPE_INTERFACE_ONLY' because the modules code currently always sets 'lang->interface_unknown = true' on read with the comment "Redetermine interface". But I see now as well this comment: // Interfaceness is recalculated upon reading. May have to revisit? // How do dllexport and dllimport interact across a module? // lang->interface_only // lang->interface_unknown So it seems reasonable to reconsider this now. I hadn't considered declarations in the GMF but I think you're right that we'll want to propagate it; I guess in general things in the GMF should try to behave as close to how they would have if they were #included directly in the importing TU. I suspect this fix will be more involved but I'll try to wrap my head around it and see what I can come up with. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820] 2024-01-15 23:10 ` Jason Merrill 2024-01-16 12:14 ` Nathaniel Shead @ 2024-01-17 6:33 ` Nathaniel Shead 2024-01-17 15:51 ` Jason Merrill 1 sibling, 1 reply; 11+ messages in thread From: Nathaniel Shead @ 2024-01-17 6:33 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, Nathan Sidwell On Mon, Jan 15, 2024 at 06:10:55PM -0500, Jason Merrill wrote: > Under what circumstances does it make sense for CLASSTYPE_INTERFACE_ONLY to > be set in the context of modules, anyway? We probably want to propagate it > for things in the global module so that various libstdc++ explicit > instantiations work the same with import std. > > For an class imported from a named module, this ties into the earlier > discussion about vtables and inlines that hasn't resolved yet in the ABI > committee. But it's certainly significantly interface-like. And I would > expect maybe_suppress_debug_info to suppress the debug info for such a class > on the assumption that the module unit has the needed debug info. > > Jason > Here's another approach for this patch. This still only fixes the specific issues in the PR, I think vtable handling etc. should wait till stage 1 because it involves a lot of messing around in decl2.cc. As mentioned in the commit message, after thinking more about it I don't think we (in general) want to propagate CLASSTYPE_INTERFACE_ONLY, even for declarations in the GMF. This makes sense to me because typically it can only be accurately determined at the end of the TU, which we haven't yet arrived at after importing. For instance, for a polymorphic class in the GMF without a key method, that we import from a module and then proceed to define the key method later on in this TU. Bootstrapped and partially regtested on x86_64-pc-linux-gnu (so far only modules.exp): OK for trunk if full regtesting passes? -- >8 -- Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 36 +++++++++++++----------- gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 ++++++ gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++ gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 ++++++ gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++ gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 ++++++ 6 files changed, 63 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 350ad15dc62..efc1d532a6e 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -5806,10 +5806,8 @@ trees_out::lang_type_bools (tree t) WB ((lang->gets_delete >> 0) & 1); WB ((lang->gets_delete >> 1) & 1); - // Interfaceness is recalculated upon reading. May have to revisit? - // How do dllexport and dllimport interact across a module? - // lang->interface_only - // lang->interface_unknown + WB (lang->interface_only); + WB (lang->interface_unknown); WB (lang->contains_empty_class_p); WB (lang->anon_aggr); WB (lang->non_zero_init); @@ -5877,9 +5875,8 @@ trees_in::lang_type_bools (tree t) v = b () << 0; v |= b () << 1; lang->gets_delete = v; - // lang->interface_only - // lang->interface_unknown - lang->interface_unknown = true; // Redetermine interface + RB (lang->interface_only); + RB (lang->interface_unknown); RB (lang->contains_empty_class_p); RB (lang->anon_aggr); RB (lang->non_zero_init); @@ -8246,6 +8243,22 @@ trees_in::decl_value () /* Set the TEMPLATE_DECL's type. */ TREE_TYPE (decl) = TREE_TYPE (inner); + /* Redetermine whether we need to import or export this declaration + for this TU. But for extern templates we know we must import: + they'll be defined in a different TU. + FIXME How do dllexport and dllimport interact across a module? + May have to revisit? */ + if (type + && CLASS_TYPE_P (type) + && TYPE_LANG_SPECIFIC (type) + && !(CLASSTYPE_EXPLICIT_INSTANTIATION (type) + && CLASSTYPE_INTERFACE_KNOWN (type) + && CLASSTYPE_INTERFACE_ONLY (type))) + { + CLASSTYPE_INTERFACE_ONLY (type) = false; + CLASSTYPE_INTERFACE_UNKNOWN (type) = true; + } + /* Add to specialization tables now that constraints etc are added. */ if (mk == MK_partial) @@ -12070,15 +12083,6 @@ trees_in::read_class_def (tree defn, tree maybe_template) bool installing = maybe_dup && !TYPE_SIZE (type); if (installing) { - if (DECL_EXTERNAL (defn) && TYPE_LANG_SPECIFIC (type)) - { - /* We don't deal with not-really-extern, because, for a - module you want the import to be the interface, and for a - header-unit, you're doing it wrong. */ - CLASSTYPE_INTERFACE_UNKNOWN (type) = false; - CLASSTYPE_INTERFACE_ONLY (type) = true; - } - if (maybe_dup != defn) { // FIXME: This is needed on other defns too, almost diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C new file mode 100644 index 00000000000..eed0905542b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi io } + +export module io; + +export struct error { + virtual const char* what() const noexcept; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C new file mode 100644 index 00000000000..fc9afbc02e0 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C @@ -0,0 +1,8 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } + +module io; + +const char* error::what() const noexcept { + return "bla"; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C new file mode 100644 index 00000000000..37117f69dcd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import io; + +int main() { + error{}; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C new file mode 100644 index 00000000000..9e33d8260fd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C @@ -0,0 +1,8 @@ +// PR c++/102607 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi mod } + +export module mod; +export struct B { + virtual ~B() = default; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C new file mode 100644 index 00000000000..03c78b71b5d --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C @@ -0,0 +1,9 @@ +// PR c++/102607 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import mod; +int main() { + struct D : B {}; + (void)D{}; +} -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820] 2024-01-17 6:33 ` [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820] Nathaniel Shead @ 2024-01-17 15:51 ` Jason Merrill 2024-01-22 11:04 ` Nathaniel Shead 0 siblings, 1 reply; 11+ messages in thread From: Jason Merrill @ 2024-01-17 15:51 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Patrick Palka, gcc-patches, Nathan Sidwell On 1/17/24 01:33, Nathaniel Shead wrote: > On Mon, Jan 15, 2024 at 06:10:55PM -0500, Jason Merrill wrote: >> Under what circumstances does it make sense for CLASSTYPE_INTERFACE_ONLY to >> be set in the context of modules, anyway? We probably want to propagate it >> for things in the global module so that various libstdc++ explicit >> instantiations work the same with import std. >> >> For an class imported from a named module, this ties into the earlier >> discussion about vtables and inlines that hasn't resolved yet in the ABI >> committee. But it's certainly significantly interface-like. And I would >> expect maybe_suppress_debug_info to suppress the debug info for such a class >> on the assumption that the module unit has the needed debug info. >> >> Jason >> > > Here's another approach for this patch. This still only fixes the > specific issues in the PR, I think vtable handling etc. should wait till > stage 1 because it involves a lot of messing around in decl2.cc. > > As mentioned in the commit message, after thinking more about it I don't > think we (in general) want to propagate CLASSTYPE_INTERFACE_ONLY, even > for declarations in the GMF. This makes sense to me because typically it > can only be accurately determined at the end of the TU, which we haven't > yet arrived at after importing. For instance, for a polymorphic class in > the GMF without a key method, that we import from a module and then > proceed to define the key method later on in this TU. That sounds right for a module implementation unit or the GMF. > Bootstrapped and partially regtested on x86_64-pc-linux-gnu (so far only > modules.exp): OK for trunk if full regtesting passes? Please add a reference to ABI issue 170 (https://github.com/itanium-cxx-abi/cxx-abi/issues/170). OK with that change if Nathan doesn't have any further comments this week. > -- >8 -- > > Currently, extern templates are detected by looking for the > DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: > TYPE_DECLs don't actually set this flag, and it happens to work by > coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same > underlying bit. This however causes issues with other TYPE_DECLs that > also happen to have suppressed debug information. > > Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is > always emitted into the module BMI and can then be used to check for an > extern template correctly. > > Otherwise, for other declarations we want to redetermine this: even for > declarations from the GMF, we may change our mind on whether to import > or export depending on decisions made later in the TU after importing so > we shouldn't decide this now, or necessarily reuse what the module we'd > imported had decided. > > PR c++/112820 > PR c++/102607 > > gcc/cp/ChangeLog: > > * module.cc (trees_out::lang_type_bools): Write interface_only > and interface_unknown. > (trees_in::lang_type_bools): Read the above flags. > (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for > extern templates. > (trees_in::read_class_def): Remove buggy extern template > handling. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/debug-2_a.C: New test. > * g++.dg/modules/debug-2_b.C: New test. > * g++.dg/modules/debug-2_c.C: New test. > * g++.dg/modules/debug-3_a.C: New test. > * g++.dg/modules/debug-3_b.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 36 +++++++++++++----------- > gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 ++++++ > gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 ++++++ > gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 ++++++ > gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 ++++++ > gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 ++++++ > 6 files changed, 63 insertions(+), 16 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 350ad15dc62..efc1d532a6e 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -5806,10 +5806,8 @@ trees_out::lang_type_bools (tree t) > > WB ((lang->gets_delete >> 0) & 1); > WB ((lang->gets_delete >> 1) & 1); > - // Interfaceness is recalculated upon reading. May have to revisit? > - // How do dllexport and dllimport interact across a module? > - // lang->interface_only > - // lang->interface_unknown > + WB (lang->interface_only); > + WB (lang->interface_unknown); > WB (lang->contains_empty_class_p); > WB (lang->anon_aggr); > WB (lang->non_zero_init); > @@ -5877,9 +5875,8 @@ trees_in::lang_type_bools (tree t) > v = b () << 0; > v |= b () << 1; > lang->gets_delete = v; > - // lang->interface_only > - // lang->interface_unknown > - lang->interface_unknown = true; // Redetermine interface > + RB (lang->interface_only); > + RB (lang->interface_unknown); > RB (lang->contains_empty_class_p); > RB (lang->anon_aggr); > RB (lang->non_zero_init); > @@ -8246,6 +8243,22 @@ trees_in::decl_value () > /* Set the TEMPLATE_DECL's type. */ > TREE_TYPE (decl) = TREE_TYPE (inner); > > + /* Redetermine whether we need to import or export this declaration > + for this TU. But for extern templates we know we must import: > + they'll be defined in a different TU. > + FIXME How do dllexport and dllimport interact across a module? > + May have to revisit? */ > + if (type > + && CLASS_TYPE_P (type) > + && TYPE_LANG_SPECIFIC (type) > + && !(CLASSTYPE_EXPLICIT_INSTANTIATION (type) > + && CLASSTYPE_INTERFACE_KNOWN (type) > + && CLASSTYPE_INTERFACE_ONLY (type))) > + { > + CLASSTYPE_INTERFACE_ONLY (type) = false; > + CLASSTYPE_INTERFACE_UNKNOWN (type) = true; > + } > + > /* Add to specialization tables now that constraints etc are > added. */ > if (mk == MK_partial) > @@ -12070,15 +12083,6 @@ trees_in::read_class_def (tree defn, tree maybe_template) > bool installing = maybe_dup && !TYPE_SIZE (type); > if (installing) > { > - if (DECL_EXTERNAL (defn) && TYPE_LANG_SPECIFIC (type)) > - { > - /* We don't deal with not-really-extern, because, for a > - module you want the import to be the interface, and for a > - header-unit, you're doing it wrong. */ > - CLASSTYPE_INTERFACE_UNKNOWN (type) = false; > - CLASSTYPE_INTERFACE_ONLY (type) = true; > - } > - > if (maybe_dup != defn) > { > // FIXME: This is needed on other defns too, almost > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C > new file mode 100644 > index 00000000000..eed0905542b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi io } > + > +export module io; > + > +export struct error { > + virtual const char* what() const noexcept; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C > new file mode 100644 > index 00000000000..fc9afbc02e0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C > @@ -0,0 +1,8 @@ > +// PR c++/112820 > +// { dg-additional-options "-fmodules-ts -g" } > + > +module io; > + > +const char* error::what() const noexcept { > + return "bla"; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C > new file mode 100644 > index 00000000000..37117f69dcd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C > @@ -0,0 +1,9 @@ > +// PR c++/112820 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import io; > + > +int main() { > + error{}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C > new file mode 100644 > index 00000000000..9e33d8260fd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C > @@ -0,0 +1,8 @@ > +// PR c++/102607 > +// { dg-additional-options "-fmodules-ts -g" } > +// { dg-module-cmi mod } > + > +export module mod; > +export struct B { > + virtual ~B() = default; > +}; > diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C > new file mode 100644 > index 00000000000..03c78b71b5d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C > @@ -0,0 +1,9 @@ > +// PR c++/102607 > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts -g" } > + > +import mod; > +int main() { > + struct D : B {}; > + (void)D{}; > +} ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820] 2024-01-17 15:51 ` Jason Merrill @ 2024-01-22 11:04 ` Nathaniel Shead 0 siblings, 0 replies; 11+ messages in thread From: Nathaniel Shead @ 2024-01-22 11:04 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, Nathan Sidwell On Wed, Jan 17, 2024 at 10:51:16AM -0500, Jason Merrill wrote: > On 1/17/24 01:33, Nathaniel Shead wrote: > > On Mon, Jan 15, 2024 at 06:10:55PM -0500, Jason Merrill wrote: > > > Under what circumstances does it make sense for CLASSTYPE_INTERFACE_ONLY to > > > be set in the context of modules, anyway? We probably want to propagate it > > > for things in the global module so that various libstdc++ explicit > > > instantiations work the same with import std. > > > > > > For an class imported from a named module, this ties into the earlier > > > discussion about vtables and inlines that hasn't resolved yet in the ABI > > > committee. But it's certainly significantly interface-like. And I would > > > expect maybe_suppress_debug_info to suppress the debug info for such a class > > > on the assumption that the module unit has the needed debug info. > > > > > > Jason > > > > > > > Here's another approach for this patch. This still only fixes the > > specific issues in the PR, I think vtable handling etc. should wait till > > stage 1 because it involves a lot of messing around in decl2.cc. > > > > As mentioned in the commit message, after thinking more about it I don't > > think we (in general) want to propagate CLASSTYPE_INTERFACE_ONLY, even > > for declarations in the GMF. This makes sense to me because typically it > > can only be accurately determined at the end of the TU, which we haven't > > yet arrived at after importing. For instance, for a polymorphic class in > > the GMF without a key method, that we import from a module and then > > proceed to define the key method later on in this TU. > > That sounds right for a module implementation unit or the GMF. > > > Bootstrapped and partially regtested on x86_64-pc-linux-gnu (so far only > > modules.exp): OK for trunk if full regtesting passes? > > Please add a reference to ABI issue 170 > (https://github.com/itanium-cxx-abi/cxx-abi/issues/170). OK with that > change if Nathan doesn't have any further comments this week. > Thanks. Here's what I'll push tomorrow unless I hear otherwise. -- >8 -- Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for https://github.com/itanium-cxx-abi/cxx-abi/issues/170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 37 ++++++++++++++---------- gcc/testsuite/g++.dg/modules/debug-2_a.C | 9 ++++++ gcc/testsuite/g++.dg/modules/debug-2_b.C | 8 +++++ gcc/testsuite/g++.dg/modules/debug-2_c.C | 9 ++++++ gcc/testsuite/g++.dg/modules/debug-3_a.C | 8 +++++ gcc/testsuite/g++.dg/modules/debug-3_b.C | 9 ++++++ 6 files changed, 64 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 8db662c0267..70785493561 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -5806,10 +5806,8 @@ trees_out::lang_type_bools (tree t) WB ((lang->gets_delete >> 0) & 1); WB ((lang->gets_delete >> 1) & 1); - // Interfaceness is recalculated upon reading. May have to revisit? - // How do dllexport and dllimport interact across a module? - // lang->interface_only - // lang->interface_unknown + WB (lang->interface_only); + WB (lang->interface_unknown); WB (lang->contains_empty_class_p); WB (lang->anon_aggr); WB (lang->non_zero_init); @@ -5877,9 +5875,8 @@ trees_in::lang_type_bools (tree t) v = b () << 0; v |= b () << 1; lang->gets_delete = v; - // lang->interface_only - // lang->interface_unknown - lang->interface_unknown = true; // Redetermine interface + RB (lang->interface_only); + RB (lang->interface_unknown); RB (lang->contains_empty_class_p); RB (lang->anon_aggr); RB (lang->non_zero_init); @@ -8246,6 +8243,23 @@ trees_in::decl_value () /* Set the TEMPLATE_DECL's type. */ TREE_TYPE (decl) = TREE_TYPE (inner); + /* Redetermine whether we need to import or export this declaration + for this TU. But for extern templates we know we must import: + they'll be defined in a different TU. + FIXME: How do dllexport and dllimport interact across a module? + See also https://github.com/itanium-cxx-abi/cxx-abi/issues/170. + May have to revisit? */ + if (type + && CLASS_TYPE_P (type) + && TYPE_LANG_SPECIFIC (type) + && !(CLASSTYPE_EXPLICIT_INSTANTIATION (type) + && CLASSTYPE_INTERFACE_KNOWN (type) + && CLASSTYPE_INTERFACE_ONLY (type))) + { + CLASSTYPE_INTERFACE_ONLY (type) = false; + CLASSTYPE_INTERFACE_UNKNOWN (type) = true; + } + /* Add to specialization tables now that constraints etc are added. */ if (mk == MK_partial) @@ -12068,15 +12082,6 @@ trees_in::read_class_def (tree defn, tree maybe_template) bool installing = maybe_dup && !TYPE_SIZE (type); if (installing) { - if (DECL_EXTERNAL (defn) && TYPE_LANG_SPECIFIC (type)) - { - /* We don't deal with not-really-extern, because, for a - module you want the import to be the interface, and for a - header-unit, you're doing it wrong. */ - CLASSTYPE_INTERFACE_UNKNOWN (type) = false; - CLASSTYPE_INTERFACE_ONLY (type) = true; - } - if (maybe_dup != defn) { // FIXME: This is needed on other defns too, almost diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C new file mode 100644 index 00000000000..eed0905542b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi io } + +export module io; + +export struct error { + virtual const char* what() const noexcept; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C new file mode 100644 index 00000000000..fc9afbc02e0 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C @@ -0,0 +1,8 @@ +// PR c++/112820 +// { dg-additional-options "-fmodules-ts -g" } + +module io; + +const char* error::what() const noexcept { + return "bla"; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C new file mode 100644 index 00000000000..37117f69dcd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C @@ -0,0 +1,9 @@ +// PR c++/112820 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import io; + +int main() { + error{}; +} diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C new file mode 100644 index 00000000000..9e33d8260fd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C @@ -0,0 +1,8 @@ +// PR c++/102607 +// { dg-additional-options "-fmodules-ts -g" } +// { dg-module-cmi mod } + +export module mod; +export struct B { + virtual ~B() = default; +}; diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C new file mode 100644 index 00000000000..03c78b71b5d --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C @@ -0,0 +1,9 @@ +// PR c++/102607 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts -g" } + +import mod; +int main() { + struct D : B {}; + (void)D{}; +} -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-01-22 11:04 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-03 12:46 [PATCH] c++/modules: Prevent treating suppressed debug info as extern template [PR112820] Nathaniel Shead 2024-01-02 22:52 ` Nathaniel Shead 2024-01-04 20:39 ` Patrick Palka 2024-01-08 9:57 ` [PATCH v2] c++/modules: Differentiate extern templates and TYPE_DECL_SUPPRESS_DEBUG [PR112820] Nathaniel Shead 2024-01-08 11:50 ` Richard Biener 2024-01-08 15:27 ` Patrick Palka 2024-01-15 23:10 ` Jason Merrill 2024-01-16 12:14 ` Nathaniel Shead 2024-01-17 6:33 ` [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820] Nathaniel Shead 2024-01-17 15:51 ` Jason Merrill 2024-01-22 11:04 ` 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).