- Notifications
You must be signed in to change notification settings - Fork14.5k
[KeyInstr] Fix verifier check#149043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
llvmbot commentedJul 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-clang Author: Orlando Cazalet-Hyams (OCHyams) ChangesThe 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:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cppindex 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.cppindex 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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Given that we've found a verifier error that necessitates this patch, it presumably wants a test.
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 |
So, to be clear:
I suppose I could add a check for the verifier condition independently of coro-dwarf-key-instrs.cpp ... coming right up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM with some inline nits.
llvm/lib/IR/Verifier.cpp Outdated
| if (DL->getAtomGroup()) | ||
| CheckDI(DL->getScope()->getSubprogram()->getKeyInstructionsEnabled(), | ||
| "DbgLoc uses atomGroup but DISubprogram doesn't have Key" | ||
| "Instructions enabled", | ||
| DL, DL->getScope()->getSubprogram()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit, braces.
| @@ -7,6 +7,8 @@ | |||
| define dso_local void @f() !dbg !10 { | |||
| entry: | |||
| ; include non-key location to check verifier is checking the whole function. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ;include non-key location to check verifier is checking the whole function. | |
| ;Include non-key location to check verifier is checking the whole function. |
653872f intollvm:mainUh oh!
There was an error while loading.Please reload this page.
/cherry-pick653872f |
/pull-request#149053 |
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.