Page MenuHomePhabricator

Update the linkage name of coro-split functions where applicable
ClosedPublic

Authored by aprantl on Mar 31 2021, 4:52 PM.

Details

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.

Diff Detail

Event Timeline

aprantl created this revision.Mar 31 2021, 4:52 PM
aprantl requested review of this revision.Mar 31 2021, 4:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 4:52 PM
kastiglione added inline comments.
llvm/lib/Transforms/Coroutines/CoroSplit.cpp
866

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

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.

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 added inline comments.Apr 1 2021, 9:56 AM
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 added inline comments.Apr 1 2021, 6:57 PM
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 updated this revision to Diff 336145.Apr 8 2021, 9:10 AM

Sorry for the long delay! Address review feedback.

ChuanqiXu accepted this revision.Apr 8 2021, 6:43 PM

LGTM.

This revision is now accepted and ready to land.Apr 8 2021, 6:43 PM

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?

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.

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?

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.

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".

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?

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.

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 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.