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 <nathanielosh...@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:} } >
