[coro] Preserve scope line for compiler generated functions

[kastiglione]

Summary

Coro-split functions with an active suspend point have their scope line set to
the line of the suspend point. However for compiler generated functions, this
results in debug info with unconventional results: a file named
<compiler-generated> with a non-zero line number. The convention for
<compiler-generated> is that the line number is zero.

This change propagates the scope line only for non-compiler generated
functions.

[aprantl]

<compiler-generated> is just something the Swift frontend picks as filename but LLVM shouldn't treat it as special.

A better way to fix this is to

• either set scopeline and filename together or leave the scopeline alone if the original scopeline is 0

and

• fix the frontend to not emit real source locations into an artificial compiler-generated function (it puzzles me how this happens in the first place)

[kastiglione]

Change logic to be based on current scope line, not current filename.

[kastiglione]

I've updated this to follow this advice:

leave the scopeline alone if the original scopeline is 0

Adrian also suggested "set scopeline and filename together" but there I see no (existing) API for setting the filename. It could be that I've overlooked it, but I think it's better to not set the filename if reasonable.

[kastiglione]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

874–875

@aprantl do you think it's worth checking flags as well? I noticed in the cases where ScopeLine was zero, that flags was FlagArtificial. Should this logic check that too?

[aprantl]

This is good, but we should add a test for this. There are already one or two tests that are affected by this, so you can probably just modify one of them to use line 0.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

874–875

Would SP->getFile() == DL->getFile() also work as a condition? That would maybe be the closest to what we really want to check. Otherwise checking for 0 sounds good to me.

[kastiglione]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

874–875

That condition works too.

[kastiglione]

only propagate scope line when files are the same

[aprantl]

Looks good (with a testcase). It should be easy to adapt an existing testcase for this by artificially changing a suspend point's !dbg location to use a different file name.

[aprantl]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

868

Instead of this sentence we could just say: ... to ensure line number and file name belong together.

[kastiglione]

following up here, looks like test/Transforms/Coroutines/coro-async.ll is the test that has the right "existing testcase" however it looks like I need to duplicate the whole thing and then tweak it. That test covers the SP->getFile() == DL->getFile() case, and if I change it by "artificially changing a suspend point's !dbg location to use a different file name", then it will instead test cover the inverted case (SP->getFile() != DL->getFile()).

Is there a better way than copying the test and modifying the one small part?

[aprantl]

Could you attach new debug info to one of the other functions in this file? Or is there only a single .resume function being generated for the entire test?

[kastiglione]

There is a second .resume.1, thanks I hadn't noticed that, I'll see if I can use that.

[kastiglione]

Test new logic in coro-async.ll

[aprantl]

Thanks! LGTM

[kastiglione]

thanks for your help. As you suggested, using a #line directive and viewing the IR generated by clang, led to showing how to change the file via DILexicalBlockFile.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

void CoroCloner::create() {
   NewF->setLinkage(llvm::GlobalValue::ExternalLinkage);
 
   CloneFunctionInto(NewF, &OrigF, VMap,
                     CloneFunctionChangeType::LocalChangesOnly, Returns);
 
   auto &Context = NewF->getContext();
 
   // For async functions / continuations, adjust the scope line of the
-  // clone to the line number of the suspend point. The scope line is
+  // clone to the line number of the suspend point. However, only
+  // adjust the scope line when the files are the same. This ensures
+  // line number and file name belong together. The scope line is
   // associated with all pre-prologue instructions. This avoids a jump
   // in the linetable from the function declaration to the suspend point.
   if (DISubprogram *SP = NewF->getSubprogram()) {
     assert(SP != OrigF.getSubprogram() && SP->isDistinct());
     if (ActiveSuspend)
       if (auto DL = ActiveSuspend->getDebugLoc())
+        if (SP->getFile() == DL->getFile())
           SP->setScopeLine(DL->getLine());
     // Update the linkage name to reflect the modified symbol name. It
     // is necessary to update the linkage name in Swift, since the
     // mangling changes for resume functions. It might also be the
     // right thing to do in C++, but due to a limitation in LLVM's
     // AsmPrinter we can only do this if the function doesn't have an
     // abstract specification, since the DWARF backend expects the
     // abstract specification to contain the linkage name and asserts
     // that they are identical.

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

<{ i32 trunc ( ; Relative pointer to async function
        i64 sub (
          i64 ptrtoint (void (%async.task*, %async.actor*, i8*)* @my_async_function2 to i64),
          i64 ptrtoint (i32* getelementptr inbounds (<{ i32, i32 }>, <{ i32, i32 }>* @my_async_function2_fp, i32 0, i32 1) to i64)
        )
      to i32),
      i32 128    ; Initial async context size without space for frame
   }>
 
-define swiftcc void @my_async_function2(%async.task* %task, %async.actor* %actor, i8* %async.ctxt) "frame-pointer"="all" {
+define swiftcc void @my_async_function2(%async.task* %task, %async.actor* %actor, i8* %async.ctxt) "frame-pointer"="all" !dbg !6 {
 entry:
 
   %id = call token @llvm.coro.id.async(i32 128, i32 16, i32 2, i8* bitcast (<{i32, i32}>* @my_async_function2_fp to i8*))
   %hdl = call i8* @llvm.coro.begin(token %id, i8* null)
   ; setup callee context
   %arg0 = bitcast %async.task* %task to i8*
   %arg1 = bitcast <{ i32, i32}>* @my_other_async_function_fp to i8*
   %callee_context = call i8* @llvm.coro.async.context.alloc(i8* %arg0, i8* %arg1)
 
   %callee_context.0 = bitcast i8* %callee_context to %async.ctxt*
   %callee_context.return_to_caller.addr = getelementptr inbounds %async.ctxt, %async.ctxt* %callee_context.0, i32 0, i32 1
   %return_to_caller.addr = bitcast void(i8*, %async.task*, %async.actor*)** %callee_context.return_to_caller.addr to i8**
   %resume.func_ptr = call i8* @llvm.coro.async.resume()
   store i8* %resume.func_ptr, i8** %return_to_caller.addr
   %callee_context.caller_context.addr = getelementptr inbounds %async.ctxt, %async.ctxt* %callee_context.0, i32 0, i32 0
   store i8* %async.ctxt, i8** %callee_context.caller_context.addr
   %resume_proj_fun = bitcast i8*(i8*)* @resume_context_projection to i8*
   %callee = bitcast void(i8*, %async.task*, %async.actor*)* @asyncSuspend to i8*
   %res = call {i8*, i8*, i8*} (i32, i8*, i8*, ...) @llvm.coro.suspend.async(i32 2,
                                                   i8* %resume.func_ptr,
                                                   i8* %resume_proj_fun,
                                                   void (i8*, i8*, %async.task*, %async.actor*)* @my_async_function.my_other_async_function_fp.apply,
-                                                  i8* %callee, i8* %callee_context, %async.task* %task, %async.actor *%actor)
+                                                  i8* %callee, i8* %callee_context, %async.task* %task, %async.actor *%actor), !dbg !9
 
   %continuation_task_arg = extractvalue {i8*, i8*, i8*} %res, 0
   %task.2 =  bitcast i8* %continuation_task_arg to %async.task*
 
   %callee_context.0.1 = bitcast i8* %callee_context to %async.ctxt*
   %callee_context.return_to_caller.addr.1 = getelementptr inbounds %async.ctxt, %async.ctxt* %callee_context.0.1, i32 0, i32 1
   %return_to_caller.addr.1 = bitcast void(i8*, %async.task*, %async.actor*)** %callee_context.return_to_caller.addr.1 to i8**
   %resume.func_ptr.1 = call i8* @llvm.coro.async.resume()
entry:
 
   tail call swiftcc void @asyncReturn(i8* %async.ctxt, %async.task* %task.2, %async.actor* %actor.2)
   call i1 @llvm.coro.end(i8* %hdl, i1 0)
   unreachable
 }
 
 ; CHECK-LABEL: define swiftcc void @my_async_function2(%async.task* %task, %async.actor* %actor, i8* %async.ctxt)
 ; CHECK-SAME: #[[FRAMEPOINTER:[0-9]+]]
+; CHECK-SAME: !dbg ![[SP3:[0-9]+]]
 ; CHECK: store i8* %async.ctxt,
 ; CHECK: store %async.actor* %actor,
 ; CHECK: store %async.task* %task,
 ; CHECK: [[CALLEE_CTXT:%.*]] =  tail call i8* @llvm.coro.async.context.alloc(
 ; CHECK: store i8* [[CALLEE_CTXT]],
 ; CHECK: store i8* bitcast (void (i8*, i8*, i8*)* @my_async_function2.resume.0 to i8*),
 ; CHECK: store i8* %async.ctxt,
 ; CHECK: tail call swiftcc void @asyncSuspend(i8* [[CALLEE_CTXT]], %async.task* %task, %async.actor* %actor)
 ; CHECK: ret void
 
 ; CHECK-LABEL: define internal swiftcc void @my_async_function2.resume.0(i8* %0, i8* nocapture readnone %1, i8* nocapture readonly %2)
 ; CHECK-SAME: #[[FRAMEPOINTER]]
+; CHECK-SAME: !dbg ![[SP4:[0-9]+]]
 ; CHECK: [[CALLEE_CTXT_ADDR:%.*]] = bitcast i8* %2 to i8**
 ; CHECK: [[CALLEE_CTXT:%.*]] = load i8*, i8** [[CALLEE_CTXT_ADDR]]
 ; CHECK: [[CALLEE_CTXT_SPILL_ADDR:%.*]] = getelementptr inbounds i8, i8* [[CALLEE_CTXT]], i64 152
 ; CHECK: [[CALLEE_CTXT_SPILL_ADDR2:%.*]] = bitcast i8* [[CALLEE_CTXT_SPILL_ADDR]] to i8**
 ; CHECK: store i8* bitcast (void (i8*, i8*, i8*)* @my_async_function2.resume.1 to i8*),
 ; CHECK: [[CALLLE_CTXT_RELOAD:%.*]] = load i8*, i8** [[CALLEE_CTXT_SPILL_ADDR2]]
 ; CHECK: tail call swiftcc void @asyncSuspend(i8* [[CALLEE_CTXT_RELOAD]]
 ; CHECK: ret void

 ; CHECK: ![[SP2]] = distinct !DISubprogram(name: "my_async_function",
 ; CHECK-SAME:                              linkageName: "my_async_function.resume.0",
 ; CHECK-SAME:                              scopeLine: 2
 !2 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !3, emissionKind: FullDebug)
 !3 = !DIFile(filename: "/tmp/1.swift", directory: "/")
 !4 = !DISubroutineType(types: !{})
 !5 = !DILocation(line: 2, column: 0, scope: !1)
 
+; CHECK: ![[SP3]] = distinct !DISubprogram(name: "my_async_function2",
+; CHECK-SAME:                              linkageName: "my_async_function2",
+; CHECK-SAME:                              scopeLine: 1
+!6 = distinct !DISubprogram(name: "my_async_function2",
+                            linkageName: "my_async_function2",
+                            scope: !2, file: !3, line: 1, type: !4,
+                            scopeLine: 1, spFlags: DISPFlagDefinition, unit: !2)
+; CHECK: ![[SP4]] = distinct !DISubprogram(name: "my_async_function2",
+; CHECK-SAME:                              linkageName: "my_async_function2.resume.0",
+; CHECK-SAME:                              scopeLine: 1
+!7 = !DILexicalBlockFile(scope: !6, file: !8, discriminator: 0)
+!8 = !DIFile(filename: "/tmp/fake.cpp", directory: "/")
+!9 = !DILocation(line: 2, column: 0, scope: !7)