Page MenuHomePhabricator

Improve the debug info for coro-split .resume functions
ClosedPublic

Authored by aprantl on Feb 23 2021, 4:21 PM.

Details

Summary

This patch updates the scope line to point to the suspend point. This makes the first address in the function point to the first source line in the resume function rather than the function declaration. Without this the line table "jumps" from the beginning of the function to the suspend point at the beginning.

rdar://73386346

Diff Detail

Event Timeline

aprantl created this revision.Feb 23 2021, 4:21 PM
aprantl requested review of this revision.Feb 23 2021, 4:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2021, 4:21 PM

@dexonsmith I'm curious if you have any comment about the safety of updating a distinct DISubprogram right after cloning it. I looked into injecting the replacement code into CloneFunction/ValueMapper but that didn't look too appealing either.

aprantl updated this revision to Diff 325933.Feb 23 2021, 4:28 PM

Strip out unnecessary change.

Is this change only works for continuation and async ABIs? If so, what can we do for switch ABIs?

I'm wonder whether this also affects backtrace of coroutine?

@aprantl
hello, this patch might cause debug llvm crash, the backtrace is :
#0 0x00007ffff672f8af in raise () from /lib64/libc.so.6
#1 0x00007ffff67314aa in abort () from /lib64/libc.so.6
#2 0x00007ffff6727d37 in assert_fail_base () from /lib64/libc.so.6
#3 0x00007ffff6727de2 in
assert_fail () from /lib64/libc.so.6
#4 0x00000000063af226 in llvm::DwarfUnit::applySubprogramDefinitionAttributes (this=0x13e3f980, SP=0x13fdd8f0, SPDie=...) at /disk1/yifeng.dongyifeng/my_code/llvm/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1169
#5 0x00000000063af368 in llvm::DwarfUnit::applySubprogramAttributes (this=0x13e3f980, SP=0x13fdd8f0, SPDie=..., SkipSPAttributes=false) at /disk1/yifeng.dongyifeng/my_code/llvm/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1193
#6 0x0000000006435376 in llvm::DwarfCompileUnit::applySubprogramAttributesToDefinition (this=0x13e3f980, SP=0x13fdd8f0, SPDie=...) at /disk1/yifeng.dongyifeng/my_code/llvm/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1448

the following assert will be triggle if the link name dose not match decl name:

1169      assert(((LinkageName.empty() || DeclLinkageName.empty()) ||
1170              LinkageName == DeclLinkageName) &&
1171             "decl has a linkage name and it is different");
aprantl updated this revision to Diff 326537.Feb 25 2021, 3:48 PM

Thanks @dongAxis1944 that makes sense. In C++ all resume functions share a common declaration which is supposed to hold the linkage name. Since this change was cosmetic only anyway, I just removed it from the patch.

aprantl edited the summary of this revision. (Show Details)Feb 25 2021, 5:23 PM
ChuanqiXu added inline comments.Feb 25 2021, 6:05 PM
llvm/lib/Transforms/Coroutines/CoroSplit.cpp
849

Since this code only works under continuation and async ABIs. So it maybe better to add comment or add explicit guard-if to tell the semantic.

aprantl updated this revision to Diff 327963.Mar 3 2021, 4:30 PM

Improved comment as suggested.

ChuanqiXu accepted this revision.Mar 3 2021, 6:07 PM

This patch LGTM.

This revision is now accepted and ready to land.Mar 3 2021, 6:07 PM
This revision was landed with ongoing or failed builds.Mar 4 2021, 11:06 AM
This revision was automatically updated to reflect the committed changes.