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

[Wei Wang via Phabricator via llvm-commits]

weiwang added a comment.

In D146543#4211913 <https://reviews.llvm.org/D146543#4211913>, @ChuanqiXu wrote:

> In fact, we did similar things in the downstream too. But we didn't contribute it since we feel it might not be so good. Since it is a little bit dirty and it is natural that the debug information is missing. If you still want to do this, I feel `CoroCloner::salvageDebugInfo()` may be a better place.



In D146543#4211913 <https://reviews.llvm.org/D146543#4211913>, @ChuanqiXu wrote:

> In fact, we did similar things in the downstream too. But we didn't contribute it since we feel it might not be so good. Since it is a little bit dirty and it is natural that the debug information is missing. If you still want to do this, I feel `CoroCloner::salvageDebugInfo()` may be a better place.

Ha, I was wonder why nobody reported this issue before :)

For us, we've seen that "this" pointer debug info is gone after the first suspension point. And there are quite some reports internally, so this is really hurting the debug experiences. The current proposal is very specific to the "this" pointer problem we have. For some reason the "this" pointer is stored into some temp reg at entry block which complicates the look-up of `dbg.declare`. For example, in https://godbolt.org/z/q3rP77919

  define linkonce_odr dso_local ptr @_ZN9Container3fooEi(ptr noundef nonnull align 4 dereferenceable(4) %this, i32 noundef %arg)
  entry:
    %this.addr = alloca ptr, align 8
    %arg.addr = alloca i32, align 4
    %arg2 = alloca i32, align 4
    ... ...
    store ptr %this, ptr %this.addr, align 8
    call void @llvm.dbg.declare(metadata ptr %this.addr, metadata !658, metadata !DIExpression()), !dbg !660
    store i32 %arg, ptr %arg.addr, align 4
    call void @llvm.dbg.declare(metadata ptr %arg.addr, metadata !661, metadata !DIExpression()), !dbg !662
    %this1 = load ptr, ptr %this.addr, align 8
  
  coro.init:
    call void @llvm.lifetime.start.p0(i64 4, ptr %arg2) #5, !dbg !665
    call void @llvm.dbg.declare(metadata ptr %arg2, metadata !661, metadata !DIExpression()), !dbg !660
    %6 = load i32, ptr %arg.addr, align 4, !dbg !665
    store i32 %6, ptr %arg2, align 4, !dbg !665

There is no use of `%this.addr` after this, so `%this` or `%this.addr` won't be in coro frame, but `%this1` is.

A side note: the other function parameter `arg` is stored into an alloca `%args2`, which has a `dbg.declare`. Not sure why frontend treats them differently. If `this1` is an alloca instead of a temp, it could already have a `dbg.declare`. Maybe this is another angle.

Anyway, I feel that in order to have the same level of debug info availability as its non-async counterparts, coroutine function needs to make sure debug info of frame lived value is preserved across suspension points if that value maps to a source variable. From the examples I've seen, I think allocas are generally fine here. They usually come with needed debug info. And most temps do not map to source variable, no need of debug info. That comes down to those temps that map to source variable.

Does your downstream patch address the same issue? Maybe we can consolidate here.

The reason I put the change in `coro::buildCoroutineFrame` before spilt happens is that the missing `dbg.declare` can be created at the reload site so that after split and in `CoroCloner::salvageDebugInfo`, it would automatically be converted to a `dbg.declare` of `DEREF` from frame offset. And we only need to do that once in the original function. So this from the original function (`dbg.declare` is created in `coro::buildCoroutineFrame`)

  %this1.reload.addr = getelementptr inbounds %_ZN9Container3fooEi.Frame, ptr %0, i32 0, i32 4, !dbg !1794
  %this1.reload = load ptr, ptr %this1.reload.addr, align 8, !dbg !1794
  call void @llvm.dbg.declare(metadata ptr %this1.reload, metadata !1779, metadata !DIExpression()), !dbg !1780

becomes

  call void @llvm.dbg.declare(metadata ptr %.debug, metadata !1781, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 32)), !dbg !1783

in the entry block of resume/destroy/cleanup function.

This seems to be the most direct way of adding the missing debug info, but if there is a better way, please advise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146543