From d2396b5d6a390d294b1ce239a27bbe1dc241eda8 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Tue, 16 Sep 2025 15:01:28 -0700 Subject: [PATCH 1/4] [IR] Strengthen COFF IR verifier check Find issues like the whole-program-vtables one earlier. --- llvm/lib/IR/Verifier.cpp | 145 ++++++++++++++++++++--------------- llvm/test/Verifier/comdat.ll | 40 +++++++++- 2 files changed, 120 insertions(+), 65 deletions(-) diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index c06b60fd2d9a9..0f47049a6457d 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -271,7 +271,7 @@ struct VerifierSupport { } template - void WriteTs(const T1 &V1, const Ts &... Vs) { + void WriteTs(const T1 &V1, const Ts &...Vs) { Write(V1); WriteTs(Vs...); } @@ -294,7 +294,7 @@ struct VerifierSupport { /// This calls the Message-only version so that the above is easier to set a /// breakpoint on. template - void CheckFailed(const Twine &Message, const T1 &V1, const Ts &... Vs) { + void CheckFailed(const Twine &Message, const T1 &V1, const Ts &...Vs) { CheckFailed(Message); if (OS) WriteTs(V1, Vs...); @@ -311,7 +311,7 @@ struct VerifierSupport { /// A debug info check failed (with values to print). template void DebugInfoCheckFailed(const Twine &Message, const T1 &V1, - const Ts &... Vs) { + const Ts &...Vs) { DebugInfoCheckFailed(Message); if (OS) WriteTs(V1, Vs...); @@ -715,7 +715,8 @@ void Verifier::visit(Instruction &I) { InstVisitor::visit(I); } -// Helper to iterate over indirect users. By returning false, the callback can ask to stop traversing further. +// Helper to iterate over indirect users. By returning false, the callback can +// ask to stop traversing further. static void forEachUser(const Value *User, SmallPtrSet &Visited, llvm::function_ref Callback) { @@ -724,7 +725,7 @@ static void forEachUser(const Value *User, SmallVector WorkList(User->materialized_users()); while (!WorkList.empty()) { - const Value *Cur = WorkList.pop_back_val(); + const Value *Cur = WorkList.pop_back_val(); if (!Visited.insert(Cur).second) continue; if (Callback(Cur)) @@ -876,8 +877,8 @@ void Verifier::visitGlobalVariable(const GlobalVariable &GV) { } } - if (GV.hasName() && (GV.getName() == "llvm.used" || - GV.getName() == "llvm.compiler.used")) { + if (GV.hasName() && + (GV.getName() == "llvm.used" || GV.getName() == "llvm.compiler.used")) { Check(!GV.hasInitializer() || GV.hasAppendingLinkage(), "invalid linkage for intrinsic global variable", &GV); Check(GV.materialized_use_empty(), @@ -936,13 +937,14 @@ void Verifier::visitGlobalVariable(const GlobalVariable &GV) { } void Verifier::visitAliaseeSubExpr(const GlobalAlias &GA, const Constant &C) { - SmallPtrSet Visited; + SmallPtrSet Visited; Visited.insert(&GA); visitAliaseeSubExpr(Visited, GA, C); } -void Verifier::visitAliaseeSubExpr(SmallPtrSetImpl &Visited, - const GlobalAlias &GA, const Constant &C) { +void Verifier::visitAliaseeSubExpr( + SmallPtrSetImpl &Visited, const GlobalAlias &GA, + const Constant &C) { if (GA.hasAvailableExternallyLinkage()) { Check(isa(C) && cast(C).hasAvailableExternallyLinkage(), @@ -1760,12 +1762,20 @@ void Verifier::visitDIImportedEntity(const DIImportedEntity &N) { } void Verifier::visitComdat(const Comdat &C) { - // In COFF the Module is invalid if the GlobalValue has private linkage. - // Entities with private linkage don't have entries in the symbol table. - if (TT.isOSBinFormatCOFF()) - if (const GlobalValue *GV = M.getNamedValue(C.getName())) + // Comdats in COFF modules must have a corresponding global that is not + // private. If the global is private, there will be no symbol table entry. + // We consider unused or dead comdat entries to be valid, since they won't + // make it into the COFF object file. + if (TT.isOSBinFormatCOFF()) { + const GlobalValue *GV = M.getNamedValue(C.getName()); + bool IsDefined = GV != nullptr && !GV->isDeclarationForLinker(); + Check(IsDefined || C.getUsers().empty(), + "COFF comdats must have a defined global value with the same name", + GV); + if (IsDefined) Check(!GV->hasPrivateLinkage(), "comdat global value has private linkage", GV); + } } void Verifier::visitModuleIdents() { @@ -1805,11 +1815,12 @@ void Verifier::visitModuleCommandLines() { void Verifier::visitModuleFlags() { const NamedMDNode *Flags = M.getModuleFlagsMetadata(); - if (!Flags) return; + if (!Flags) + return; // Scan each flag, and track the flags and requirements. - DenseMap SeenIDs; - SmallVector Requirements; + DenseMap SeenIDs; + SmallVector Requirements; uint64_t PAuthABIPlatform = -1; uint64_t PAuthABIVersion = -1; for (const MDNode *MDN : Flags->operands()) { @@ -1854,10 +1865,9 @@ void Verifier::visitModuleFlags() { } } -void -Verifier::visitModuleFlag(const MDNode *Op, - DenseMap &SeenIDs, - SmallVectorImpl &Requirements) { +void Verifier::visitModuleFlag( + const MDNode *Op, DenseMap &SeenIDs, + SmallVectorImpl &Requirements) { // Each module flag should have three arguments, the merge behavior (a // constant int), the flag ID (an MDString), and the value. Check(Op->getNumOperands() == 3, @@ -1936,8 +1946,8 @@ Verifier::visitModuleFlag(const MDNode *Op, } if (ID->getString() == "wchar_size") { - ConstantInt *Value - = mdconst::dyn_extract_or_null(Op->getOperand(2)); + ConstantInt *Value = + mdconst::dyn_extract_or_null(Op->getOperand(2)); Check(Value, "wchar_size metadata requires constant integer argument"); } @@ -2099,7 +2109,8 @@ void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty, if (!Attr.isStringAttribute() && IncompatibleAttrs.contains(Attr.getKindAsEnum())) { CheckFailed("Attribute '" + Attr.getAsString() + - "' applied to incompatible type!", V); + "' applied to incompatible type!", + V); return; } } @@ -2335,15 +2346,15 @@ void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs, } Check(!Attrs.hasAttrSomewhere(Attribute::Writable) || - isModSet(Attrs.getMemoryEffects().getModRef(IRMemLocation::ArgMem)), + isModSet(Attrs.getMemoryEffects().getModRef(IRMemLocation::ArgMem)), "Attribute writable and memory without argmem: write are incompatible!", V); if (Attrs.hasFnAttr("aarch64_pstate_sm_enabled")) { Check(!Attrs.hasFnAttr("aarch64_pstate_sm_compatible"), - "Attributes 'aarch64_pstate_sm_enabled and " - "aarch64_pstate_sm_compatible' are incompatible!", - V); + "Attributes 'aarch64_pstate_sm_enabled and " + "aarch64_pstate_sm_compatible' are incompatible!", + V); } Check((Attrs.hasFnAttr("aarch64_new_za") + Attrs.hasFnAttr("aarch64_in_za") + @@ -2724,7 +2735,8 @@ void Verifier::verifyStatepoint(const CallBase &Call) { Check(TargetFuncType, "gc.statepoint callee elementtype must be function type", Call); - const int NumCallArgs = cast(Call.getArgOperand(3))->getZExtValue(); + const int NumCallArgs = + cast(Call.getArgOperand(3))->getZExtValue(); Check(NumCallArgs >= 0, "gc.statepoint number of arguments to underlying call " "must be positive", @@ -2743,8 +2755,8 @@ void Verifier::verifyStatepoint(const CallBase &Call) { Check(NumCallArgs == NumParams, "gc.statepoint mismatch in number of call args", Call); - const uint64_t Flags - = cast(Call.getArgOperand(4))->getZExtValue(); + const uint64_t Flags = + cast(Call.getArgOperand(4))->getZExtValue(); Check((Flags & ~(uint64_t)StatepointFlags::MaskAll) == 0, "unknown flag used in gc.statepoint flags argument", Call); @@ -3241,7 +3253,7 @@ void Verifier::visitBasicBlock(BasicBlock &BB) { // it. if (isa(BB.front())) { SmallVector Preds(predecessors(&BB)); - SmallVector, 8> Values; + SmallVector, 8> Values; llvm::sort(Preds); for (const PHINode &PN : BB.phis()) { Check(PN.getNumIncomingValues() == Preds.size(), @@ -3278,8 +3290,7 @@ void Verifier::visitBasicBlock(BasicBlock &BB) { } // Check that all instructions have their parent pointers set up correctly. - for (auto &I : BB) - { + for (auto &I : BB) { Check(I.getParent() == &BB, "Instruction has bogus parent pointer!"); } @@ -3327,7 +3338,7 @@ void Verifier::visitSwitchInst(SwitchInst &SI) { // Check to make sure that all of the constants in the switch instruction // have the same type as the switched-on value. Type *SwitchTy = SI.getCondition()->getType(); - SmallPtrSet Constants; + SmallPtrSet Constants; for (auto &Case : SI.cases()) { Check(isa(SI.getOperand(Case.getCaseIndex() * 2 + 2)), "Case value is not a constant integer.", &SI); @@ -3754,7 +3765,8 @@ void Verifier::visitCallBase(CallBase &Call) { for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i) { if (Call.paramHasAttr(i, Attribute::SwiftError)) { Value *SwiftErrorArg = Call.getArgOperand(i); - if (auto AI = dyn_cast(SwiftErrorArg->stripInBoundsOffsets())) { + if (auto AI = + dyn_cast(SwiftErrorArg->stripInBoundsOffsets())) { Check(AI->isSwiftError(), "swifterror argument for call has mismatched alloca", AI, Call); continue; @@ -3999,7 +4011,8 @@ static bool isTypeCongruent(Type *L, Type *R) { return PL->getAddressSpace() == PR->getAddressSpace(); } -static AttrBuilder getParameterABIAttributes(LLVMContext& C, unsigned I, AttributeList Attrs) { +static AttrBuilder getParameterABIAttributes(LLVMContext &C, unsigned I, + AttributeList Attrs) { static const Attribute::AttrKind ABIAttrs[] = { Attribute::StructRet, Attribute::ByVal, Attribute::InAlloca, Attribute::InReg, Attribute::StackAlignment, Attribute::SwiftSelf, @@ -4067,12 +4080,14 @@ void Verifier::verifyMustTailCall(CallInst &CI) { // - Only sret, byval, swiftself, and swiftasync ABI-impacting attributes // are allowed in swifttailcc call for (unsigned I = 0, E = CallerTy->getNumParams(); I != E; ++I) { - AttrBuilder ABIAttrs = getParameterABIAttributes(F->getContext(), I, CallerAttrs); + AttrBuilder ABIAttrs = + getParameterABIAttributes(F->getContext(), I, CallerAttrs); SmallString<32> Context{CCName, StringRef(" musttail caller")}; verifyTailCCMustTailAttrs(ABIAttrs, Context); } for (unsigned I = 0, E = CalleeTy->getNumParams(); I != E; ++I) { - AttrBuilder ABIAttrs = getParameterABIAttributes(F->getContext(), I, CalleeAttrs); + AttrBuilder ABIAttrs = + getParameterABIAttributes(F->getContext(), I, CalleeAttrs); SmallString<32> Context{CCName, StringRef(" musttail callee")}; verifyTailCCMustTailAttrs(ABIAttrs, Context); } @@ -4098,8 +4113,10 @@ void Verifier::verifyMustTailCall(CallInst &CI) { // - All ABI-impacting function attributes, such as sret, byval, inreg, // returned, preallocated, and inalloca, must match. for (unsigned I = 0, E = CallerTy->getNumParams(); I != E; ++I) { - AttrBuilder CallerABIAttrs = getParameterABIAttributes(F->getContext(), I, CallerAttrs); - AttrBuilder CalleeABIAttrs = getParameterABIAttributes(F->getContext(), I, CalleeAttrs); + AttrBuilder CallerABIAttrs = + getParameterABIAttributes(F->getContext(), I, CallerAttrs); + AttrBuilder CalleeABIAttrs = + getParameterABIAttributes(F->getContext(), I, CalleeAttrs); Check(CallerABIAttrs == CalleeABIAttrs, "cannot guarantee tail call due to mismatched ABI impacting " "function attributes", @@ -4491,7 +4508,7 @@ void Verifier::verifySwiftErrorValue(const Value *SwiftErrorVal) { void Verifier::visitAllocaInst(AllocaInst &AI) { Type *Ty = AI.getAllocatedType(); - SmallPtrSet Visited; + SmallPtrSet Visited; Check(Ty->isSized(&Visited), "Cannot allocate unsized type", &AI); // Check if it's a target extension type that disallows being used on the // stack. @@ -4541,7 +4558,8 @@ void Verifier::visitAtomicRMWInst(AtomicRMWInst &RMWI) { } else if (AtomicRMWInst::isFPOperation(Op)) { Check(ElTy->isFPOrFPVectorTy() && !isa(ElTy), "atomicrmw " + AtomicRMWInst::getOperationName(Op) + - " operand must have floating-point or fixed vector of floating-point " + " operand must have floating-point or fixed vector of " + "floating-point " "type!", &RMWI, ElTy); } else { @@ -5020,7 +5038,7 @@ void Verifier::verifyDominatesUse(Instruction &I, unsigned i) { Check(DT.dominates(Op, U), "Instruction does not dominate all uses!", Op, &I); } -void Verifier::visitDereferenceableMetadata(Instruction& I, MDNode* MD) { +void Verifier::visitDereferenceableMetadata(Instruction &I, MDNode *MD) { Check(I.getType()->isPointerTy(), "dereferenceable, dereferenceable_or_null " "apply only to pointer types", @@ -5353,7 +5371,7 @@ void Verifier::visitInstruction(Instruction &I) { BasicBlock *BB = I.getParent(); Check(BB, "Instruction not embedded in basic block!", &I); - if (!isa(I)) { // Check that non-phi nodes are not self referential + if (!isa(I)) { // Check that non-phi nodes are not self referential for (User *U : I.users()) { Check(U != (User *)&I || !DT.isReachableFromEntry(BB), "Only PHI nodes may reference their own value!", &I); @@ -7097,7 +7115,8 @@ void Verifier::visitVPIntrinsic(VPIntrinsic &VPI) { case Intrinsic::vp_llrint: Check( RetTy->isIntOrIntVectorTy() && ValTy->isFPOrFPVectorTy(), - "llvm.vp.fptoui, llvm.vp.fptosi, llvm.vp.lrint or llvm.vp.llrint" "intrinsic first argument element " + "llvm.vp.fptoui, llvm.vp.fptosi, llvm.vp.lrint or llvm.vp.llrint" + "intrinsic first argument element " "type must be floating-point and result element type must be integer", *VPCast); break; @@ -7587,8 +7606,7 @@ struct VerifierLegacyPass : public FunctionPass { initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry()); } explicit VerifierLegacyPass(bool FatalErrors) - : FunctionPass(ID), - FatalErrors(FatalErrors) { + : FunctionPass(ID), FatalErrors(FatalErrors) { initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry()); } @@ -7626,7 +7644,7 @@ struct VerifierLegacyPass : public FunctionPass { } // end anonymous namespace /// Helper to issue failure from the TBAA verification -template void TBAAVerifier::CheckFailed(Tys &&... Args) { +template void TBAAVerifier::CheckFailed(Tys &&...Args) { if (Diagnostic) return Diagnostic->CheckFailed(Args...); } @@ -7676,7 +7694,8 @@ TBAAVerifier::verifyTBAABaseNodeImpl(Instruction &I, const MDNode *BaseNode, if (IsNewFormat) { if (BaseNode->getNumOperands() % 3 != 0) { CheckFailed("Access tag nodes must have the number of operands that is a " - "multiple of 3!", BaseNode); + "multiple of 3!", + BaseNode); return InvalidNode; } } else { @@ -7689,8 +7708,8 @@ TBAAVerifier::verifyTBAABaseNodeImpl(Instruction &I, const MDNode *BaseNode, // Check the type size field. if (IsNewFormat) { - auto *TypeSizeNode = mdconst::dyn_extract_or_null( - BaseNode->getOperand(1)); + auto *TypeSizeNode = + mdconst::dyn_extract_or_null(BaseNode->getOperand(1)); if (!TypeSizeNode) { CheckFailed("Type size nodes must be constants!", &I, BaseNode); return InvalidNode; @@ -7714,7 +7733,7 @@ TBAAVerifier::verifyTBAABaseNodeImpl(Instruction &I, const MDNode *BaseNode, unsigned FirstFieldOpNo = IsNewFormat ? 3 : 1; unsigned NumOpsPerField = IsNewFormat ? 3 : 2; for (unsigned Idx = FirstFieldOpNo; Idx < BaseNode->getNumOperands(); - Idx += NumOpsPerField) { + Idx += NumOpsPerField) { const MDOperand &FieldTy = BaseNode->getOperand(Idx); const MDOperand &FieldOffset = BaseNode->getOperand(Idx + 1); if (!isa(FieldTy)) { @@ -7828,7 +7847,7 @@ MDNode *TBAAVerifier::getFieldNodeFromTBAABaseNode(Instruction &I, unsigned FirstFieldOpNo = IsNewFormat ? 3 : 1; unsigned NumOpsPerField = IsNewFormat ? 3 : 2; for (unsigned Idx = FirstFieldOpNo; Idx < BaseNode->getNumOperands(); - Idx += NumOpsPerField) { + Idx += NumOpsPerField) { auto *OffsetEntryCI = mdconst::extract(BaseNode->getOperand(Idx + 1)); if (OffsetEntryCI->getValue().ugt(Offset)) { @@ -7847,8 +7866,8 @@ MDNode *TBAAVerifier::getFieldNodeFromTBAABaseNode(Instruction &I, } unsigned LastIdx = BaseNode->getNumOperands() - NumOpsPerField; - auto *LastOffsetEntryCI = mdconst::extract( - BaseNode->getOperand(LastIdx + 1)); + auto *LastOffsetEntryCI = + mdconst::extract(BaseNode->getOperand(LastIdx + 1)); Offset -= LastOffsetEntryCI->getValue(); return cast(BaseNode->getOperand(LastIdx)); } @@ -7893,8 +7912,8 @@ bool TBAAVerifier::visitTBAAMetadata(Instruction &I, const MDNode *MD) { // Check the access size field. if (IsNewFormat) { - auto *AccessSizeNode = mdconst::dyn_extract_or_null( - MD->getOperand(3)); + auto *AccessSizeNode = + mdconst::dyn_extract_or_null(MD->getOperand(3)); CheckTBAA(AccessSizeNode, "Access size field must be a constant", &I, MD); } @@ -7932,8 +7951,8 @@ bool TBAAVerifier::visitTBAAMetadata(Instruction &I, const MDNode *MD) { SmallPtrSet StructPath; for (/* empty */; BaseNode && !IsRootTBAANode(BaseNode); - BaseNode = getFieldNodeFromTBAABaseNode(I, BaseNode, Offset, - IsNewFormat)) { + BaseNode = + getFieldNodeFromTBAABaseNode(I, BaseNode, Offset, IsNewFormat)) { if (!StructPath.insert(BaseNode).second) { CheckFailed("Cycle detected in struct path", &I, MD); return false; @@ -7941,8 +7960,8 @@ bool TBAAVerifier::visitTBAAMetadata(Instruction &I, const MDNode *MD) { bool Invalid; unsigned BaseNodeBitWidth; - std::tie(Invalid, BaseNodeBitWidth) = verifyTBAABaseNode(I, BaseNode, - IsNewFormat); + std::tie(Invalid, BaseNodeBitWidth) = + verifyTBAABaseNode(I, BaseNode, IsNewFormat); // If the base node is invalid in itself, then we've already printed all the // errors we wanted to print. @@ -7987,7 +8006,7 @@ VerifierAnalysis::Result VerifierAnalysis::run(Module &M, VerifierAnalysis::Result VerifierAnalysis::run(Function &F, FunctionAnalysisManager &) { - return { llvm::verifyFunction(F, &dbgs()), false }; + return {llvm::verifyFunction(F, &dbgs()), false}; } PreservedAnalyses VerifierPass::run(Module &M, ModuleAnalysisManager &AM) { diff --git a/llvm/test/Verifier/comdat.ll b/llvm/test/Verifier/comdat.ll index dcf67d89f8d7d..84f401a321480 100644 --- a/llvm/test/Verifier/comdat.ll +++ b/llvm/test/Verifier/comdat.ll @@ -1,5 +1,41 @@ -; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s +; RUN: split-file %s %t +;--- common.ll +; RUN: not llvm-as %t/common.ll -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-COMMON $v = comdat any @v = common global i32 0, comdat($v) -; CHECK: 'common' global may not be in a Comdat! +; CHECK-COMMON: 'common' global may not be in a Comdat! + +;--- private.ll +; RUN: llvm-as %t/private.ll -o /dev/null +; RUN: opt -mtriple=x86_64-unknown-linux %t/private.ll -o /dev/null +; RUN: not opt -mtriple=x86_64-pc-win32 %t/private.ll -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-PRIVATE +$v = comdat any +@v = private global i32 0, comdat($v) +; CHECK-PRIVATE: comdat global value has private linkage + +;--- noleader.ll +; RUN: llvm-as %t/noleader.ll -o /dev/null +; RUN: opt -mtriple=x86_64-unknown-linux %t/noleader.ll -o /dev/null +; RUN: not opt -mtriple=x86_64-pc-win32 %t/noleader.ll -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-NOLEADER + +$v = comdat any +@unrelated = internal global i32 0, comdat($v) +; CHECK-NOLEADER: COFF comdats must have a defined global value with the same name + +;--- undefined.ll +; RUN: llvm-as %t/undefined.ll -o /dev/null +; RUN: opt -mtriple=x86_64-unknown-linux %t/undefined.ll -o /dev/null +; RUN: not opt -mtriple=x86_64-pc-win32 %t/undefined.ll -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED + +$v = comdat any +@v = external global i32 +@unrelated = internal global i32 0, comdat($v) +; CHECK-UNDEFINED: COFF comdats must have a defined global value with the same name + +;--- largest.ll +; RUN: llvm-as %t/largest.ll -o /dev/null +; This used to be invalid, but now it's valid. Ensure the verifier +; doesn't reject it. + +$v = comdat largest From 7e22cd2850ad15234f3868ca5e85edb4ab9a1a41 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Wed, 17 Sep 2025 13:06:35 -0700 Subject: [PATCH 2/4] Revert unintended verifier format changes and delete stale test cases that were folded into comdat.ll --- llvm/lib/IR/Verifier.cpp | 129 ++++++++++++++++------------------ llvm/test/Verifier/comdat2.ll | 7 -- llvm/test/Verifier/comdat3.ll | 5 -- 3 files changed, 59 insertions(+), 82 deletions(-) delete mode 100644 llvm/test/Verifier/comdat2.ll delete mode 100644 llvm/test/Verifier/comdat3.ll diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 0f47049a6457d..3c176b42ad3ae 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -271,7 +271,7 @@ struct VerifierSupport { } template - void WriteTs(const T1 &V1, const Ts &...Vs) { + void WriteTs(const T1 &V1, const Ts &... Vs) { Write(V1); WriteTs(Vs...); } @@ -294,7 +294,7 @@ struct VerifierSupport { /// This calls the Message-only version so that the above is easier to set a /// breakpoint on. template - void CheckFailed(const Twine &Message, const T1 &V1, const Ts &...Vs) { + void CheckFailed(const Twine &Message, const T1 &V1, const Ts &... Vs) { CheckFailed(Message); if (OS) WriteTs(V1, Vs...); @@ -311,7 +311,7 @@ struct VerifierSupport { /// A debug info check failed (with values to print). template void DebugInfoCheckFailed(const Twine &Message, const T1 &V1, - const Ts &...Vs) { + const Ts &... Vs) { DebugInfoCheckFailed(Message); if (OS) WriteTs(V1, Vs...); @@ -715,8 +715,7 @@ void Verifier::visit(Instruction &I) { InstVisitor::visit(I); } -// Helper to iterate over indirect users. By returning false, the callback can -// ask to stop traversing further. +// Helper to iterate over indirect users. By returning false, the callback can ask to stop traversing further. static void forEachUser(const Value *User, SmallPtrSet &Visited, llvm::function_ref Callback) { @@ -725,7 +724,7 @@ static void forEachUser(const Value *User, SmallVector WorkList(User->materialized_users()); while (!WorkList.empty()) { - const Value *Cur = WorkList.pop_back_val(); + const Value *Cur = WorkList.pop_back_val(); if (!Visited.insert(Cur).second) continue; if (Callback(Cur)) @@ -877,8 +876,8 @@ void Verifier::visitGlobalVariable(const GlobalVariable &GV) { } } - if (GV.hasName() && - (GV.getName() == "llvm.used" || GV.getName() == "llvm.compiler.used")) { + if (GV.hasName() && (GV.getName() == "llvm.used" || + GV.getName() == "llvm.compiler.used")) { Check(!GV.hasInitializer() || GV.hasAppendingLinkage(), "invalid linkage for intrinsic global variable", &GV); Check(GV.materialized_use_empty(), @@ -937,14 +936,13 @@ void Verifier::visitGlobalVariable(const GlobalVariable &GV) { } void Verifier::visitAliaseeSubExpr(const GlobalAlias &GA, const Constant &C) { - SmallPtrSet Visited; + SmallPtrSet Visited; Visited.insert(&GA); visitAliaseeSubExpr(Visited, GA, C); } -void Verifier::visitAliaseeSubExpr( - SmallPtrSetImpl &Visited, const GlobalAlias &GA, - const Constant &C) { +void Verifier::visitAliaseeSubExpr(SmallPtrSetImpl &Visited, + const GlobalAlias &GA, const Constant &C) { if (GA.hasAvailableExternallyLinkage()) { Check(isa(C) && cast(C).hasAvailableExternallyLinkage(), @@ -1815,12 +1813,11 @@ void Verifier::visitModuleCommandLines() { void Verifier::visitModuleFlags() { const NamedMDNode *Flags = M.getModuleFlagsMetadata(); - if (!Flags) - return; + if (!Flags) return; // Scan each flag, and track the flags and requirements. - DenseMap SeenIDs; - SmallVector Requirements; + DenseMap SeenIDs; + SmallVector Requirements; uint64_t PAuthABIPlatform = -1; uint64_t PAuthABIVersion = -1; for (const MDNode *MDN : Flags->operands()) { @@ -1865,9 +1862,10 @@ void Verifier::visitModuleFlags() { } } -void Verifier::visitModuleFlag( - const MDNode *Op, DenseMap &SeenIDs, - SmallVectorImpl &Requirements) { +void +Verifier::visitModuleFlag(const MDNode *Op, + DenseMap &SeenIDs, + SmallVectorImpl &Requirements) { // Each module flag should have three arguments, the merge behavior (a // constant int), the flag ID (an MDString), and the value. Check(Op->getNumOperands() == 3, @@ -1946,8 +1944,8 @@ void Verifier::visitModuleFlag( } if (ID->getString() == "wchar_size") { - ConstantInt *Value = - mdconst::dyn_extract_or_null(Op->getOperand(2)); + ConstantInt *Value + = mdconst::dyn_extract_or_null(Op->getOperand(2)); Check(Value, "wchar_size metadata requires constant integer argument"); } @@ -2109,8 +2107,7 @@ void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty, if (!Attr.isStringAttribute() && IncompatibleAttrs.contains(Attr.getKindAsEnum())) { CheckFailed("Attribute '" + Attr.getAsString() + - "' applied to incompatible type!", - V); + "' applied to incompatible type!", V); return; } } @@ -2346,15 +2343,15 @@ void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs, } Check(!Attrs.hasAttrSomewhere(Attribute::Writable) || - isModSet(Attrs.getMemoryEffects().getModRef(IRMemLocation::ArgMem)), + isModSet(Attrs.getMemoryEffects().getModRef(IRMemLocation::ArgMem)), "Attribute writable and memory without argmem: write are incompatible!", V); if (Attrs.hasFnAttr("aarch64_pstate_sm_enabled")) { Check(!Attrs.hasFnAttr("aarch64_pstate_sm_compatible"), - "Attributes 'aarch64_pstate_sm_enabled and " - "aarch64_pstate_sm_compatible' are incompatible!", - V); + "Attributes 'aarch64_pstate_sm_enabled and " + "aarch64_pstate_sm_compatible' are incompatible!", + V); } Check((Attrs.hasFnAttr("aarch64_new_za") + Attrs.hasFnAttr("aarch64_in_za") + @@ -2735,8 +2732,7 @@ void Verifier::verifyStatepoint(const CallBase &Call) { Check(TargetFuncType, "gc.statepoint callee elementtype must be function type", Call); - const int NumCallArgs = - cast(Call.getArgOperand(3))->getZExtValue(); + const int NumCallArgs = cast(Call.getArgOperand(3))->getZExtValue(); Check(NumCallArgs >= 0, "gc.statepoint number of arguments to underlying call " "must be positive", @@ -2755,8 +2751,8 @@ void Verifier::verifyStatepoint(const CallBase &Call) { Check(NumCallArgs == NumParams, "gc.statepoint mismatch in number of call args", Call); - const uint64_t Flags = - cast(Call.getArgOperand(4))->getZExtValue(); + const uint64_t Flags + = cast(Call.getArgOperand(4))->getZExtValue(); Check((Flags & ~(uint64_t)StatepointFlags::MaskAll) == 0, "unknown flag used in gc.statepoint flags argument", Call); @@ -3253,7 +3249,7 @@ void Verifier::visitBasicBlock(BasicBlock &BB) { // it. if (isa(BB.front())) { SmallVector Preds(predecessors(&BB)); - SmallVector, 8> Values; + SmallVector, 8> Values; llvm::sort(Preds); for (const PHINode &PN : BB.phis()) { Check(PN.getNumIncomingValues() == Preds.size(), @@ -3290,7 +3286,8 @@ void Verifier::visitBasicBlock(BasicBlock &BB) { } // Check that all instructions have their parent pointers set up correctly. - for (auto &I : BB) { + for (auto &I : BB) + { Check(I.getParent() == &BB, "Instruction has bogus parent pointer!"); } @@ -3338,7 +3335,7 @@ void Verifier::visitSwitchInst(SwitchInst &SI) { // Check to make sure that all of the constants in the switch instruction // have the same type as the switched-on value. Type *SwitchTy = SI.getCondition()->getType(); - SmallPtrSet Constants; + SmallPtrSet Constants; for (auto &Case : SI.cases()) { Check(isa(SI.getOperand(Case.getCaseIndex() * 2 + 2)), "Case value is not a constant integer.", &SI); @@ -3765,8 +3762,7 @@ void Verifier::visitCallBase(CallBase &Call) { for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i) { if (Call.paramHasAttr(i, Attribute::SwiftError)) { Value *SwiftErrorArg = Call.getArgOperand(i); - if (auto AI = - dyn_cast(SwiftErrorArg->stripInBoundsOffsets())) { + if (auto AI = dyn_cast(SwiftErrorArg->stripInBoundsOffsets())) { Check(AI->isSwiftError(), "swifterror argument for call has mismatched alloca", AI, Call); continue; @@ -4011,8 +4007,7 @@ static bool isTypeCongruent(Type *L, Type *R) { return PL->getAddressSpace() == PR->getAddressSpace(); } -static AttrBuilder getParameterABIAttributes(LLVMContext &C, unsigned I, - AttributeList Attrs) { +static AttrBuilder getParameterABIAttributes(LLVMContext& C, unsigned I, AttributeList Attrs) { static const Attribute::AttrKind ABIAttrs[] = { Attribute::StructRet, Attribute::ByVal, Attribute::InAlloca, Attribute::InReg, Attribute::StackAlignment, Attribute::SwiftSelf, @@ -4080,14 +4075,12 @@ void Verifier::verifyMustTailCall(CallInst &CI) { // - Only sret, byval, swiftself, and swiftasync ABI-impacting attributes // are allowed in swifttailcc call for (unsigned I = 0, E = CallerTy->getNumParams(); I != E; ++I) { - AttrBuilder ABIAttrs = - getParameterABIAttributes(F->getContext(), I, CallerAttrs); + AttrBuilder ABIAttrs = getParameterABIAttributes(F->getContext(), I, CallerAttrs); SmallString<32> Context{CCName, StringRef(" musttail caller")}; verifyTailCCMustTailAttrs(ABIAttrs, Context); } for (unsigned I = 0, E = CalleeTy->getNumParams(); I != E; ++I) { - AttrBuilder ABIAttrs = - getParameterABIAttributes(F->getContext(), I, CalleeAttrs); + AttrBuilder ABIAttrs = getParameterABIAttributes(F->getContext(), I, CalleeAttrs); SmallString<32> Context{CCName, StringRef(" musttail callee")}; verifyTailCCMustTailAttrs(ABIAttrs, Context); } @@ -4113,10 +4106,8 @@ void Verifier::verifyMustTailCall(CallInst &CI) { // - All ABI-impacting function attributes, such as sret, byval, inreg, // returned, preallocated, and inalloca, must match. for (unsigned I = 0, E = CallerTy->getNumParams(); I != E; ++I) { - AttrBuilder CallerABIAttrs = - getParameterABIAttributes(F->getContext(), I, CallerAttrs); - AttrBuilder CalleeABIAttrs = - getParameterABIAttributes(F->getContext(), I, CalleeAttrs); + AttrBuilder CallerABIAttrs = getParameterABIAttributes(F->getContext(), I, CallerAttrs); + AttrBuilder CalleeABIAttrs = getParameterABIAttributes(F->getContext(), I, CalleeAttrs); Check(CallerABIAttrs == CalleeABIAttrs, "cannot guarantee tail call due to mismatched ABI impacting " "function attributes", @@ -4508,7 +4499,7 @@ void Verifier::verifySwiftErrorValue(const Value *SwiftErrorVal) { void Verifier::visitAllocaInst(AllocaInst &AI) { Type *Ty = AI.getAllocatedType(); - SmallPtrSet Visited; + SmallPtrSet Visited; Check(Ty->isSized(&Visited), "Cannot allocate unsized type", &AI); // Check if it's a target extension type that disallows being used on the // stack. @@ -4558,8 +4549,7 @@ void Verifier::visitAtomicRMWInst(AtomicRMWInst &RMWI) { } else if (AtomicRMWInst::isFPOperation(Op)) { Check(ElTy->isFPOrFPVectorTy() && !isa(ElTy), "atomicrmw " + AtomicRMWInst::getOperationName(Op) + - " operand must have floating-point or fixed vector of " - "floating-point " + " operand must have floating-point or fixed vector of floating-point " "type!", &RMWI, ElTy); } else { @@ -5038,7 +5028,7 @@ void Verifier::verifyDominatesUse(Instruction &I, unsigned i) { Check(DT.dominates(Op, U), "Instruction does not dominate all uses!", Op, &I); } -void Verifier::visitDereferenceableMetadata(Instruction &I, MDNode *MD) { +void Verifier::visitDereferenceableMetadata(Instruction& I, MDNode* MD) { Check(I.getType()->isPointerTy(), "dereferenceable, dereferenceable_or_null " "apply only to pointer types", @@ -5371,7 +5361,7 @@ void Verifier::visitInstruction(Instruction &I) { BasicBlock *BB = I.getParent(); Check(BB, "Instruction not embedded in basic block!", &I); - if (!isa(I)) { // Check that non-phi nodes are not self referential + if (!isa(I)) { // Check that non-phi nodes are not self referential for (User *U : I.users()) { Check(U != (User *)&I || !DT.isReachableFromEntry(BB), "Only PHI nodes may reference their own value!", &I); @@ -7115,8 +7105,7 @@ void Verifier::visitVPIntrinsic(VPIntrinsic &VPI) { case Intrinsic::vp_llrint: Check( RetTy->isIntOrIntVectorTy() && ValTy->isFPOrFPVectorTy(), - "llvm.vp.fptoui, llvm.vp.fptosi, llvm.vp.lrint or llvm.vp.llrint" - "intrinsic first argument element " + "llvm.vp.fptoui, llvm.vp.fptosi, llvm.vp.lrint or llvm.vp.llrint" "intrinsic first argument element " "type must be floating-point and result element type must be integer", *VPCast); break; @@ -7606,7 +7595,8 @@ struct VerifierLegacyPass : public FunctionPass { initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry()); } explicit VerifierLegacyPass(bool FatalErrors) - : FunctionPass(ID), FatalErrors(FatalErrors) { + : FunctionPass(ID), + FatalErrors(FatalErrors) { initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry()); } @@ -7644,7 +7634,7 @@ struct VerifierLegacyPass : public FunctionPass { } // end anonymous namespace /// Helper to issue failure from the TBAA verification -template void TBAAVerifier::CheckFailed(Tys &&...Args) { +template void TBAAVerifier::CheckFailed(Tys &&... Args) { if (Diagnostic) return Diagnostic->CheckFailed(Args...); } @@ -7694,8 +7684,7 @@ TBAAVerifier::verifyTBAABaseNodeImpl(Instruction &I, const MDNode *BaseNode, if (IsNewFormat) { if (BaseNode->getNumOperands() % 3 != 0) { CheckFailed("Access tag nodes must have the number of operands that is a " - "multiple of 3!", - BaseNode); + "multiple of 3!", BaseNode); return InvalidNode; } } else { @@ -7708,8 +7697,8 @@ TBAAVerifier::verifyTBAABaseNodeImpl(Instruction &I, const MDNode *BaseNode, // Check the type size field. if (IsNewFormat) { - auto *TypeSizeNode = - mdconst::dyn_extract_or_null(BaseNode->getOperand(1)); + auto *TypeSizeNode = mdconst::dyn_extract_or_null( + BaseNode->getOperand(1)); if (!TypeSizeNode) { CheckFailed("Type size nodes must be constants!", &I, BaseNode); return InvalidNode; @@ -7733,7 +7722,7 @@ TBAAVerifier::verifyTBAABaseNodeImpl(Instruction &I, const MDNode *BaseNode, unsigned FirstFieldOpNo = IsNewFormat ? 3 : 1; unsigned NumOpsPerField = IsNewFormat ? 3 : 2; for (unsigned Idx = FirstFieldOpNo; Idx < BaseNode->getNumOperands(); - Idx += NumOpsPerField) { + Idx += NumOpsPerField) { const MDOperand &FieldTy = BaseNode->getOperand(Idx); const MDOperand &FieldOffset = BaseNode->getOperand(Idx + 1); if (!isa(FieldTy)) { @@ -7847,7 +7836,7 @@ MDNode *TBAAVerifier::getFieldNodeFromTBAABaseNode(Instruction &I, unsigned FirstFieldOpNo = IsNewFormat ? 3 : 1; unsigned NumOpsPerField = IsNewFormat ? 3 : 2; for (unsigned Idx = FirstFieldOpNo; Idx < BaseNode->getNumOperands(); - Idx += NumOpsPerField) { + Idx += NumOpsPerField) { auto *OffsetEntryCI = mdconst::extract(BaseNode->getOperand(Idx + 1)); if (OffsetEntryCI->getValue().ugt(Offset)) { @@ -7866,8 +7855,8 @@ MDNode *TBAAVerifier::getFieldNodeFromTBAABaseNode(Instruction &I, } unsigned LastIdx = BaseNode->getNumOperands() - NumOpsPerField; - auto *LastOffsetEntryCI = - mdconst::extract(BaseNode->getOperand(LastIdx + 1)); + auto *LastOffsetEntryCI = mdconst::extract( + BaseNode->getOperand(LastIdx + 1)); Offset -= LastOffsetEntryCI->getValue(); return cast(BaseNode->getOperand(LastIdx)); } @@ -7912,8 +7901,8 @@ bool TBAAVerifier::visitTBAAMetadata(Instruction &I, const MDNode *MD) { // Check the access size field. if (IsNewFormat) { - auto *AccessSizeNode = - mdconst::dyn_extract_or_null(MD->getOperand(3)); + auto *AccessSizeNode = mdconst::dyn_extract_or_null( + MD->getOperand(3)); CheckTBAA(AccessSizeNode, "Access size field must be a constant", &I, MD); } @@ -7951,8 +7940,8 @@ bool TBAAVerifier::visitTBAAMetadata(Instruction &I, const MDNode *MD) { SmallPtrSet StructPath; for (/* empty */; BaseNode && !IsRootTBAANode(BaseNode); - BaseNode = - getFieldNodeFromTBAABaseNode(I, BaseNode, Offset, IsNewFormat)) { + BaseNode = getFieldNodeFromTBAABaseNode(I, BaseNode, Offset, + IsNewFormat)) { if (!StructPath.insert(BaseNode).second) { CheckFailed("Cycle detected in struct path", &I, MD); return false; @@ -7960,8 +7949,8 @@ bool TBAAVerifier::visitTBAAMetadata(Instruction &I, const MDNode *MD) { bool Invalid; unsigned BaseNodeBitWidth; - std::tie(Invalid, BaseNodeBitWidth) = - verifyTBAABaseNode(I, BaseNode, IsNewFormat); + std::tie(Invalid, BaseNodeBitWidth) = verifyTBAABaseNode(I, BaseNode, + IsNewFormat); // If the base node is invalid in itself, then we've already printed all the // errors we wanted to print. @@ -8006,7 +7995,7 @@ VerifierAnalysis::Result VerifierAnalysis::run(Module &M, VerifierAnalysis::Result VerifierAnalysis::run(Function &F, FunctionAnalysisManager &) { - return {llvm::verifyFunction(F, &dbgs()), false}; + return { llvm::verifyFunction(F, &dbgs()), false }; } PreservedAnalyses VerifierPass::run(Module &M, ModuleAnalysisManager &AM) { diff --git a/llvm/test/Verifier/comdat2.ll b/llvm/test/Verifier/comdat2.ll deleted file mode 100644 index b461d5846f9ca..0000000000000 --- a/llvm/test/Verifier/comdat2.ll +++ /dev/null @@ -1,7 +0,0 @@ -; RUN: llvm-as %s -o /dev/null -; RUN: opt -mtriple=x86_64-unknown-linux -o /dev/null -; RUN: not opt -mtriple=x86_64-pc-win32 %s -o /dev/null 2>&1 | FileCheck %s - -$v = comdat any -@v = private global i32 0, comdat($v) -; CHECK: comdat global value has private linkage diff --git a/llvm/test/Verifier/comdat3.ll b/llvm/test/Verifier/comdat3.ll deleted file mode 100644 index 28df930d9dddf..0000000000000 --- a/llvm/test/Verifier/comdat3.ll +++ /dev/null @@ -1,5 +0,0 @@ -; This used to be invalid, but now it's valid. Ensure the verifier -; doesn't reject it. -; RUN: llvm-as %s -o /dev/null - -$v = comdat largest From 72af173cd9eac3f05c73566b52de06e91092e47c Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Wed, 17 Sep 2025 14:40:05 -0700 Subject: [PATCH 3/4] Update llvm/lib/IR/Verifier.cpp Co-authored-by: Matheus Izvekov --- llvm/lib/IR/Verifier.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 3c176b42ad3ae..109373fbc68e2 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -1767,12 +1767,13 @@ void Verifier::visitComdat(const Comdat &C) { if (TT.isOSBinFormatCOFF()) { const GlobalValue *GV = M.getNamedValue(C.getName()); bool IsDefined = GV != nullptr && !GV->isDeclarationForLinker(); - Check(IsDefined || C.getUsers().empty(), - "COFF comdats must have a defined global value with the same name", - GV); if (IsDefined) Check(!GV->hasPrivateLinkage(), "comdat global value has private linkage", GV); + else + Check(C.getUsers().empty(), + "COFF comdats must have a defined global value with the same name", + GV); } } From dc7944742c8724a09a220dfd4070841eb65382ce Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Wed, 17 Sep 2025 14:50:23 -0700 Subject: [PATCH 4/4] Format the new code --- llvm/lib/IR/Verifier.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 109373fbc68e2..adc9a4c17b88b 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -1772,8 +1772,8 @@ void Verifier::visitComdat(const Comdat &C) { GV); else Check(C.getUsers().empty(), - "COFF comdats must have a defined global value with the same name", - GV); + "COFF comdats must have a defined global value with the same name", + GV); } }