From c9218bd12ac2e2e44fee67c3c2f2cd50b5f65a4d Mon Sep 17 00:00:00 2001 From: Bruno Cardoso Lopes Date: Tue, 10 Nov 2020 11:20:56 -0800 Subject: [PATCH] [Coroutines] Add missing llvm.dbg.declare's to cover for more allocas Tracking local variables across suspend points is still somewhat incomplete. Consider this coroutine snippet: ``` resumable foo() { int x[10] = {}; int a = 3; co_await std::experimental::suspend_always(); a++; x[0] = 1; a += 2; x[1] = 2; a += 3; x[2] = 3; } ``` Can't manage to print `a` or `x` if they turn out to be allocas during CoroSplit (which happens if you build this code with `-O0` prior to this commit): ``` * thread #1, queue = 'com.apple.main-thread', stop reason = step over frame #0: 0x0000000100003729 main-noprint`foo() at main-noprint.cpp:43:5 40 co_await std::experimental::suspend_always(); 41 a++; 42 x[0] = 1; -> 43 a += 2; 44 x[1] = 2; 45 a += 3; 46 x[2] = 3; (lldb) p x error: :1:1: use of undeclared identifier 'x' x ^ ``` The generated IR contains a `llvm.dbg.declare` for `x` in it's initialization basic block. After CoroSplit, the `llvm.dbg.declare` might not dominate all of `x` uses and we lose debugging quality. Add `llvm.dbg.value`s to all relevant basic blocks such that if later transformations break the dominance the reliable debug info is already in place. For instance, this BB: ``` await.ready: ... %arrayidx = getelementptr inbounds [10 x i32], [10 x i32]* %x.reload.addr, i64 0, i64 0, !dbg !760 ... %arrayidx19 = getelementptr inbounds [10 x i32], [10 x i32]* %x.reload.addr, i64 0, i64 1, !dbg !763 ... %arrayidx21 = getelementptr inbounds [10 x i32], [10 x i32]* %x.reload.addr, i64 0, i64 2, !dbg !766 ``` becomes: ``` await.ready: ... call void @llvm.dbg.value(metadata [10 x i32]* %x.reload.addr, metadata !751, metadata !DIExpression()), !dbg !753 ... %arrayidx = getelementptr inbounds [10 x i32], [10 x i32]* %x.reload.addr, i64 0, i64 0, !dbg !760 ... %arrayidx19 = getelementptr inbounds [10 x i32], [10 x i32]* %x.reload.addr, i64 0, i64 1, !dbg !763 ... %arrayidx21 = getelementptr inbounds [10 x i32], [10 x i32]* %x.reload.addr, i64 0, i64 2, !dbg !766 ``` Differential Revision: https://reviews.llvm.org/D90772 --- lib/Transforms/Coroutines/CoroFrame.cpp | 46 ++++++++++++++++--- .../Coroutines/coro-debug-frame-variable.ll | 34 ++++++++++++++ 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/lib/Transforms/Coroutines/CoroFrame.cpp b/lib/Transforms/Coroutines/CoroFrame.cpp index af07d28fc7c..5265977ea5a 100644 --- a/lib/Transforms/Coroutines/CoroFrame.cpp +++ b/lib/Transforms/Coroutines/CoroFrame.cpp @@ -1191,19 +1191,51 @@ static Instruction *insertSpills(const FrameDataInfo &FrameData, continue; auto *G = GetFramePointer(Alloca); G->setName(Alloca->getName() + Twine(".reload.addr")); + + SmallPtrSet SeenDbgBBs; TinyPtrVector DIs = FindDbgDeclareUses(Alloca); - if (!DIs.empty()) - DIBuilder(*Alloca->getModule(), - /*AllowUnresolved*/ false) - .insertDeclare(G, DIs.front()->getVariable(), - DIs.front()->getExpression(), - DIs.front()->getDebugLoc(), DIs.front()); + DIBuilder DIB(*Alloca->getModule(), /*AllowUnresolved*/ false); + Instruction *FirstDbgDecl = nullptr; + + if (!DIs.empty()) { + FirstDbgDecl = DIB.insertDeclare(G, DIs.front()->getVariable(), + DIs.front()->getExpression(), + DIs.front()->getDebugLoc(), DIs.front()); + SeenDbgBBs.insert(DIs.front()->getParent()); + } for (auto *DI : FindDbgDeclareUses(Alloca)) DI->eraseFromParent(); replaceDbgUsesWithUndef(Alloca); - for (Instruction *I : UsersToUpdate) + for (Instruction *I : UsersToUpdate) { I->replaceUsesOfWith(Alloca, G); + + // After cloning, transformations might not guarantee that all uses + // of this alloca are dominated by the already existing dbg.declare's, + // compromising the debug quality. Instead of writing another + // transformation to patch each clone, go ahead and early populate + // basic blocks that use such allocas with more debug info. + if (SeenDbgBBs.count(I->getParent())) + continue; + + // If there isn't a prior dbg.declare for this alloca, it probably + // means the state hasn't changed prior to one of the relevant suspend + // point for this frame access. + if (!FirstDbgDecl) + continue; + + // These instructions are all dominated by the alloca, insert the + // dbg.value in the beginning of the BB to enhance debugging + // experience and allow values to be inspected as early as possible. + // Prefer dbg.value over dbg.declare since it better sets expectations + // that control flow can be later changed by other passes. + auto *DI = cast(FirstDbgDecl); + BasicBlock *CurrentBlock = I->getParent(); + DIB.insertDbgValueIntrinsic(G, DI->getVariable(), DI->getExpression(), + DI->getDebugLoc(), + &*CurrentBlock->getFirstInsertionPt()); + SeenDbgBBs.insert(CurrentBlock); + } } Builder.SetInsertPoint(FramePtr->getNextNode()); for (const auto &A : FrameData.Allocas) { diff --git a/test/Transforms/Coroutines/coro-debug-frame-variable.ll b/test/Transforms/Coroutines/coro-debug-frame-variable.ll index 889816f61fa..00a77cb790d 100644 --- a/test/Transforms/Coroutines/coro-debug-frame-variable.ll +++ b/test/Transforms/Coroutines/coro-debug-frame-variable.ll @@ -7,15 +7,19 @@ ; void foo() { ; int i = 0; ; ++i; +; int x = {}; ; print(i); // Prints '1' ; ; co_await suspend_always(); ; ; int j = 0; +; x[0] = 1; +; x[1] = 2; ; ++i; ; print(i); // Prints '2' ; ++j; ; print(j); // Prints '1' +; print(x); // Print '1' ; } ; ; The CHECKs verify that dbg.declare intrinsics are created for the coroutine @@ -28,28 +32,39 @@ ; CHECK: entry: ; CHECK: %j = alloca i32, align 4 ; CHECK: [[IGEP:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4 +; CHECK: [[XGEP:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 6 ; CHECK: init.ready: ; CHECK: call void @llvm.dbg.declare(metadata i32* [[IGEP]], metadata ![[IVAR:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC:[0-9]+]] +; CHECK: call void @llvm.dbg.declare(metadata [10 x i32]* [[XGEP]], metadata ![[XVAR:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC]] ; CHECK: await.ready: +; CHECK: call void @llvm.dbg.value(metadata [10 x i32]* [[XGEP]], metadata ![[XVAR]], metadata !DIExpression()), !dbg ![[IDBGLOC]] +; CHECK: call void @llvm.dbg.value(metadata i32* [[IGEP]], metadata ![[IVAR]], metadata !DIExpression()), !dbg ![[IDBGLOC]] ; CHECK: call void @llvm.dbg.declare(metadata i32* %j, metadata ![[JVAR:[0-9]+]], metadata !DIExpression()), !dbg ![[JDBGLOC:[0-9]+]] ; ; CHECK-LABEL: define internal fastcc void @f.resume({{.*}}) { ; CHECK: entry.resume: ; CHECK: %j = alloca i32, align 4 ; CHECK: [[IGEP_RESUME:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4 +; CHECK: [[XGEP_RESUME:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 6 ; CHECK: init.ready: ; CHECK: call void @llvm.dbg.declare(metadata i32* [[IGEP_RESUME]], metadata ![[IVAR_RESUME:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC_RESUME:[0-9]+]] +; CHECK: call void @llvm.dbg.declare(metadata [10 x i32]* [[XGEP_RESUME]], metadata ![[XVAR_RESUME:[0-9]+]], metadata !DIExpression()), !dbg ![[IDBGLOC_RESUME]] ; CHECK: await.ready: +; CHECK: call void @llvm.dbg.value(metadata [10 x i32]* [[XGEP_RESUME]], metadata ![[XVAR_RESUME]], metadata !DIExpression()), !dbg ![[IDBGLOC_RESUME]] +; CHECK: call void @llvm.dbg.value(metadata i32* [[IGEP_RESUME]], metadata ![[IVAR_RESUME]], metadata !DIExpression()), !dbg ![[IDBGLOC_RESUME]] ; CHECK: call void @llvm.dbg.declare(metadata i32* %j, metadata ![[JVAR_RESUME:[0-9]+]], metadata !DIExpression()), !dbg ![[JDBGLOC_RESUME:[0-9]+]] ; ; CHECK: ![[IVAR]] = !DILocalVariable(name: "i" ; CHECK: ![[SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: !8, file: !1, line: 23, column: 12) ; CHECK: ![[IDBGLOC]] = !DILocation(line: 24, column: 7, scope: ![[SCOPE]]) +; CHECK: ![[XVAR]] = !DILocalVariable(name: "x" ; CHECK: ![[JVAR]] = !DILocalVariable(name: "j" ; CHECK: ![[JDBGLOC]] = !DILocation(line: 32, column: 7, scope: ![[SCOPE]]) + ; CHECK: ![[IVAR_RESUME]] = !DILocalVariable(name: "i" ; CHECK: ![[RESUME_SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: !8, file: !1, line: 23, column: 12) ; CHECK: ![[IDBGLOC_RESUME]] = !DILocation(line: 24, column: 7, scope: ![[RESUME_SCOPE]]) +; CHECK: ![[XVAR_RESUME]] = !DILocalVariable(name: "x" ; CHECK: ![[JVAR_RESUME]] = !DILocalVariable(name: "j" ; CHECK: ![[JDBGLOC_RESUME]] = !DILocation(line: 32, column: 7, scope: ![[RESUME_SCOPE]]) define void @f() { @@ -57,6 +72,7 @@ entry: %__promise = alloca i8, align 8 %i = alloca i32, align 4 %j = alloca i32, align 4 + %x = alloca [10 x i32], align 16 %id = call token @llvm.coro.id(i32 16, i8* %__promise, i8* null, i8* null) %alloc = call i1 @llvm.coro.alloc(token %id) br i1 %alloc, label %coro.alloc, label %coro.init @@ -91,6 +107,9 @@ init.ready: ; preds = %init.suspend, %coro %i.init.ready.load = load i32, i32* %i, align 4 %i.init.ready.inc = add nsw i32 %i.init.ready.load, 1 store i32 %i.init.ready.inc, i32* %i, align 4 + call void @llvm.dbg.declare(metadata [10 x i32]* %x, metadata !14, metadata !DIExpression()), !dbg !11 + %memset = bitcast [10 x i32]* %x to i8*, !dbg !11 + call void @llvm.memset.p0i8.i64(i8* align 16 %memset, i8 0, i64 40, i1 false), !dbg !11 %i.init.ready.reload = load i32, i32* %i, align 4 call void @print(i32 %i.init.ready.reload) %ready.again = call zeroext i1 @await_ready() @@ -113,6 +132,10 @@ await.ready: ; preds = %await.suspend, %ini call void @await_resume() call void @llvm.dbg.declare(metadata i32* %j, metadata !12, metadata !DIExpression()), !dbg !13 store i32 0, i32* %j, align 4 + %arrayidx0 = getelementptr inbounds [10 x i32], [10 x i32]* %x, i64 0, i64 0, !dbg !18 + store i32 1, i32* %arrayidx0, align 16, !dbg !19 + %arrayidx1 = getelementptr inbounds [10 x i32], [10 x i32]* %x, i64 0, i64 1, !dbg !20 + store i32 2, i32* %arrayidx1, align 4, !dbg !21 %i.await.ready.load = load i32, i32* %i, align 4 %i.await.ready.inc = add nsw i32 %i.await.ready.load, 1 store i32 %i.await.ready.inc, i32* %i, align 4 @@ -195,6 +218,8 @@ declare i8* @from_address(i8*) declare void @return_void() declare void @final_suspend() +declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg) + !llvm.dbg.cu = !{!0} !llvm.linker.options = !{} !llvm.module.flags = !{!3, !4} @@ -214,3 +239,12 @@ declare void @final_suspend() !11 = !DILocation(line: 24, column: 7, scope: !7) !12 = !DILocalVariable(name: "j", scope: !7, file: !1, line: 32, type: !10) !13 = !DILocation(line: 32, column: 7, scope: !7) +!14 = !DILocalVariable(name: "x", scope: !22, file: !1, line: 34, type: !15) +!15 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, size: 320, elements: !16) +!16 = !{!17} +!17 = !DISubrange(count: 10) +!18 = !DILocation(line: 42, column: 3, scope: !7) +!19 = !DILocation(line: 42, column: 8, scope: !7) +!20 = !DILocation(line: 43, column: 3, scope: !7) +!21 = !DILocation(line: 43, column: 8, scope: !7) +!22 = distinct !DILexicalBlock(scope: !8, file: !1, line: 23, column: 12) \ No newline at end of file