* [PATCH] c++: Check module attachment instead of purview when necessary [PR112631] @ 2023-11-20 9:47 Nathaniel Shead 2023-11-23 20:03 ` Nathan Sidwell 0 siblings, 1 reply; 9+ messages in thread From: Nathaniel Shead @ 2023-11-20 9:47 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write access. -- >8 -- Block-scope declarations of functions or extern values are not allowed when attached to a named module. Similarly, class member functions are not inline if attached to a named module. However, in both these cases we currently only check if the declaration is within the module purview; it is possible for such a declaration to occur within the module purview but not be attached to a named module (e.g. in an 'extern "C++"' block). This patch makes the required adjustments. PR c++/112631 gcc/cp/ChangeLog: * cp-tree.h (named_module_attach_p): New function. * decl.cc (start_decl): Use named_module_attach_p instead of named_module_purview_p. (grokmethod): Likewise. gcc/testsuite/ChangeLog: * g++.dg/modules/pr112631.C: New test. --- gcc/cp/cp-tree.h | 2 ++ gcc/cp/decl.cc | 10 +++++----- gcc/testsuite/g++.dg/modules/pr112631.C | 8 ++++++++ 3 files changed, 15 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/pr112631.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 7b0b7c6a17e..9a3981cef58 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7315,6 +7315,8 @@ inline bool module_attach_p () inline bool named_module_purview_p () { return named_module_p () && module_purview_p (); } +inline bool named_module_attach_p () +{ return named_module_p () && module_attach_p (); } /* We're currently exporting declarations. */ inline bool module_exporting_p () diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index e6f75d771e0..395f108aec7 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -5917,10 +5917,10 @@ start_decl (const cp_declarator *declarator, { /* A function-scope decl of some namespace-scope decl. */ DECL_LOCAL_DECL_P (decl) = true; - if (named_module_purview_p ()) + if (named_module_attach_p ()) error_at (declarator->id_loc, - "block-scope extern declaration %q#D not permitted" - " in module purview", decl); + "block-scope extern declaration %q#D must not be" + " attached to a named module", decl); } /* Enter this declaration into the symbol table. Don't push the plain @@ -18513,10 +18513,10 @@ grokmethod (cp_decl_specifier_seq *declspecs, check_template_shadow (fndecl); /* p1779 ABI-Isolation makes inline not a default for in-class - definitions in named module purview. If the user explicitly + definitions attached to a named module. If the user explicitly made it inline, grokdeclarator will already have done the right things. */ - if ((!named_module_purview_p () + if ((!named_module_attach_p () || flag_module_implicit_inline /* Lambda's operator function remains inline. */ || LAMBDA_TYPE_P (DECL_CONTEXT (fndecl))) diff --git a/gcc/testsuite/g++.dg/modules/pr112631.C b/gcc/testsuite/g++.dg/modules/pr112631.C new file mode 100644 index 00000000000..b5e81a1041b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr112631.C @@ -0,0 +1,8 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi bla } + +export module bla; + +extern "C++" inline void fun() { + void oops(); // { dg-bogus "block-scope extern declaration" } +} -- 2.42.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Check module attachment instead of purview when necessary [PR112631] 2023-11-20 9:47 [PATCH] c++: Check module attachment instead of purview when necessary [PR112631] Nathaniel Shead @ 2023-11-23 20:03 ` Nathan Sidwell 2023-11-27 4:59 ` Nathaniel Shead 0 siblings, 1 reply; 9+ messages in thread From: Nathan Sidwell @ 2023-11-23 20:03 UTC (permalink / raw) To: Nathaniel Shead, gcc-patches; +Cc: Jason Merrill On 11/20/23 04:47, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write > access. > > -- >8 -- > > Block-scope declarations of functions or extern values are not allowed > when attached to a named module. Similarly, class member functions are > not inline if attached to a named module. However, in both these cases > we currently only check if the declaration is within the module purview; > it is possible for such a declaration to occur within the module purview > but not be attached to a named module (e.g. in an 'extern "C++"' block). > This patch makes the required adjustments. Ah I'd been puzzling over the default inlinedness of member-fns of block-scope structs. Could you augment the testcase to make sure that's right too? Something like: // dg-module-do link export module Mod; export auto Get () { struct X { void Fn () {} }; return X(); } /// import Mod void Frob () { Get().Fn(); } > > PR c++/112631 > > gcc/cp/ChangeLog: > > * cp-tree.h (named_module_attach_p): New function. > * decl.cc (start_decl): Use named_module_attach_p instead of > named_module_purview_p. > (grokmethod): Likewise. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/pr112631.C: New test. > --- > gcc/cp/cp-tree.h | 2 ++ > gcc/cp/decl.cc | 10 +++++----- > gcc/testsuite/g++.dg/modules/pr112631.C | 8 ++++++++ > 3 files changed, 15 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/pr112631.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 7b0b7c6a17e..9a3981cef58 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7315,6 +7315,8 @@ inline bool module_attach_p () > > inline bool named_module_purview_p () > { return named_module_p () && module_purview_p (); } > +inline bool named_module_attach_p () > +{ return named_module_p () && module_attach_p (); } > > /* We're currently exporting declarations. */ > inline bool module_exporting_p () > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index e6f75d771e0..395f108aec7 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -5917,10 +5917,10 @@ start_decl (const cp_declarator *declarator, > { > /* A function-scope decl of some namespace-scope decl. */ > DECL_LOCAL_DECL_P (decl) = true; > - if (named_module_purview_p ()) > + if (named_module_attach_p ()) > error_at (declarator->id_loc, > - "block-scope extern declaration %q#D not permitted" > - " in module purview", decl); > + "block-scope extern declaration %q#D must not be" > + " attached to a named module", decl); > } > > /* Enter this declaration into the symbol table. Don't push the plain > @@ -18513,10 +18513,10 @@ grokmethod (cp_decl_specifier_seq *declspecs, > check_template_shadow (fndecl); > > /* p1779 ABI-Isolation makes inline not a default for in-class > - definitions in named module purview. If the user explicitly > + definitions attached to a named module. If the user explicitly > made it inline, grokdeclarator will already have done the right > things. */ > - if ((!named_module_purview_p () > + if ((!named_module_attach_p () > || flag_module_implicit_inline > /* Lambda's operator function remains inline. */ > || LAMBDA_TYPE_P (DECL_CONTEXT (fndecl))) > diff --git a/gcc/testsuite/g++.dg/modules/pr112631.C b/gcc/testsuite/g++.dg/modules/pr112631.C > new file mode 100644 > index 00000000000..b5e81a1041b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/pr112631.C > @@ -0,0 +1,8 @@ > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi bla } > + > +export module bla; > + > +extern "C++" inline void fun() { > + void oops(); // { dg-bogus "block-scope extern declaration" } > +} -- Nathan Sidwell ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Check module attachment instead of purview when necessary [PR112631] 2023-11-23 20:03 ` Nathan Sidwell @ 2023-11-27 4:59 ` Nathaniel Shead 2024-03-08 2:55 ` [PATCH v2] c++: Check module attachment instead of just " Nathaniel Shead 0 siblings, 1 reply; 9+ messages in thread From: Nathaniel Shead @ 2023-11-27 4:59 UTC (permalink / raw) To: Nathan Sidwell; +Cc: gcc-patches, Jason Merrill On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: > On 11/20/23 04:47, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write > > access. > > > > -- >8 -- > > > > Block-scope declarations of functions or extern values are not allowed > > when attached to a named module. Similarly, class member functions are > > not inline if attached to a named module. However, in both these cases > > we currently only check if the declaration is within the module purview; > > it is possible for such a declaration to occur within the module purview > > but not be attached to a named module (e.g. in an 'extern "C++"' block). > > This patch makes the required adjustments. > > > Ah I'd been puzzling over the default inlinedness of member-fns of > block-scope structs. Could you augment the testcase to make sure that's > right too? > > Something like: > > // dg-module-do link > export module Mod; > > export auto Get () { > struct X { void Fn () {} }; > return X(); > } > > > /// > import Mod > void Frob () { Get().Fn(); } > I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be marked 'inline' for this to link (whether or not 'Get' itself is inline). I've tried tracing the code to work out what's going on but I've been struggling to work out how all the different flags (e.g. TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) interact, which flags we want to be set where, and where the decision of what function definitions to emit is actually made. I did find that doing 'mark_used(decl)' on all member functions in block-scope structs seems to work however, but I wonder if that's maybe too aggressive or if there's something else we should be doing? ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631] 2023-11-27 4:59 ` Nathaniel Shead @ 2024-03-08 2:55 ` Nathaniel Shead 2024-03-08 15:19 ` Jason Merrill 0 siblings, 1 reply; 9+ messages in thread From: Nathaniel Shead @ 2024-03-08 2:55 UTC (permalink / raw) To: gcc-patches; +Cc: Nathan Sidwell, Jason Merrill On Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote: > On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: > > On 11/20/23 04:47, Nathaniel Shead wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write > > > access. > > > > > > -- >8 -- > > > > > > Block-scope declarations of functions or extern values are not allowed > > > when attached to a named module. Similarly, class member functions are > > > not inline if attached to a named module. However, in both these cases > > > we currently only check if the declaration is within the module purview; > > > it is possible for such a declaration to occur within the module purview > > > but not be attached to a named module (e.g. in an 'extern "C++"' block). > > > This patch makes the required adjustments. > > > > > > Ah I'd been puzzling over the default inlinedness of member-fns of > > block-scope structs. Could you augment the testcase to make sure that's > > right too? > > > > Something like: > > > > // dg-module-do link > > export module Mod; > > > > export auto Get () { > > struct X { void Fn () {} }; > > return X(); > > } > > > > > > /// > > import Mod > > void Frob () { Get().Fn(); } > > > > I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be > marked 'inline' for this to link (whether or not 'Get' itself is > inline). I've tried tracing the code to work out what's going on but > I've been struggling to work out how all the different flags (e.g. > TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) > interact, which flags we want to be set where, and where the decision of > what function definitions to emit is actually made. > > I did find that doing 'mark_used(decl)' on all member functions in > block-scope structs seems to work however, but I wonder if that's maybe > too aggressive or if there's something else we should be doing? I got around to looking at this again, here's an updated version of this patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? (I'm not sure if 'start_preparsed_function' is the right place to be putting this kind of logic or if it should instead be going in 'grokfndecl', e.g. decl.cc:10761 where the rules for making local functions have no linkage are initially determined, but I found this easier to implement: happy to move around though if preferred.) -- >8 -- Block-scope declarations of functions or extern values are not allowed when attached to a named module. Similarly, class member functions are not inline if attached to a named module. However, in both these cases we currently only check if the declaration is within the module purview; it is possible for such a declaration to occur within the module purview but not be attached to a named module (e.g. in an 'extern "C++"' block). This patch makes the required adjustments. While implementing this we discovered that block-scope local functions are not correctly emitted, causing link failures; this patch also corrects some assumptions here and ensures that they are emitted when needed. PR c++/112631 gcc/cp/ChangeLog: * cp-tree.h (named_module_attach_p): New function. * decl.cc (start_decl): Check for attachment not purview. (grokmethod): Likewise. (start_preparsed_function): Ensure block-scope functions are emitted in module interfaces. * decl2.cc (determine_visibility): Likewise. gcc/testsuite/ChangeLog: * g++.dg/modules/block-decl-1_a.C: New test. * g++.dg/modules/block-decl-1_b.C: New test. * g++.dg/modules/block-decl-2_a.C: New test. * g++.dg/modules/block-decl-2_b.C: New test. * g++.dg/modules/block-decl-2_c.C: New test. * g++.dg/modules/block-decl-3.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/cp-tree.h | 2 + gcc/cp/decl.cc | 22 ++- gcc/cp/decl2.cc | 23 +-- gcc/testsuite/g++.dg/modules/block-decl-1_a.C | 9 ++ gcc/testsuite/g++.dg/modules/block-decl-1_b.C | 10 ++ gcc/testsuite/g++.dg/modules/block-decl-2_a.C | 143 ++++++++++++++++++ gcc/testsuite/g++.dg/modules/block-decl-2_b.C | 8 + gcc/testsuite/g++.dg/modules/block-decl-2_c.C | 25 +++ gcc/testsuite/g++.dg/modules/block-decl-3.C | 21 +++ 9 files changed, 249 insertions(+), 14 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-1_a.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-1_b.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_a.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_b.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_c.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 14895bc6585..05913861e06 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7381,6 +7381,8 @@ inline bool module_attach_p () inline bool named_module_purview_p () { return named_module_p () && module_purview_p (); } +inline bool named_module_attach_p () +{ return named_module_p () && module_attach_p (); } /* We're currently exporting declarations. */ inline bool module_exporting_p () diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index dbc3df24e77..92475ecc28f 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -6092,10 +6092,10 @@ start_decl (const cp_declarator *declarator, { /* A function-scope decl of some namespace-scope decl. */ DECL_LOCAL_DECL_P (decl) = true; - if (named_module_purview_p ()) + if (named_module_attach_p ()) error_at (declarator->id_loc, - "block-scope extern declaration %q#D not permitted" - " in module purview", decl); + "block-scope extern declaration %q#D must not be" + " attached to a named module", decl); } /* Enter this declaration into the symbol table. Don't push the plain @@ -18054,6 +18054,18 @@ start_preparsed_function (tree decl1, tree attrs, int flags) /* This is a function in a local class in an extern inline or template function. */ comdat_linkage (decl1); + + if (ctx != NULL_TREE && TREE_PUBLIC (ctx) && module_has_cmi_p ()) + { + /* Ensure that functions in local classes within named modules + have their definitions exported, in case they are (directly + or indirectly) used by an importer. */ + TREE_PUBLIC (decl1) = true; + if (DECL_DECLARED_INLINE_P (decl1)) + comdat_linkage (decl1); + else + mark_needed (decl1); + } } /* If this function belongs to an interface, it is public. If it belongs to someone else's interface, it is also external. @@ -18907,10 +18919,10 @@ grokmethod (cp_decl_specifier_seq *declspecs, check_template_shadow (fndecl); /* p1779 ABI-Isolation makes inline not a default for in-class - definitions in named module purview. If the user explicitly + definitions attached to a named module. If the user explicitly made it inline, grokdeclarator will already have done the right things. */ - if ((!named_module_purview_p () + if ((!named_module_attach_p () || flag_module_implicit_inline /* Lambda's operator function remains inline. */ || LAMBDA_TYPE_P (DECL_CONTEXT (fndecl))) diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index 6c9fd415d40..94eaf98c609 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -3050,15 +3050,20 @@ determine_visibility (tree decl) constrain_visibility (decl, tvis, false); } else if (no_linkage_check (TREE_TYPE (decl), /*relaxed_p=*/true)) - /* DR 757: A type without linkage shall not be used as the type of a - variable or function with linkage, unless - o the variable or function has extern "C" linkage (7.5 [dcl.link]), or - o the variable or function is not used (3.2 [basic.def.odr]) or is - defined in the same translation unit. - - Since non-extern "C" decls need to be defined in the same - translation unit, we can make the type internal. */ - constrain_visibility (decl, VISIBILITY_ANON, false); + { + /* DR 757: A type without linkage shall not be used as the type of a + variable or function with linkage, unless + o the variable or function has extern "C" linkage (7.5 [dcl.link]), or + o the variable or function is not used (3.2 [basic.def.odr]) or is + defined in the same translation unit. + + Since non-extern "C" decls need to be defined in the same + translation unit, we can make the type internal, unless this + type is part of a module CMI, in which case importers may need + to access it. */ + if (!module_has_cmi_p ()) + constrain_visibility (decl, VISIBILITY_ANON, false); + } /* If visibility changed and DECL already has DECL_RTL, ensure symbol flags are updated. */ diff --git a/gcc/testsuite/g++.dg/modules/block-decl-1_a.C b/gcc/testsuite/g++.dg/modules/block-decl-1_a.C new file mode 100644 index 00000000000..e7ffc629192 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-1_a.C @@ -0,0 +1,9 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi bla } + +export module bla; + +export extern "C++" inline void fun() { + void oops(); // { dg-bogus "block-scope extern declaration" } + oops(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-1_b.C b/gcc/testsuite/g++.dg/modules/block-decl-1_b.C new file mode 100644 index 00000000000..c0d724f25ac --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-1_b.C @@ -0,0 +1,10 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import bla; + +void oops() {} + +int main() { + fun(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_a.C b/gcc/testsuite/g++.dg/modules/block-decl-2_a.C new file mode 100644 index 00000000000..00a4f229ae8 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-2_a.C @@ -0,0 +1,143 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi mod } + +export module mod; + +namespace { + void internal() {} +} + +// singly-nested (i=inline, n=non-inline) + +export auto n_n() { + internal(); + struct X { void f() { internal(); } }; + return X{}; +} + +export auto n_i() { + internal(); + struct X { inline void f() {} }; + return X{}; +} + +export inline auto i_n() { + // `f` is not inline here, so this is OK + struct X { void f() { internal(); } }; + return X{}; +} + +export inline auto i_i() { + struct X { inline void f() {} }; + return X{}; +} + + +// doubly nested + +export auto n_n_n() { + struct X { + auto f() { + struct Y { + void g() { internal(); } + }; + return Y{}; + } + }; + return X{}; +} + +export auto n_i_n() { + struct X { + inline auto f() { + struct Y { + void g() { internal(); } + }; + return Y{}; + } + }; + return X{}; +} + +export inline auto i_n_i() { + struct X { + auto f() { + struct Y { + inline void g() {} + }; + return Y {}; + } + }; + return X{}; +} + +export inline auto i_i_i() { + struct X { + inline auto f() { + struct Y { + inline void g() {} + }; + return Y{}; + } + }; + return X{}; +} + + +// multiple types + +export auto multi_n_n() { + struct X { + void f() { internal(); } + }; + struct Y { + X x; + }; + return Y {}; +} + +export auto multi_n_i() { + struct X { + inline void f() {} + }; + struct Y { + X x; + }; + return Y {}; +} + +export inline auto multi_i_i() { + struct X { + inline void f() {} + }; + struct Y { + X x; + }; + return Y {}; +}; + + +// extern "C++" + +export extern "C++" auto extern_n_i() { + struct X { + void f() {} // implicitly inline + }; + return X{}; +}; + +export extern "C++" inline auto extern_i_i() { + struct X { + void f() {} + }; + return X{}; +}; + + +// can access from implementation unit + +auto only_used_in_impl() { + struct X { void f() {} }; + return X{}; +} +export void test_from_impl_unit(); diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_b.C b/gcc/testsuite/g++.dg/modules/block-decl-2_b.C new file mode 100644 index 00000000000..bc9b2a213f0 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-2_b.C @@ -0,0 +1,8 @@ +// { dg-additional-options "-fmodules-ts" } + +module mod; + +// Test that we can access (and link) to declarations from the interface +void test_from_impl_unit() { + only_used_in_impl().f(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_c.C b/gcc/testsuite/g++.dg/modules/block-decl-2_c.C new file mode 100644 index 00000000000..ef275d10f0e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-2_c.C @@ -0,0 +1,25 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import mod; + +int main() { + n_n().f(); + n_i().f(); + i_n().f(); + i_i().f(); + + n_n_n().f().g(); + n_i_n().f().g(); + i_n_i().f().g(); + i_i_i().f().g(); + + multi_n_n().x.f(); + multi_n_i().x.f(); + multi_i_i().x.f(); + + extern_n_i().f(); + extern_i_i().f(); + + test_from_impl_unit(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3.C b/gcc/testsuite/g++.dg/modules/block-decl-3.C new file mode 100644 index 00000000000..8bbebd06bab --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-3.C @@ -0,0 +1,21 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi !mod } + +export module mod; + +namespace { + void internal(); +} + +export extern "C++" auto foo() { + struct X { + // `foo` is not attached to a named module, and as such + // `X::f` should be implicitly `inline` here + void f() { // { dg-error "references internal linkage entity" } + internal(); + } + }; + return X{}; +} + +// { dg-prune-output "failed to write compiled module" } -- 2.43.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631] 2024-03-08 2:55 ` [PATCH v2] c++: Check module attachment instead of just " Nathaniel Shead @ 2024-03-08 15:19 ` Jason Merrill 2024-03-08 23:18 ` Nathaniel Shead 0 siblings, 1 reply; 9+ messages in thread From: Jason Merrill @ 2024-03-08 15:19 UTC (permalink / raw) To: Nathaniel Shead, gcc-patches; +Cc: Nathan Sidwell On 3/7/24 21:55, Nathaniel Shead wrote: > On Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote: >> On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: >>> On 11/20/23 04:47, Nathaniel Shead wrote: >>>> Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write >>>> access. >>>> >>>> -- >8 -- >>>> >>>> Block-scope declarations of functions or extern values are not allowed >>>> when attached to a named module. Similarly, class member functions are >>>> not inline if attached to a named module. However, in both these cases >>>> we currently only check if the declaration is within the module purview; >>>> it is possible for such a declaration to occur within the module purview >>>> but not be attached to a named module (e.g. in an 'extern "C++"' block). >>>> This patch makes the required adjustments. >>> >>> >>> Ah I'd been puzzling over the default inlinedness of member-fns of >>> block-scope structs. Could you augment the testcase to make sure that's >>> right too? >>> >>> Something like: >>> >>> // dg-module-do link >>> export module Mod; >>> >>> export auto Get () { >>> struct X { void Fn () {} }; >>> return X(); >>> } >>> >>> >>> /// >>> import Mod >>> void Frob () { Get().Fn(); } >>> >> >> I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be >> marked 'inline' for this to link (whether or not 'Get' itself is >> inline). I've tried tracing the code to work out what's going on but >> I've been struggling to work out how all the different flags (e.g. >> TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) >> interact, which flags we want to be set where, and where the decision of >> what function definitions to emit is actually made. >> >> I did find that doing 'mark_used(decl)' on all member functions in >> block-scope structs seems to work however, but I wonder if that's maybe >> too aggressive or if there's something else we should be doing? > > I got around to looking at this again, here's an updated version of this > patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > (I'm not sure if 'start_preparsed_function' is the right place to be > putting this kind of logic or if it should instead be going in > 'grokfndecl', e.g. decl.cc:10761 where the rules for making local > functions have no linkage are initially determined, but I found this > easier to implement: happy to move around though if preferred.) > > -- >8 -- > > Block-scope declarations of functions or extern values are not allowed > when attached to a named module. Similarly, class member functions are > not inline if attached to a named module. However, in both these cases > we currently only check if the declaration is within the module purview; > it is possible for such a declaration to occur within the module purview > but not be attached to a named module (e.g. in an 'extern "C++"' block). > This patch makes the required adjustments. > > While implementing this we discovered that block-scope local functions > are not correctly emitted, causing link failures; this patch also > corrects some assumptions here and ensures that they are emitted when > needed. > > PR c++/112631 > > gcc/cp/ChangeLog: > > * cp-tree.h (named_module_attach_p): New function. > * decl.cc (start_decl): Check for attachment not purview. > (grokmethod): Likewise. These changes are OK; the others I want to consider more. > +export auto n_n() { > + internal(); > + struct X { void f() { internal(); } }; > + return X{}; Hmm, is this not a prohibited exposure? Seems like X has no linkage because it's at block scope, and the deduced return type names it. I know we try to support this "voldemort" pattern, but is that actually correct? Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631] 2024-03-08 15:19 ` Jason Merrill @ 2024-03-08 23:18 ` Nathaniel Shead 2024-03-11 18:13 ` Jason Merrill 0 siblings, 1 reply; 9+ messages in thread From: Nathaniel Shead @ 2024-03-08 23:18 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches, Nathan Sidwell On Fri, Mar 08, 2024 at 10:19:52AM -0500, Jason Merrill wrote: > On 3/7/24 21:55, Nathaniel Shead wrote: > > On Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote: > > > On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: > > > > On 11/20/23 04:47, Nathaniel Shead wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write > > > > > access. > > > > > > > > > > -- >8 -- > > > > > > > > > > Block-scope declarations of functions or extern values are not allowed > > > > > when attached to a named module. Similarly, class member functions are > > > > > not inline if attached to a named module. However, in both these cases > > > > > we currently only check if the declaration is within the module purview; > > > > > it is possible for such a declaration to occur within the module purview > > > > > but not be attached to a named module (e.g. in an 'extern "C++"' block). > > > > > This patch makes the required adjustments. > > > > > > > > > > > > Ah I'd been puzzling over the default inlinedness of member-fns of > > > > block-scope structs. Could you augment the testcase to make sure that's > > > > right too? > > > > > > > > Something like: > > > > > > > > // dg-module-do link > > > > export module Mod; > > > > > > > > export auto Get () { > > > > struct X { void Fn () {} }; > > > > return X(); > > > > } > > > > > > > > > > > > /// > > > > import Mod > > > > void Frob () { Get().Fn(); } > > > > > > > > > > I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be > > > marked 'inline' for this to link (whether or not 'Get' itself is > > > inline). I've tried tracing the code to work out what's going on but > > > I've been struggling to work out how all the different flags (e.g. > > > TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) > > > interact, which flags we want to be set where, and where the decision of > > > what function definitions to emit is actually made. > > > > > > I did find that doing 'mark_used(decl)' on all member functions in > > > block-scope structs seems to work however, but I wonder if that's maybe > > > too aggressive or if there's something else we should be doing? > > > > I got around to looking at this again, here's an updated version of this > > patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > (I'm not sure if 'start_preparsed_function' is the right place to be > > putting this kind of logic or if it should instead be going in > > 'grokfndecl', e.g. decl.cc:10761 where the rules for making local > > functions have no linkage are initially determined, but I found this > > easier to implement: happy to move around though if preferred.) > > > > -- >8 -- > > > > Block-scope declarations of functions or extern values are not allowed > > when attached to a named module. Similarly, class member functions are > > not inline if attached to a named module. However, in both these cases > > we currently only check if the declaration is within the module purview; > > it is possible for such a declaration to occur within the module purview > > but not be attached to a named module (e.g. in an 'extern "C++"' block). > > This patch makes the required adjustments. > > > > While implementing this we discovered that block-scope local functions > > are not correctly emitted, causing link failures; this patch also > > corrects some assumptions here and ensures that they are emitted when > > needed. > > > > PR c++/112631 > > > > gcc/cp/ChangeLog: > > > > * cp-tree.h (named_module_attach_p): New function. > > * decl.cc (start_decl): Check for attachment not purview. > > (grokmethod): Likewise. > > These changes are OK; the others I want to consider more. > Thanks, I can commit this as a separate commit if you prefer? > > +export auto n_n() { > > + internal(); > > + struct X { void f() { internal(); } }; > > + return X{}; > > Hmm, is this not a prohibited exposure? Seems like X has no linkage because > it's at block scope, and the deduced return type names it. > > I know we try to support this "voldemort" pattern, but is that actually > correct? > > Jason > I had similar doubts, but this is not an especially uncommon pattern in the wild either. I also asked some other people for their thoughts and got told: "no linkage" doesn't mean it's ill-formed to name it in other scopes. It means a declaration in another scope cannot correspond to it And that the wording in [basic.link] p2.4 is imprecise. (Apparently they were going to raise a core issue about this too, I think?) As for whether it's an exposure, looking at [basic.link] p15, the entity 'X' doesn't actually appear to be TU-local: it doesn't have a name with internal linkage (no linkage is different) and is not declared or introduced within the definition of a TU-local entity (n_n is not TU-local). So I think this example is OK, but this example is not: namespace { auto x() { struct X { void f() {} }; return X{}; } } export auto illegal() { return x(); } Which we correctly complain about already: error: 'struct {anonymous}::x()::X' references internal linkage entity 'auto {anonymous}::x()' 6 | struct X { void f() {} }; | ^ Nathaniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631] 2024-03-08 23:18 ` Nathaniel Shead @ 2024-03-11 18:13 ` Jason Merrill 2024-03-16 11:23 ` [PATCH v3] c++: Fix handling of no-linkage decls for modules Nathaniel Shead 0 siblings, 1 reply; 9+ messages in thread From: Jason Merrill @ 2024-03-11 18:13 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Nathan Sidwell On 3/8/24 18:18, Nathaniel Shead wrote: > On Fri, Mar 08, 2024 at 10:19:52AM -0500, Jason Merrill wrote: >> On 3/7/24 21:55, Nathaniel Shead wrote: >>> On Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote: >>>> On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: >>>>> On 11/20/23 04:47, Nathaniel Shead wrote: >>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write >>>>>> access. >>>>>> >>>>>> -- >8 -- >>>>>> >>>>>> Block-scope declarations of functions or extern values are not allowed >>>>>> when attached to a named module. Similarly, class member functions are >>>>>> not inline if attached to a named module. However, in both these cases >>>>>> we currently only check if the declaration is within the module purview; >>>>>> it is possible for such a declaration to occur within the module purview >>>>>> but not be attached to a named module (e.g. in an 'extern "C++"' block). >>>>>> This patch makes the required adjustments. >>>>> >>>>> >>>>> Ah I'd been puzzling over the default inlinedness of member-fns of >>>>> block-scope structs. Could you augment the testcase to make sure that's >>>>> right too? >>>>> >>>>> Something like: >>>>> >>>>> // dg-module-do link >>>>> export module Mod; >>>>> >>>>> export auto Get () { >>>>> struct X { void Fn () {} }; >>>>> return X(); >>>>> } >>>>> >>>>> >>>>> /// >>>>> import Mod >>>>> void Frob () { Get().Fn(); } >>>>> >>>> >>>> I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be >>>> marked 'inline' for this to link (whether or not 'Get' itself is >>>> inline). I've tried tracing the code to work out what's going on but >>>> I've been struggling to work out how all the different flags (e.g. >>>> TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) >>>> interact, which flags we want to be set where, and where the decision of >>>> what function definitions to emit is actually made. >>>> >>>> I did find that doing 'mark_used(decl)' on all member functions in >>>> block-scope structs seems to work however, but I wonder if that's maybe >>>> too aggressive or if there's something else we should be doing? >>> >>> I got around to looking at this again, here's an updated version of this >>> patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? >>> >>> (I'm not sure if 'start_preparsed_function' is the right place to be >>> putting this kind of logic or if it should instead be going in >>> 'grokfndecl', e.g. decl.cc:10761 where the rules for making local >>> functions have no linkage are initially determined, but I found this >>> easier to implement: happy to move around though if preferred.) >>> >>> -- >8 -- >>> >>> Block-scope declarations of functions or extern values are not allowed >>> when attached to a named module. Similarly, class member functions are >>> not inline if attached to a named module. However, in both these cases >>> we currently only check if the declaration is within the module purview; >>> it is possible for such a declaration to occur within the module purview >>> but not be attached to a named module (e.g. in an 'extern "C++"' block). >>> This patch makes the required adjustments. >>> >>> While implementing this we discovered that block-scope local functions >>> are not correctly emitted, causing link failures; this patch also >>> corrects some assumptions here and ensures that they are emitted when >>> needed. >>> >>> PR c++/112631 >>> >>> gcc/cp/ChangeLog: >>> >>> * cp-tree.h (named_module_attach_p): New function. >>> * decl.cc (start_decl): Check for attachment not purview. >>> (grokmethod): Likewise. >> >> These changes are OK; the others I want to consider more. > > Thanks, I can commit this as a separate commit if you prefer? Please. >>> +export auto n_n() { >>> + internal(); >>> + struct X { void f() { internal(); } }; >>> + return X{}; >> >> Hmm, is this not a prohibited exposure? Seems like X has no linkage because >> it's at block scope, and the deduced return type names it. >> >> I know we try to support this "voldemort" pattern, but is that actually >> correct? > > I had similar doubts, but this is not an especially uncommon pattern in > the wild either. I also asked some other people for their thoughts and > got told: > > "no linkage" doesn't mean it's ill-formed to name it in other scopes. > It means a declaration in another scope cannot correspond to it > > And that the wording in [basic.link] p2.4 is imprecise. (Apparently they > were going to raise a core issue about this too, I think?) > > As for whether it's an exposure, looking at [basic.link] p15, the entity > 'X' doesn't actually appear to be TU-local: it doesn't have a name with > internal linkage (no linkage is different) and is not declared or > introduced within the definition of a TU-local entity (n_n is not > TU-local). Hmm, I think you're right. And this rule: > - /* DR 757: A type without linkage shall not be used as the type of a > - variable or function with linkage, unless > - o the variable or function has extern "C" linkage (7.5 [dcl.link]), or > - o the variable or function is not used (3.2 [basic.def.odr]) or is > - defined in the same translation unit. is no longer part of the standard since C++20; the remnant of this rule is the example in https://eel.is/c++draft/basic#def.odr-11 > auto f() { > struct A {}; > return A{}; > } > decltype(f()) g(); > auto x = g(); > A program containing this translation unit is ill-formed because g is odr-used but not defined, and cannot be defined in any other translation unit because the local class A cannot be named outside this translation unit. But g could be defined in another translation unit if f is inline or in a module interface unit. So, I think no_linkage_check needs to consider module_has_cmi_p as well as vague_linkage_p for relaxed mode. And in no_linkage_error if no_linkage_check returns null in relaxed mode, reduce the permerror to a pedwarn before C++20, and no diagnostic at all in C++20 and above. > + if (ctx != NULL_TREE && TREE_PUBLIC (ctx) && module_has_cmi_p ()) > + { > + /* Ensure that functions in local classes within named modules > + have their definitions exported, in case they are (directly > + or indirectly) used by an importer. */ > + TREE_PUBLIC (decl1) = true; > + if (DECL_DECLARED_INLINE_P (decl1)) > + comdat_linkage (decl1); > + else > + mark_needed (decl1); > + } Isn't the inline case handled by the comdat_linkage just above? Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] c++: Fix handling of no-linkage decls for modules 2024-03-11 18:13 ` Jason Merrill @ 2024-03-16 11:23 ` Nathaniel Shead 2024-03-19 0:58 ` Jason Merrill 0 siblings, 1 reply; 9+ messages in thread From: Nathaniel Shead @ 2024-03-16 11:23 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches, Nathan Sidwell On Mon, Mar 11, 2024 at 02:13:34PM -0400, Jason Merrill wrote: > On 3/8/24 18:18, Nathaniel Shead wrote: > > On Fri, Mar 08, 2024 at 10:19:52AM -0500, Jason Merrill wrote: > > > On 3/7/24 21:55, Nathaniel Shead wrote: > > > > On Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote: > > > > > On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: > > > > > > On 11/20/23 04:47, Nathaniel Shead wrote: > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write > > > > > > > access. > > > > > > > > > > > > > > -- >8 -- > > > > > > > > > > > > > > Block-scope declarations of functions or extern values are not allowed > > > > > > > when attached to a named module. Similarly, class member functions are > > > > > > > not inline if attached to a named module. However, in both these cases > > > > > > > we currently only check if the declaration is within the module purview; > > > > > > > it is possible for such a declaration to occur within the module purview > > > > > > > but not be attached to a named module (e.g. in an 'extern "C++"' block). > > > > > > > This patch makes the required adjustments. > > > > > > > > > > > > > > > > > > Ah I'd been puzzling over the default inlinedness of member-fns of > > > > > > block-scope structs. Could you augment the testcase to make sure that's > > > > > > right too? > > > > > > > > > > > > Something like: > > > > > > > > > > > > // dg-module-do link > > > > > > export module Mod; > > > > > > > > > > > > export auto Get () { > > > > > > struct X { void Fn () {} }; > > > > > > return X(); > > > > > > } > > > > > > > > > > > > > > > > > > /// > > > > > > import Mod > > > > > > void Frob () { Get().Fn(); } > > > > > > > > > > > > > > > > I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be > > > > > marked 'inline' for this to link (whether or not 'Get' itself is > > > > > inline). I've tried tracing the code to work out what's going on but > > > > > I've been struggling to work out how all the different flags (e.g. > > > > > TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) > > > > > interact, which flags we want to be set where, and where the decision of > > > > > what function definitions to emit is actually made. > > > > > > > > > > I did find that doing 'mark_used(decl)' on all member functions in > > > > > block-scope structs seems to work however, but I wonder if that's maybe > > > > > too aggressive or if there's something else we should be doing? > > > > > > > > I got around to looking at this again, here's an updated version of this > > > > patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > > > > > (I'm not sure if 'start_preparsed_function' is the right place to be > > > > putting this kind of logic or if it should instead be going in > > > > 'grokfndecl', e.g. decl.cc:10761 where the rules for making local > > > > functions have no linkage are initially determined, but I found this > > > > easier to implement: happy to move around though if preferred.) > > > > > > > > -- >8 -- > > > > > > > > Block-scope declarations of functions or extern values are not allowed > > > > when attached to a named module. Similarly, class member functions are > > > > not inline if attached to a named module. However, in both these cases > > > > we currently only check if the declaration is within the module purview; > > > > it is possible for such a declaration to occur within the module purview > > > > but not be attached to a named module (e.g. in an 'extern "C++"' block). > > > > This patch makes the required adjustments. > > > > > > > > While implementing this we discovered that block-scope local functions > > > > are not correctly emitted, causing link failures; this patch also > > > > corrects some assumptions here and ensures that they are emitted when > > > > needed. > > > > > > > > PR c++/112631 > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * cp-tree.h (named_module_attach_p): New function. > > > > * decl.cc (start_decl): Check for attachment not purview. > > > > (grokmethod): Likewise. > > > > > > These changes are OK; the others I want to consider more. > > > > Thanks, I can commit this as a separate commit if you prefer? > > Please. > Thanks, committed as r14-9501-gead3075406ece9. > > > > +export auto n_n() { > > > > + internal(); > > > > + struct X { void f() { internal(); } }; > > > > + return X{}; > > > > > > Hmm, is this not a prohibited exposure? Seems like X has no linkage because > > > it's at block scope, and the deduced return type names it. > > > > > > I know we try to support this "voldemort" pattern, but is that actually > > > correct? > > > > I had similar doubts, but this is not an especially uncommon pattern in > > the wild either. I also asked some other people for their thoughts and > > got told: > > > > "no linkage" doesn't mean it's ill-formed to name it in other scopes. > > It means a declaration in another scope cannot correspond to it > > > > And that the wording in [basic.link] p2.4 is imprecise. (Apparently they > > were going to raise a core issue about this too, I think?) > > > > As for whether it's an exposure, looking at [basic.link] p15, the entity > > 'X' doesn't actually appear to be TU-local: it doesn't have a name with > > internal linkage (no linkage is different) and is not declared or > > introduced within the definition of a TU-local entity (n_n is not > > TU-local). > > Hmm, I think you're right. And this rule: > > > - /* DR 757: A type without linkage shall not be used as the type of a > > - variable or function with linkage, unless > > - o the variable or function has extern "C" linkage (7.5 [dcl.link]), or > > - o the variable or function is not used (3.2 [basic.def.odr]) or is > > - defined in the same translation unit. > > is no longer part of the standard since C++20; the remnant of this rule is > the example in > > https://eel.is/c++draft/basic#def.odr-11 > > > auto f() { > > struct A {}; > > return A{}; > > } > > decltype(f()) g(); > > auto x = g(); > > > A program containing this translation unit is ill-formed because g is odr-used but not defined, and cannot be defined in any other translation unit because the local class A cannot be named outside this translation unit. > > But g could be defined in another translation unit if f is inline or in a > module interface unit. > > So, I think no_linkage_check needs to consider module_has_cmi_p as well as > vague_linkage_p for relaxed mode. And in no_linkage_error if > no_linkage_check returns null in relaxed mode, reduce the permerror to a > pedwarn before C++20, and no diagnostic at all in C++20 and above. > Thanks for the pointers, I've implemented this below. > > + if (ctx != NULL_TREE && TREE_PUBLIC (ctx) && module_has_cmi_p ()) > > + { > > + /* Ensure that functions in local classes within named modules > > + have their definitions exported, in case they are (directly > > + or indirectly) used by an importer. */ > > + TREE_PUBLIC (decl1) = true; > > + if (DECL_DECLARED_INLINE_P (decl1)) > > + comdat_linkage (decl1); > > + else > > + mark_needed (decl1); > > + } > > Isn't the inline case handled by the comdat_linkage just above? > > Jason > It wasn't, because 'TREE_PUBLIC (decl1)' wasn't yet set. But this means that it's ended up a lot cleaner to do this in grokfndecl instead then, which (along with the no_linkage_check changes) means I didn't actually need to change this code at all. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- When testing the changes for PR c++/112631 we discovered that currently we don't emit definitions of block-scope function declarations if they're not used in the module interface TU, which causes issues if they are used by importers. This patch fixes the handling of no-linkage declarations for C++20. In particular, a type declared in a function with vague linkage or declared in a module CMI could potentially be accessible outside its defining TU, and as such we can't assume that function declarations using that type can never be defined in another TU. A complication with handling this is that we're only strictly interested in declarations with a module CMI, but when parsing the global module fragment we don't yet know whether or not this module will have a CMI until we reach the "export module" line (or not). Since this case is IFNDR anyway (by [basic.def.odr] p11) we just tentatively assume while parsing the GMF that this module will have a CMI; once we see (or don't see) an 'export module' declaration we can commit to that knowledge for future declarations. gcc/cp/ChangeLog: * cp-tree.h (module_maybe_has_cmi_p): New function. * decl.cc (grokfndecl): Mark block-scope functions as public if they could be visible in other TUs. * decl2.cc (no_linkage_error): Don't error for declarations that could be defined in other TUs since C++20. Suppress duplicate errors from 'check_global_declaration'. * tree.cc (no_linkage_check): In relaxed mode, don't consider types in a module CMI to have no linkage. gcc/testsuite/ChangeLog: * g++.dg/cpp2a/linkage-1.C: New test. * g++.dg/modules/block-decl-3.h: New test. * g++.dg/modules/block-decl-3_a.C: New test. * g++.dg/modules/block-decl-3_b.C: New test. * g++.dg/modules/block-decl-3_c.C: New test. * g++.dg/modules/linkage-1_a.C: New test. * g++.dg/modules/linkage-1_b.C: New test. * g++.dg/modules/linkage-1_c.C: New test. * g++.dg/modules/linkage-2.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/cp-tree.h | 6 + gcc/cp/decl.cc | 10 +- gcc/cp/decl2.cc | 39 ++++- gcc/cp/tree.cc | 21 ++- gcc/testsuite/g++.dg/cpp2a/linkage-1.C | 18 ++ gcc/testsuite/g++.dg/modules/block-decl-3.h | 39 +++++ gcc/testsuite/g++.dg/modules/block-decl-3_a.C | 157 ++++++++++++++++++ gcc/testsuite/g++.dg/modules/block-decl-3_b.C | 8 + gcc/testsuite/g++.dg/modules/block-decl-3_c.C | 30 ++++ gcc/testsuite/g++.dg/modules/linkage-1_a.C | 15 ++ gcc/testsuite/g++.dg/modules/linkage-1_b.C | 6 + gcc/testsuite/g++.dg/modules/linkage-1_c.C | 9 + gcc/testsuite/g++.dg/modules/linkage-2.C | 26 +++ 13 files changed, 372 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/linkage-1.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3.h create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3_a.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3_b.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3_c.C create mode 100644 gcc/testsuite/g++.dg/modules/linkage-1_a.C create mode 100644 gcc/testsuite/g++.dg/modules/linkage-1_b.C create mode 100644 gcc/testsuite/g++.dg/modules/linkage-1_c.C create mode 100644 gcc/testsuite/g++.dg/modules/linkage-2.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 05913861e06..52d53589e51 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7384,6 +7384,12 @@ inline bool named_module_purview_p () inline bool named_module_attach_p () { return named_module_p () && module_attach_p (); } +/* We don't know if this TU will have a CMI while parsing the GMF, + so tentatively assume that it might, for the purpose of determining + whether no-linkage decls could be used by an importer. */ +inline bool module_maybe_has_cmi_p () +{ return module_has_cmi_p () || (named_module_p () && !module_purview_p ()); } + /* We're currently exporting declarations. */ inline bool module_exporting_p () { return module_kind & MK_EXPORTING; } diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index 7a97b867199..65ab64885ff 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -10756,9 +10756,15 @@ grokfndecl (tree ctype, /* Members of anonymous types and local classes have no linkage; make them internal. If a typedef is made later, this will be changed. */ - if (ctype && (!TREE_PUBLIC (TYPE_MAIN_DECL (ctype)) - || decl_function_context (TYPE_MAIN_DECL (ctype)))) + if (ctype && !TREE_PUBLIC (TYPE_MAIN_DECL (ctype))) publicp = 0; + else if (ctype && decl_function_context (TYPE_MAIN_DECL (ctype))) + /* But members of local classes in a module CMI should have their + definitions exported, in case they are (directly or indirectly) + used by an importer. We don't just use module_has_cmi_p here + because for entities in the GMF we don't yet know whether this + module will have a CMI, so we'll conservatively assume it might. */ + publicp = module_maybe_has_cmi_p (); if (publicp && cxx_dialect == cxx98) { diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index 6c9fd415d40..2562d8aeff6 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -4696,8 +4696,19 @@ no_linkage_error (tree decl) bool d = false; auto_diagnostic_group grp; if (cxx_dialect >= cxx11) - d = permerror (DECL_SOURCE_LOCATION (decl), "%q#D, declared using " - "unnamed type, is used but never defined", decl); + { + /* If t is declared in a module CMI, then decl could actually + be defined in a different TU, so don't warn since C++20. */ + tree relaxed = no_linkage_check (t, /*relaxed_p=*/true); + if (relaxed != NULL_TREE) + d = permerror (DECL_SOURCE_LOCATION (decl), + "%q#D, declared using an unnamed type, " + "is used but never defined", decl); + else if (cxx_dialect < cxx20) + d = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wc__20_extensions, + "%q#D, declared using an unnamed type, " + "is used but not defined", decl); + } else if (DECL_EXTERN_C_P (decl)) /* Allow this; it's pretty common in C. */; else if (VAR_P (decl)) @@ -4716,13 +4727,31 @@ no_linkage_error (tree decl) inform (DECL_SOURCE_LOCATION (TYPE_NAME (t)), "%q#D does not refer " "to the unqualified type, so it is not used for linkage", TYPE_NAME (t)); + /* Suppress warning from check_global_declaration if needed. */ + if (d) + suppress_warning (decl, OPT_Wunused); } else if (cxx_dialect >= cxx11) { if (VAR_P (decl) || !DECL_PURE_VIRTUAL_P (decl)) - permerror (DECL_SOURCE_LOCATION (decl), - "%q#D, declared using local type " - "%qT, is used but never defined", decl, t); + { + /* Similarly for local types in a function with vague linkage or + defined in a module CMI, then decl could actually be defined + in a different TU, so don't warn since C++20. */ + bool d = false; + tree relaxed = no_linkage_check (t, /*relaxed_p=*/true); + if (relaxed != NULL_TREE) + d = permerror (DECL_SOURCE_LOCATION (decl), + "%q#D, declared using local type " + "%qT, is used but never defined", decl, t); + else if (cxx_dialect < cxx20) + d = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wc__20_extensions, + "%q#D, declared using local type " + "%qT, is used but not defined here", decl, t); + /* Suppress warning from check_global_declaration if needed. */ + if (d) + suppress_warning (decl, OPT_Wunused); + } } else if (VAR_P (decl)) warning_at (DECL_SOURCE_LOCATION (decl), 0, "type %qT with no linkage " diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc index e75be9a4e66..f1a23ffe817 100644 --- a/gcc/cp/tree.cc +++ b/gcc/cp/tree.cc @@ -2971,7 +2971,8 @@ verify_stmt_tree (tree t) /* Check if the type T depends on a type with no linkage and if so, return it. If RELAXED_P then do not consider a class type declared - within a vague-linkage function to have no linkage. Remember: + within a vague-linkage function or in a module CMI to have no linkage, + since it can still be accessed within a different TU. Remember: no-linkage is not the same as internal-linkage. */ tree @@ -3012,7 +3013,15 @@ no_linkage_check (tree t, bool relaxed_p) /* Only treat unnamed types as having no linkage if they're at namespace scope. This is core issue 966. */ if (TYPE_UNNAMED_P (t) && TYPE_NAMESPACE_SCOPE_P (t)) - return t; + { + if (relaxed_p + && TREE_PUBLIC (CP_TYPE_CONTEXT (t)) + && module_maybe_has_cmi_p ()) + /* This type could possibly be accessed outside this TU. */ + return NULL_TREE; + else + return t; + } for (r = CP_TYPE_CONTEXT (t); ; ) { @@ -3023,10 +3032,12 @@ no_linkage_check (tree t, bool relaxed_p) return no_linkage_check (TYPE_CONTEXT (t), relaxed_p); else if (TREE_CODE (r) == FUNCTION_DECL) { - if (!relaxed_p || !vague_linkage_p (r)) - return t; - else + if (relaxed_p + && (vague_linkage_p (r) + || (TREE_PUBLIC (r) && module_maybe_has_cmi_p ()))) r = CP_DECL_CONTEXT (r); + else + return t; } else break; diff --git a/gcc/testsuite/g++.dg/cpp2a/linkage-1.C b/gcc/testsuite/g++.dg/cpp2a/linkage-1.C new file mode 100644 index 00000000000..888ed6fa5b5 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/linkage-1.C @@ -0,0 +1,18 @@ +// { dg-do compile { target c++11 } } + +inline auto f() { + struct A {}; + return A{}; +} +decltype(f()) a(); // { dg-error "used but not defined" "" { target c++17_down } } + +auto g() { + struct A {}; + return A{}; +} +decltype(g()) b(); // { dg-error "used but never defined" } + +int main() { + a(); + b(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3.h b/gcc/testsuite/g++.dg/modules/block-decl-3.h new file mode 100644 index 00000000000..4b155eb0054 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-3.h @@ -0,0 +1,39 @@ +// GMF + +// Non-inline function definitions in headers are a recipe for ODR violations, +// but we should probably support that anyway as its not inherently wrong +// if only ever included into the GMF of a single module. + +auto gmf_n_i() { + struct X { void f() {} }; + return X{}; +} + +inline auto gmf_i_i() { + struct X { void f() {} }; + return X{}; +} + +auto gmf_n_i_i() { + struct X { + auto f() { + struct Y { + void g() {} + }; + return Y{}; + } + }; + return X{}; +} + +inline auto gmf_i_i_i() { + struct X { + auto f() { + struct Y { + void g() {} + }; + return Y{}; + } + }; + return X{}; +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3_a.C b/gcc/testsuite/g++.dg/modules/block-decl-3_a.C new file mode 100644 index 00000000000..8cb4dde74d1 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-3_a.C @@ -0,0 +1,157 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi mod } + +// Test that we can link various forms of local class functions. +// Function names use i=inline, n=non-inline, for each nesting. + +module; +#include "block-decl-3.h" + +export module mod; + +namespace { + void internal() {} +} + +// singly-nested + +export auto n_n() { + internal(); + struct X { void f() { internal(); } }; + return X{}; +} + +export auto n_i() { + internal(); + struct X { inline void f() {} }; + return X{}; +} + +export inline auto i_n() { + // `f` is not inline here, so this is OK + struct X { void f() { internal(); } }; + return X{}; +} + +export inline auto i_i() { + struct X { inline void f() {} }; + return X{}; +} + + +// doubly nested + +export auto n_n_n() { + struct X { + auto f() { + struct Y { + void g() { internal(); } + }; + return Y{}; + } + }; + return X{}; +} + +export auto n_i_n() { + struct X { + inline auto f() { + struct Y { + void g() { internal(); } + }; + return Y{}; + } + }; + return X{}; +} + +export inline auto i_n_i() { + struct X { + auto f() { + struct Y { + inline void g() {} + }; + return Y {}; + } + }; + return X{}; +} + +export inline auto i_i_i() { + struct X { + inline auto f() { + struct Y { + inline void g() {} + }; + return Y{}; + } + }; + return X{}; +} + + +// multiple types + +export auto multi_n_n() { + struct X { + void f() { internal(); } + }; + struct Y { + X x; + }; + return Y {}; +} + +export auto multi_n_i() { + struct X { + inline void f() {} + }; + struct Y { + X x; + }; + return Y {}; +} + +export inline auto multi_i_i() { + struct X { + inline void f() {} + }; + struct Y { + X x; + }; + return Y {}; +}; + + +// extern "C++" + +export extern "C++" auto extern_n_i() { + struct X { + void f() {} // implicitly inline + }; + return X{}; +}; + +export extern "C++" inline auto extern_i_i() { + struct X { + void f() {} + }; + return X{}; +}; + + +// GMF + +export using ::gmf_n_i; +export using ::gmf_i_i; +export using ::gmf_n_i_i; +export using ::gmf_i_i_i; + + +// can access from implementation unit + +auto only_used_in_impl() { + struct X { void f() {} }; + return X{}; +} +export void test_from_impl_unit(); diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3_b.C b/gcc/testsuite/g++.dg/modules/block-decl-3_b.C new file mode 100644 index 00000000000..bc9b2a213f0 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-3_b.C @@ -0,0 +1,8 @@ +// { dg-additional-options "-fmodules-ts" } + +module mod; + +// Test that we can access (and link) to declarations from the interface +void test_from_impl_unit() { + only_used_in_impl().f(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3_c.C b/gcc/testsuite/g++.dg/modules/block-decl-3_c.C new file mode 100644 index 00000000000..5b39e038327 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-3_c.C @@ -0,0 +1,30 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import mod; + +int main() { + n_n().f(); + n_i().f(); + i_n().f(); + i_i().f(); + + n_n_n().f().g(); + n_i_n().f().g(); + i_n_i().f().g(); + i_i_i().f().g(); + + multi_n_n().x.f(); + multi_n_i().x.f(); + multi_i_i().x.f(); + + extern_n_i().f(); + extern_i_i().f(); + + gmf_n_i().f(); + gmf_i_i().f(); + gmf_n_i_i().f().g(); + gmf_i_i_i().f().g(); + + test_from_impl_unit(); +} diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_a.C b/gcc/testsuite/g++.dg/modules/linkage-1_a.C new file mode 100644 index 00000000000..750e31ff347 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/linkage-1_a.C @@ -0,0 +1,15 @@ +// { dg-additional-options "-fmodules-ts -Wno-error=c++20-extensions" } +// { dg-module-cmi M } + +export module M; + +auto f() { + struct A {}; + return A{}; +} +decltype(f()) g(); // { dg-warning "used but not defined" "" { target c++17_down } } +export auto x = g(); + +struct {} s; +decltype(s) h(); // { dg-warning "used but not defined" "" { target c++17_down } } +export auto y = h(); diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_b.C b/gcc/testsuite/g++.dg/modules/linkage-1_b.C new file mode 100644 index 00000000000..f23962d76b7 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/linkage-1_b.C @@ -0,0 +1,6 @@ +// { dg-additional-options "-fmodules-ts" } + +module M; + +decltype(f()) g() { return {}; } +decltype(s) h() { return {}; } diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_c.C b/gcc/testsuite/g++.dg/modules/linkage-1_c.C new file mode 100644 index 00000000000..f1406b99032 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/linkage-1_c.C @@ -0,0 +1,9 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import M; + +int main() { + auto a = x; + auto b = y; +} diff --git a/gcc/testsuite/g++.dg/modules/linkage-2.C b/gcc/testsuite/g++.dg/modules/linkage-2.C new file mode 100644 index 00000000000..eb4d7b051af --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/linkage-2.C @@ -0,0 +1,26 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi !M } + +export module M; + +// Same as a linkage-1 except within an anonymous namespace; +// now these declarations cannot possibly be defined outside this TU, +// so we should error. + +namespace { + auto f() { + struct A {}; + return A{}; + } + decltype(f()) g(); // { dg-error "used but never defined" } + + struct {} s; + decltype(s) h(); // { dg-error "used but never defined" } +} + +export void use() { + g(); + h(); +} + +// { dg-prune-output "not writing module" } -- 2.43.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] c++: Fix handling of no-linkage decls for modules 2024-03-16 11:23 ` [PATCH v3] c++: Fix handling of no-linkage decls for modules Nathaniel Shead @ 2024-03-19 0:58 ` Jason Merrill 0 siblings, 0 replies; 9+ messages in thread From: Jason Merrill @ 2024-03-19 0:58 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Nathan Sidwell On 3/16/24 07:23, Nathaniel Shead wrote: > On Mon, Mar 11, 2024 at 02:13:34PM -0400, Jason Merrill wrote: >> On 3/8/24 18:18, Nathaniel Shead wrote: >>> On Fri, Mar 08, 2024 at 10:19:52AM -0500, Jason Merrill wrote: >>>> On 3/7/24 21:55, Nathaniel Shead wrote: >>>>> On Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote: >>>>>> On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: >>>>>>> On 11/20/23 04:47, Nathaniel Shead wrote: >>>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write >>>>>>>> access. >>>>>>>> >>>>>>>> -- >8 -- >>>>>>>> >>>>>>>> Block-scope declarations of functions or extern values are not allowed >>>>>>>> when attached to a named module. Similarly, class member functions are >>>>>>>> not inline if attached to a named module. However, in both these cases >>>>>>>> we currently only check if the declaration is within the module purview; >>>>>>>> it is possible for such a declaration to occur within the module purview >>>>>>>> but not be attached to a named module (e.g. in an 'extern "C++"' block). >>>>>>>> This patch makes the required adjustments. >>>>>>> >>>>>>> >>>>>>> Ah I'd been puzzling over the default inlinedness of member-fns of >>>>>>> block-scope structs. Could you augment the testcase to make sure that's >>>>>>> right too? >>>>>>> >>>>>>> Something like: >>>>>>> >>>>>>> // dg-module-do link >>>>>>> export module Mod; >>>>>>> >>>>>>> export auto Get () { >>>>>>> struct X { void Fn () {} }; >>>>>>> return X(); >>>>>>> } >>>>>>> >>>>>>> >>>>>>> /// >>>>>>> import Mod >>>>>>> void Frob () { Get().Fn(); } >>>>>>> >>>>>> >>>>>> I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be >>>>>> marked 'inline' for this to link (whether or not 'Get' itself is >>>>>> inline). I've tried tracing the code to work out what's going on but >>>>>> I've been struggling to work out how all the different flags (e.g. >>>>>> TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) >>>>>> interact, which flags we want to be set where, and where the decision of >>>>>> what function definitions to emit is actually made. >>>>>> >>>>>> I did find that doing 'mark_used(decl)' on all member functions in >>>>>> block-scope structs seems to work however, but I wonder if that's maybe >>>>>> too aggressive or if there's something else we should be doing? >>>>> >>>>> I got around to looking at this again, here's an updated version of this >>>>> patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? >>>>> >>>>> (I'm not sure if 'start_preparsed_function' is the right place to be >>>>> putting this kind of logic or if it should instead be going in >>>>> 'grokfndecl', e.g. decl.cc:10761 where the rules for making local >>>>> functions have no linkage are initially determined, but I found this >>>>> easier to implement: happy to move around though if preferred.) >>>>> >>>>> -- >8 -- >>>>> >>>>> Block-scope declarations of functions or extern values are not allowed >>>>> when attached to a named module. Similarly, class member functions are >>>>> not inline if attached to a named module. However, in both these cases >>>>> we currently only check if the declaration is within the module purview; >>>>> it is possible for such a declaration to occur within the module purview >>>>> but not be attached to a named module (e.g. in an 'extern "C++"' block). >>>>> This patch makes the required adjustments. >>>>> >>>>> While implementing this we discovered that block-scope local functions >>>>> are not correctly emitted, causing link failures; this patch also >>>>> corrects some assumptions here and ensures that they are emitted when >>>>> needed. >>>>> >>>>> PR c++/112631 >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * cp-tree.h (named_module_attach_p): New function. >>>>> * decl.cc (start_decl): Check for attachment not purview. >>>>> (grokmethod): Likewise. >>>> >>>> These changes are OK; the others I want to consider more. >>> >>> Thanks, I can commit this as a separate commit if you prefer? >> >> Please. >> > > Thanks, committed as r14-9501-gead3075406ece9. > >>>>> +export auto n_n() { >>>>> + internal(); >>>>> + struct X { void f() { internal(); } }; >>>>> + return X{}; >>>> >>>> Hmm, is this not a prohibited exposure? Seems like X has no linkage because >>>> it's at block scope, and the deduced return type names it. >>>> >>>> I know we try to support this "voldemort" pattern, but is that actually >>>> correct? >>> >>> I had similar doubts, but this is not an especially uncommon pattern in >>> the wild either. I also asked some other people for their thoughts and >>> got told: >>> >>> "no linkage" doesn't mean it's ill-formed to name it in other scopes. >>> It means a declaration in another scope cannot correspond to it >>> >>> And that the wording in [basic.link] p2.4 is imprecise. (Apparently they >>> were going to raise a core issue about this too, I think?) >>> >>> As for whether it's an exposure, looking at [basic.link] p15, the entity >>> 'X' doesn't actually appear to be TU-local: it doesn't have a name with >>> internal linkage (no linkage is different) and is not declared or >>> introduced within the definition of a TU-local entity (n_n is not >>> TU-local). >> >> Hmm, I think you're right. And this rule: >> >>> - /* DR 757: A type without linkage shall not be used as the type of a >>> - variable or function with linkage, unless >>> - o the variable or function has extern "C" linkage (7.5 [dcl.link]), or >>> - o the variable or function is not used (3.2 [basic.def.odr]) or is >>> - defined in the same translation unit. >> >> is no longer part of the standard since C++20; the remnant of this rule is >> the example in >> >> https://eel.is/c++draft/basic#def.odr-11 >> >>> auto f() { >>> struct A {}; >>> return A{}; >>> } >>> decltype(f()) g(); >>> auto x = g(); >> >>> A program containing this translation unit is ill-formed because g is odr-used but not defined, and cannot be defined in any other translation unit because the local class A cannot be named outside this translation unit. >> >> But g could be defined in another translation unit if f is inline or in a >> module interface unit. >> >> So, I think no_linkage_check needs to consider module_has_cmi_p as well as >> vague_linkage_p for relaxed mode. And in no_linkage_error if >> no_linkage_check returns null in relaxed mode, reduce the permerror to a >> pedwarn before C++20, and no diagnostic at all in C++20 and above. >> > > Thanks for the pointers, I've implemented this below. > >>> + if (ctx != NULL_TREE && TREE_PUBLIC (ctx) && module_has_cmi_p ()) >>> + { >>> + /* Ensure that functions in local classes within named modules >>> + have their definitions exported, in case they are (directly >>> + or indirectly) used by an importer. */ >>> + TREE_PUBLIC (decl1) = true; >>> + if (DECL_DECLARED_INLINE_P (decl1)) >>> + comdat_linkage (decl1); >>> + else >>> + mark_needed (decl1); >>> + } >> >> Isn't the inline case handled by the comdat_linkage just above? >> >> Jason >> > > It wasn't, because 'TREE_PUBLIC (decl1)' wasn't yet set. But this means > that it's ended up a lot cleaner to do this in grokfndecl instead then, > which (along with the no_linkage_check changes) means I didn't actually > need to change this code at all. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? OK. > -- >8 -- > > When testing the changes for PR c++/112631 we discovered that currently > we don't emit definitions of block-scope function declarations if > they're not used in the module interface TU, which causes issues if they > are used by importers. > > This patch fixes the handling of no-linkage declarations for C++20. In > particular, a type declared in a function with vague linkage or declared > in a module CMI could potentially be accessible outside its defining TU, > and as such we can't assume that function declarations using that type > can never be defined in another TU. > > A complication with handling this is that we're only strictly interested > in declarations with a module CMI, but when parsing the global module > fragment we don't yet know whether or not this module will have a CMI > until we reach the "export module" line (or not). Since this case is > IFNDR anyway (by [basic.def.odr] p11) we just tentatively assume while > parsing the GMF that this module will have a CMI; once we see (or don't > see) an 'export module' declaration we can commit to that knowledge for > future declarations. > > gcc/cp/ChangeLog: > > * cp-tree.h (module_maybe_has_cmi_p): New function. > * decl.cc (grokfndecl): Mark block-scope functions as public if > they could be visible in other TUs. > * decl2.cc (no_linkage_error): Don't error for declarations that > could be defined in other TUs since C++20. Suppress duplicate > errors from 'check_global_declaration'. > * tree.cc (no_linkage_check): In relaxed mode, don't consider > types in a module CMI to have no linkage. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp2a/linkage-1.C: New test. > * g++.dg/modules/block-decl-3.h: New test. > * g++.dg/modules/block-decl-3_a.C: New test. > * g++.dg/modules/block-decl-3_b.C: New test. > * g++.dg/modules/block-decl-3_c.C: New test. > * g++.dg/modules/linkage-1_a.C: New test. > * g++.dg/modules/linkage-1_b.C: New test. > * g++.dg/modules/linkage-1_c.C: New test. > * g++.dg/modules/linkage-2.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/cp-tree.h | 6 + > gcc/cp/decl.cc | 10 +- > gcc/cp/decl2.cc | 39 ++++- > gcc/cp/tree.cc | 21 ++- > gcc/testsuite/g++.dg/cpp2a/linkage-1.C | 18 ++ > gcc/testsuite/g++.dg/modules/block-decl-3.h | 39 +++++ > gcc/testsuite/g++.dg/modules/block-decl-3_a.C | 157 ++++++++++++++++++ > gcc/testsuite/g++.dg/modules/block-decl-3_b.C | 8 + > gcc/testsuite/g++.dg/modules/block-decl-3_c.C | 30 ++++ > gcc/testsuite/g++.dg/modules/linkage-1_a.C | 15 ++ > gcc/testsuite/g++.dg/modules/linkage-1_b.C | 6 + > gcc/testsuite/g++.dg/modules/linkage-1_c.C | 9 + > gcc/testsuite/g++.dg/modules/linkage-2.C | 26 +++ > 13 files changed, 372 insertions(+), 12 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/linkage-1.C > create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3.h > create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3_c.C > create mode 100644 gcc/testsuite/g++.dg/modules/linkage-1_a.C > create mode 100644 gcc/testsuite/g++.dg/modules/linkage-1_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/linkage-1_c.C > create mode 100644 gcc/testsuite/g++.dg/modules/linkage-2.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 05913861e06..52d53589e51 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7384,6 +7384,12 @@ inline bool named_module_purview_p () > inline bool named_module_attach_p () > { return named_module_p () && module_attach_p (); } > > +/* We don't know if this TU will have a CMI while parsing the GMF, > + so tentatively assume that it might, for the purpose of determining > + whether no-linkage decls could be used by an importer. */ > +inline bool module_maybe_has_cmi_p () > +{ return module_has_cmi_p () || (named_module_p () && !module_purview_p ()); } > + > /* We're currently exporting declarations. */ > inline bool module_exporting_p () > { return module_kind & MK_EXPORTING; } > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 7a97b867199..65ab64885ff 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -10756,9 +10756,15 @@ grokfndecl (tree ctype, > > /* Members of anonymous types and local classes have no linkage; make > them internal. If a typedef is made later, this will be changed. */ > - if (ctype && (!TREE_PUBLIC (TYPE_MAIN_DECL (ctype)) > - || decl_function_context (TYPE_MAIN_DECL (ctype)))) > + if (ctype && !TREE_PUBLIC (TYPE_MAIN_DECL (ctype))) > publicp = 0; > + else if (ctype && decl_function_context (TYPE_MAIN_DECL (ctype))) > + /* But members of local classes in a module CMI should have their > + definitions exported, in case they are (directly or indirectly) > + used by an importer. We don't just use module_has_cmi_p here > + because for entities in the GMF we don't yet know whether this > + module will have a CMI, so we'll conservatively assume it might. */ > + publicp = module_maybe_has_cmi_p (); > > if (publicp && cxx_dialect == cxx98) > { > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > index 6c9fd415d40..2562d8aeff6 100644 > --- a/gcc/cp/decl2.cc > +++ b/gcc/cp/decl2.cc > @@ -4696,8 +4696,19 @@ no_linkage_error (tree decl) > bool d = false; > auto_diagnostic_group grp; > if (cxx_dialect >= cxx11) > - d = permerror (DECL_SOURCE_LOCATION (decl), "%q#D, declared using " > - "unnamed type, is used but never defined", decl); > + { > + /* If t is declared in a module CMI, then decl could actually > + be defined in a different TU, so don't warn since C++20. */ > + tree relaxed = no_linkage_check (t, /*relaxed_p=*/true); > + if (relaxed != NULL_TREE) > + d = permerror (DECL_SOURCE_LOCATION (decl), > + "%q#D, declared using an unnamed type, " > + "is used but never defined", decl); > + else if (cxx_dialect < cxx20) > + d = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wc__20_extensions, > + "%q#D, declared using an unnamed type, " > + "is used but not defined", decl); > + } > else if (DECL_EXTERN_C_P (decl)) > /* Allow this; it's pretty common in C. */; > else if (VAR_P (decl)) > @@ -4716,13 +4727,31 @@ no_linkage_error (tree decl) > inform (DECL_SOURCE_LOCATION (TYPE_NAME (t)), "%q#D does not refer " > "to the unqualified type, so it is not used for linkage", > TYPE_NAME (t)); > + /* Suppress warning from check_global_declaration if needed. */ > + if (d) > + suppress_warning (decl, OPT_Wunused); > } > else if (cxx_dialect >= cxx11) > { > if (VAR_P (decl) || !DECL_PURE_VIRTUAL_P (decl)) > - permerror (DECL_SOURCE_LOCATION (decl), > - "%q#D, declared using local type " > - "%qT, is used but never defined", decl, t); > + { > + /* Similarly for local types in a function with vague linkage or > + defined in a module CMI, then decl could actually be defined > + in a different TU, so don't warn since C++20. */ > + bool d = false; > + tree relaxed = no_linkage_check (t, /*relaxed_p=*/true); > + if (relaxed != NULL_TREE) > + d = permerror (DECL_SOURCE_LOCATION (decl), > + "%q#D, declared using local type " > + "%qT, is used but never defined", decl, t); > + else if (cxx_dialect < cxx20) > + d = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wc__20_extensions, > + "%q#D, declared using local type " > + "%qT, is used but not defined here", decl, t); > + /* Suppress warning from check_global_declaration if needed. */ > + if (d) > + suppress_warning (decl, OPT_Wunused); > + } > } > else if (VAR_P (decl)) > warning_at (DECL_SOURCE_LOCATION (decl), 0, "type %qT with no linkage " > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc > index e75be9a4e66..f1a23ffe817 100644 > --- a/gcc/cp/tree.cc > +++ b/gcc/cp/tree.cc > @@ -2971,7 +2971,8 @@ verify_stmt_tree (tree t) > > /* Check if the type T depends on a type with no linkage and if so, > return it. If RELAXED_P then do not consider a class type declared > - within a vague-linkage function to have no linkage. Remember: > + within a vague-linkage function or in a module CMI to have no linkage, > + since it can still be accessed within a different TU. Remember: > no-linkage is not the same as internal-linkage. */ > > tree > @@ -3012,7 +3013,15 @@ no_linkage_check (tree t, bool relaxed_p) > /* Only treat unnamed types as having no linkage if they're at > namespace scope. This is core issue 966. */ > if (TYPE_UNNAMED_P (t) && TYPE_NAMESPACE_SCOPE_P (t)) > - return t; > + { > + if (relaxed_p > + && TREE_PUBLIC (CP_TYPE_CONTEXT (t)) > + && module_maybe_has_cmi_p ()) > + /* This type could possibly be accessed outside this TU. */ > + return NULL_TREE; > + else > + return t; > + } > > for (r = CP_TYPE_CONTEXT (t); ; ) > { > @@ -3023,10 +3032,12 @@ no_linkage_check (tree t, bool relaxed_p) > return no_linkage_check (TYPE_CONTEXT (t), relaxed_p); > else if (TREE_CODE (r) == FUNCTION_DECL) > { > - if (!relaxed_p || !vague_linkage_p (r)) > - return t; > - else > + if (relaxed_p > + && (vague_linkage_p (r) > + || (TREE_PUBLIC (r) && module_maybe_has_cmi_p ()))) > r = CP_DECL_CONTEXT (r); > + else > + return t; > } > else > break; > diff --git a/gcc/testsuite/g++.dg/cpp2a/linkage-1.C b/gcc/testsuite/g++.dg/cpp2a/linkage-1.C > new file mode 100644 > index 00000000000..888ed6fa5b5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/linkage-1.C > @@ -0,0 +1,18 @@ > +// { dg-do compile { target c++11 } } > + > +inline auto f() { > + struct A {}; > + return A{}; > +} > +decltype(f()) a(); // { dg-error "used but not defined" "" { target c++17_down } } > + > +auto g() { > + struct A {}; > + return A{}; > +} > +decltype(g()) b(); // { dg-error "used but never defined" } > + > +int main() { > + a(); > + b(); > +} > diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3.h b/gcc/testsuite/g++.dg/modules/block-decl-3.h > new file mode 100644 > index 00000000000..4b155eb0054 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/block-decl-3.h > @@ -0,0 +1,39 @@ > +// GMF > + > +// Non-inline function definitions in headers are a recipe for ODR violations, > +// but we should probably support that anyway as its not inherently wrong > +// if only ever included into the GMF of a single module. > + > +auto gmf_n_i() { > + struct X { void f() {} }; > + return X{}; > +} > + > +inline auto gmf_i_i() { > + struct X { void f() {} }; > + return X{}; > +} > + > +auto gmf_n_i_i() { > + struct X { > + auto f() { > + struct Y { > + void g() {} > + }; > + return Y{}; > + } > + }; > + return X{}; > +} > + > +inline auto gmf_i_i_i() { > + struct X { > + auto f() { > + struct Y { > + void g() {} > + }; > + return Y{}; > + } > + }; > + return X{}; > +} > diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3_a.C b/gcc/testsuite/g++.dg/modules/block-decl-3_a.C > new file mode 100644 > index 00000000000..8cb4dde74d1 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/block-decl-3_a.C > @@ -0,0 +1,157 @@ > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi mod } > + > +// Test that we can link various forms of local class functions. > +// Function names use i=inline, n=non-inline, for each nesting. > + > +module; > +#include "block-decl-3.h" > + > +export module mod; > + > +namespace { > + void internal() {} > +} > + > +// singly-nested > + > +export auto n_n() { > + internal(); > + struct X { void f() { internal(); } }; > + return X{}; > +} > + > +export auto n_i() { > + internal(); > + struct X { inline void f() {} }; > + return X{}; > +} > + > +export inline auto i_n() { > + // `f` is not inline here, so this is OK > + struct X { void f() { internal(); } }; > + return X{}; > +} > + > +export inline auto i_i() { > + struct X { inline void f() {} }; > + return X{}; > +} > + > + > +// doubly nested > + > +export auto n_n_n() { > + struct X { > + auto f() { > + struct Y { > + void g() { internal(); } > + }; > + return Y{}; > + } > + }; > + return X{}; > +} > + > +export auto n_i_n() { > + struct X { > + inline auto f() { > + struct Y { > + void g() { internal(); } > + }; > + return Y{}; > + } > + }; > + return X{}; > +} > + > +export inline auto i_n_i() { > + struct X { > + auto f() { > + struct Y { > + inline void g() {} > + }; > + return Y {}; > + } > + }; > + return X{}; > +} > + > +export inline auto i_i_i() { > + struct X { > + inline auto f() { > + struct Y { > + inline void g() {} > + }; > + return Y{}; > + } > + }; > + return X{}; > +} > + > + > +// multiple types > + > +export auto multi_n_n() { > + struct X { > + void f() { internal(); } > + }; > + struct Y { > + X x; > + }; > + return Y {}; > +} > + > +export auto multi_n_i() { > + struct X { > + inline void f() {} > + }; > + struct Y { > + X x; > + }; > + return Y {}; > +} > + > +export inline auto multi_i_i() { > + struct X { > + inline void f() {} > + }; > + struct Y { > + X x; > + }; > + return Y {}; > +}; > + > + > +// extern "C++" > + > +export extern "C++" auto extern_n_i() { > + struct X { > + void f() {} // implicitly inline > + }; > + return X{}; > +}; > + > +export extern "C++" inline auto extern_i_i() { > + struct X { > + void f() {} > + }; > + return X{}; > +}; > + > + > +// GMF > + > +export using ::gmf_n_i; > +export using ::gmf_i_i; > +export using ::gmf_n_i_i; > +export using ::gmf_i_i_i; > + > + > +// can access from implementation unit > + > +auto only_used_in_impl() { > + struct X { void f() {} }; > + return X{}; > +} > +export void test_from_impl_unit(); > diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3_b.C b/gcc/testsuite/g++.dg/modules/block-decl-3_b.C > new file mode 100644 > index 00000000000..bc9b2a213f0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/block-decl-3_b.C > @@ -0,0 +1,8 @@ > +// { dg-additional-options "-fmodules-ts" } > + > +module mod; > + > +// Test that we can access (and link) to declarations from the interface > +void test_from_impl_unit() { > + only_used_in_impl().f(); > +} > diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3_c.C b/gcc/testsuite/g++.dg/modules/block-decl-3_c.C > new file mode 100644 > index 00000000000..5b39e038327 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/block-decl-3_c.C > @@ -0,0 +1,30 @@ > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts" } > + > +import mod; > + > +int main() { > + n_n().f(); > + n_i().f(); > + i_n().f(); > + i_i().f(); > + > + n_n_n().f().g(); > + n_i_n().f().g(); > + i_n_i().f().g(); > + i_i_i().f().g(); > + > + multi_n_n().x.f(); > + multi_n_i().x.f(); > + multi_i_i().x.f(); > + > + extern_n_i().f(); > + extern_i_i().f(); > + > + gmf_n_i().f(); > + gmf_i_i().f(); > + gmf_n_i_i().f().g(); > + gmf_i_i_i().f().g(); > + > + test_from_impl_unit(); > +} > diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_a.C b/gcc/testsuite/g++.dg/modules/linkage-1_a.C > new file mode 100644 > index 00000000000..750e31ff347 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/linkage-1_a.C > @@ -0,0 +1,15 @@ > +// { dg-additional-options "-fmodules-ts -Wno-error=c++20-extensions" } > +// { dg-module-cmi M } > + > +export module M; > + > +auto f() { > + struct A {}; > + return A{}; > +} > +decltype(f()) g(); // { dg-warning "used but not defined" "" { target c++17_down } } > +export auto x = g(); > + > +struct {} s; > +decltype(s) h(); // { dg-warning "used but not defined" "" { target c++17_down } } > +export auto y = h(); > diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_b.C b/gcc/testsuite/g++.dg/modules/linkage-1_b.C > new file mode 100644 > index 00000000000..f23962d76b7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/linkage-1_b.C > @@ -0,0 +1,6 @@ > +// { dg-additional-options "-fmodules-ts" } > + > +module M; > + > +decltype(f()) g() { return {}; } > +decltype(s) h() { return {}; } > diff --git a/gcc/testsuite/g++.dg/modules/linkage-1_c.C b/gcc/testsuite/g++.dg/modules/linkage-1_c.C > new file mode 100644 > index 00000000000..f1406b99032 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/linkage-1_c.C > @@ -0,0 +1,9 @@ > +// { dg-module-do link } > +// { dg-additional-options "-fmodules-ts" } > + > +import M; > + > +int main() { > + auto a = x; > + auto b = y; > +} > diff --git a/gcc/testsuite/g++.dg/modules/linkage-2.C b/gcc/testsuite/g++.dg/modules/linkage-2.C > new file mode 100644 > index 00000000000..eb4d7b051af > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/linkage-2.C > @@ -0,0 +1,26 @@ > +// { dg-additional-options "-fmodules-ts" } > +// { dg-module-cmi !M } > + > +export module M; > + > +// Same as a linkage-1 except within an anonymous namespace; > +// now these declarations cannot possibly be defined outside this TU, > +// so we should error. > + > +namespace { > + auto f() { > + struct A {}; > + return A{}; > + } > + decltype(f()) g(); // { dg-error "used but never defined" } > + > + struct {} s; > + decltype(s) h(); // { dg-error "used but never defined" } > +} > + > +export void use() { > + g(); > + h(); > +} > + > +// { dg-prune-output "not writing module" } ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-19 0:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-20 9:47 [PATCH] c++: Check module attachment instead of purview when necessary [PR112631] Nathaniel Shead 2023-11-23 20:03 ` Nathan Sidwell 2023-11-27 4:59 ` Nathaniel Shead 2024-03-08 2:55 ` [PATCH v2] c++: Check module attachment instead of just " Nathaniel Shead 2024-03-08 15:19 ` Jason Merrill 2024-03-08 23:18 ` Nathaniel Shead 2024-03-11 18:13 ` Jason Merrill 2024-03-16 11:23 ` [PATCH v3] c++: Fix handling of no-linkage decls for modules Nathaniel Shead 2024-03-19 0:58 ` Jason Merrill
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).