[PATCH] D146543: [Coroutines] Look for dbg.declare for temp spills

[Wei Wang via Phabricator via llvm-commits]

weiwang added a comment.

In D146543#4655197 <https://reviews.llvm.org/D146543#4655197>, @fdeazeve wrote:

> While debugging a crash in the compiler (see my comment about GlobalVariable) below, I found this patch and I am struggling to understand if we are doing the right thing here.
>
> Besides the crash, I am not sure this patch is correct with respect to the debug intrinsics. Consider this:
>
>   %indirect_storage = ptr ...
>   %AliveAccrossAplit = load ptr %indirect_storage
>   call void llvm.dbg.declare(%indirect_storage, ...)
>   
>   ... split point ...
>   
>   use %UseAliveAccrossAplit
>
> With this patch, we rewrite the above into:
>
>   %indirect_storage = ptr ...
>   %AliveAccrossSplit = load ptr %indirect_storage
>   call void llvm.dbg.declare(%indirect_storage, !MyVar, ...)
>   
>   ... split point ...
>   
>   %reload = ... reload of %AliveAccrossSplit inside the coroutine frame ...
>   call void llvm.dbg.declare(%reload, !MyVar, ...)
>
> Note that the dbg.declare after the rewrite is *not* equivalent to the original intrinsic: we are now claiming the address of !MyVar is `%reload`, and `%reload == %AliveAccrossSplit`.
>
> Does this make sense? In fact, the debug instruction that is being created doesn't even show up in the test added by this patch. If we run this test on a debugger and put a breakpoint in this line:
>
>   -> 1881           coro::salvageDebugInfo(ArgToAllocaMap, DDI, Shape.OptimizeFrame,
>      1882                                  false /*UseEntryValue*/);
>
> We should this:
>
>   (lldb) p CurrentBlock->dump()
>   
>   resume:                                           ; preds = %AfterCoroSuspend
>     %this1.reload.addr = getelementptr inbounds %foo.Frame, ptr %hdl, i32 0, i32 4
>     %this1.reload = load ptr, ptr %this1.reload.addr, align 8
>     call void @llvm.dbg.declare(metadata ptr %this1.reload, metadata !34, metadata !DIExpression()), !dbg !36
>     call void @bar(ptr %this1)
>     br label %cleanup
>
> Note that this dbg.declare doesn't survive the end of this test (it is not in the final output).
>
> I'm fairly sure that if we revert this patch and rerun the test, it will still pass.

Thanks for the comment. Yeah, I think there are issues with the change here. And the wording of "alias" is misleading.

The issue that this change specifically tries to fix is that, we saw the debug info for "this" pointer no loner exists after coroutine resumes. i.e. https://godbolt.org/z/sWxa4f94j, we can print "this" in lldb at line 24, but not at line 26. This is because "this" is stored into a temp that does not have debug info.

  define ptr @foo(ptr noundef nonnull align 1 dereferenceable(1) %this) !dbg !11 {
  entry:
    %this.addr = alloca ptr, align 8
    call void @llvm.dbg.declare(metadata ptr %this.addr, metadata !34, metadata !DIExpression()), !dbg !36
    %__promise = alloca i8, align 1
    call void @llvm.dbg.declare(metadata ptr %__promise, metadata !37, metadata !DIExpression()), !dbg !36
    store ptr %this, ptr %this.addr, align 8
    %this1 = load ptr, ptr %this.addr, align 8
  
  ... ...
  
  PostSpill:                                        ; preds = %AllocaSpillBB
    %this1.spill.addr = getelementptr inbounds %foo.Frame, ptr %hdl, i32 0, i32 4
    store ptr %this1, ptr %this1.spill.addr, align 8

So this change walks up the pointer Deref chain and tries to find the `llvm.dbg.declare` for `%this.addr` and use it for the temp `%this1`.

In the test included with the change.

Before:

  define internal fastcc void @foo.resume(ptr noundef nonnull align 8 dereferenceable(32) %hdl) !dbg !38 {
  entry.resume:
    %hdl.debug = alloca ptr, align 8
    call void @llvm.dbg.declare(metadata ptr %hdl.debug, metadata !40, metadata !DIExpression(DW_OP_deref)), !dbg !41
  
  ... ...
  
  !40 = !DILocalVariable(name: "__coro_frame", scope: !38, file: !1, type: !22, flags: DIFlagArtificial)

After:

  define internal fastcc void @foo.resume(ptr noundef nonnull align 8 dereferenceable(32) %hdl) !dbg !38 {
  entry.resume:
    %hdl.debug = alloca ptr, align 8
    call void @llvm.dbg.declare(metadata ptr %hdl.debug, metadata !41, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 24)), !dbg !42
    call void @llvm.dbg.declare(metadata ptr %hdl.debug, metadata !40, metadata !DIExpression(DW_OP_deref)), !dbg !42
  
  ... ...
  
  !40 = !DILocalVariable(name: "__coro_frame", scope: !38, file: !1, type: !22, flags: DIFlagArtificial)
  !41 = !DILocalVariable(name: "this", arg: 1, scope: !38, type: !35, flags: DIFlagArtificial | DIFlagObjectPointer)



> Note that this will crash the compiler if CurDef is a GlobalVariable.

You are right, and we also observed the crash and fixed in https://reviews.llvm.org/D157423. Since the original change is targeting a very specific case, the fix puts more restriction on the case it can affect.

Do you have any suggestions on how can we make it more generic and applicable to more use cases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146543/new/

https://reviews.llvm.org/D146543