[KeyInstr] Fix verifier check

[OCHyams]

The verifier check was in the wrong place, meaning it wasn't actually checking many instructions.

Fixing that causes a test failure (coro-dwarf-key-instrs.cpp) because coros turn off the feature but still annotate instructions with the metadata (which is a supported situation, but the verifier doesn't like it, and it's hard to teach the verifier to like it).

Fix that by avoiding emitting any key instruction metadata if the DISubprogram has opted out of key instructions.

[llvmbot]

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

The verifier check was in the wrong place, meaning it wasn't actually checking many instructions.

Fixing that causes a test failure (coro-dwarf-key-instrs.cpp) because coros turn off the feature but still annotate instructions with the metadata (which is a supported situation, but the verifier doesn't like it, and it's hard to teach the verifier to like it).

Fix that by avoiding emitting any key instruction metadata if the DISubprogram has opted out of key instructions.

---

Full diff: https://github.com/llvm/llvm-project/pull/149043.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+4)
• (modified) llvm/lib/IR/Verifier.cpp (+7-6)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index f97c7b6445984..0dde045453e3a 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -170,6 +170,10 @@ void CGDebugInfo::addInstToSpecificSourceAtom(llvm::Instruction *KeyInstruction,
   if (!Group || !CGM.getCodeGenOpts().DebugKeyInstructions)
     return;
 
+  llvm::DISubprogram *SP = KeyInstruction->getFunction()->getSubprogram();
+  if (!SP || !SP->getKeyInstructionsEnabled())
+    return;
+
   addInstSourceAtomMetadata(KeyInstruction, Group, /*Rank=*/1);
 
   llvm::Instruction *BackupI =
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 8004077b92665..ccaa8ccba085d 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -3185,12 +3185,6 @@ void Verifier::visitFunction(const Function &F) {
     CheckDI(SP->describes(&F),
             "!dbg attachment points at wrong subprogram for function", N, &F,
             &I, DL, Scope, SP);
-
-    if (DL->getAtomGroup())
-      CheckDI(DL->getScope()->getSubprogram()->getKeyInstructionsEnabled(),
-              "DbgLoc uses atomGroup but DISubprogram doesn't have Key "
-              "Instructions enabled",
-              DL, DL->getScope()->getSubprogram());
   };
   for (auto &BB : F)
     for (auto &I : BB) {
@@ -5492,6 +5486,13 @@ void Verifier::visitInstruction(Instruction &I) {
   if (MDNode *N = I.getDebugLoc().getAsMDNode()) {
     CheckDI(isa<DILocation>(N), "invalid !dbg metadata attachment", &I, N);
     visitMDNode(*N, AreDebugLocsAllowed::Yes);
+
+    auto *DL = cast<DILocation>(N);
+    if (DL->getAtomGroup())
+      CheckDI(DL->getScope()->getSubprogram()->getKeyInstructionsEnabled(),
+              "DbgLoc uses atomGroup but DISubprogram doesn't have Key "
+              "Instructions enabled",
+              DL, DL->getScope()->getSubprogram());
   }
 
   if (auto *DII = dyn_cast<DbgVariableIntrinsic>(&I)) {

[jmorse]

Given that we've found a verifier error that necessitates this patch, it presumably wants a test.

[SLTozer]

So just to makes sure my understanding is correct: we have an existing bug and an existing test which should exercise that bug, but the verifier doesn't catch it currently. This patch then fixes both the verifier and the bug, and therefore a new test isn't needed for the non-verifier change because the coverage for the non-verifier change already exists in coro-dwarf-key-instrs.cpp - is that about right? And agreeing with the above, this should include a small test that the verifier check works.

[OCHyams]

So, to be clear:

• Verifier check was checking very few instructions per function
• Fixing the check causes coro-dwarf-key-instrs.cpp to fail
• Fixing the problem stops coro-dwarf-key-instrs.cpp failing

I suppose I could add a check for the verifier condition independently of coro-dwarf-key-instrs.cpp ... coming right up

[SLTozer]

LGTM with some inline nits.

[SLTozer]

Nit, braces.

[SLTozer]

Suggested change

	; include non-key location to check verifier is checking the whole function.
	; Include non-key location to check verifier is checking the whole function.

[OCHyams]

/cherry-pick 653872f

[llvmbot]

/pull-request #149053