PR58819: Correct linkage and mangling of lambdas in inline static member initializers

[dblaikie]

Summary

https://llvm.org/pr58819 - clang is giving an externally visible lambda in a static data member internal linkage and the wrong linkage name.

Looks like we should be classifying this case the same as a non-static data member, so far as I can tell from the ABI docs and template examples (seems like the non-template inline-defined case should be the same).

This is a change in ABI, but not sure it qualifies as an ABI break as far as Apple and Sony are concerned - do you folks want this change? (it should fix the example in the bug where a static member in such a lambda ends up bifurcated, and I don't /think/ it'll break existing code since the symbol was previously internal anyway)

Looks like GCC has got this mangling slightly wrong (so we'd still end up with GCC+Clang bifurcation of the local static in the lambda, function address inequality, etc) in that they miss the variable name in the mangling in the non-template case. GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107741

[probinson]

If this is specifically for C++17, I believe Sony doesn't officially support that yet although I am checking.

It looks like this is only part of the fix for #58819? The original report also had a static int n with an internal-linkage name.

[dblaikie]

In D138247#3936928, @probinson wrote:

If this is specifically for C++17, I believe Sony doesn't officially support that yet although I am checking.

Cool, thanks!

It looks like this is only part of the fix for #58819? The original report also had a static int n with an internal-linkage name.

Nah, it's the full fix - but the static member handling works correctly once you fix the type mangling itself, so the bug is in the type linkage/mangling, the static member was just the canary in the coal mine/a more visible effect of the bug. I could add test coverage for it, but by default I prefer to test more narrowly.

[mgorny]

From Gentoo perspective, correct is better, correct and matching GCC is the best (we support mixing compilers to some degree). I think it'd be best to wait for confirmation on GCC end.

[dblaikie]

In D138247#3942744, @mgorny wrote:

From Gentoo perspective, correct is better, correct and matching GCC is the best (we support mixing compilers to some degree). I think it'd be best to wait for confirmation on GCC end.

fair - if you know anyone over there who might be able to take a look/an interest, would be great if you could ping them/point them in that direction!

[dblaikie]

GCC bug hasn't got much traction, so I pinged it.

But probably still the right thing to continue with this fix, I think.

@rjmccall Could you give a quick glance/confirm my ABI understanding/diagnosis/direction here?

[dblaikie]

Ping

[efriedma]

The relevant text of the current Itanium ABI (which was updated in https://github.com/itanium-cxx-abi/cxx-abi/commit/d8e9d102c83f177970f0db6cc8bee170f2779bc1)

In the following contexts, however, the one-definition rule requires closure types in different translation units to "correspond":

• default arguments appearing in class definitions
• default member initializers
• the bodies of inline or templated functions
• the initializers of inline or templated variables

Could you update the references to the ABI document to use the new text?

Given the new rules, I think ContextKind::StaticDataMember shouldn't exist? It's not one of the given categories; should be subsumed by the existing categories.

[dblaikie]

Remove StaticDataMember and classify static member variables of templates as variable templates

[dblaikie]

Rename VariableTemplate classification to TemplatedVariable to match ABI wording

[dblaikie]

In D138247#4217189, @efriedma wrote:

The relevant text of the current Itanium ABI (which was updated in https://github.com/itanium-cxx-abi/cxx-abi/commit/d8e9d102c83f177970f0db6cc8bee170f2779bc1)

In the following contexts, however, the one-definition rule requires closure types in different translation units to "correspond":

• default arguments appearing in class definitions
• default member initializers
• the bodies of inline or templated functions
• the initializers of inline or templated variables

Could you update the references to the ABI document to use the new text?

Oh, yeah - good call. Done.

Given the new rules, I think ContextKind::StaticDataMember shouldn't exist? It's not one of the given categories; should be subsumed by the existing categories.

Fair - I assume the "initializers of inline or templated variables" is intended to cover the case of static member variables of class templates, though they aren't technically variable templates presumably they are "templated variables".

So I renamed the VariableTemplate kind to TemplatedVariable to match the ABI wording/semantics a bit better.

[efriedma]

LGTM

clang/lib/Sema/SemaLambda.cpp

Sema::getCurrentMangleNumberContext(const DeclContext *DC) {
   // Compute the context for allocating mangling numbers in the current
   // expression, if the ABI requires them.
   Decl *ManglingContextDecl = ExprEvalContexts.back().ManglingContextDecl;
 
   enum ContextKind {
     Normal,
     DefaultArgument,
     DataMember,
-    StaticDataMember,
     InlineVariable,
     VariableTemplate,
     Concept
   } Kind = Normal;
 
+  bool IsInNonspecializedTemplate =
+      inTemplateInstantiation() || CurContext->isDependentContext();
+
   // Default arguments of member function parameters that appear in a class
   // definition, as well as the initializers of data members, receive special
   // treatment. Identify them.
   if (ManglingContextDecl) {
     if (ParmVarDecl *Param = dyn_cast<ParmVarDecl>(ManglingContextDecl)) {
       if (const DeclContext *LexicalDC
           = Param->getDeclContext()->getLexicalParent())
         if (LexicalDC->isRecord())
           Kind = DefaultArgument;
     } else if (VarDecl *Var = dyn_cast<VarDecl>(ManglingContextDecl)) {
-      if (Var->getDeclContext()->isRecord())
-        Kind = StaticDataMember;
-      else if (Var->getMostRecentDecl()->isInline())
+      if (Var->getMostRecentDecl()->isInline())
         Kind = InlineVariable;
+      else if (Var->getDeclContext()->isRecord() && IsInNonspecializedTemplate)
+        Kind = VariableTemplate;
       else if (Var->getDescribedVarTemplate())
         Kind = VariableTemplate;
       else if (auto *VTS = dyn_cast<VarTemplateSpecializationDecl>(Var)) {
         if (!VTS->isExplicitSpecialization())
           Kind = VariableTemplate;
       }
     } else if (isa<FieldDecl>(ManglingContextDecl)) {
       Kind = DataMember;
     } else if (isa<ImplicitConceptSpecializationDecl>(ManglingContextDecl)) {
       Kind = Concept;
     }
   }
 
   // Itanium ABI [5.1.7]:
   //   In the following contexts [...] the one-definition rule requires closure
   //   types in different translation units to "correspond":
-  bool IsInNonspecializedTemplate =
-      inTemplateInstantiation() || CurContext->isDependentContext();
   switch (Kind) {
   case Normal: {
-    //  -- the bodies of non-exported nonspecialized template functions
-    //  -- the bodies of inline functions
+    //  -- the bodies of inline or templated functions
     if ((IsInNonspecializedTemplate &&
          !(ManglingContextDecl && isa<ParmVarDecl>(ManglingContextDecl))) ||
         isInInlineFunction(CurContext)) {
       while (auto *CD = dyn_cast<CapturedDecl>(DC))
         DC = CD->getParent();
       return std::make_tuple(&Context.getManglingNumberContext(DC), nullptr);
     }
 
     return std::make_tuple(nullptr, nullptr);
   }
 
   case Concept:
     // Concept definitions aren't code generated and thus aren't mangled,
     // however the ManglingContextDecl is important for the purposes of
     // re-forming the template argument list of the lambda for constraint
     // evaluation.
-  case StaticDataMember:
-    //  -- the initializers of nonspecialized static members of template classes
-    if (!IsInNonspecializedTemplate)
-      return std::make_tuple(nullptr, ManglingContextDecl);
-    // Fall through to get the current context.
-    [[fallthrough]];
-
   case DataMember:
-    //  -- the in-class initializers of class members
+    //  -- default member initializers
   case DefaultArgument:
     //  -- default arguments appearing in class definitions
   case InlineVariable:
-    //  -- the initializers of inline variables
   case VariableTemplate:
-    //  -- the initializers of templated variables
+    //  -- the initializers of inline or templated variables
     return std::make_tuple(
         &Context.getManglingNumberContext(ASTContext::NeedExtraManglingDecl,
                                           ManglingContextDecl),
         ManglingContextDecl);
   }
 
   llvm_unreachable("unexpected context");
 }

clang/test/CodeGenCXX/mangle-lambdas.cpp

void ft4() {
   };
   lc().mf();
 }
 template void ft4<int>();
 // CHECK: call noundef i32 @_ZZZ3ft4IiEvvEN2lc2mfEiEd_NKUlvE_clEv
 // CHECK-LABEL: define linkonce_odr noundef i32 @_ZZZ3ft4IiEvvEN2lc2mfEiEd_NKUlvE_clEv
 
 
+extern int ExternalVariable;
+struct StaticInlineMember {
+  static constexpr auto x = [] { return ExternalVariable; };
+};
+
+// CHECK-LABEL: define void @_Z23test_StaticInlineMemberv
+// CHECK: call {{.*}} @_ZNK18StaticInlineMember1xMUlvE_clEv
+void test_StaticInlineMember() {
+  StaticInlineMember::x();
+}
+
 // Check linkage of the various lambdas.
 // CHECK-LABEL: define linkonce_odr noundef i32 @_ZZ11inline_funciENKUlvE_clEv
 // CHECK: ret i32 1
 // CHECK-LABEL: define linkonce_odr noundef i32 @_ZZ11inline_funciENKUlvE0_clEv
 // CHECK: ret i32
 // CHECK-LABEL: define linkonce_odr noundef double @_ZZ11inline_funciENKUlvE1_clEv
 // CHECK: ret double
 // CHECK-LABEL: define linkonce_odr noundef i32 @_ZZ11inline_funciENKUliE_clEi