| Message ID | 65ea7e1e.630a0220.d1c27.476d@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] c++: Check module attachment instead of just purview when necessary [PR112631] | expand |
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
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
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
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" }