IR: Disable verifier check for GlobalValues with private linkage named after a comdat for non-COFF.

[pcc]

Summary

This check is only meaningful for COFF and it is perfectly valid to create
such a GlobalValue in ELF.

[pcc]

• Remove decl

[rnk]

I haven't had time to really think about this, but I believe @majnemer felt it was important to add verifier checks like these. There's no reason we can't make this triple dependent, right? If there's no triple on the module, we can skip the check.

[pcc]

Making it conditional on the triple is fine with me I suppose. It's unfortunate that we use the same IR for ELF and COFF here, maybe longer term we should move towards exclusively using !associated in COFF.

[pcc]

• Check the triple

[rnk]

In the past, Rafael advocated for having more verifier check more things that are unrepresentable at an object file level. That's how we ended up with most of the checks in Verifier::visitGlobalAlias. I think this is consistent with that, even if we have to check which object file invariant we're enforcing.

lgtm

llvm/lib/IR/Verifier.cpp

for (const GlobalVariable &GV : M.globals())
       visitGlobalVariable(GV);
 
     for (const GlobalAlias &GA : M.aliases())
       visitGlobalAlias(GA);
 
     for (const NamedMDNode &NMD : M.named_metadata())
       visitNamedMDNode(NMD);
 
-    for (const StringMapEntry<Comdat> &SMEC : M.getComdatSymbolTable())
-      visitComdat(SMEC.getValue());
-
     visitModuleFlags(M);
     visitModuleIdents(M);
     visitModuleCommandLines(M);
 
     verifyCompileUnits();
 
     verifyDeoptimizeCallingConvs();
     DISubprogramAttachments.clear();
     return !Broken;
   }
 
 private:
   // Verification methods...
   void visitGlobalValue(const GlobalValue &GV);
   void visitGlobalVariable(const GlobalVariable &GV);
   void visitGlobalAlias(const GlobalAlias &GA);
   void visitAliaseeSubExpr(const GlobalAlias &A, const Constant &C);
   void visitAliaseeSubExpr(SmallPtrSetImpl<const GlobalAlias *> &Visited,
                            const GlobalAlias &A, const Constant &C);
   void visitNamedMDNode(const NamedMDNode &NMD);
   void visitMDNode(const MDNode &MD);
   void visitMetadataAsValue(const MetadataAsValue &MD, Function *F);
   void visitValueAsMetadata(const ValueAsMetadata &MD, Function *F);
-  void visitComdat(const Comdat &C);
   void visitModuleIdents(const Module &M);
   void visitModuleCommandLines(const Module &M);
   void visitModuleFlags(const Module &M);
   void visitModuleFlag(const MDNode *Op,
                        DenseMap<const MDString *, const MDNode *> &SeenIDs,
                        SmallVectorImpl<const MDNode *> &Requirements);
   void visitModuleFlagCGProfileEntry(const MDOperand &MDO);
   void visitFunction(const Function &F);
AssertDI(N.getTag() == dwarf::DW_TAG_imported_module ||
                N.getTag() == dwarf::DW_TAG_imported_declaration,
            "invalid tag", &N);
   if (auto *S = N.getRawScope())
     AssertDI(isa<DIScope>(S), "invalid scope for imported entity", &N, S);
   AssertDI(isDINode(N.getRawEntity()), "invalid imported entity", &N,
            N.getRawEntity());
 }
 
-void Verifier::visitComdat(const Comdat &C) {
-  // The Module is invalid if the GlobalValue has private linkage.  Entities
-  // with private linkage don't have entries in the symbol table.
-  if (const GlobalValue *GV = M.getNamedValue(C.getName()))
-    Assert(!GV->hasPrivateLinkage(), "comdat global value has private linkage",
-           GV);
-}
-
 void Verifier::visitModuleIdents(const Module &M) {
   const NamedMDNode *Idents = M.getNamedMetadata("llvm.ident");
   if (!Idents)
     return;
 
   // llvm.ident takes a list of metadata entry. Each entry has only one string.
   // Scan each llvm.ident entry and make sure that this requirement is met.
   for (const MDNode *N : Idents->operands()) {

llvm/test/Verifier/comdat2.ll

-; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+; 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 any
 @v = private global i32 0, comdat($v)
-; CHECK: comdat global value has private linkage