Re: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631]
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{};
}
Re: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631]
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:
Re: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631]
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
[PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631]
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
---
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 ()
