Salvage debug info for function arguments in coro-split funclets.

[aprantl]

Summary

This patch improves the availability for variables stored in the coroutine frame by emitting an alloca to hold the pointer to the frame object and rewriting dbg.declare intrinsics to point inside the frame object using salvaged DIExpressions. Finally, a new alloca is created in the funclet to hold the FramePtr pointer to ensure that it is available throughout the entire function at -O0.

rdar://71866936

[aprantl]

Fix a potential cast error.

[junparser]

Thanks for the patch! Although I may not the right person to accept the patch. BTW, Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

[dongAxis1944]

I think the patch might occur an error when compilling the following code (commit id of clang is 9d70dbdc2bf294abffd4b2c9ae524055f72d017a).

// clang++ -fcoroutines-ts  -std=c++2a coro-debug-1.cpp  -O0 -g -o e2.out
struct test {
    int i;
    int j;
    double k;
};

struct coro {
  ...
};

coro foo(struct test & t) {
  int i = 0;
  int j = 0;
  i = t.i + t.j + t.k;
  printf("%d\n", i);

  co_await suspend_always();
  printf("%d, %d, %d\n", i, j, t.i); // --> A

  co_await suspend_always();
  ++i;
  ++j;
  printf("%d, %d, %d\n", j, i, t.k); 

  co_await suspend_always();
  printf("%d \n", j);
}

int main(int argc, char *argv[]) {
  struct test t = {0xee,0xff, 1.0};
  auto c = foo(t);
  c.handle.resume();
  c.handle.resume();
  c.handle.resume();
}

when compiling finished, I use the debugger to load the elf and print variable named t:
lldb (commit id is 9d70dbdc2bf294abffd4b2c9ae524055f72d017a) prints:

Process 106638 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.2
    frame #0: 0x0000000000401bd0 a.out`foo(t=0x0000000000000000) at coro-debug-1.cpp:39:26
   36     //   (int) i = 1
   37
   38     co_await suspend_always();
-> 39     printf("%d, %d, %d\n", i, j, t.i); // 2, 1
                                 ^
   40     // Breakpoint 2:
   41     //   (lldb) frame variable i
   42     //   (int) i = <variable not available>
(lldb) p t
error: Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory: 0

According to the output of lldb, i think it might be wrong when lldb read dwarf,
so I download the dwarf info of this elf:

0x00002319:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000401e90)
                DW_AT_high_pc   (0x0000000000402490)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_linkage_name      ("_Z3fooR4test")
                DW_AT_name      ("foo")
                DW_AT_decl_file ("/disk1/yifeng.dongyifeng/debug_coro/debug/debug1/coro-debug-1.cpp")
                DW_AT_decl_line (29)
                DW_AT_type      (0x00000106 "coro")
                DW_AT_external  (true)

0x00002336:     DW_TAG_formal_parameter
                  DW_AT_location        (DW_OP_fbreg -128)
                  DW_AT_name    ("t")
                  DW_AT_decl_file       ("/disk1/yifeng.dongyifeng/debug_coro/debug/debug1/coro-debug-1.cpp")
                  DW_AT_decl_line       (29)
                  DW_AT_type    (0x00002442 "test&")

...

0x00002355:     DW_TAG_variable
                  DW_AT_location        (DW_OP_fbreg -8, DW_OP_deref, DW_OP_deref, DW_OP_plus_uconst 0x18)
                  DW_AT_name    ("t")
                  DW_AT_type    (0x00002442 "test&")
                  DW_AT_artificial      (true)

...

There are two variables named t in the same scope (named DW_TAG_subprogram),
and the patch just change the location list of the second one (because it attached to an alloc ), but not the first one.
When lldb wants to print variable t, it will get the first t, but unluckily it is not right.

[aprantl]

I think the patch might occur an error when compilling the following code (commit id of clang is 9d70dbdc2bf294abffd4b2c9ae524055f72d017a).

Could you give me a standalone example so I can reproduce this? I wrote this patch with Swift coroutines in mind and am not familiar enough with C++ coroutines to turn your example into something I can compile yet. Either source code or LLVM IR that I can pipe into opt would be fine.

thanks!
adrian

[aprantl]

Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

I was hoping that in an optimization pipeline the coroutine splitting would be invoked before mem2reg. This patch is really depending on the semantics of dbg.declare, which guarantees that the address of the variable doesn't change throughout the entire function. With a dbg.value we don't have any such guarantees, since it's just a snapshot at one particular location.

[dongAxis1944]

This comment was removed by dongAxis1944.

[dongAxis1944]

I write a simple patch which might be work under O2, but because we will not have any dbg.declare under O2 level, it might work in some locations. And I will submit it later

In D93497#2488109, @aprantl wrote:

Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

I was hoping that in an optimization pipeline the coroutine splitting would be invoked before mem2reg. This patch is really depending on the semantics of dbg.declare, which guarantees that the address of the variable doesn't change throughout the entire function. With a dbg.value we don't have any such guarantees, since it's just a snapshot at one particular location.

[junparser]

In D93497#2488109, @aprantl wrote:

Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

I was hoping that in an optimization pipeline the coroutine splitting would be invoked before mem2reg. This patch is really depending on the semantics of dbg.declare, which guarantees that the address of the variable doesn't change throughout the entire function. With a dbg.value we don't have any such guarantees, since it's just a snapshot at one particular location.

Before mem2reg may solve this issue, however, it may cause performance regression (I'm not sure about this) since we design the pipeline which heavily depend on cgscc. We may need fully test.

[dongAxis1944]

test.tar.gz88 KBDownload

please check the attached file.

In D93497#2488109, @aprantl wrote:

Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

I was hoping that in an optimization pipeline the coroutine splitting would be invoked before mem2reg. This patch is really depending on the semantics of dbg.declare, which guarantees that the address of the variable doesn't change throughout the entire function. With a dbg.value we don't have any such guarantees, since it's just a snapshot at one particular location.

[aprantl]

Thanks, I can reproduce the the issue! However, it looks like it reproduces even without my patch. The frontend (or perhaps CoroSplit?) apparently insert both a "t" function argument and a "t" local variable in the same scope:

(lldb) b CoroCloner::create()
(lldb) r
(lldb) p OrigF.getParent()->dump();
...
!1957 = !DILocalVariable(name: "t", arg: 2, scope: !1948, file: !883, line: 30, type: !1954)
...
!1965 = !DILocalVariable(name: "t", scope: !1948, type: !1954, flags: DIFlagArtificial)

It's seems just that with my patch both variables survive and show up in DWARF.

[dongAxis1944]

yes, clang will emit parameters of coroutine twice. so i use the patch to avoid this: https://reviews.llvm.org/D94388

and i think it might related the folowing code of clang:

CoroutineBodyStmt *CoroutineBodyStmt::Create(const ASTContext &C, EmptyShell,
                                             unsigned NumParams) {
  std::size_t Size = totalSizeToAlloc<Stmt *>(
      CoroutineBodyStmt::FirstParamMove + NumParams);

  void *Mem = C.Allocate(Size, alignof(CoroutineBodyStmt));
  auto *Result = new (Mem) CoroutineBodyStmt(CtorArgs());
  Result->NumParams = NumParams;
  auto *ParamBegin = Result->getStoredStmts() + SubStmt::FirstParamMove;
  std::uninitialized_fill(ParamBegin, ParamBegin + NumParams,
                          static_cast<Stmt *>(nullptr));
  return Result;
}

But i do not know why clang will do this.

In D93497#2491823, @aprantl wrote:

Thanks, I can reproduce the the issue! However, it looks like it reproduces even without my patch. The frontend (or perhaps CoroSplit?) apparently insert both a "t" function argument and a "t" local variable in the same scope:

(lldb) b CoroCloner::create()
(lldb) r
(lldb) p OrigF.getParent()->dump();
...
!1957 = !DILocalVariable(name: "t", arg: 2, scope: !1948, file: !883, line: 30, type: !1954)
...
!1965 = !DILocalVariable(name: "t", scope: !1948, type: !1954, flags: DIFlagArtificial)

It's seems just that with my patch both variables survive and show up in DWARF.

[aprantl]

It sounds like the bug @dongAxis1944 has found is unrelated to this patch. Should we move forward with this one, or are there any other problems you see?

[dongAxis1944]

I do not see any other problems, but if the code use c++ program, it will cause lldb read invalid address like I write before:

Process 106638 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.2
    frame #0: 0x0000000000401bd0 a.out`foo(t=0x0000000000000000) at coro-debug-1.cpp:39:26
   36     //   (int) i = 1
   37
   38     co_await suspend_always();
-> 39     printf("%d, %d, %d\n", i, j, t.i); // 2, 1
                                 ^
   40     // Breakpoint 2:
   41     //   (lldb) frame variable i
   42     //   (int) i = <variable not available>
(lldb) p t
error: Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory: 0     ---->

In D93497#2494295, @aprantl wrote:

It sounds like the bug @dongAxis1944 has found is unrelated to this patch. Should we move forward with this one, or are there any other problems you see?

[aprantl]

In D93497#2495103, @dongAxis1944 wrote:

I do not see any other problems, but if the code use c++ program, it will cause lldb read invalid address like I write before:

Process 106638 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.2
    frame #0: 0x0000000000401bd0 a.out`foo(t=0x0000000000000000) at coro-debug-1.cpp:39:26
   36     //   (int) i = 1
   37
   38     co_await suspend_always();
-> 39     printf("%d, %d, %d\n", i, j, t.i); // 2, 1
                                 ^
   40     // Breakpoint 2:
   41     //   (lldb) frame variable i
   42     //   (int) i = <variable not available>
(lldb) p t
error: Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory: 0     ---->

In D93497#2494295, @aprantl wrote:

It sounds like the bug @dongAxis1944 has found is unrelated to this patch. Should we move forward with this one, or are there any other problems you see?

I'm terribly sorry for being so slow to respond. I can't reproduce this particular issue:

   30  	coro foo(struct test & t) {
   31  	  int i = 0;
   32  	  int j = 0;
   33  	  i = t.i + t.j + t.k;
   34  	  printf("%d\n", i);
   35  	
   36  	  co_await suspend_always();
-> 37  	  printf("%d, %d, %d\n", i, j, t.i); // --> A
    	                         ^
   38  	
   39  	  co_await suspend_always();
   40  	  ++i;
Target 0: (e2.out) stopped.
(lldb) v
v
(int) j = 0
(int) i = 494
(lldb) p t
p t
(test) $0 = (i = 238, j = 255, k = 1)
(lldb)

When you get the could not materialize error, is this for one of the variables that are duplicated by the frontend? If yes, then I wouldn't be too concerned about it. The frontend variable duplication is something we definitely have to fix, too, bu t I think it's orthogonal to this patch.

[aprantl]

Please ignore my last comment. I managed to use the wrong clang binary.

[aprantl]

I looked into the duplicate variables. For your first testcase, there is first the function parameter t:

ParmVarDecl 0x1585a3de8 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:10, col:24> col:24 used t 'struct test &'

followed by an implicit VarDecl (thus the DW_AT_artificial attribute) for another t:

VarDecl 0x1585a8c30 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:6> col:6 implicit used __coro_gro 'coro' nrvo cinit
`-CXXMemberCallExpr 0x1585a8b88 <col:6> 'coro'
  `-MemberExpr 0x1585a8b58 <col:6> '<bound member function type>' .get_return_object 0x1585a0990
    `-DeclRefExpr 0x1585a8b38 <col:6> 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type' lvalue Var 0x1585a57a8 '__promise' 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type'

The second one is produced by void EmitGroAlloca() in CGCoroutine.cpp. I'm assuming that this is the declaration of the function parameter as it is being captured for the CoroFrame function argument that is being passed to the various .resume functions.

[aprantl]

I made a small bugfix to my patch after which the salvaged debug info for the "artificial" variables is working as expected. What's missing now is a way to erase the the artificial variables from the main entry point function, and the non-artificial variables from the .resume funclets. Then we should be in a good shape.

[junparser]

In D93497#2514162, @aprantl wrote:

I looked into the duplicate variables. For your first testcase, there is first the function parameter t:

ParmVarDecl 0x1585a3de8 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:10, col:24> col:24 used t 'struct test &'

followed by an implicit VarDecl (thus the DW_AT_artificial attribute) for another t:

VarDecl 0x1585a8c30 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:6> col:6 implicit used __coro_gro 'coro' nrvo cinit
`-CXXMemberCallExpr 0x1585a8b88 <col:6> 'coro'
  `-MemberExpr 0x1585a8b58 <col:6> '<bound member function type>' .get_return_object 0x1585a0990
    `-DeclRefExpr 0x1585a8b38 <col:6> 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type' lvalue Var 0x1585a57a8 '__promise' 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type'

The second one is produced by void EmitGroAlloca() in CGCoroutine.cpp. I'm assuming that this is the declaration of the function parameter as it is being captured for the CoroFrame function argument that is being passed to the various .resume functions.

Yes, that's right.

[aprantl]

I have now uploaded a variant of the patch that incorporates logic that is similar in spirit to https://reviews.llvm.org/D94388 in that it deletes the "dead" dbg.declare intrinsics. Doing it inside of CoroFrame should be safe, since we are only deleting dbg.declares that we previously salvaged.

At least in your example, it has the desired effect. We should probably still prepare a patch that turns the artificial variables into real function parameters, but that's a separate issue.

(lldb) v t
(test &) t = 0x00007ffeefbff9f0 (i = 238, j = 255, k = 1)

[dongAxis1944]

thanks, i will check the patch asap.

In D93497#2517239, @aprantl wrote:

I have now uploaded a variant of the patch that incorporates logic that is similar in spirit to https://reviews.llvm.org/D94388 in that it deletes the "dead" dbg.declare intrinsics. Doing it inside of CoroFrame should be safe, since we are only deleting dbg.declares that we previously salvaged.

At least in your example, it has the desired effect. We should probably still prepare a patch that turns the artificial variables into real function parameters, but that's a separate issue.

(lldb) v t
(test &) t = 0x00007ffeefbff9f0 (i = 238, j = 255, k = 1)

[bruno]

Hi Adrian, this is nice, I really like the approach! Do you think that this supersedes https://reviews.llvm.org/D90772? I'm curious if we can stop generating the extra dbg.values inserted, or whether those are necessary for your reconstruction work?

[dongAxis1944]

we might use another patch to rewrite the additional parameters, I think.
Dose anyone has some ideas?

In D93497#2522122, @bruno wrote:

Hi Adrian, this is nice, I really like the approach! Do you think that this supersedes https://reviews.llvm.org/D90772? I'm curious if we can stop generating the extra dbg.values inserted, or whether those are necessary for your reconstruction work?

[aprantl]

In D93497#2522122, @bruno wrote:

Hi Adrian, this is nice, I really like the approach! Do you think that this supersedes https://reviews.llvm.org/D90772? I'm curious if we can stop generating the extra dbg.values inserted, or whether those are necessary for your reconstruction work?

Yes, after this patch, we should be able to revert D90772 without loosing any functionality. This patch inserts dbg.declare intrinsics into the coro-split funclets which are by definition valid throughout the entire function, so any dbg.value intrinsics describing the same variables would be redundant.

[aprantl]

I've updated the patch to include a revert of D90772. The testcase updates highlight nicely how every removed CHECK for a dbg.value is preceded by a new CHECK for a dbg.declare. Thanks for pointing this out, Bruno!

[bruno]

Awesome. I've also tested the last version of the patch against some testcases I care about and they all debug fine under lldb!

[aprantl]

In D93497#2523816, @bruno wrote:

Awesome. I've also tested the last version of the patch against some testcases I care about and they all debug fine under lldb!

If you have any testcases that would make sense to add to the LLDB testsuite, let me know! It will make it harder for me to break C++ coroutines as I'm working on Swift coroutine support.

[bruno]

If you have any testcases that would make sense to add to the LLDB testsuite, let me know! It will make it harder for me to break C++ coroutines as I'm working on Swift coroutine support.

Thanks for bringing this up. I actually do, it's pretty simple stuff, but might help with end-to-end testing. Will put up a patch soon, hopefully others in this thread also have more testcases to contribute :)

[ChuanqiXu]

Hi all, I want to ask some question about the debug information in coroutine. I wonder this patch may be a suitable place.
I know this patch intentionally to solve problems under O0.
And I want to enable the ability to debug coroutine program with optimization.
Now I create following instruction with '-O2 -emit-llvm':

%local.reload.addr = getelementptr %FramePtrTy, %FramePtrTy %Frame, ...
call void @llvm.dbg.declare(metadata %"struct_type"* %a.reload.addr, metadata !1, metadata !DIExpression()), !dbg !2
; ...
!1 = !DILocalVariable(name: "a", ... );

But I find I can't print the value of a in debugger. To my understanding, %a.reload.addr marks the address of a. So gdb should know the address of a and print its value.
What's going wrong?

[aprantl]

In D93497#2541578, @ChuanqiXu wrote:

Hi all, I want to ask some question about the debug information in coroutine. I wonder this patch may be a suitable place.
I know this patch intentionally to solve problems under O0.
And I want to enable the ability to debug coroutine program with optimization.
Now I create following instruction with '-O2 -emit-llvm':

%local.reload.addr = getelementptr %FramePtrTy, %FramePtrTy %Frame, ...
call void @llvm.dbg.declare(metadata %"struct_type"* %a.reload.addr, metadata !1, metadata !DIExpression()), !dbg !2
; ...
!1 = !DILocalVariable(name: "a", ... );

But I find I can't print the value of a in debugger. To my understanding, %a.reload.addr marks the address of a. So gdb should know the address of a and print its value.
What's going wrong?

Does that dbg.declare actually make it into DWARF? If you run llvm-dwarfdump on the resulting binary, does the variable have a DW_AT_location attribute? If not, some pass in LLVM is dropping the info — which happens a lot in optimized code. I would recommend dumping the output of clang -mllvm -print-after-all into a file and searching for the place the debug info disappears.

[ChuanqiXu]

In D93497#2560170, @aprantl wrote:

In D93497#2541578, @ChuanqiXu wrote:

Hi all, I want to ask some question about the debug information in coroutine. I wonder this patch may be a suitable place.
I know this patch intentionally to solve problems under O0.
And I want to enable the ability to debug coroutine program with optimization.
Now I create following instruction with '-O2 -emit-llvm':

%local.reload.addr = getelementptr %FramePtrTy, %FramePtrTy %Frame, ...
call void @llvm.dbg.declare(metadata %"struct_type"* %a.reload.addr, metadata !1, metadata !DIExpression()), !dbg !2
; ...
!1 = !DILocalVariable(name: "a", ... );

But I find I can't print the value of a in debugger. To my understanding, %a.reload.addr marks the address of a. So gdb should know the address of a and print its value.
What's going wrong?

Does that dbg.declare actually make it into DWARF? If you run llvm-dwarfdump on the resulting binary, does the variable have a DW_AT_location attribute? If not, some pass in LLVM is dropping the info — which happens a lot in optimized code. I would recommend dumping the output of clang -mllvm -print-after-all into a file and searching for the place the debug info disappears.

Hi, thanks for the reply. Now I made it in https://reviews.llvm.org/D96938. I am not familiar with the debug info system. And my patch deletes the paragraph about if (auto Arg = dyn_cast_or_null<llvm::Argument>(Storage)) since it aimed at O2. But I am really not sure about the correctness. In my local tests for C++ programs, this patch could print alloca under O2. But I am not sure if it would obstruct the debugbility about the parameter of coroutine. Do you have any suggestion?

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

static Instruction *insertSpills(const FrameDataInfo &FrameData,
   auto *CB = Shape.CoroBegin;
   LLVMContext &C = CB->getContext();
   IRBuilder<> Builder(CB->getNextNode());
   StructType *FrameTy = Shape.FrameTy;
   PointerType *FramePtrTy = FrameTy->getPointerTo();
   auto *FramePtr =
       cast<Instruction>(Builder.CreateBitCast(CB, FramePtrTy, "FramePtr"));
   DominatorTree DT(*CB->getFunction());
+  SmallDenseMap<llvm::Value *, llvm::AllocaInst *, 4> DbgPtrAllocaCache;
 
   // Create a GEP with the given index into the coroutine frame for the original
   // value Orig. Appends an extra 0 index for array-allocas, preserving the
   // original type.
   auto GetFramePointer = [&](Value *Orig) -> Value * {
     FieldIDType Index = FrameData.getFieldIndex(Orig);
     SmallVector<Value *, 3> Indices = {
         ConstantInt::get(Type::getInt32Ty(C), 0),
for (auto *U : E.second) {
         CurrentBlock = U->getParent();
         Builder.SetInsertPoint(&*CurrentBlock->getFirstInsertionPt());
 
         auto *GEP = GetFramePointer(E.first);
         GEP->setName(E.first->getName() + Twine(".reload.addr"));
         CurrentReload = Builder.CreateLoad(
             FrameTy->getElementType(FrameData.getFieldIndex(E.first)), GEP,
             E.first->getName() + Twine(".reload"));
+
+        TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(Def);
+        for (DbgDeclareInst *DDI : DIs) {
+          bool AllowUnresolved = false;
+          // This dbg.declare is preserved for all coro-split function
+          // fragments. It will be unreachable in the main function, and
+          // processed by coro::salvageDebugInfo() by CoroCloner.
+          DIBuilder(*CurrentBlock->getParent()->getParent(), AllowUnresolved)
+              .insertDeclare(CurrentReload, DDI->getVariable(),
+                             DDI->getExpression(), DDI->getDebugLoc(),
+                             &*Builder.GetInsertPoint());
+          // This dbg.declare is for the main function entry point.  It
+          // will be deleted in all coro-split functions.
+          coro::salvageDebugInfo(DbgPtrAllocaCache, DDI);
+        }
       }
 
       // If we have a single edge PHINode, remove it and replace it with a
       // reload from the coroutine frame. (We already took care of multi edge
       // PHINodes by rewriting them in the rewritePHIs function).
       if (auto *PN = dyn_cast<PHINode>(U)) {
         assert(PN->getNumIncomingValues() == 1 &&
                "unexpected number of incoming "
for (const auto &A : FrameData.Allocas) {
     }
     if (UsersToUpdate.empty())
       continue;
     auto *G = GetFramePointer(Alloca);
     G->setName(Alloca->getName() + Twine(".reload.addr"));
 
     SmallPtrSet<BasicBlock *, 4> SeenDbgBBs;
     TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(Alloca);
-    DIBuilder DIB(*Alloca->getModule(), /*AllowUnresolved*/ false);
-    Instruction *FirstDbgDecl = nullptr;
-
-    if (!DIs.empty()) {
-      FirstDbgDecl = DIB.insertDeclare(G, DIs.front()->getVariable(),
-                                       DIs.front()->getExpression(),
-                                       DIs.front()->getDebugLoc(), DIs.front());
-      SeenDbgBBs.insert(DIs.front()->getParent());
-    }
+    if (!DIs.empty())
+      DIBuilder(*Alloca->getModule(),
+                /*AllowUnresolved*/ false)
+          .insertDeclare(G, DIs.front()->getVariable(),
     for (auto *DI : FindDbgDeclareUses(Alloca))
       DI->eraseFromParent();
     replaceDbgUsesWithUndef(Alloca);
 
-    for (Instruction *I : UsersToUpdate) {
+    for (Instruction *I : UsersToUpdate)
       I->replaceUsesOfWith(Alloca, G);
-
-      // After cloning, transformations might not guarantee that all uses
-      // of this alloca are dominated by the already existing dbg.declare's,
-      // compromising the debug quality. Instead of writing another
-      // transformation to patch each clone, go ahead and early populate
-      // basic blocks that use such allocas with more debug info.
-      if (SeenDbgBBs.count(I->getParent()))
-        continue;
-
-      // If there isn't a prior dbg.declare for this alloca, it probably
-      // means the state hasn't changed prior to one of the relevant suspend
-      // point for this frame access.
-      if (!FirstDbgDecl)
-        continue;
-
-      // These instructions are all dominated by the alloca, insert the
-      // dbg.value in the beginning of the BB to enhance debugging
-      // experience and allow values to be inspected as early as possible.
-      // Prefer dbg.value over dbg.declare since it better sets expectations
-      // that control flow can be later changed by other passes.
-      auto *DI = cast<DbgDeclareInst>(FirstDbgDecl);
-      BasicBlock *CurrentBlock = I->getParent();
-      auto *DerefExpr =
-          DIExpression::append(DI->getExpression(), dwarf::DW_OP_deref);
-      DIB.insertDbgValueIntrinsic(G, DI->getVariable(), DerefExpr,
-                                  DI->getDebugLoc(),
-                                  &*CurrentBlock->getFirstInsertionPt());
-      SeenDbgBBs.insert(CurrentBlock);
-    }
   }
   Builder.SetInsertPoint(FramePtr->getNextNode());
   for (const auto &A : FrameData.Allocas) {
     AllocaInst *Alloca = A.Alloca;
     if (A.MayWriteBeforeCoroBegin) {
       // isEscaped really means potentially modified before CoroBegin.
       if (Alloca->isArrayAllocation())
         report_fatal_error(
for (Instruction &I : instructions(F)) {
     Visitor.visitPtr(*AI);
     if (!Visitor.getShouldLiveOnFrame())
       continue;
     Allocas.emplace_back(AI, Visitor.getAliasesCopy(),
                          Visitor.getMayWriteBeforeCoroBegin());
   }
 }
 
+void coro::salvageDebugInfo(
+    SmallDenseMap<llvm::Value *, llvm::AllocaInst *, 4> &DbgPtrAllocaCache,
+    DbgDeclareInst *DDI, bool LoadFromFramePtr) {
+  Function *F = DDI->getFunction();
+  IRBuilder<> Builder(F->getContext());
+  auto InsertPt = F->getEntryBlock().getFirstInsertionPt();
+  while (isa<IntrinsicInst>(InsertPt))
+    ++InsertPt;
+  Builder.SetInsertPoint(&F->getEntryBlock(), InsertPt);
+  DIExpression *Expr = DDI->getExpression();
+  // Follow the pointer arithmetic all the way to the incoming
+  // function argument and convert into a DIExpression.
+  Value *Storage = DDI->getAddress();
+  while (Storage) {
+    if (auto *LdInst = dyn_cast<LoadInst>(Storage)) {
+      Storage = LdInst->getOperand(0);
+    } else if (auto *StInst = dyn_cast<StoreInst>(Storage)) {
+      Storage = StInst->getOperand(0);
+    } else if (auto *GEPInst = dyn_cast<GetElementPtrInst>(Storage)) {
+      Expr = llvm::salvageDebugInfoImpl(*GEPInst, Expr,
+                                        /*WithStackValue=*/false);
+      Storage = GEPInst->getOperand(0);
+    } else if (auto *BCInst = dyn_cast<llvm::BitCastInst>(Storage))
+      Storage = BCInst->getOperand(0);
+    else
+      break;
+  }
+  // Store a pointer to the coroutine frame object in an alloca so it
+  // is available throughout the function when producing unoptimized
+  // code. Extending the lifetime this way is correct because the
+  // variable has been declared by a dbg.declare intrinsic.
+  if (auto Arg = dyn_cast_or_null<llvm::Argument>(Storage)) {
+    auto &Cached = DbgPtrAllocaCache[Storage];
+    if (!Cached) {
+      Cached = Builder.CreateAlloca(Storage->getType(), 0, nullptr,
+                                    Arg->getName() + ".debug");
+      Builder.CreateStore(Storage, Cached);
+    }
+    Storage = Cached;
+  }
+  // The FramePtr object adds one extra layer of indirection that
+  // needs to be unwrapped.
+  if (LoadFromFramePtr)
+    Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore);
+  auto &VMContext = DDI->getFunction()->getContext();
+  DDI->setOperand(
+      0, MetadataAsValue::get(VMContext, ValueAsMetadata::get(Storage)));
+  DDI->setOperand(2, MetadataAsValue::get(VMContext, Expr));
+  if (auto *InsertPt = dyn_cast_or_null<Instruction>(Storage))
+    DDI->moveAfter(InsertPt);
+}
+
 void coro::buildCoroutineFrame(Function &F, Shape &Shape) {
   eliminateSwiftError(F, Shape);
 
   if (Shape.ABI == coro::ABI::Switch &&
       Shape.SwitchLowering.PromiseAlloca) {
     Shape.getSwitchCoroId()->clearPromise();
   }
 

llvm/lib/Transforms/Coroutines/CoroInternal.h

 
 namespace coro {
 
 bool declaresIntrinsics(const Module &M,
                         const std::initializer_list<StringRef>);
 void replaceCoroFree(CoroIdInst *CoroId, bool Elide);
 void updateCallGraph(Function &Caller, ArrayRef<Function *> Funcs,
                      CallGraph &CG, CallGraphSCC &SCC);
+/// Recover a dbg.declare prepared by the frontend and emit an alloca
+/// holding a pointer to the coroutine frame.
+void salvageDebugInfo(
+    SmallDenseMap<llvm::Value *, llvm::AllocaInst *, 4> &DbgPtrAllocaCache,
+    DbgDeclareInst *DDI, bool LoadFromCoroFrame = false);
 
 // Keeps data and helper functions for lowering coroutine intrinsics.
 struct LowererBase {
   Module &TheModule;
   LLVMContext &Context;
   PointerType *const Int8Ptr;
   FunctionType *const ResumeFnType;
   ConstantPointerNull *const NullPtr;

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

private:
   }
 
   void replaceEntryBlock();
   Value *deriveNewFramePointer();
   void replaceRetconOrAsyncSuspendUses();
   void replaceCoroSuspends();
   void replaceCoroEnds();
   void replaceSwiftErrorOps();
+  void salvageDebugInfo();
   void handleFinalSuspend();
 };
 
 } // end anonymous namespace
 
 static void maybeFreeRetconStorage(IRBuilder<> &Builder,
                                    const coro::Shape &Shape, Value *FramePtr,
                                    CallGraph *CG) {
if (VMap == nullptr) {
     Shape.SwiftErrorOps.clear();
   }
 }
 
 void CoroCloner::replaceSwiftErrorOps() {
   ::replaceSwiftErrorOps(*NewF, Shape, &VMap);
 }
 
+void CoroCloner::salvageDebugInfo() {
+  SmallVector<DbgDeclareInst *, 8> Worklist;
+  SmallDenseMap<llvm::Value *, llvm::AllocaInst *, 4> DbgPtrAllocaCache;
+  for (auto &BB : *NewF)
+    for (auto &I : BB)
+      if (auto *DDI = dyn_cast<DbgDeclareInst>(&I))
+        Worklist.push_back(DDI);
+  for (DbgDeclareInst *DDI : Worklist) {
+    // This is a heuristic that detects declares left by CoroFrame.
+    bool LoadFromFramePtr = !isa<AllocaInst>(DDI->getAddress());
+    coro::salvageDebugInfo(DbgPtrAllocaCache, DDI, LoadFromFramePtr);
+  }
+  // Remove all salvaged dbg.declare intrinsics that became
+  // either unreachable or stale due to the CoroSplit transformation.
+  auto IsUnreachableBlock = [&](BasicBlock *BB) {
+    return BB->hasNPredecessors(0) && BB != &NewF->getEntryBlock();
+  };
+  for (DbgDeclareInst *DDI : Worklist) {
+    if (IsUnreachableBlock(DDI->getParent()))
+      DDI->eraseFromParent();
+    else if (auto *Alloca = dyn_cast_or_null<AllocaInst>(DDI->getAddress())) {
+      // Count all non-debuginfo uses in reachable blocks.
+      unsigned Uses = 0;
+      for (auto *User : DDI->getAddress()->users())
+        if (auto *I = dyn_cast<Instruction>(User))
+          if (!isa<AllocaInst>(I) && !IsUnreachableBlock(I->getParent()))
+            ++Uses;
+      if (!Uses)
+        DDI->eraseFromParent();
+    }
+  }
+}
+
 void CoroCloner::replaceEntryBlock() {
   // In the original function, the AllocaSpillBlock is a block immediately
   // following the allocation of the frame object which defines GEPs for
   // all the allocas that have been moved into the frame, and it ends by
   // branching to the original beginning of the coroutine.  Make this
   // the entry block of the cloned function.
   auto *Entry = cast<BasicBlock>(VMap[Shape.AllocaSpillBlock]);
   auto *OldEntry = &NewF->getEntryBlock();
void CoroCloner::create() {
   replaceCoroSuspends();
 
   // Handle swifterror.
   replaceSwiftErrorOps();
 
   // Remove coro.end intrinsics.
   replaceCoroEnds();
 
+  // Salvage debug info that points into the coroutine frame.
+  salvageDebugInfo();
+
   // Eliminate coro.free from the clones, replacing it with 'null' in cleanup,
   // to suppress deallocation code.
   if (Shape.ABI == coro::ABI::Switch)
     coro::replaceCoroFree(cast<CoroIdInst>(VMap[Shape.CoroBegin->getId()]),
                           /*Elide=*/ FKind == CoroCloner::Kind::SwitchCleanup);
 }
 
 // Create a resume clone by cloning the body of the original function, setting

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

 ; CHECK:       entry:
 ; CHECK:         %j = alloca i32, align 4
 ; CHECK:         [[IGEP:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4
 ; CHECK:         [[XGEP:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 6
 ; CHECK:       init.ready:
 ; CHECK:         call void @llvm.dbg.declare(metadata i32* [[IGEP]], metadata ![[IVAR:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC:[0-9]+]]
 ; CHECK:         call void @llvm.dbg.declare(metadata [10 x i32]* [[XGEP]], metadata ![[XVAR:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC]]
 ; CHECK:       await.ready:
-; CHECK:         call void @llvm.dbg.value(metadata [10 x i32]* [[XGEP]], metadata ![[XVAR]], metadata !DIExpression(DW_OP_deref)), !dbg ![[IDBGLOC]]
-; CHECK:         call void @llvm.dbg.value(metadata i32* [[IGEP]], metadata ![[IVAR]], metadata !DIExpression(DW_OP_deref)), !dbg ![[IDBGLOC]]
 ; CHECK:         call void @llvm.dbg.declare(metadata i32* %j, metadata ![[JVAR:[0-9]+]], metadata !DIExpression()), !dbg ![[JDBGLOC:[0-9]+]]
 ;
 ; CHECK-LABEL: define internal fastcc void @f.resume({{.*}}) {
 ; CHECK:       entry.resume:
-; CHECK:         %j = alloca i32, align 4
-; CHECK:         [[IGEP_RESUME:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4
-; CHECK:         [[XGEP_RESUME:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 6
+; CHECK-NEXT:    %[[DBG_PTR:.*]] = alloca %f.Frame*
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata %f.Frame** %[[DBG_PTR]], metadata ![[XVAR_RESUME:[0-9]+]],   metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 32)), !dbg
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata %f.Frame** %[[DBG_PTR]], metadata ![[IVAR_RESUME:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 20)), !dbg ![[IDBGLOC_RESUME:[0-9]+]]
+; CHECK-NEXT:    store %f.Frame* {{.*}}, %f.Frame** %[[DBG_PTR]]
+; CHECK:         %[[J:.*]] = alloca i32, align 4
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata i32* %[[J]], metadata ![[JVAR_RESUME:[0-9]+]], metadata !DIExpression()), !dbg ![[JDBGLOC_RESUME:[0-9]+]]
 ; CHECK:       init.ready:
-; CHECK:         call void @llvm.dbg.declare(metadata i32* [[IGEP_RESUME]], metadata ![[IVAR_RESUME:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC_RESUME:[0-9]+]]
-; CHECK:         call void @llvm.dbg.declare(metadata [10 x i32]* [[XGEP_RESUME]], metadata ![[XVAR_RESUME:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC_RESUME]]
 ; CHECK:       await.ready:
-; CHECK:         call void @llvm.dbg.value(metadata [10 x i32]* [[XGEP_RESUME]], metadata ![[XVAR_RESUME]], metadata !DIExpression(DW_OP_deref)), !dbg ![[IDBGLOC_RESUME]]
-; CHECK:         call void @llvm.dbg.value(metadata i32* [[IGEP_RESUME]], metadata ![[IVAR_RESUME]], metadata !DIExpression(DW_OP_deref)), !dbg ![[IDBGLOC_RESUME]]
-; CHECK:         call void @llvm.dbg.declare(metadata i32* %j, metadata ![[JVAR_RESUME:[0-9]+]], metadata !DIExpression()), !dbg ![[JDBGLOC_RESUME:[0-9]+]]
 ;
 ; CHECK: ![[IVAR]] = !DILocalVariable(name: "i"
 ; CHECK: ![[SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: !8, file: !1, line: 23, column: 12)
 ; CHECK: ![[IDBGLOC]] = !DILocation(line: 24, column: 7, scope: ![[SCOPE]])
 ; CHECK: ![[XVAR]] = !DILocalVariable(name: "x"
 ; CHECK: ![[JVAR]] = !DILocalVariable(name: "j"
 ; CHECK: ![[JDBGLOC]] = !DILocation(line: 32, column: 7, scope: ![[SCOPE]])
 
-; CHECK: ![[IVAR_RESUME]] = !DILocalVariable(name: "i"
-; CHECK: ![[RESUME_SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: !8, file: !1, line: 23, column: 12)
-; CHECK: ![[IDBGLOC_RESUME]] = !DILocation(line: 24, column: 7, scope: ![[RESUME_SCOPE]])
 ; CHECK: ![[XVAR_RESUME]] = !DILocalVariable(name: "x"
+; CHECK: ![[IDBGLOC_RESUME]] = !DILocation(line: 24, column: 7, scope: ![[RESUME_SCOPE:[0-9]+]])
+; CHECK: ![[RESUME_SCOPE]] = distinct !DILexicalBlock(scope: !8, file: !1, line: 23, column: 12)
+; CHECK: ![[IVAR_RESUME]] = !DILocalVariable(name: "i"
 ; CHECK: ![[JVAR_RESUME]] = !DILocalVariable(name: "j"
 ; CHECK: ![[JDBGLOC_RESUME]] = !DILocation(line: 32, column: 7, scope: ![[RESUME_SCOPE]])
 define void @f() {
 entry:
   %__promise = alloca i8, align 8
   %i = alloca i32, align 4
   %j = alloca i32, align 4
   %x = alloca [10 x i32], align 16

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

 !21 = !DILocation(line: 59, column: 10, scope: !6)
 !22 = !DILocation(line: 59, column: 7, scope: !6)
 !23 = !DILocation(line: 59, column: 5, scope: !6)
 !24 = !DILocation(line: 62, column: 3, scope: !6)
 
 ; CHECK: define i8* @f(i32 %x) #0 !dbg ![[ORIG:[0-9]+]]
 ; CHECK: define internal fastcc void @f.resume(%f.Frame* noalias nonnull align 8 dereferenceable(32) %FramePtr) #0 !dbg ![[RESUME:[0-9]+]]
 ; CHECK: entry.resume:
+; CHECK: %[[DBG_PTR:.*]] = alloca %f.Frame*
+; CHECK: call void @llvm.dbg.declare(metadata %f.Frame** %[[DBG_PTR]], metadata ![[RESUME_COROHDL:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst,
+; CHECK: call void @llvm.dbg.declare(metadata %f.Frame** %[[DBG_PTR]], metadata ![[RESUME_X:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst,
+; CHECK: store %f.Frame* {{.*}}, %f.Frame** %[[DBG_PTR]]
+; CHECK-NOT: alloca %struct.test*
 ; CHECK: call void @coro.devirt.trigger(i8* null)
-; CHECK: call void @llvm.dbg.declare(metadata i32* %x.addr.reload.addr, metadata ![[RESUME_VAR:[0-9]+]]
 ; CHECK: define internal fastcc void @f.destroy(%f.Frame* noalias nonnull align 8 dereferenceable(32) %FramePtr) #0 !dbg ![[DESTROY:[0-9]+]]
 ; CHECK: define internal fastcc void @f.cleanup(%f.Frame* noalias nonnull align 8 dereferenceable(32) %FramePtr) #0 !dbg ![[CLEANUP:[0-9]+]]
 
 ; CHECK: ![[ORIG]] = distinct !DISubprogram(name: "f", linkageName: "flink"
 
 ; CHECK: ![[RESUME]] = distinct !DISubprogram(name: "f", linkageName: "flink"
-; CHECK: ![[RESUME_VAR]] = !DILocalVariable(name: "x", arg: 1, scope: ![[RESUME]]
+; CHECK: ![[RESUME_COROHDL]] = !DILocalVariable(name: "coro_hdl", scope: ![[RESUME]]
+; CHECK: ![[RESUME_X]] = !DILocalVariable(name: "x", arg: 1, scope: ![[RESUME]]
 
 ; CHECK: ![[DESTROY]] = distinct !DISubprogram(name: "f", linkageName: "flink"
 
 ; CHECK: ![[CLEANUP]] = distinct !DISubprogram(name: "f", linkageName: "flink"