Add -verify-with-context option to enable better reporting of verifier errors #84867
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-ir Author: Roman L (opti-mix) ChangesThe LLVM IR verifier does not provide a lot of context about places where verification errors occur. This makes it difficult to find such places when you have big LLVM IR modules. This PR adds a new option called -verify-with-context, which enables better verifier error reporting by providing the information about function, basic block and instruction where the error happens. This is achieved by keeping track of the current function, basic block and instruction being visited by the verifier. Here is an example output with a detailed error context: Full diff: https://github.com/llvm/llvm-project/pull/84867.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/InstVisitor.h b/llvm/include/llvm/IR/InstVisitor.h
index 311e0ac47ddfad..89e7daf5068be9 100644
--- a/llvm/include/llvm/IR/InstVisitor.h
+++ b/llvm/include/llvm/IR/InstVisitor.h
@@ -74,14 +74,41 @@ namespace llvm {
/// virtual function call overhead. Defining and using an InstVisitor is just
/// as efficient as having your own switch statement over the instruction
/// opcode.
-template<typename SubClass, typename RetTy=void>
+
+/// Helper type to manage the current context of an InstVisitor.
+struct DefaultCtxManager {
+ void pushCtx(const Value *V) {}
+ void popCtx() {}
+};
+
+template<typename SubClass, typename RetTy=void, typename CtxManager = DefaultCtxManager>
class InstVisitor {
+ CtxManager *ctxMgr;
+
+ struct ContextUpdater {
+ CtxManager *ctxMgr;
+ llvm::SmallVector<const Value *, 4> path;
+ ContextUpdater(CtxManager *ctxMgr, const Value *V) : ctxMgr(ctxMgr) {
+ path.push_back(V);
+ if (ctxMgr)
+ ctxMgr->pushCtx(V);
+ }
+ ~ContextUpdater() {
+ if (ctxMgr)
+ ctxMgr->popCtx();
+ }
+ };
+
//===--------------------------------------------------------------------===//
// Interface code - This is the public interface of the InstVisitor that you
// use to visit instructions...
//
public:
+ // Ctors
+ InstVisitor() : ctxMgr(nullptr) {}
+ InstVisitor(CtxManager *ctxMgr) : ctxMgr(ctxMgr) {}
+
// Generic visit method - Allow visitation to all instructions in a range
template<class Iterator>
void visit(Iterator Start, Iterator End) {
@@ -96,10 +123,12 @@ class InstVisitor {
visit(M.begin(), M.end());
}
void visit(Function &F) {
+ ContextUpdater ctxManager(ctxMgr, &F);
static_cast<SubClass*>(this)->visitFunction(F);
visit(F.begin(), F.end());
}
void visit(BasicBlock &BB) {
+ ContextUpdater ctxManager(ctxMgr, &BB);
static_cast<SubClass*>(this)->visitBasicBlock(BB);
visit(BB.begin(), BB.end());
}
@@ -113,6 +142,7 @@ class InstVisitor {
// visit - Finally, code to visit an instruction...
//
RetTy visit(Instruction &I) {
+ ContextUpdater ctxManager(ctxMgr, &I);
static_assert(std::is_base_of<InstVisitor, SubClass>::value,
"Must pass the derived type to this template!");
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 0e6c01802cfb8c..0c57acb755da36 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -132,9 +132,66 @@ static cl::opt<bool> VerifyNoAliasScopeDomination(
cl::desc("Ensure that llvm.experimental.noalias.scope.decl for identical "
"scopes are not dominating"));
+static cl::opt<bool> VerifyWithContext(
+ "verify-with-context", cl::Hidden, cl::init(false),
+ cl::desc("Enable a detailed context reporting for verification errors"));
+
namespace llvm {
-struct VerifierSupport {
+/// Helper type to manage the current context of a Verifier.
+struct VerifierCtxManager {
+ /// Current context.
+ SmallVector<const Value *> ContextPath;
+ /// The number of the current instruction in the current basic block.
+ size_t CurInstNumInThisBlock;
+
+ VerifierCtxManager() : CurInstNumInThisBlock(0) {}
+
+ void pushCtx(const Value *V) {
+ if (isa<Instruction>(V))
+ CurInstNumInThisBlock++;
+ else if (isa<BasicBlock>(V) || isa<Function>(V))
+ CurInstNumInThisBlock = 0;
+ ContextPath.emplace_back(V);
+ }
+ void popCtx() {
+ ContextPath.pop_back();
+ }
+ void printCtx(raw_ostream &OS, bool NL = false) {
+ if (ContextPath.empty())
+ return;
+ OS << "Context [";
+ for (size_t i = 0; i < ContextPath.size(); ++i) {
+ if (i > 0) {
+ OS << " -> ";
+ }
+ auto C = ContextPath[i];
+ if (isa<Function>(C)) {
+ OS << "Function '";
+ } else if (isa<BasicBlock>(C)) {
+ OS << "BasicBlock '";
+ } else if (isa<Instruction>(C)) {
+ OS << "Instruction '";
+ } else {
+ OS << "Value '";
+ }
+ OS << C->getName() << "'";
+ if (CurInstNumInThisBlock && isa<Instruction>(C)) {
+ OS << " (number " << CurInstNumInThisBlock << " inside BB)";
+ }
+ }
+ OS << "]";
+ if (NL)
+ OS << "\n";
+ }
+
+ void printCtx(raw_ostream &OS, const char *msg, bool NL = false) {
+ OS << msg;
+ printCtx(OS, NL);
+ }
+};
+
+struct VerifierSupport : VerifierCtxManager {
raw_ostream *OS;
const Module &M;
ModuleSlotTracker MST;
@@ -158,6 +215,24 @@ struct VerifierSupport {
*OS << "; ModuleID = '" << M->getModuleIdentifier() << "'\n";
}
+ void Write(const Instruction &I) {
+ if (VerifyWithContext) {
+ if (auto const *BB = I.getParent()) {
+ VerifierCtxManager ctxMgr;
+ if (auto *F = BB->getParent())
+ ctxMgr.pushCtx(F);
+ ctxMgr.pushCtx(BB);
+ ctxMgr.printCtx(*OS, true);
+ }
+ }
+ Write((const Value &)I);
+ }
+
+ void Write(const Instruction *I) {
+ if (I)
+ Write(*I);
+ }
+
void Write(const Value *V) {
if (V)
Write(*V);
@@ -280,8 +355,12 @@ struct VerifierSupport {
/// This provides a nice place to put a breakpoint if you want to see why
/// something is not correct.
void CheckFailed(const Twine &Message) {
- if (OS)
+ if (VerifyWithContext && OS) {
+ *OS << "Verification Error: " << Message << '\n';
+ printCtx(*OS, "Verification Error: ", /* NL */ true);
+ } else if (OS) {
*OS << Message << '\n';
+ }
Broken = true;
}
@@ -318,8 +397,10 @@ struct VerifierSupport {
namespace {
-class Verifier : public InstVisitor<Verifier>, VerifierSupport {
- friend class InstVisitor<Verifier>;
+class Verifier : public InstVisitor<Verifier, void, VerifierSupport>,
+ VerifierSupport {
+ using VerifierInstVisitor = InstVisitor<Verifier, void, VerifierSupport>;
+ friend VerifierInstVisitor;
// ISD::ArgFlagsTy::MemAlign only have 4 bits for alignment, so
// the alignment size should not exceed 2^15. Since encode(Align)
@@ -398,7 +479,7 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
public:
explicit Verifier(raw_ostream *OS, bool ShouldTreatBrokenDebugInfoAsError,
const Module &M)
- : VerifierSupport(OS, M), LandingPadResultTy(nullptr),
+ : InstVisitor(this), VerifierSupport(OS, M), LandingPadResultTy(nullptr),
SawFrameEscape(false), TBAAVerifyHelper(this) {
TreatBrokenDebugInfoAsError = ShouldTreatBrokenDebugInfoAsError;
}
@@ -547,7 +628,7 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
void visit(DPLabel &DPL);
void visit(DPValue &DPV);
// InstVisitor overrides...
- using InstVisitor<Verifier>::visit;
+ using VerifierInstVisitor::visit;
void visitDbgRecords(Instruction &I);
void visit(Instruction &I);
@@ -706,7 +787,7 @@ void Verifier::visit(Instruction &I) {
visitDbgRecords(I);
for (unsigned i = 0, e = I.getNumOperands(); i != e; ++i)
Check(I.getOperand(i) != nullptr, "Operand is null", &I);
- InstVisitor<Verifier>::visit(I);
+ VerifierInstVisitor::visit(I);
}
// Helper to iterate over indirect users. By returning false, the callback can ask to stop traversing further.
@@ -2989,6 +3070,7 @@ void Verifier::visitFunction(const Function &F) {
void Verifier::visitBasicBlock(BasicBlock &BB) {
InstsInThisBlock.clear();
ConvergenceVerifyHelper.visit(BB);
+ CurInstNumInThisBlock = 0;
// Ensure that basic blocks have terminators!
Check(BB.getTerminator(), "Basic Block does not have terminator!", &BB);
diff --git a/llvm/test/Verifier/verify-with-context.ll b/llvm/test/Verifier/verify-with-context.ll
new file mode 100644
index 00000000000000..cc0ca61bc0884e
--- /dev/null
+++ b/llvm/test/Verifier/verify-with-context.ll
@@ -0,0 +1,31 @@
+; RUN: not opt -S %s -passes=verify -verify-with-context 2>&1 | FileCheck %s
+
+declare i32 @foo(i32)
+define i32 @test_callbr_landingpad_not_first_inst() {
+entry:
+ %0 = callbr i32 asm "", "=r,!i"()
+ to label %asm.fallthrough [label %landingpad]
+
+asm.fallthrough:
+ ret i32 42
+
+landingpad:
+ %foo = call i32 @foo(i32 42)
+; CHECK: Verification Error: No other instructions may proceed intrinsic
+; CHECK-NEXT: Verification Error: Context [Function 'test_callbr_landingpad_not_first_inst' -> BasicBlock 'landingpad' -> Instruction 'out' (number 2 inside BB)]
+; CHECK-NEXT: Context [Function 'test_callbr_landingpad_not_first_inst' -> BasicBlock 'landingpad']
+; CHECK-NEXT: %out = call i32 @llvm.callbr.landingpad.i32(i32 %0)
+ %out = call i32 @llvm.callbr.landingpad.i32(i32 %0)
+ ret i32 %out
+}
+
+declare <2 x double> @llvm.masked.load.v2f64.p0(ptr, i32, <2 x i1>, <2 x double>)
+
+define <2 x double> @masked_load(<2 x i1> %mask, ptr %addr, <2 x double> %dst) {
+; CHECK: Verification Error: masked_load: alignment must be a power of 2
+; CHECK-NEXT: Verification Error: Context [Function 'masked_load' -> BasicBlock '' -> Instruction 'res' (number 1 inside BB)]
+; CHECK-NEXT: Context [Function 'masked_load' -> BasicBlock '']
+; CHECK-NEXT: %res = call <2 x double> @llvm.masked.load.v2f64.p0(ptr %addr, i32 3, <2 x i1> %mask, <2 x double> %dst)
+ %res = call <2 x double> @llvm.masked.load.v2f64.p0(ptr %addr, i32 3, <2 x i1>%mask, <2 x double> %dst)
+ ret <2 x double> %res
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…r errors The LLVM IR verifier does not provide a lot of context about places where verification errors occur. This makes it difficult to find such places when you have big LLVM IR modules. This PR adds a new option called -verify-with-context, which enables better verifier error reporting by providing the information about function, basic block and instruction where the error happens. This is achieved by keeping track of the current function, basic block and instruction being visited by the verifier. Here is an example output: ``` Verification Error: masked_load: alignment must be a power of 2 Verification Error: Context [Function 'masked_load' -> BasicBlock '' -> Instruction 'res' (number 1 inside BB)] Context [Function 'masked_load' -> BasicBlock ''] %res = call <2 x double> @llvm.masked.load.v2f64.p0(ptr %addr, i32 3, <2 x i1> %mask, <2 x double> %dst) ```
opti-mix
force-pushed
the
verifier-context
branch
from
bd577b7
to
a353efe
Compare
|
Ping |
arsenm
reviewed
Apr 23, 2024
Comment on lines
+135
to
+137
| static cl::opt<bool> VerifyWithContext( | ||
| "verify-with-context", cl::Hidden, cl::init(false), | ||
| cl::desc("Enable a detailed context reporting for verification errors")); |
There was a problem hiding this comment.
This shouldn't be an option. If we want to do this it should just always be enabled. If it were to be an option, it should be a pass parameter and not a cl::opt.
|
|
||
| landingpad: | ||
| %foo = call i32 @foo(i32 42) | ||
| ; CHECK: Verification Error: No other instructions may proceed intrinsic |
There was a problem hiding this comment.
Should be "precede" not "proceed"?
| namespace llvm { | ||
|
|
||
| struct VerifierSupport { | ||
| /// Helper type to manage the current context of a Verifier. | ||
| struct VerifierCtxManager { |
There was a problem hiding this comment.
I think this is more complicated than necessary. The context doesn't really need a trackable path, you already know directly from the instruction's parent the block and function.
It would be an improvement to just add the context function when applicable.
| ; CHECK-NEXT: %res = call <2 x double> @llvm.masked.load.v2f64.p0(ptr %addr, i32 3, <2 x i1> %mask, <2 x double> %dst) | ||
| %res = call <2 x double> @llvm.masked.load.v2f64.p0(ptr %addr, i32 3, <2 x i1>%mask, <2 x double> %dst) | ||
| ret <2 x double> %res | ||
| } |
There was a problem hiding this comment.
Needs some tests with anonymous functions and blocks
| } | ||
| auto C = ContextPath[i]; | ||
| if (isa<Function>(C)) { | ||
| OS << "Function '"; |
There was a problem hiding this comment.
Printing "Context", "Instruction" and so on is pretty verbose
| } else { | ||
| OS << "Value '"; | ||
| } | ||
| OS << C->getName() << "'"; |
There was a problem hiding this comment.
This won't be friendly with anonymous values
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The LLVM IR verifier does not provide a lot of context about places where verification errors occur. This makes it difficult to find such places when you have big LLVM IR modules.
This PR adds a new option called -verify-with-context, which enables better verifier error reporting by providing the information about function, basic block and instruction where the error happens. This is achieved by keeping track of the current function, basic block and instruction being visited by the verifier.
Here is an example output with a detailed error context: