Update the linkage name of coro-split functions where applicable

[aprantl]

Summary

This patch updates the linkage name in the DISubprogram of coro-split functions, which is particularly important for Swift, where the funclets have a special name mangling. This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

[kastiglione]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

866

Is !SP->getDeclaration() the implicit check for swift but not C++?

[aprantl]

This is an improved version of a patch that used to be part of https://reviews.llvm.org/D97345 but was removed because it didn't work for C++.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

866

Yes. If you have a top-level C++ coroutine — and I have no idea if that's a thing, you would get the "real" linkage name.

[ChuanqiXu]

I am curious about the test case for coro-debug.ll. The ABI used in this test case is Switch ABI. And this patch still affects it.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

866

Does this mean the DISubprogram for every C++ coroutine did contain a declaration? May I ask where the constraint is?

[aprantl]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

866

The problem we're working around here is (as far as I understand) a limitation of LLVM itself. The distinct DISubprograms for C++ *methods* always have a a declaration: pointing into the uniqued DISubprogram in the type system that contains their abstract declaration. This is roughly equivalent to the DW_AT_specification in the DWARF for the concrete method pointing to the abstract declaration that holds all the shared attributes.

When emitting a DW_AT_specification attribute AsmPrinter asserts that all attributes in the distinct DISubprogram are equivalent to the ones in the abstract DISubprogram declaration. That's why we can't modify the linkage name for a DISubprogram that had a declaration: specified, because it would clash with the linkage name in the specification.

0x200:
DW_TAG_subprogram // abstract decl
  DW_AT_linkage_name (foo)
  DW_TAG_formal_paramater ...
  ...

DW_TAG_subprogram // concrete instance 1
  DW_AT_low_pc (0x1000)
  ...
  DW_AT_specification (0x200) // points to decl for common  bits

DW_TAG_subprogram // concrete instance 2
  DW_AT_low_pc (0x1500)
  ...
  DW_AT_specification (0x200) // points to decl for common  bits

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

866

Got it. I am prefer to add guard if or assertion to make the semantics explicitly. Like:

-     if (!SP->getDeclaration())
+    if (!SP->getDeclaration() && Shape.ABI == coro::ABI::Retcon)

or:

+   assert(Shape.ABI == coro::ABI::Retcon && "We should only update a linkageName for Swift.");
     SP->replaceLinkageName(MDString::get(Context, NewF->getName()));

I am not sure about the constraint Shape.ABI == coro::ABI::Retcon since MLIR would also use this code.

[aprantl]

Sorry for the long delay! Address review feedback.

[ChuanqiXu]

LGTM.

[dblaikie]

This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

So does this mean that for C++ we'll have a DW_TAG_subprogram with a DW_AT_linkage_name that doesn't match the symbol for that code in the object's symbol table?

[aprantl]

In D99693#2687996, @dblaikie wrote:

This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

So does this mean that for C++ we'll have a DW_TAG_subprogram with a DW_AT_linkage_name that doesn't match the symbol for that code in the object's symbol table?

Correct — and I would say this is probably a bug, unless we want DW_AT_linkage_name to show the "original" linkage name for purposes of matching breakpoints refering to the original name without the .resume suffix or something like that.

[dblaikie]

In D99693#2706501, @aprantl wrote:

In D99693#2687996, @dblaikie wrote:

This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

So does this mean that for C++ we'll have a DW_TAG_subprogram with a DW_AT_linkage_name that doesn't match the symbol for that code in the object's symbol table?

Correct — and I would say this is probably a bug, unless we want DW_AT_linkage_name to show the "original" linkage name for purposes of matching breakpoints refering to the original name without the .resume suffix or something like that.

I'm a bit confused then.

since the DW_AT_specification is expected to hold the (original) linkage name.

What's this claim ^ about/based on? What happens if it doesn't hold the original linkage name?

@tmsriram - I think you hit a problem with debug info linkage names not matching the symbol names in the unique linkage name work. Do you recall what the problem was when those things diverged?

[aprantl]

What's this claim ^ about/based on? What happens if it doesn't hold the original linkage name?

See the comment I left on line 861:

// Update the linkage name to reflect the modified symbol name. It
// is necessary to update the linkage name in Swift, since the
// mangling changes for resume functions. It might also be the
// right thing to do in C++, but due to a limitation in LLVM's
// AsmPrinter we can only do this if the function doesn't have an
// abstract specification, since the DWARF backend expects the
// abstract specification to contain the linkage name and asserts
// that they are identical.

I believe this is primarily a limitation of DwarfDebug.cpp in AsmPrinter and not required by any tools. But contrary to Swift, where resume funclets actually have different mangled names with rich information in the mangled names, in C++ we are just slapping a .resume.[0-9]+ at the end, and I don't know how useful that is to preserve.

[tmsriram]

In D99693#2706719, @dblaikie wrote:

In D99693#2706501, @aprantl wrote:

In D99693#2687996, @dblaikie wrote:

This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

So does this mean that for C++ we'll have a DW_TAG_subprogram with a DW_AT_linkage_name that doesn't match the symbol for that code in the object's symbol table?

Correct — and I would say this is probably a bug, unless we want DW_AT_linkage_name to show the "original" linkage name for purposes of matching breakpoints refering to the original name without the .resume suffix or something like that.

I'm a bit confused then.

since the DW_AT_specification is expected to hold the (original) linkage name.

What's this claim ^ about/based on? What happens if it doesn't hold the original linkage name?

@tmsriram - I think you hit a problem with debug info linkage names not matching the symbol names in the unique linkage name work. Do you recall what the problem was when those things diverged?

TLDR; breakpoints on these functions set with "b foo" for function foo will no longer work.

In the unique linkage names work, we originally appended a suffix that could have numeric and alphanumeric characters mixed. Such suffixes are not demangler friendly.
IIRC, In the presence of DW_AT_linkage_name, the debugger uses that when setting a breakpoint rather than DW_AT_name. If a function foo had a linkage name say "_ZXXfoo" and didn't demangle, the debugger will not honor "b foo".

[dblaikie]

In D99693#2706885, @tmsriram wrote:

In D99693#2706719, @dblaikie wrote:

In D99693#2706501, @aprantl wrote:

In D99693#2687996, @dblaikie wrote:

This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

So does this mean that for C++ we'll have a DW_TAG_subprogram with a DW_AT_linkage_name that doesn't match the symbol for that code in the object's symbol table?

Correct — and I would say this is probably a bug, unless we want DW_AT_linkage_name to show the "original" linkage name for purposes of matching breakpoints refering to the original name without the .resume suffix or something like that.

I'm a bit confused then.

since the DW_AT_specification is expected to hold the (original) linkage name.

What's this claim ^ about/based on? What happens if it doesn't hold the original linkage name?

@tmsriram - I think you hit a problem with debug info linkage names not matching the symbol names in the unique linkage name work. Do you recall what the problem was when those things diverged?

TLDR; breakpoints on these functions set with "b foo" for function foo will no longer work.

In the unique linkage names work, we originally appended a suffix that could have numeric and alphanumeric characters mixed. Such suffixes are not demangler friendly.
IIRC, In the presence of DW_AT_linkage_name, the debugger uses that when setting a breakpoint rather than DW_AT_name. If a function foo had a linkage name say "_ZXXfoo" and didn't demangle, the debugger will not honor "b foo".

Ah, so that's more about whether the name was demangleable - but I thought there was also an issue when the DW_AT_linkage_name differed from the real mangled name of the symbol, even if both names could be demangled?

[aprantl]

Ah, so that's more about whether the name was demangleable - but I thought there was also an issue when the DW_AT_linkage_name differed from the real mangled name of the symbol, even if both names could be demangled?

The issue I'm working around is that AsmPrinter expects the DW_AT_specification to have the canonical DW_AT_linkage_name for all instances of a function. DW_AT_linkage_name is emitted in the abstract specification and omitted in all instances. And because of this assumption it asserts that the linkage name of each instance DISubprogram is identical to the specification DISubprogram.

[aprantl]

I believe that this is purely a limitation of AsmPrinter and that we could probably change its behavior to emit different linkage names in different instances, but I have not looked at what various debuggers expect here and DWARF GCC emits for C++ coroutines. I was mostly focussed on getting the Swift case right, while not disturbing C++.

[dblaikie]

In D99693#2709931, @aprantl wrote:

I believe that this is purely a limitation of AsmPrinter and that we could probably change its behavior to emit different linkage names in different instances, but I have not looked at what various debuggers expect here and DWARF GCC emits for C++ coroutines. I was mostly focussed on getting the Swift case right, while not disturbing C++.

I'm with you there - I'm trying to remember/understand/gather that there's actually a problem with linkage names not matching DW_AT_linkage_names, so we know how to weigh the costs here/know what's broken and needs fixing.

I'm not sure what the right longer term path is - I haven't looked at what coroutine DWARF looks like, why we have (multiple?) functions using the same DW_AT_subprogram declaration? or why the functions name needs to change if it's not multiple of them... & how a DWARF consumer would deal with all that, whatever it is.

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
   //
   // Returns true if this subprogram is a thunk generated by the compiler.
   bool isThunk() const { return getFlags() & FlagThunk; }
 
   DIScope *getScope() const { return cast_or_null<DIScope>(getRawScope()); }
 
   StringRef getName() const { return getStringOperand(2); }
   StringRef getLinkageName() const { return getStringOperand(3); }
+  /// Only used by clients of CloneFunction, and only right after the cloning.
+  void replaceLinkageName(MDString *LN) { replaceOperandWith(3, LN); }
 
   DISubroutineType *getType() const {
     return cast_or_null<DISubroutineType>(getRawType());
   }
   DIType *getContainingType() const {
     return cast_or_null<DIType>(getRawContainingType());
   }
 

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

void CoroCloner::create() {
   // NewF's linkage (which CloneFunctionInto does *not* change) might not
   // be compatible with the visibility of OrigF (which it *does* change),
   // so protect against that.
   auto savedLinkage = NewF->getLinkage();
   NewF->setLinkage(llvm::GlobalValue::ExternalLinkage);
 
   CloneFunctionInto(NewF, &OrigF, VMap,
                     CloneFunctionChangeType::LocalChangesOnly, Returns);
+
+  auto &Context = NewF->getContext();
+
   // For async functions / continuations, adjust the scope line of the
   // clone to the line number of the suspend point. The scope line is
   // associated with all pre-prologue instructions. This avoids a jump
   // in the linetable from the function declaration to the suspend point.
   if (DISubprogram *SP = NewF->getSubprogram()) {
     assert(SP != OrigF.getSubprogram() && SP->isDistinct());
     if (ActiveSuspend)
       if (auto DL = ActiveSuspend->getDebugLoc())
         SP->setScopeLine(DL->getLine());
+    // Update the linkage name to reflect the modified symbol name. It
+    // is necessary to update the linkage name in Swift, since the
+    // mangling changes for resume functions. It might also be the
+    // right thing to do in C++, but due to a limitation in LLVM's
+    // AsmPrinter we can only do this if the function doesn't have an
+    // abstract specification, since the DWARF backend expects the
+    // abstract specification to contain the linkage name and asserts
+    // that they are identical.
+    if (!SP->getDeclaration() && SP->getUnit() &&
+        SP->getUnit()->getSourceLanguage() == dwarf::DW_LANG_Swift)
+      SP->replaceLinkageName(MDString::get(Context, NewF->getName()));
   }
 
   NewF->setLinkage(savedLinkage);
   NewF->setVisibility(savedVisibility);
   NewF->setUnnamedAddr(savedUnnamedAddr);
   NewF->setDLLStorageClass(savedDLLStorageClass);
 
-  auto &Context = NewF->getContext();
-
   // Replace the attributes of the new function:
   auto OrigAttrs = NewF->getAttributes();
   auto NewAttrs = AttributeList();
 
   switch (Shape.ABI) {
   case coro::ABI::Switch:
     // Bootstrap attributes by copying function attributes from the
     // original function.  This should include optimization settings and so on.

llvm/test/Transforms/Coroutines/coro-async.ll

 ; CHECK: ![[SP1]] = distinct !DISubprogram(name: "my_async_function",
 ; CHECK-SAME:                              linkageName: "my_async_function",
 ; CHECK-SAME:                              scopeLine: 1
 !1 = distinct !DISubprogram(name: "my_async_function",
                             linkageName: "my_async_function",
                             scope: !2, file: !3, line: 1, type: !4,
                             scopeLine: 1, spFlags: DISPFlagDefinition, unit: !2)
 ; CHECK: ![[SP2]] = distinct !DISubprogram(name: "my_async_function",
-; CHECK-SAME:                              linkageName: "my_async_function",
+; CHECK-SAME:                              linkageName: "my_async_function.resume.0",
 ; CHECK-SAME:                              scopeLine: 2
 !2 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !3, emissionKind: FullDebug)
 !3 = !DIFile(filename: "/tmp/1.swift", directory: "/")
 !4 = !DISubroutineType(types: !{})
 !5 = !DILocation(line: 2, column: 0, scope: !1)
 

llvm/test/Transforms/Coroutines/coro-debug.ll

 !llvm.ident = !{!5}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 5.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
 !1 = !DIFile(filename: "<stdin>", directory: "C:\5CGitHub\5Cllvm\5Cbuild\5CDebug\5Cbin")
 !2 = !{}
 !3 = !{i32 2, !"Dwarf Version", i32 4}
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 !5 = !{!"clang version 5.0.0"}
-!6 = distinct !DISubprogram(name: "f", linkageName: "flink", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, flags: DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !2)
+!6 = distinct !DISubprogram(name: "f", linkageName: "flink", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, flags: DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !2, declaration: !DISubprogram(name: "f", linkageName: "flink", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: false, flags: DIFlagPrototyped))
 !7 = !DIFile(filename: "simple-repro.c", directory: "C:\5CGitHub\5Cllvm\5Cbuild\5CDebug\5Cbin")
 !8 = !DISubroutineType(types: !9)
 !9 = !{!10, !11}
 !10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
 !11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
 !12 = !DILocalVariable(name: "x", arg: 1, scope: !6, file: !7, line: 55, type: !11)
 !13 = !DIExpression()
 !14 = !DILocation(line: 55, column: 13, scope: !6)