Message ID | 672f1bca.050a0220.1f2cda.8a47@mx.google.com |
---|---|
State | New |
Headers | show |
Series | [v2,1/3] c++: Fix mangling of otherwise unattached class-scope lambdas [PR107741] | expand |
On 11/9/24 9:22 AM, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? Given > that this doesn't actually fix the modules PR c++/116568 anymore I've > pulled my workaround for that out as a separate patch (3/3). In general, mangling changes should depend on -fabi-version, and tests should verify both the old and new mangling. Likewise for patch 2/3. > This is a step closer to implementing the suggested changes for > https://github.com/itanium-cxx-abi/cxx-abi/pull/85. Most lambdas > defined within a class should have an extra scope of that class so that > uses across different TUs are properly merged by the linker. This also > needs to happen during template instantiation. > > While I was working on this I found some other cases where the mangling > of lambdas was incorrect and causing issues, notably the testcase > lambda-ctx3.C which currently emits the same mangling for the base class > and member lambdas, causing mysterious assembler errors. However, this > doesn't fix the A::x case of the linker PR at this time so I've left > that as an XFAIL. > > One notable case not handled either here or in the ABI is what is > supposed to happen with lambdas declared in alias templates; see > lambda-ctx4.C. I believe that by the C++ standard, such lambdas should > also dedup across TUs, but this isn't currently implemented (for > class-scope or not). I wasn't able to work out how to fix the mangling > logic for this case easily so I've just excluded alias templates from > the class-scope mangling rules in template instantiation. > > PR c++/107741 > > gcc/cp/ChangeLog: > > * cp-tree.h (LAMBDA_EXPR_EXTRA_SCOPE): Adjust comment. > * parser.cc (cp_parser_class_head): Start (and do not finish) > lambda scope for all valid types. > (cp_parser_class_specifier): Finish lambda scope after parsing > members instead. > (cp_parser_member_declaration): Adjust comment to mention > missing lambda scoping for static member initializers. > * pt.cc (instantiate_class_template): Add lambda scoping. > (instantiate_template): Likewise. > > gcc/testsuite/ChangeLog: > > * g++.dg/abi/lambda-ctx2.C: New test. > * g++.dg/abi/lambda-ctx3.C: New test. > * g++.dg/abi/lambda-ctx4.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/cp-tree.h | 3 ++- > gcc/cp/parser.cc | 31 ++++++++++++++--------- > gcc/cp/pt.cc | 14 ++++++++++- > gcc/testsuite/g++.dg/abi/lambda-ctx2.C | 34 ++++++++++++++++++++++++++ > gcc/testsuite/g++.dg/abi/lambda-ctx3.C | 21 ++++++++++++++++ > gcc/testsuite/g++.dg/abi/lambda-ctx4.C | 22 +++++++++++++++++ > 6 files changed, 111 insertions(+), 14 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx2.C > create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx3.C > create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx4.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index f98a1de42ca..f6cf1754d86 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -1513,7 +1513,8 @@ enum cp_lambda_default_capture_mode_type { > (((struct tree_lambda_expr *)LAMBDA_EXPR_CHECK (NODE))->locus) > > /* The mangling scope for the lambda: FUNCTION_DECL, PARM_DECL, VAR_DECL, > - FIELD_DECL or NULL_TREE. If this is NULL_TREE, we have no linkage. */ > + FIELD_DECL, TYPE_DECL, or NULL_TREE. If this is NULL_TREE, we have no > + linkage. */ > #define LAMBDA_EXPR_EXTRA_SCOPE(NODE) \ > (((struct tree_lambda_expr *)LAMBDA_EXPR_CHECK (NODE))->extra_scope) > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > index c1375ecdbb5..7f22384d8a7 100644 > --- a/gcc/cp/parser.cc > +++ b/gcc/cp/parser.cc > @@ -27107,6 +27107,8 @@ cp_parser_class_specifier (cp_parser* parser) > if (!braces.require_open (parser)) > { > pop_deferring_access_checks (); > + if (type != error_mark_node) > + finish_lambda_scope (); > return error_mark_node; > } > > @@ -27171,7 +27173,10 @@ cp_parser_class_specifier (cp_parser* parser) > if (cp_parser_allow_gnu_extensions_p (parser)) > attributes = cp_parser_gnu_attributes_opt (parser); > if (type != error_mark_node) > - type = finish_struct (type, attributes); > + { > + type = finish_struct (type, attributes); > + finish_lambda_scope (); > + } > if (nested_name_specifier_p) > pop_inner_scope (old_scope, scope); > > @@ -28011,6 +28016,12 @@ cp_parser_class_head (cp_parser* parser, > if (flag_concepts) > type = associate_classtype_constraints (type); > > + /* Lambdas in bases and members must have the same mangling scope for ABI. > + We open this scope now, and will close it in cp_parser_class_specifier > + after parsing the member list. */ > + if (type && type != error_mark_node) > + start_lambda_scope (TYPE_NAME (type)); > + > /* We will have entered the scope containing the class; the names of > base classes should be looked up in that context. For example: > > @@ -28025,16 +28036,10 @@ cp_parser_class_head (cp_parser* parser, > if (cp_lexer_next_token_is (parser->lexer, CPP_COLON)) > { > if (type) > - { > - pushclass (type); > - start_lambda_scope (TYPE_NAME (type)); > - } > + pushclass (type); > bases = cp_parser_base_clause (parser); > if (type) > - { > - finish_lambda_scope (); > - popclass (); > - } > + popclass (); > } > else > bases = NULL_TREE; > @@ -28700,9 +28705,11 @@ cp_parser_member_declaration (cp_parser* parser) > pure-specifier. It is not correct to parse the > initializer before registering the member declaration > since the member declaration should be in scope while > - its initializer is processed. However, the rest of the > - front end does not yet provide an interface that allows > - us to handle this correctly. */ > + its initializer is processed. And similarly, the ABI of > + lambdas declared in the initializer should be scoped to > + the member. However, the rest of the front end does not > + yet provide an interface that allows us to handle this > + correctly. */ > if (cp_lexer_next_token_is (parser->lexer, CPP_EQ)) > { > /* In [class.mem]: > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index f4213f88b99..06d13fda872 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -12508,6 +12508,10 @@ instantiate_class_template (tree type) > gcc_assert (!DECL_CLASS_SCOPE_P (TYPE_MAIN_DECL (pattern)) > || COMPLETE_OR_OPEN_TYPE_P (TYPE_CONTEXT (type))); > > + /* When instantiating nested lambdas, ensure that they get the mangling > + scope of the new class type. */ > + start_lambda_scope (TYPE_NAME (type)); > + > base_list = NULL_TREE; > /* Defer access checking while we substitute into the types named in > the base-clause. */ > @@ -12867,6 +12871,8 @@ instantiate_class_template (tree type) > finish_struct_1 (type); > TYPE_BEING_DEFINED (type) = 0; > > + finish_lambda_scope (); > + > /* Remember if instantiating this class ran into errors, so we can avoid > instantiating member functions in limit_bad_template_recursion. We set > this flag even if the problem was in another instantiation triggered by > @@ -22275,6 +22281,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain) > ctx = tsubst_entering_scope (DECL_CONTEXT (gen_tmpl), targ_ptr, > complain, gen_tmpl); > push_nested_class (ctx); > + if (!DECL_ALIAS_TEMPLATE_P (gen_tmpl)) > + start_lambda_scope (TYPE_NAME (ctx)); > } > > tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl); > @@ -22304,7 +22312,11 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain) > if (fndecl == NULL_TREE) > fndecl = tsubst_decl (pattern, targ_ptr, complain, /*use_spec_table=*/false); > if (DECL_CLASS_SCOPE_P (gen_tmpl)) > - pop_nested_class (); > + { > + if (!DECL_ALIAS_TEMPLATE_P (gen_tmpl)) > + finish_lambda_scope (); > + pop_nested_class (); > + } > pop_from_top_level (); > > if (fndecl == error_mark_node) > diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx2.C b/gcc/testsuite/g++.dg/abi/lambda-ctx2.C > new file mode 100644 > index 00000000000..26896105a6c > --- /dev/null > +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx2.C > @@ -0,0 +1,34 @@ > +// PR c++/107741 > +// { dg-do compile { target c++17 } } > + > +struct A { > + // We currently parse static member initializers for non-templates before we > + // see their decls, and so don't get the chance to attach it as scope. > + static constexpr auto x = []{ return 1; }; > +}; > + > +template <typename> > +struct B { > + static constexpr auto x = []{ return 2; }; > +}; > + > +template <typename> > +struct C { > + static int x; > +}; > + > +void side_effect(); > + > +template <typename T> > +int C<T>::x = (side_effect(), []{ return 3; }()); > + > +template int C<int>::x; > + > +void f() { > + A::x(); > + B<int>::x(); > +} > + > +// { dg-final { scan-assembler {_ZNK1A1xMUlvE_clEv:} { xfail *-*-* } } } > +// { dg-final { scan-assembler {_ZNK1BIiE1xMUlvE_clEv:} } } > +// { dg-final { scan-assembler {_ZNK1CIiE1xMUlvE_clEv:} } } > diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx3.C b/gcc/testsuite/g++.dg/abi/lambda-ctx3.C > new file mode 100644 > index 00000000000..f92f2500531 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx3.C > @@ -0,0 +1,21 @@ > +// { dg-do compile { target c++20 } } > +// { dg-additional-options "-fkeep-inline-functions" } > +// See also https://github.com/itanium-cxx-abi/cxx-abi/pull/85 > + > +struct A { > + decltype([]{ return 1; }) f; > +}; > + > +struct B : decltype([]{ return 2; }) { > + decltype([]{ return 3; }) f; > +}; > + > +struct C : decltype([]{ return 4; }) { > + decltype([]{ return 5; }) f; > +}; > + > +// { dg-final { scan-assembler {_ZNK1AUlvE_clEv:} } } > +// { dg-final { scan-assembler {_ZNK1BUlvE_clEv:} } } > +// { dg-final { scan-assembler {_ZNK1BUlvE0_clEv:} } } > +// { dg-final { scan-assembler {_ZNK1CUlvE_clEv:} } } > +// { dg-final { scan-assembler {_ZNK1CUlvE0_clEv:} } } > diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx4.C b/gcc/testsuite/g++.dg/abi/lambda-ctx4.C > new file mode 100644 > index 00000000000..d6544a84652 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx4.C > @@ -0,0 +1,22 @@ > +// { dg-do compile { target c++20 } } > +// { dg-additional-options "-fkeep-inline-functions" } > + > +struct S { > + template <int I> > + using T = decltype([]{ return I; }); > +}; > + > +S::T<0> a; > +S::T<1> b; > + > +int main() { > + a(); > + b(); > +} > + > +// Currently we don't implement any special mangling rules for template aliases > +// (though we probably should; an alias template is a definable item by > +// [basic.def.odr] p1.5 and as such contained lambdas in different TUs should have > +// the same type, see [basic.def.odr] p15.6) > +// { scan_assembler {_ZNK1SUlvE_clEv:} } > +// { scan_assembler {_ZNK1SUlvE0_clEv:} }
On Thu, Nov 21, 2024 at 07:51:55PM +0100, Jason Merrill wrote: > On 11/9/24 9:22 AM, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? Given > > that this doesn't actually fix the modules PR c++/116568 anymore I've > > pulled my workaround for that out as a separate patch (3/3). > > In general, mangling changes should depend on -fabi-version, and tests > should verify both the old and new mangling. Likewise for patch 2/3. > OK, thanks. Does this include C++20-only ABI-changes, such as for unevaluated lambdas or lambdas in template arguments? (Since my understanding is that we currently consider C++20 to be unstable.) If we're going to need to do this anyway I think I might wait until I can create actual correct manglings in all cases, not just this slightly better one (see [1] for where I got stuck last time I had a chance to look at this). Also FYI, due to some recent changes in life circumstances I do not currently have much time to make contributions, so I probably won't be able to work on this until next year. I haven't merged patch 3/3 because it turns out that it does depend on these patches to avoid regressions. [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-November/668298.html) > > This is a step closer to implementing the suggested changes for > > https://github.com/itanium-cxx-abi/cxx-abi/pull/85. Most lambdas > > defined within a class should have an extra scope of that class so that > > uses across different TUs are properly merged by the linker. This also > > needs to happen during template instantiation. > > > > While I was working on this I found some other cases where the mangling > > of lambdas was incorrect and causing issues, notably the testcase > > lambda-ctx3.C which currently emits the same mangling for the base class > > and member lambdas, causing mysterious assembler errors. However, this > > doesn't fix the A::x case of the linker PR at this time so I've left > > that as an XFAIL. > > > > One notable case not handled either here or in the ABI is what is > > supposed to happen with lambdas declared in alias templates; see > > lambda-ctx4.C. I believe that by the C++ standard, such lambdas should > > also dedup across TUs, but this isn't currently implemented (for > > class-scope or not). I wasn't able to work out how to fix the mangling > > logic for this case easily so I've just excluded alias templates from > > the class-scope mangling rules in template instantiation. > > > > PR c++/107741 > > > > gcc/cp/ChangeLog: > > > > * cp-tree.h (LAMBDA_EXPR_EXTRA_SCOPE): Adjust comment. > > * parser.cc (cp_parser_class_head): Start (and do not finish) > > lambda scope for all valid types. > > (cp_parser_class_specifier): Finish lambda scope after parsing > > members instead. > > (cp_parser_member_declaration): Adjust comment to mention > > missing lambda scoping for static member initializers. > > * pt.cc (instantiate_class_template): Add lambda scoping. > > (instantiate_template): Likewise. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/abi/lambda-ctx2.C: New test. > > * g++.dg/abi/lambda-ctx3.C: New test. > > * g++.dg/abi/lambda-ctx4.C: New test. > > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > > --- > > gcc/cp/cp-tree.h | 3 ++- > > gcc/cp/parser.cc | 31 ++++++++++++++--------- > > gcc/cp/pt.cc | 14 ++++++++++- > > gcc/testsuite/g++.dg/abi/lambda-ctx2.C | 34 ++++++++++++++++++++++++++ > > gcc/testsuite/g++.dg/abi/lambda-ctx3.C | 21 ++++++++++++++++ > > gcc/testsuite/g++.dg/abi/lambda-ctx4.C | 22 +++++++++++++++++ > > 6 files changed, 111 insertions(+), 14 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx2.C > > create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx3.C > > create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx4.C > > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > index f98a1de42ca..f6cf1754d86 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -1513,7 +1513,8 @@ enum cp_lambda_default_capture_mode_type { > > (((struct tree_lambda_expr *)LAMBDA_EXPR_CHECK (NODE))->locus) > > /* The mangling scope for the lambda: FUNCTION_DECL, PARM_DECL, VAR_DECL, > > - FIELD_DECL or NULL_TREE. If this is NULL_TREE, we have no linkage. */ > > + FIELD_DECL, TYPE_DECL, or NULL_TREE. If this is NULL_TREE, we have no > > + linkage. */ > > #define LAMBDA_EXPR_EXTRA_SCOPE(NODE) \ > > (((struct tree_lambda_expr *)LAMBDA_EXPR_CHECK (NODE))->extra_scope) > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > > index c1375ecdbb5..7f22384d8a7 100644 > > --- a/gcc/cp/parser.cc > > +++ b/gcc/cp/parser.cc > > @@ -27107,6 +27107,8 @@ cp_parser_class_specifier (cp_parser* parser) > > if (!braces.require_open (parser)) > > { > > pop_deferring_access_checks (); > > + if (type != error_mark_node) > > + finish_lambda_scope (); > > return error_mark_node; > > } > > @@ -27171,7 +27173,10 @@ cp_parser_class_specifier (cp_parser* parser) > > if (cp_parser_allow_gnu_extensions_p (parser)) > > attributes = cp_parser_gnu_attributes_opt (parser); > > if (type != error_mark_node) > > - type = finish_struct (type, attributes); > > + { > > + type = finish_struct (type, attributes); > > + finish_lambda_scope (); > > + } > > if (nested_name_specifier_p) > > pop_inner_scope (old_scope, scope); > > @@ -28011,6 +28016,12 @@ cp_parser_class_head (cp_parser* parser, > > if (flag_concepts) > > type = associate_classtype_constraints (type); > > + /* Lambdas in bases and members must have the same mangling scope for ABI. > > + We open this scope now, and will close it in cp_parser_class_specifier > > + after parsing the member list. */ > > + if (type && type != error_mark_node) > > + start_lambda_scope (TYPE_NAME (type)); > > + > > /* We will have entered the scope containing the class; the names of > > base classes should be looked up in that context. For example: > > @@ -28025,16 +28036,10 @@ cp_parser_class_head (cp_parser* parser, > > if (cp_lexer_next_token_is (parser->lexer, CPP_COLON)) > > { > > if (type) > > - { > > - pushclass (type); > > - start_lambda_scope (TYPE_NAME (type)); > > - } > > + pushclass (type); > > bases = cp_parser_base_clause (parser); > > if (type) > > - { > > - finish_lambda_scope (); > > - popclass (); > > - } > > + popclass (); > > } > > else > > bases = NULL_TREE; > > @@ -28700,9 +28705,11 @@ cp_parser_member_declaration (cp_parser* parser) > > pure-specifier. It is not correct to parse the > > initializer before registering the member declaration > > since the member declaration should be in scope while > > - its initializer is processed. However, the rest of the > > - front end does not yet provide an interface that allows > > - us to handle this correctly. */ > > + its initializer is processed. And similarly, the ABI of > > + lambdas declared in the initializer should be scoped to > > + the member. However, the rest of the front end does not > > + yet provide an interface that allows us to handle this > > + correctly. */ > > if (cp_lexer_next_token_is (parser->lexer, CPP_EQ)) > > { > > /* In [class.mem]: > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > > index f4213f88b99..06d13fda872 100644 > > --- a/gcc/cp/pt.cc > > +++ b/gcc/cp/pt.cc > > @@ -12508,6 +12508,10 @@ instantiate_class_template (tree type) > > gcc_assert (!DECL_CLASS_SCOPE_P (TYPE_MAIN_DECL (pattern)) > > || COMPLETE_OR_OPEN_TYPE_P (TYPE_CONTEXT (type))); > > + /* When instantiating nested lambdas, ensure that they get the mangling > > + scope of the new class type. */ > > + start_lambda_scope (TYPE_NAME (type)); > > + > > base_list = NULL_TREE; > > /* Defer access checking while we substitute into the types named in > > the base-clause. */ > > @@ -12867,6 +12871,8 @@ instantiate_class_template (tree type) > > finish_struct_1 (type); > > TYPE_BEING_DEFINED (type) = 0; > > + finish_lambda_scope (); > > + > > /* Remember if instantiating this class ran into errors, so we can avoid > > instantiating member functions in limit_bad_template_recursion. We set > > this flag even if the problem was in another instantiation triggered by > > @@ -22275,6 +22281,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain) > > ctx = tsubst_entering_scope (DECL_CONTEXT (gen_tmpl), targ_ptr, > > complain, gen_tmpl); > > push_nested_class (ctx); > > + if (!DECL_ALIAS_TEMPLATE_P (gen_tmpl)) > > + start_lambda_scope (TYPE_NAME (ctx)); > > } > > tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl); > > @@ -22304,7 +22312,11 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain) > > if (fndecl == NULL_TREE) > > fndecl = tsubst_decl (pattern, targ_ptr, complain, /*use_spec_table=*/false); > > if (DECL_CLASS_SCOPE_P (gen_tmpl)) > > - pop_nested_class (); > > + { > > + if (!DECL_ALIAS_TEMPLATE_P (gen_tmpl)) > > + finish_lambda_scope (); > > + pop_nested_class (); > > + } > > pop_from_top_level (); > > if (fndecl == error_mark_node) > > diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx2.C b/gcc/testsuite/g++.dg/abi/lambda-ctx2.C > > new file mode 100644 > > index 00000000000..26896105a6c > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx2.C > > @@ -0,0 +1,34 @@ > > +// PR c++/107741 > > +// { dg-do compile { target c++17 } } > > + > > +struct A { > > + // We currently parse static member initializers for non-templates before we > > + // see their decls, and so don't get the chance to attach it as scope. > > + static constexpr auto x = []{ return 1; }; > > +}; > > + > > +template <typename> > > +struct B { > > + static constexpr auto x = []{ return 2; }; > > +}; > > + > > +template <typename> > > +struct C { > > + static int x; > > +}; > > + > > +void side_effect(); > > + > > +template <typename T> > > +int C<T>::x = (side_effect(), []{ return 3; }()); > > + > > +template int C<int>::x; > > + > > +void f() { > > + A::x(); > > + B<int>::x(); > > +} > > + > > +// { dg-final { scan-assembler {_ZNK1A1xMUlvE_clEv:} { xfail *-*-* } } } > > +// { dg-final { scan-assembler {_ZNK1BIiE1xMUlvE_clEv:} } } > > +// { dg-final { scan-assembler {_ZNK1CIiE1xMUlvE_clEv:} } } > > diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx3.C b/gcc/testsuite/g++.dg/abi/lambda-ctx3.C > > new file mode 100644 > > index 00000000000..f92f2500531 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx3.C > > @@ -0,0 +1,21 @@ > > +// { dg-do compile { target c++20 } } > > +// { dg-additional-options "-fkeep-inline-functions" } > > +// See also https://github.com/itanium-cxx-abi/cxx-abi/pull/85 > > + > > +struct A { > > + decltype([]{ return 1; }) f; > > +}; > > + > > +struct B : decltype([]{ return 2; }) { > > + decltype([]{ return 3; }) f; > > +}; > > + > > +struct C : decltype([]{ return 4; }) { > > + decltype([]{ return 5; }) f; > > +}; > > + > > +// { dg-final { scan-assembler {_ZNK1AUlvE_clEv:} } } > > +// { dg-final { scan-assembler {_ZNK1BUlvE_clEv:} } } > > +// { dg-final { scan-assembler {_ZNK1BUlvE0_clEv:} } } > > +// { dg-final { scan-assembler {_ZNK1CUlvE_clEv:} } } > > +// { dg-final { scan-assembler {_ZNK1CUlvE0_clEv:} } } > > diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx4.C b/gcc/testsuite/g++.dg/abi/lambda-ctx4.C > > new file mode 100644 > > index 00000000000..d6544a84652 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx4.C > > @@ -0,0 +1,22 @@ > > +// { dg-do compile { target c++20 } } > > +// { dg-additional-options "-fkeep-inline-functions" } > > + > > +struct S { > > + template <int I> > > + using T = decltype([]{ return I; }); > > +}; > > + > > +S::T<0> a; > > +S::T<1> b; > > + > > +int main() { > > + a(); > > + b(); > > +} > > + > > +// Currently we don't implement any special mangling rules for template aliases > > +// (though we probably should; an alias template is a definable item by > > +// [basic.def.odr] p1.5 and as such contained lambdas in different TUs should have > > +// the same type, see [basic.def.odr] p15.6) > > +// { scan_assembler {_ZNK1SUlvE_clEv:} } > > +// { scan_assembler {_ZNK1SUlvE0_clEv:} } >
On 11/27/24 4:12 AM, Nathaniel Shead wrote: > On Thu, Nov 21, 2024 at 07:51:55PM +0100, Jason Merrill wrote: >> On 11/9/24 9:22 AM, Nathaniel Shead wrote: >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? Given >>> that this doesn't actually fix the modules PR c++/116568 anymore I've >>> pulled my workaround for that out as a separate patch (3/3). >> >> In general, mangling changes should depend on -fabi-version, and tests >> should verify both the old and new mangling. Likewise for patch 2/3. > > OK, thanks. Does this include C++20-only ABI-changes, such as for > unevaluated lambdas or lambdas in template arguments? (Since my > understanding is that we currently consider C++20 to be unstable.) Good point, it isn't needed for changes that only affect experimental modes. So patch 2 doesn't need it. > If we're going to need to do this anyway I think I might wait until I > can create actual correct manglings in all cases, not just this slightly > better one (see [1] for where I got stuck last time I had a chance to > look at this). > > Also FYI, due to some recent changes in life circumstances I do not > currently have much time to make contributions, so I probably won't be > able to work on this until next year. Ah, sorry to hear that, but thanks for letting me know. Jason
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index f98a1de42ca..f6cf1754d86 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -1513,7 +1513,8 @@ enum cp_lambda_default_capture_mode_type { (((struct tree_lambda_expr *)LAMBDA_EXPR_CHECK (NODE))->locus) /* The mangling scope for the lambda: FUNCTION_DECL, PARM_DECL, VAR_DECL, - FIELD_DECL or NULL_TREE. If this is NULL_TREE, we have no linkage. */ + FIELD_DECL, TYPE_DECL, or NULL_TREE. If this is NULL_TREE, we have no + linkage. */ #define LAMBDA_EXPR_EXTRA_SCOPE(NODE) \ (((struct tree_lambda_expr *)LAMBDA_EXPR_CHECK (NODE))->extra_scope) diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index c1375ecdbb5..7f22384d8a7 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -27107,6 +27107,8 @@ cp_parser_class_specifier (cp_parser* parser) if (!braces.require_open (parser)) { pop_deferring_access_checks (); + if (type != error_mark_node) + finish_lambda_scope (); return error_mark_node; } @@ -27171,7 +27173,10 @@ cp_parser_class_specifier (cp_parser* parser) if (cp_parser_allow_gnu_extensions_p (parser)) attributes = cp_parser_gnu_attributes_opt (parser); if (type != error_mark_node) - type = finish_struct (type, attributes); + { + type = finish_struct (type, attributes); + finish_lambda_scope (); + } if (nested_name_specifier_p) pop_inner_scope (old_scope, scope); @@ -28011,6 +28016,12 @@ cp_parser_class_head (cp_parser* parser, if (flag_concepts) type = associate_classtype_constraints (type); + /* Lambdas in bases and members must have the same mangling scope for ABI. + We open this scope now, and will close it in cp_parser_class_specifier + after parsing the member list. */ + if (type && type != error_mark_node) + start_lambda_scope (TYPE_NAME (type)); + /* We will have entered the scope containing the class; the names of base classes should be looked up in that context. For example: @@ -28025,16 +28036,10 @@ cp_parser_class_head (cp_parser* parser, if (cp_lexer_next_token_is (parser->lexer, CPP_COLON)) { if (type) - { - pushclass (type); - start_lambda_scope (TYPE_NAME (type)); - } + pushclass (type); bases = cp_parser_base_clause (parser); if (type) - { - finish_lambda_scope (); - popclass (); - } + popclass (); } else bases = NULL_TREE; @@ -28700,9 +28705,11 @@ cp_parser_member_declaration (cp_parser* parser) pure-specifier. It is not correct to parse the initializer before registering the member declaration since the member declaration should be in scope while - its initializer is processed. However, the rest of the - front end does not yet provide an interface that allows - us to handle this correctly. */ + its initializer is processed. And similarly, the ABI of + lambdas declared in the initializer should be scoped to + the member. However, the rest of the front end does not + yet provide an interface that allows us to handle this + correctly. */ if (cp_lexer_next_token_is (parser->lexer, CPP_EQ)) { /* In [class.mem]: diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index f4213f88b99..06d13fda872 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -12508,6 +12508,10 @@ instantiate_class_template (tree type) gcc_assert (!DECL_CLASS_SCOPE_P (TYPE_MAIN_DECL (pattern)) || COMPLETE_OR_OPEN_TYPE_P (TYPE_CONTEXT (type))); + /* When instantiating nested lambdas, ensure that they get the mangling + scope of the new class type. */ + start_lambda_scope (TYPE_NAME (type)); + base_list = NULL_TREE; /* Defer access checking while we substitute into the types named in the base-clause. */ @@ -12867,6 +12871,8 @@ instantiate_class_template (tree type) finish_struct_1 (type); TYPE_BEING_DEFINED (type) = 0; + finish_lambda_scope (); + /* Remember if instantiating this class ran into errors, so we can avoid instantiating member functions in limit_bad_template_recursion. We set this flag even if the problem was in another instantiation triggered by @@ -22275,6 +22281,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain) ctx = tsubst_entering_scope (DECL_CONTEXT (gen_tmpl), targ_ptr, complain, gen_tmpl); push_nested_class (ctx); + if (!DECL_ALIAS_TEMPLATE_P (gen_tmpl)) + start_lambda_scope (TYPE_NAME (ctx)); } tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl); @@ -22304,7 +22312,11 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain) if (fndecl == NULL_TREE) fndecl = tsubst_decl (pattern, targ_ptr, complain, /*use_spec_table=*/false); if (DECL_CLASS_SCOPE_P (gen_tmpl)) - pop_nested_class (); + { + if (!DECL_ALIAS_TEMPLATE_P (gen_tmpl)) + finish_lambda_scope (); + pop_nested_class (); + } pop_from_top_level (); if (fndecl == error_mark_node) diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx2.C b/gcc/testsuite/g++.dg/abi/lambda-ctx2.C new file mode 100644 index 00000000000..26896105a6c --- /dev/null +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx2.C @@ -0,0 +1,34 @@ +// PR c++/107741 +// { dg-do compile { target c++17 } } + +struct A { + // We currently parse static member initializers for non-templates before we + // see their decls, and so don't get the chance to attach it as scope. + static constexpr auto x = []{ return 1; }; +}; + +template <typename> +struct B { + static constexpr auto x = []{ return 2; }; +}; + +template <typename> +struct C { + static int x; +}; + +void side_effect(); + +template <typename T> +int C<T>::x = (side_effect(), []{ return 3; }()); + +template int C<int>::x; + +void f() { + A::x(); + B<int>::x(); +} + +// { dg-final { scan-assembler {_ZNK1A1xMUlvE_clEv:} { xfail *-*-* } } } +// { dg-final { scan-assembler {_ZNK1BIiE1xMUlvE_clEv:} } } +// { dg-final { scan-assembler {_ZNK1CIiE1xMUlvE_clEv:} } } diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx3.C b/gcc/testsuite/g++.dg/abi/lambda-ctx3.C new file mode 100644 index 00000000000..f92f2500531 --- /dev/null +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx3.C @@ -0,0 +1,21 @@ +// { dg-do compile { target c++20 } } +// { dg-additional-options "-fkeep-inline-functions" } +// See also https://github.com/itanium-cxx-abi/cxx-abi/pull/85 + +struct A { + decltype([]{ return 1; }) f; +}; + +struct B : decltype([]{ return 2; }) { + decltype([]{ return 3; }) f; +}; + +struct C : decltype([]{ return 4; }) { + decltype([]{ return 5; }) f; +}; + +// { dg-final { scan-assembler {_ZNK1AUlvE_clEv:} } } +// { dg-final { scan-assembler {_ZNK1BUlvE_clEv:} } } +// { dg-final { scan-assembler {_ZNK1BUlvE0_clEv:} } } +// { dg-final { scan-assembler {_ZNK1CUlvE_clEv:} } } +// { dg-final { scan-assembler {_ZNK1CUlvE0_clEv:} } } diff --git a/gcc/testsuite/g++.dg/abi/lambda-ctx4.C b/gcc/testsuite/g++.dg/abi/lambda-ctx4.C new file mode 100644 index 00000000000..d6544a84652 --- /dev/null +++ b/gcc/testsuite/g++.dg/abi/lambda-ctx4.C @@ -0,0 +1,22 @@ +// { dg-do compile { target c++20 } } +// { dg-additional-options "-fkeep-inline-functions" } + +struct S { + template <int I> + using T = decltype([]{ return I; }); +}; + +S::T<0> a; +S::T<1> b; + +int main() { + a(); + b(); +} + +// Currently we don't implement any special mangling rules for template aliases +// (though we probably should; an alias template is a definable item by +// [basic.def.odr] p1.5 and as such contained lambdas in different TUs should have +// the same type, see [basic.def.odr] p15.6) +// { scan_assembler {_ZNK1SUlvE_clEv:} } +// { scan_assembler {_ZNK1SUlvE0_clEv:} }
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? Given that this doesn't actually fix the modules PR c++/116568 anymore I've pulled my workaround for that out as a separate patch (3/3). -- >8 -- This is a step closer to implementing the suggested changes for https://github.com/itanium-cxx-abi/cxx-abi/pull/85. Most lambdas defined within a class should have an extra scope of that class so that uses across different TUs are properly merged by the linker. This also needs to happen during template instantiation. While I was working on this I found some other cases where the mangling of lambdas was incorrect and causing issues, notably the testcase lambda-ctx3.C which currently emits the same mangling for the base class and member lambdas, causing mysterious assembler errors. However, this doesn't fix the A::x case of the linker PR at this time so I've left that as an XFAIL. One notable case not handled either here or in the ABI is what is supposed to happen with lambdas declared in alias templates; see lambda-ctx4.C. I believe that by the C++ standard, such lambdas should also dedup across TUs, but this isn't currently implemented (for class-scope or not). I wasn't able to work out how to fix the mangling logic for this case easily so I've just excluded alias templates from the class-scope mangling rules in template instantiation. PR c++/107741 gcc/cp/ChangeLog: * cp-tree.h (LAMBDA_EXPR_EXTRA_SCOPE): Adjust comment. * parser.cc (cp_parser_class_head): Start (and do not finish) lambda scope for all valid types. (cp_parser_class_specifier): Finish lambda scope after parsing members instead. (cp_parser_member_declaration): Adjust comment to mention missing lambda scoping for static member initializers. * pt.cc (instantiate_class_template): Add lambda scoping. (instantiate_template): Likewise. gcc/testsuite/ChangeLog: * g++.dg/abi/lambda-ctx2.C: New test. * g++.dg/abi/lambda-ctx3.C: New test. * g++.dg/abi/lambda-ctx4.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/cp-tree.h | 3 ++- gcc/cp/parser.cc | 31 ++++++++++++++--------- gcc/cp/pt.cc | 14 ++++++++++- gcc/testsuite/g++.dg/abi/lambda-ctx2.C | 34 ++++++++++++++++++++++++++ gcc/testsuite/g++.dg/abi/lambda-ctx3.C | 21 ++++++++++++++++ gcc/testsuite/g++.dg/abi/lambda-ctx4.C | 22 +++++++++++++++++ 6 files changed, 111 insertions(+), 14 deletions(-) create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx2.C create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx3.C create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx4.C