| Message ID | 65a774d4.a70a0220.f1919.243b@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] c++/modules: Fix handling of extern templates in modules [PR112820] | expand |
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{}; > +}
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{}; +}
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{}; +}