Separate the Verifier into an analysis and a transformation pass optionally stripping malformed debug info.

[aprantl]

Summary

This patch separates the Verifier into an analysis and a transformation pass, with the transformation pass optionally stripping malformed debug info.

The problem I'm trying to solve with this sequence of patches is that historically we've done a really bad job at verifying debug info. We want to be able to make the verifier stricter without having to worry about breaking bitcode compatibility with existing producers. For example, we don't necessarily want IR produced by an older version of clang to be rejected by an LTO link just because of malformed debug info, and rather provide an option to strip it. Note that merely outdated (but well-formed) debug info would continue to be auto-upgraded in this scenario.

rdar://problem/25818489

[aprantl]

This patch is doing the the same thing that D19987 does for LTO for the regular pipeline. Frontends like clang invoke the Verifier by adding it to the pass pipeline.

[aprantl]

Rebase on trunk, address review feedback, and upload the patch to the correct review this time m(

[mehdi_amini]

include/llvm/IR/Verifier.h

51

Doxygen?

lib/IR/DiagnosticInfo.cpp

127

I'm worried what this displays, should it be getModule().getModuleIdentifier() or something similar?

lib/IR/Verifier.cpp

4514

So from now on the verifier will never assert on invalid debug info but will drop them right?

[aprantl]

lib/IR/Verifier.cpp

4514

No, in an Asserts build, it will assert on invalid debug info, iff fatal errors are requested (which is the default and pretty much only disabled for unit tests).

[aprantl]

Thanks! Committed as r268937.

include/llvm/IR/DiagnosticInfo.h

 /// \brief Defines the different supported kind of a diagnostic.
 /// This enum should be extended with a new ID for each added concrete subclass.
 enum DiagnosticKind {
   DK_Bitcode,
   DK_InlineAsm,
   DK_StackSize,
   DK_Linker,
   DK_DebugMetadataVersion,
+  DK_DebugMetadataInvalid,
   DK_SampleProfile,
   DK_OptimizationRemark,
   DK_OptimizationRemarkMissed,
   DK_OptimizationRemarkAnalysis,
   DK_OptimizationRemarkAnalysisFPCommute,
   DK_OptimizationRemarkAnalysisAliasing,
   DK_OptimizationFailure,
   DK_FirstRemark = DK_OptimizationRemark,
public:
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
 
   static bool classof(const DiagnosticInfo *DI) {
     return DI->getKind() == DK_DebugMetadataVersion;
   }
 };
 
+/// Diagnostic information for stripping invalid debug metadata.
+class DiagnosticInfoIgnoringInvalidDebugMetadata : public DiagnosticInfo {
+private:
+  /// The module that is concerned by this debug metadata version diagnostic.
+  const Module &M;
+
+public:
+  /// \p The module that is concerned by this debug metadata version diagnostic.
+  DiagnosticInfoIgnoringInvalidDebugMetadata(
+      const Module &M, DiagnosticSeverity Severity = DS_Warning)
+      : DiagnosticInfo(DK_DebugMetadataVersion, Severity), M(M) {}
+
+  const Module &getModule() const { return M; }
+
+  /// \see DiagnosticInfo::print.
+  void print(DiagnosticPrinter &DP) const override;
+
+  static bool classof(const DiagnosticInfo *DI) {
+    return DI->getKind() == DK_DebugMetadataInvalid;
+  }
+};
+
+
 /// Diagnostic information for the sample profiler.
 class DiagnosticInfoSampleProfile : public DiagnosticInfo {
 public:
   DiagnosticInfoSampleProfile(StringRef FileName, unsigned LineNum,
                               const Twine &Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), FileName(FileName),
         LineNum(LineNum), Msg(Msg) {}

include/llvm/IR/Verifier.h

 
 /// \brief Check a module for errors.
 ///
 /// If there are no errors, the function returns false. If an error is found,
 /// a message describing the error is written to OS (if non-null) and true is
 /// returned.
 bool verifyModule(const Module &M, raw_ostream *OS = nullptr);
 
+/// Check a module for errors, but report debug info errors separately.
+/// Otherwise behaves as the normal verifyModule. Debug info errors can be
+/// "recovered" from by stripping the debug info.
+bool verifyModule(bool &BrokenDebugInfo, const Module &M, raw_ostream *OS);
+
+FunctionPass *createVerifierPass(bool FatalErrors = true);
+
+class VerifierAnalysis : public AnalysisInfoMixin<VerifierAnalysis> {
+  friend AnalysisInfoMixin<VerifierAnalysis>;
+  static char PassID;
+
+public:
+  struct Result {
+    bool IRBroken, DebugInfoBroken;
+  };
+  static void *ID() { return (void *)&PassID; }
+  Result run(Module &M);
+  Result run(Function &F);
+};
+
+/// Check a module for errors, but report debug info errors separately.
+/// Otherwise behaves as the normal verifyModule. Debug info errors can be
+/// "recovered" from by stripping the debug info.
+bool verifyModule(bool &BrokenDebugInfo, const Module &M, raw_ostream *OS);
+
 /// \brief Create a verifier pass.
 ///
 /// Check a module or function for validity. This is essentially a pass wrapped
 /// around the above verifyFunction and verifyModule routines and
 /// functionality. When the pass detects a verification error it is always
 /// printed to stderr, and by default they are fatal. You can override that by
 /// passing \c false to \p FatalErrors.
 ///
 /// Note that this creates a pass suitable for the legacy pass manager. It has
 /// nothing to do with \c VerifierPass.
-FunctionPass *createVerifierPass(bool FatalErrors = true);
-
 class VerifierPass : public PassInfoMixin<VerifierPass> {
   bool FatalErrors;
 
 public:
   explicit VerifierPass(bool FatalErrors = true) : FatalErrors(FatalErrors) {}
 
-  PreservedAnalyses run(Module &M);
-  PreservedAnalyses run(Function &F);
+  PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 };
 
+
 } // End llvm namespace
 
 #endif

lib/IR/DiagnosticInfo.cpp

DP << "stack size limit exceeded (" << getStackSize() << ") in "
      << getFunction();
 }
 
 void DiagnosticInfoDebugMetadataVersion::print(DiagnosticPrinter &DP) const {
   DP << "ignoring debug info with an invalid version (" << getMetadataVersion()
      << ") in " << getModule();
 }
 
+void DiagnosticInfoIgnoringInvalidDebugMetadata::print(
+    DiagnosticPrinter &DP) const {
+  DP << "ignoring invalid debug info in " << getModule();
+}
+
 void DiagnosticInfoSampleProfile::print(DiagnosticPrinter &DP) const {
   if (!FileName.empty()) {
     DP << getFileName();
     if (LineNum > 0)
       DP << ":" << getLineNum();
     DP << ": ";
   }
   DP << getMsg();

lib/IR/Verifier.cpp

for (const Function &F : M)
     if (!F.isDeclaration() && !F.isMaterializable())
       Broken |= !V.verify(F);
 
   Broken = !V.verify(M);
   BrokenDebugInfo = V.hasBrokenDebugInfo();
   return Broken;
 }
 
-
 namespace {
 struct VerifierLegacyPass : public FunctionPass {
   static char ID;
 
   Verifier V;
   bool FatalErrors;
 
   VerifierLegacyPass() : FunctionPass(ID), V(&dbgs()), FatalErrors(true) {

 
 char VerifierLegacyPass::ID = 0;
 INITIALIZE_PASS(VerifierLegacyPass, "verify", "Module Verifier", false, false)
 
 FunctionPass *llvm::createVerifierPass(bool FatalErrors) {
   return new VerifierLegacyPass(FatalErrors);
 }
 
-PreservedAnalyses VerifierPass::run(Module &M) {
-  if (verifyModule(M, &dbgs()) && FatalErrors)
-    report_fatal_error("Broken module found, compilation aborted!");
+char VerifierAnalysis::PassID;
+VerifierAnalysis::Result VerifierAnalysis::run(Module &M) {
+  Result Res;
+  Res.IRBroken = llvm::verifyModule(Res.DebugInfoBroken, M, &dbgs());
+  return Res;
+}
 
+VerifierAnalysis::Result VerifierAnalysis::run(Function &F) {
+  return { llvm::verifyFunction(F, &dbgs()), false };
+}
+
+PreservedAnalyses VerifierPass::run(Module &M, ModuleAnalysisManager &AM) {
+  auto Res = AM.getResult<VerifierAnalysis>(M);
+  if (FatalErrors) {
+    if (Res.IRBroken || Res.DebugInfoBroken)
+      report_fatal_error("Broken module found, compilation aborted!");
+  }
   return PreservedAnalyses::all();
 }
 
-PreservedAnalyses VerifierPass::run(Function &F) {
-  if (verifyFunction(F, &dbgs()) && FatalErrors)
+PreservedAnalyses VerifierPass::run(Function &F, FunctionAnalysisManager &AM) {
+  auto res = AM.getResult<VerifierAnalysis>(F);
+  if (res.IRBroken && FatalErrors)
     report_fatal_error("Broken function found, compilation aborted!");
 
   return PreservedAnalyses::all();
 }

lib/Passes/PassRegistry.def

 
 #ifndef MODULE_ANALYSIS
 #define MODULE_ANALYSIS(NAME, CREATE_PASS)
 #endif
 MODULE_ANALYSIS("callgraph", CallGraphAnalysis())
 MODULE_ANALYSIS("lcg", LazyCallGraphAnalysis())
 MODULE_ANALYSIS("no-op-module", NoOpModuleAnalysis())
 MODULE_ANALYSIS("targetlibinfo", TargetLibraryAnalysis())
+MODULE_ANALYSIS("verify", VerifierAnalysis())
 
 #ifndef MODULE_ALIAS_ANALYSIS
 #define MODULE_ALIAS_ANALYSIS(NAME, CREATE_PASS)                             \
   MODULE_ANALYSIS(NAME, CREATE_PASS)
 #endif
 MODULE_ALIAS_ANALYSIS("globals-aa", GlobalsAA())
 #undef MODULE_ALIAS_ANALYSIS
 #undef MODULE_ANALYSIS

 FUNCTION_ANALYSIS("loops", LoopAnalysis())
 FUNCTION_ANALYSIS("memdep", MemoryDependenceAnalysis())
 FUNCTION_ANALYSIS("regions", RegionInfoAnalysis())
 FUNCTION_ANALYSIS("no-op-function", NoOpFunctionAnalysis())
 FUNCTION_ANALYSIS("scalar-evolution", ScalarEvolutionAnalysis())
 FUNCTION_ANALYSIS("targetlibinfo", TargetLibraryAnalysis())
 FUNCTION_ANALYSIS("targetir",
                   TM ? TM->getTargetIRAnalysis() : TargetIRAnalysis())
+FUNCTION_ANALYSIS("verify", VerifierAnalysis())
 
 #ifndef FUNCTION_ALIAS_ANALYSIS
 #define FUNCTION_ALIAS_ANALYSIS(NAME, CREATE_PASS)                             \
   FUNCTION_ANALYSIS(NAME, CREATE_PASS)
 #endif
 FUNCTION_ALIAS_ANALYSIS("basic-aa", BasicAA())
 FUNCTION_ALIAS_ANALYSIS("cfl-aa", CFLAA())
 FUNCTION_ALIAS_ANALYSIS("scev-aa", SCEVAA())

unittests/IR/VerifierTest.cpp

TEST(VerifierTest, CrossModuleMetadataRef) {
   dbuilder.finalize();
 
   std::string Error;
   raw_string_ostream ErrorOS(Error);
   EXPECT_TRUE(verifyModule(M2, &ErrorOS));
   EXPECT_TRUE(StringRef(ErrorOS.str())
                   .startswith("Referencing global in another module!"));
 }
+
+TEST(VerifierTest, StripInvalidDebugInfo) {
+  LLVMContext C;
+  Module M("M", C);
+  DIBuilder DIB(M);
+  DIB.createCompileUnit(dwarf::DW_LANG_C89, "broken.c", "/",
+                        "unittest", false, "", 0);
+  DIB.finalize();
+  EXPECT_FALSE(verifyModule(M));
+
+  // Now break it.
+  auto *File = DIB.createFile("not-a-CU.f", ".");
+  NamedMDNode *NMD = M.getOrInsertNamedMetadata("llvm.dbg.cu");
+  NMD->addOperand(File);
+  EXPECT_TRUE(verifyModule(M));
+
+  ModulePassManager MPM(true);
+  MPM.addPass(VerifierPass(false));
+  ModuleAnalysisManager MAM(true);
+  MAM.registerPass([&] { return VerifierAnalysis(); });
+  MPM.run(M, MAM);
+  EXPECT_FALSE(verifyModule(M));
+}
+
+
 } // end anonymous namespace
 } // end namespace llvm