This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by aprantl on Dec 17 2020, 3:54 PM.

Details

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

Diff Detail

Event Timeline

aprantl created this revision.Dec 17 2020, 3:54 PM
aprantl requested review of this revision.Dec 17 2020, 3:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 3:54 PM
aprantl updated this revision to Diff 312659.Dec 17 2020, 6:06 PM

Fix a potential cast error.

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 added a comment.EditedDec 28 2020, 2:43 AM

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.

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

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.

This comment was removed by 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

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.

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.

please check the attached file.

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.

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 added a comment.EditedJan 11 2021, 9:13 PM

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.

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.

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

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?

ychen added a subscriber: ychen.Jan 14 2021, 10:58 PM

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

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.

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

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 updated this revision to Diff 318374.Jan 21 2021, 6:06 PM

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.

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 updated this revision to Diff 318715.Jan 22 2021, 6:34 PM

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)

thanks, i will check the patch asap.

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)

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 added a comment.EditedJan 26 2021, 1:20 AM

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

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?

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 updated this revision to Diff 319338.Jan 26 2021, 10:20 AM

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 accepted this revision.Jan 26 2021, 2:07 PM

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

This revision is now accepted and ready to land.Jan 26 2021, 2:07 PM

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 added a comment.Jan 26 2021, 3:04 PM

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 :)

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?

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