[Coro][Debuginfo] Add debug info to `_NoopCoro_ResumeDestroy` function

[avogelsgesang]

Summary

With this commit, we now attach an DISubprogram to the LLVM-generated
_NoopCoro_ResumeDestroy function. Thereby, lldb can show a
std::coroutine_handle to a std::noop_coroutine as

continuation = coro frame = 0x555555560d98 {
  resume = 0x0000555555555c50 (a.out`_NoopCoro_ResumeDestroy)
  destroy = 0x0000555555555c50 (a.out`_NoopCoro_ResumeDestroy)
}

instead of

continuation = coro frame = 0x555555560d98 {
    resume = 0x0000555555555c50 (a.out`___lldb_unnamed_symbol211)
    destroy = 0x0000555555555c50 (a.out`___lldb_unnamed_symbol211)
}

I renamed the function from NoopCoro.ResumeDestroy to
_NoopCoro_ResumeDestroy because:

• the leading _ makes sure this is a reserved name and should not clash with any user-provided names
• the . was replaced by a _, so the name is now a valid identifier in C, making it allows me to type its name in the debugger

[avogelsgesang]

pacify clang-format

[aprantl]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

125

nit: . at the end

129

Is there a specific reason why you want to create a new CU here? I would just use the one from the function we're currently processing and mark the new DISubprogram as artificial.

[jryans]

Hmm, the "X instead of Y" example in the change summary seems to have the same output on both sides... Maybe one side was lost when preparing the summary? (Apologies if there is a difference I'm not seeing... 😅)

[avogelsgesang]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

129

not really, it just felt the most natural to me. The user didn't write that function anywhere, hence I thought: the file name should read "<llvm-instrinsics>". Also, it seems there can be multiple compilation units attached to a single LLVM module, and I didn't know how to determine the one to which I should add this function

[avogelsgesang]

Hmm, the "X instead of Y" example in the change summary seems to have the same output on both sides...

good catch! Fixed. I forgot to recompile the old version of clang before creating the "before" snippet...

[avogelsgesang]

fixed comments

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

1

update comment...

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

129

It looks better to split these logic to another smaller function. And we should generate debug information only if the debug option -g enabled. I think we can look at the start of buildFrameDebugInfo to get this. Then we can reuse the CU.

[avogelsgesang]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

129

buildFrameDebugInfo looks at the current function to figure out if debug information should be created. This works nicely for coroutine splitting. For lowerCoroNoop, I don't think this is a good idea. The generated _NoopCoro_ResumeDestroy is shared across potentially many functions, some of which could have debug info, some of which won't

[aprantl]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

129

The user didn't write that function anywhere

The artificial flag on the subprogram and line 0 as a source location already signal that. But it should still part of the same translation/compile unit. That's how all other compiler-generated stubs / thunks / etc are also represented.

[avogelsgesang]

reuse compile unit instead of creating a new one

[avogelsgesang]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

129

I am reusing the existing CU now.

we should generate debug information only if the debug option -g enabled

If no compilation unit debug info exists, we now don't add debug info for _NoopCoro_ResumeDestroy, either

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

120

The double underscores look more like reserved.

129

I mean extract the if into a static function buildDebugInfoForNoopCoroutine or something else. I feel it would be better to read. The current one is slightly hard to read. Since the debug information may be noisy for people who want to visit how llvm handles coroutines.

[avogelsgesang]

move into separate function; use double underscore

[ChuanqiXu]

Looks like there are many unrelated changes, it'd better to remove them.

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

41

Unrelated change.

70–71

Unrelated change.

I guess you're using clang-format in a different way? Here is my way to do clang-format:

git diff -U0 --no-color --relative HEAD^ | clang/tools/clang-format/clang-format-diff.py -p1 -I

And set the clang-format at the trunk as the default clang-format.

106

We prefer short indentation.

114

Why do we set File as nullptr here? I feel like any DIFile is better here.

158–163

Unrelated changes. If we want to do such changes, it'd better to do it in a NFC patch.

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

25

I guess we need to update the test.

[avogelsgesang]

• update test case
• use <llvm-intrinsics> instead of nullptr DIFile
• remove unrelated changes

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

111

How about using existing DIFile?

146–151

Looks like unrelated change too.

[avogelsgesang]

• use existing DIFile
• remove more unrelated changes

[ChuanqiXu]

LGTM.

[avogelsgesang]

merged to main despite test failures because those test failures were only timeouts and I don't see how those could be related to this change at hand here

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

 //===- CoroEarly.cpp - Coroutine Early Function Pass ----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Coroutines/CoroEarly.h"
 #include "CoroInternal.h"
+#include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Module.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "coro-early"

 public:
   Lowerer(Module &M)
       : LowererBase(M), Builder(Context),
         AnyResumeFnPtrTy(FunctionType::get(Type::getVoidTy(Context), Int8Ptr,
                                            /*isVarArg=*/false)
                              ->getPointerTo()) {}
   void lowerEarlyIntrinsics(Function &F);
 };
 }
 
 // Replace a direct call to coro.resume or coro.destroy with an indirect call to
 // an address returned by coro.subfn.addr intrinsic. This is done so that
 // CGPassManager recognizes devirtualization when CoroElide pass replaces a call
 // to coro.subfn.addr with an appropriate function address.
 void Lowerer::lowerResumeOrDestroy(CallBase &CB,
                                    CoroSubFnInst::ResumeKind Index) {
   Value *ResumeAddr = makeSubFnCall(CB.getArgOperand(0), Index, &CB);
void Lowerer::lowerCoroPromise(CoroPromiseInst *Intrin) {
   Value *Operand = Intrin->getArgOperand(0);
   Align Alignment = Intrin->getAlignment();
   Type *Int8Ty = Builder.getInt8Ty();
 
   auto *SampleStruct =
       StructType::get(Context, {AnyResumeFnPtrTy, AnyResumeFnPtrTy, Int8Ty});
   const DataLayout &DL = TheModule.getDataLayout();
   int64_t Offset = alignTo(
       DL.getStructLayout(SampleStruct)->getElementOffset(2), Alignment);
   if (Intrin->isFromPromise())
     Offset = -Offset;
 
   Builder.SetInsertPoint(Intrin);
   Value *Replacement =
       Builder.CreateConstInBoundsGEP1_32(Int8Ty, Operand, Offset);
 
   Intrin->replaceAllUsesWith(Replacement);
   Intrin->eraseFromParent();
void Lowerer::lowerCoroDone(IntrinsicInst *II) {
   auto *Cond = Builder.CreateICmpEQ(Load, NullPtr);
 
   II->replaceAllUsesWith(Cond);
   II->eraseFromParent();
 }
 
 void Lowerer::lowerCoroNoop(IntrinsicInst *II) {
   if (!NoopCoro) {
     LLVMContext &C = Builder.getContext();
     Module &M = *II->getModule();
 
     // Create a noop.frame struct type.
     StructType *FrameTy = StructType::create(C, "NoopCoro.Frame");
     auto *FramePtrTy = FrameTy->getPointerTo();
     auto *FnTy = FunctionType::get(Type::getVoidTy(C), FramePtrTy,
                                    /*isVarArg=*/false);
     auto *FnPtrTy = FnTy->getPointerTo();
     FrameTy->setBody({FnPtrTy, FnPtrTy});
 
     // Create a Noop function that does nothing.
     Function *NoopFn =
         Function::Create(FnTy, GlobalValue::LinkageTypes::PrivateLinkage,
-                         "NoopCoro.ResumeDestroy", &M);
+                         "_NoopCoro_ResumeDestroy", &M);
     NoopFn->setCallingConv(CallingConv::Fast);
     auto *Entry = BasicBlock::Create(C, "entry", NoopFn);
     ReturnInst::Create(C, Entry);
 
+    // Attach debug info to the noop coroutine
+    DIBuilder DB(M);
+    auto *FileNode = DB.createFile("<llvm-intrinsics>", "");
+    StringRef Producer = "LLVM Version " LLVM_VERSION_STRING;
+    auto *CU =
+        DB.createCompileUnit(dwarf::DW_LANG_Mips_Assembler, FileNode, Producer,
+                             /*IsOptimized=*/false, /*Flags*/ "",
+                             /*RuntimeVersion=*/0);
+    SmallVector<Metadata *, 2> Params{nullptr, nullptr};
+    auto *SubroutineType =
+        DB.createSubroutineType(DB.getOrCreateTypeArray(Params));
+    auto *SP = DB.createFunction(
+        CU, /*Name=*/"_NoopCoro_ResumeDestroy",
+        /*LinkageName=*/"_NoopCoro_ResumeDestroy", /*File*/ FileNode,
+        /*LineNo=*/0, SubroutineType, /*ScopeLine=*/0, DINode::FlagArtificial,
+        DISubprogram::SPFlagDefinition);
+    NoopFn->setSubprogram(SP);
+    DB.finalizeSubprogram(SP);
+    DB.finalize();
+
     // Create a constant struct for the frame.
-    Constant* Values[] = {NoopFn, NoopFn};
-    Constant* NoopCoroConst = ConstantStruct::get(FrameTy, Values);
-    NoopCoro = new GlobalVariable(M, NoopCoroConst->getType(), /*isConstant=*/true,
-                                GlobalVariable::PrivateLinkage, NoopCoroConst,
-                                "NoopCoro.Frame.Const");
+    Constant *Values[] = {NoopFn, NoopFn};
+    Constant *NoopCoroConst = ConstantStruct::get(FrameTy, Values);
+    NoopCoro = new GlobalVariable(
+        M, NoopCoroConst->getType(), /*isConstant=*/true,
+        GlobalVariable::PrivateLinkage, NoopCoroConst, "NoopCoro.Frame.Const");
   }
 
   Builder.SetInsertPoint(II);
   auto *NoopCoroVoidPtr = Builder.CreateBitCast(NoopCoro, Int8Ptr);
   II->replaceAllUsesWith(NoopCoroVoidPtr);
   II->eraseFromParent();
 }
 
 // Prior to CoroSplit, calls to coro.begin needs to be marked as NoDuplicate,
void Lowerer::lowerEarlyIntrinsics(Function &F) {
   SmallVector<CoroFreeInst *, 4> CoroFrees;
   bool HasCoroSuspend = false;
   for (Instruction &I : llvm::make_early_inc_range(instructions(F))) {
     auto *CB = dyn_cast<CallBase>(&I);
     if (!CB)
       continue;
 
     switch (CB->getIntrinsicID()) {
       default:
         continue;
       case Intrinsic::coro_free:
         CoroFrees.push_back(cast<CoroFreeInst>(&I));
         break;
       case Intrinsic::coro_suspend:
         // Make sure that final suspend point is not duplicated as CoroSplit
         // pass expects that there is at most one final suspend point.
         if (cast<CoroSuspendInst>(&I)->isFinal())
           CB->setCannotDuplicate();
         HasCoroSuspend = true;
         break;
       case Intrinsic::coro_end_async:
       case Intrinsic::coro_end:

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

+; Tests that CoroEarly pass correctly lowers coro.resume, coro.destroy
+; RUN: opt < %s -S -passes=coro-early | FileCheck %s
+
+; CHECK: %NoopCoro.Frame = type { void (%NoopCoro.Frame*)*, void (%NoopCoro.Frame*)* }
+; CHECK: @NoopCoro.Frame.Const = private constant %NoopCoro.Frame { void (%NoopCoro.Frame*)* @_NoopCoro_ResumeDestroy, void (%NoopCoro.Frame*)* @_NoopCoro_ResumeDestroy }
+
+
+; CHECK-LABEL: @noop(
+define i8* @noop() {
+; CHECK-NEXT: entry
+entry:
+; CHECK-NEXT: ret i8* bitcast (%NoopCoro.Frame* @NoopCoro.Frame.Const to i8*)
+  %n = call i8* @llvm.coro.noop()
+  ret i8* %n
+}
+
+declare i8* @llvm.coro.noop()
+
+
+; CHECK: define private fastcc void @_NoopCoro_ResumeDestroy(%NoopCoro.Frame* %0) !dbg ![[RESUME:[0-9]+]] {
+; CHECK-NEXT: entry
+; CHECK-NEXT:    ret void
+
+; CHECK: ![[RESUME]] = distinct !DISubprogram(name: "_NoopCoro_ResumeDestroy", linkageName: "_NoopCoro_ResumeDestroy"
+

llvm/test/Transforms/Coroutines/coro-resume-destroy.ll

 ; Tests that CoroEarly pass correctly lowers coro.resume, coro.destroy
-; and other intrinsics managed by this pass.
 ; RUN: opt < %s -S -passes=coro-early | FileCheck %s
 
-; CHECK: %NoopCoro.Frame = type { void (%NoopCoro.Frame*)*, void (%NoopCoro.Frame*)* }
-; CHECK: @NoopCoro.Frame.Const = private constant %NoopCoro.Frame { void (%NoopCoro.Frame*)* @NoopCoro.ResumeDestroy, void (%NoopCoro.Frame*)* @NoopCoro.ResumeDestroy }
-
 ; CHECK-LABEL: @callResume(
 define void @callResume(i8* %hdl) {
 ; CHECK-NEXT: entry
 entry:
 ; CHECK-NEXT: %0 = call i8* @llvm.coro.subfn.addr(i8* %hdl, i8 0)
 ; CHECK-NEXT: %1 = bitcast i8* %0 to void (i8*)*
 ; CHECK-NEXT: call fastcc void %1(i8* %hdl)
   call void @llvm.coro.resume(i8* %hdl)
cont:
   ret void
 
 ehcleanup:
   %0 = cleanuppad within none []
   cleanupret from %0 unwind to caller
 }
 
 
-; CHECK-LABEL: @noop(
-define i8* @noop() {
-; CHECK-NEXT: entry
-entry:
-; CHECK-NEXT: ret i8* bitcast (%NoopCoro.Frame* @NoopCoro.Frame.Const to i8*)
-  %n = call i8* @llvm.coro.noop()
-  ret i8* %n
-}
-
-; CHECK-LABEL: define private fastcc void @NoopCoro.ResumeDestroy(%NoopCoro.Frame* %0) {
-; CHECK-NEXT: entry
-; CHECK-NEXT:    ret void
-
-
 declare void @llvm.coro.resume(i8*)
 declare void @llvm.coro.destroy(i8*)
-declare i8* @llvm.coro.noop()