Allow the LTO code generator to drop malformed debug info from the input

[aprantl]

Summary

This patch introduces a new option -lto-strip-invalid-debug-info, which drops malformed debug info from the input.

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

[mehdi_amini]

lib/IR/Verifier.cpp

4440

The only difference between this function and the one above is the output parameter BrokenDebugInfo right?

What about having a single function with an optional argument, something like:

// Returns true if the module is broken. If BrokenDebugInfo is supplied, DebugInfo 
// verification failures won't be considered as error and instead *BrokenDebugInfo
// will be set to true.
bool llvm::verifyModule(const Module &M, raw_ostream *OS, bool *BrokenDebugInfo) {
  // Don't use a raw_null_ostream.  Printing IR is expensive.
  Verifier V(OS, /*TreatBrokenDebugInfoAsError=*/!BrokenDebugInfo);

  bool Broken = false;
  for (const Function &F : M)
    if (!F.isDeclaration() && !F.isMaterializable())
      Broken |= !V.verify(F);
  Broken |= !V.verify(M)
  if (BrokenDebugInfo) *BrokenDebugInfo = V.hasBrokenDebugInfo();

  // Note that this function's return value is inverted from what you would
  // expect of a function called "verify".
  return Broken;
}

4442

s/treatBrokenDebugInfoAsError/TreatBrokenDebugInfoAsError/

4448

This statement unconditionally overwrite Broken, I doubt this is what you intended?

lib/LTO/LTOCodeGenerator.cpp

85 ↗

(On Diff #56318)

What about:

cl::desc("Strip invalid debug info metadata during LTO instead of aborting."),

?

[aprantl]

lib/IR/Verifier.cpp

4440

For some reason I thought that that would break the C API, however, after actually checking I now realize that it uses a wrapper function (LLVMVerifyModule) anyway, so, yes, we can definitely do that!

4448

Yes, that should be a |=.

[aprantl]

lib/IR/Verifier.cpp

4442

The function argument is really spelled like that to disambiguate it from the member.

[aprantl]

Address review feedback.

[mehdi_amini]

lib/IR/Verifier.cpp

4442

I know, but I doubt this is in line with the usual practices / dev guide.

[aprantl]

lib/IR/Verifier.cpp

4442

Is your suggestion to

• rename the function argument to match the member,
• just rename the comment,
• rename the function argument to something distinct from the member but starting with an uppercase letter

?

[mehdi_amini]

lib/IR/Verifier.cpp

4442

Here I'm only pointing at the comment, which should match the function parameter name.

I commented in D19986 for the name of the parameter itself here: http://reviews.llvm.org/D19986#inline-168017

[aprantl]

Rename function argument.

[mehdi_amini]

lib/IR/Verifier.cpp

263

My impression is that D19986 hasn't been committed, I was expecting D19986 to be updated instead?

[aprantl]

Yes, I was planning to commit all three patches in a row, and I will also apply the rename to D19986.

[aprantl]

Rebased patch.

[aprantl]

Rebased on 268778. I also noticed that I messed up while preparing the patch and posted a version that didn't actually strip the invalid debug info in the transformation. This is fixed now.

[mehdi_amini]

include/llvm/IR/Verifier.h

57

Not defined anywhere?

77

Not defined anywhere?

lib/IR/Verifier.cpp

4506

I'd be worried if Res.DebugInfoBroken && !StripDebugInfo(M) ; it seems to be the cause for an internal error.

[aprantl]

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

[mehdi_amini]

LGTM/=

[mehdi_amini]

I think we'll to double check the LTO flow after this gets in, because the verifier is also run in the pass pipeline, and we should avoid running it two times.

[aprantl]

Thanks, committed as r268936!

[mehdi_amini]

In D19987#424207, @joker.eph wrote:

I think we'll to double check the LTO flow after this gets in, because the verifier is also run in the pass pipeline, and we should avoid running it two times.

For posterity: Adrian and me double-checked offline and the second run of the verifier at the beginning of the optimizer only occurs for assert build. Even in this case, there is the internalization that takes place in-between so it is not "useless". We don't plan any further actions here.

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

 /// non-null) and true is returned.
 ///
 /// \return true if the module is broken. If BrokenDebugInfo is
 /// supplied, DebugInfo verification failures won't be considered as
 /// error and instead *BrokenDebugInfo will be set to true.
 bool verifyModule(const Module &M, raw_ostream *OS = nullptr,
                   bool *BrokenDebugInfo = 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

 #include "llvm/IR/CFG.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
class Verifier : public InstVisitor<Verifier>, VerifierSupport {
   void updateModule(const Module *NewM) {
     if (M == NewM)
       return;
     MST.emplace(NewM);
     M = NewM;
   }
 
 public:
   explicit Verifier(raw_ostream *OS, bool ShouldTreatBrokenDebugInfoAsError)
       : VerifierSupport(OS), Context(nullptr), LandingPadResultTy(nullptr),
         SawFrameEscape(false) {
     TreatBrokenDebugInfoAsError = ShouldTreatBrokenDebugInfoAsError;
   }
 
   bool hasBrokenDebugInfo() const { return BrokenDebugInfo; }
 
   bool verify(const Function &F) {
bool llvm::verifyModule(const Module &M, raw_ostream *OS,
   if (BrokenDebugInfo)
     *BrokenDebugInfo = V.hasBrokenDebugInfo();
   // Note that this function's return value is inverted from what you would
   // expect of a function called "verify".
   return Broken;
 }
 
 namespace {
 struct VerifierLegacyPass : public FunctionPass {
   static char ID;
 
   Verifier V;
   bool FatalErrors;
 
   VerifierLegacyPass()
       : FunctionPass(ID),
         V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/true),
         FatalErrors(true) {
     initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry());
   }
   explicit VerifierLegacyPass(bool FatalErrors)
       : FunctionPass(ID),
         V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/true),
         FatalErrors(FatalErrors) {
     initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry());

 
 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)
+char VerifierAnalysis::PassID;
+VerifierAnalysis::Result VerifierAnalysis::run(Module &M) {
+  Result Res;
+  Res.IRBroken = llvm::verifyModule(M, &dbgs(), &Res.DebugInfoBroken);
+  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) {
+    assert(!Res.DebugInfoBroken && "Module contains invalid debug info");
+    if (Res.IRBroken)
       report_fatal_error("Broken module found, compilation aborted!");
+  }
 
+  // Strip broken debug info.
+  if (Res.DebugInfoBroken && StripDebugInfo(M)) {
+    DiagnosticInfoIgnoringInvalidDebugMetadata DiagInvalid(M);
+    M.getContext().diagnose(DiagInvalid);
+  }
   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