[v3,2/6] c++: Fix mangling of otherwise unattached class-scope lambdas [PR118245]

Message ID 677bcadf.620a0220.3667ef.5712@mx.google.com
State New
Headers
Series c++: Add some missing LAMBDA_EXPR_EXTRA_SCOPEs |

Commit Message

Nathaniel Shead Jan. 6, 2025, 12:21 p.m. UTC
  Something like this should probably be backported to GCC 14 too, since
my change in r14-9232-g3685fae23bb008 inadvertantly caused ICEs that
this fixes.  But without the previous patch this patch will cause ABI
changes, and I'm not sure how easily it would be to divorce those
changes from the fix here.

I suppose probably the true issue is that r14-9232 inadvertantly changed
ABI for lambdas in base classes, and we should just revert the parser.cc
changes for 14.3?  (And accept that it'll regress those modules tests.)

-- >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 since r14-9232.
This is also the root cause of PR c++/118245.

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.

Since this should only affect usage of lambdas in unevaluated contexts
(a C++20 feature) this patch does not add an ABI flag to control this
behaviour.

	PR c++/118245

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-ctx3.C: New test.
	* g++.dg/abi/lambda-ctx4.C: New test.
	* g++.dg/cpp2a/lambda-uneval20.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                             |  3 ++-
 gcc/cp/parser.cc                             | 23 ++++++++++++--------
 gcc/cp/pt.cc                                 | 14 +++++++++++-
 gcc/testsuite/g++.dg/abi/lambda-ctx3.C       | 21 ++++++++++++++++++
 gcc/testsuite/g++.dg/abi/lambda-ctx4.C       | 22 +++++++++++++++++++
 gcc/testsuite/g++.dg/cpp2a/lambda-uneval20.C |  7 ++++++
 6 files changed, 79 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx3.C
 create mode 100644 gcc/testsuite/g++.dg/abi/lambda-ctx4.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/lambda-uneval20.C
  

Comments

Jason Merrill Jan. 23, 2025, 3:03 a.m. UTC | #1
On 1/6/25 7:21 AM, Nathaniel Shead wrote:
> Something like this should probably be backported to GCC 14 too, since
> my change in r14-9232-g3685fae23bb008 inadvertantly caused ICEs that
> this fixes.  But without the previous patch this patch will cause ABI
> changes, and I'm not sure how easily it would be to divorce those
> changes from the fix here.
> 
> I suppose probably the true issue is that r14-9232 inadvertantly changed
> ABI for lambdas in base classes, and we should just revert the parser.cc
> changes for 14.3?  (And accept that it'll regress those modules tests.)

I suppose so.

> -- >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 since r14-9232.
> This is also the root cause of PR c++/118245.
> 
> 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.

I'm skeptical of the instantiate_template change for other member 
templates as well, since the set of instantiations of such member 
templates might vary between translation units, so the lambda should be 
in the scope of that member template instantiation rather than just the 
class.

And I don't see any other member templates in the testcases?  Like, for

struct S {
   template <int I>
   decltype([]{ return I; }) f() { return {}; };
};

void f(decltype(S().f<24>())*) {}
void f(decltype(S().f<42>())*) {}

how are these lambdas mangled?  Before this patch, the two lambdas have 
arbitrary discriminators in S but they are treated as TU-local so it 
doesn't matter.  After this patch, they get the same number and are no 
longer TU-local, and assembly fails.

As with the member alias template example, I think it would be coherent 
to say the lambdas are TU-local because they're between a template 
parameter scope and a class/function/initializer, as a clarification to 
https://eel.is/c++draft/basic#link-15.2 .  Or we need to mangle them in 
the scope of the member template instantiation even though they aren't 
within the body of the function.

Interestingly, clang mangles them in the scope of S::f, without any 
template arguments.  That seems to have the same problem as just using 
S.  And they aren't treated as TU-local, so the arbitrary discriminators 
are an ABI problem.

Jason
  
Nathaniel Shead Jan. 23, 2025, 10:02 a.m. UTC | #2
On Wed, Jan 22, 2025 at 10:03:40PM -0500, Jason Merrill wrote:
> On 1/6/25 7:21 AM, Nathaniel Shead wrote:
> > Something like this should probably be backported to GCC 14 too, since
> > my change in r14-9232-g3685fae23bb008 inadvertantly caused ICEs that
> > this fixes.  But without the previous patch this patch will cause ABI
> > changes, and I'm not sure how easily it would be to divorce those
> > changes from the fix here.
> > 
> > I suppose probably the true issue is that r14-9232 inadvertantly changed
> > ABI for lambdas in base classes, and we should just revert the parser.cc
> > changes for 14.3?  (And accept that it'll regress those modules tests.)
> 
> I suppose so.
> 

OK, I'll send around a potential patch at some point.

> > -- >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 since r14-9232.
> > This is also the root cause of PR c++/118245.
> > 
> > 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.
> 
> I'm skeptical of the instantiate_template change for other member templates
> as well, since the set of instantiations of such member templates might vary
> between translation units, so the lambda should be in the scope of that
> member template instantiation rather than just the class.
> 
> And I don't see any other member templates in the testcases?  Like, for
> 
> struct S {
>   template <int I>
>   decltype([]{ return I; }) f() { return {}; };
> };
> 
> void f(decltype(S().f<24>())*) {}
> void f(decltype(S().f<42>())*) {}
> 
> how are these lambdas mangled?  Before this patch, the two lambdas have
> arbitrary discriminators in S but they are treated as TU-local so it doesn't
> matter.  After this patch, they get the same number and are no longer
> TU-local, and assembly fails.
> 
> As with the member alias template example, I think it would be coherent to
> say the lambdas are TU-local because they're between a template parameter
> scope and a class/function/initializer, as a clarification to
> https://eel.is/c++draft/basic#link-15.2 .  Or we need to mangle them in the
> scope of the member template instantiation even though they aren't within
> the body of the function.
> 

Thanks for looking into this, I hadn't considered this at all (I'd
gotten caught up with the alias case...).  I think I agree that just
calling such lambdas TU-local is probably the most practical approach
right now, especially given we're in stage 4.  I'll send out a new
version of the patch series (without the alias template mangling from #3
of this series) that treats them as such.

Nathaniel

> Interestingly, clang mangles them in the scope of S::f, without any template
> arguments.  That seems to have the same problem as just using S.  And they
> aren't treated as TU-local, so the arbitrary discriminators are an ABI
> problem.
> 
> Jason
>
  

Patch

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index f77a325bdd0..824b7bb61ae 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -1562,7 +1562,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 dd986444449..c851185a86d 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -27313,6 +27313,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;
     }
 
@@ -27377,7 +27379,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);
 
@@ -28217,6 +28222,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:
 
@@ -28231,16 +28242,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;
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index dfaa8906a2c..cb234b53a55 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -12604,6 +12604,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.  */
@@ -12965,6 +12969,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
@@ -22530,6 +22536,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);
@@ -22559,7 +22567,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-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:} }
diff --git a/gcc/testsuite/g++.dg/cpp2a/lambda-uneval20.C b/gcc/testsuite/g++.dg/cpp2a/lambda-uneval20.C
new file mode 100644
index 00000000000..fa009f53ff2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/lambda-uneval20.C
@@ -0,0 +1,7 @@ 
+// PR c++/118245
+// { dg-do compile { target c++20 } }
+
+template<auto> struct Cask {};
+struct T1 : Cask<[]{}> {
+  Cask<[]{}> c{};
+};