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

4438

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;
}

4440

s/treatBrokenDebugInfoAsError/TreatBrokenDebugInfoAsError/

4446

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

lib/LTO/LTOCodeGenerator.cpp

85

What about:

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

?

[aprantl]

lib/IR/Verifier.cpp

4438

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!

4446

Yes, that should be a |=.

[aprantl]

lib/IR/Verifier.cpp

4440

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

[aprantl]

Address review feedback.

[mehdi_amini]

lib/IR/Verifier.cpp

4440

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

[aprantl]

lib/IR/Verifier.cpp

4440

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

4440

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

262

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

4497

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/Verifier.h

 ///
 /// 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 verifyFunction(const Function &F, raw_ostream *OS = nullptr);
 
 /// \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);
+/// 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.
+///
+/// \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);
 
 /// \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);
 };
 
 } // End llvm namespace
 
 #endif

include/llvm/LTO/LTOCodeGenerator.h

private:
   bool determineTarget();
   std::unique_ptr<TargetMachine> createTargetMachine();
 
   static void DiagnosticHandler(const DiagnosticInfo &DI, void *Context);
 
   void DiagnosticHandler2(const DiagnosticInfo &DI);
 
   void emitError(const std::string &ErrMsg);
+  void emitWarning(const std::string &WarnMsg);
 
   LLVMContext &Context;
   std::unique_ptr<Module> MergedModule;
   std::unique_ptr<Linker> TheLinker;
   std::unique_ptr<TargetMachine> TargetMach;
   bool EmitDwarfDebugInfo = false;
   bool ScopeRestrictionsDone = false;
   bool HasVerifiedInput = false;

lib/IR/Verifier.cpp

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 treatBrokenDebugInfoAsError = true)
+  explicit Verifier(raw_ostream *OS, bool ShouldTreatBrokenDebugInfoAsError)
       : VerifierSupport(OS), Context(nullptr), LandingPadResultTy(nullptr),
         SawFrameEscape(false) {
-    TreatBrokenDebugInfoAsError = treatBrokenDebugInfoAsError;
+    TreatBrokenDebugInfoAsError = ShouldTreatBrokenDebugInfoAsError;
   }
 
   bool hasBrokenDebugInfo() const { return BrokenDebugInfo; }
 
   bool verify(const Function &F) {
     updateModule(F.getParent());
     Context = &M->getContext();
 

 //  Implement the public interfaces to this file...
 //===----------------------------------------------------------------------===//
 
 bool llvm::verifyFunction(const Function &f, raw_ostream *OS) {
   Function &F = const_cast<Function &>(f);
   assert(!F.isDeclaration() && "Cannot verify external functions");
 
   // Don't use a raw_null_ostream.  Printing IR is expensive.
-  Verifier V(OS);
+  Verifier V(OS, /*ShouldTreatBrokenDebugInfoAsError=*/true);
 
   // Note that this function's return value is inverted from what you would
   // expect of a function called "verify".
   return !V.verify(F);
 }
 
-bool llvm::verifyModule(const Module &M, raw_ostream *OS) {
+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);
+  Verifier V(OS, /*ShouldTreatBrokenDebugInfoAsError=*/!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 !V.verify(M) || Broken;
+  return Broken;
 }
 
 namespace {
 struct VerifierLegacyPass : public FunctionPass {
   static char ID;
 
   Verifier V;
   bool FatalErrors;
 
-  VerifierLegacyPass() : FunctionPass(ID), V(&dbgs()), FatalErrors(true) {
+  VerifierLegacyPass()
+      : FunctionPass(ID),
+        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/true),
+        FatalErrors(true) {
     initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry());
   }
   explicit VerifierLegacyPass(bool FatalErrors)
-      : FunctionPass(ID), V(&dbgs()), FatalErrors(FatalErrors) {
+      : FunctionPass(ID),
+        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/true),
+        FatalErrors(FatalErrors) {
     initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry());
   }
 
   bool runOnFunction(Function &F) override {
     if (!V.verify(F) && FatalErrors)
       report_fatal_error("Broken function found, compilation aborted!");
 
     return false;
PreservedAnalyses VerifierPass::run(Module &M) {
   return PreservedAnalyses::all();
 }
 
 PreservedAnalyses VerifierPass::run(Function &F) {
   if (verifyFunction(F, &dbgs()) && FatalErrors)
     report_fatal_error("Broken function found, compilation aborted!");
 
   return PreservedAnalyses::all();
 }

lib/LTO/LTOCodeGenerator.cpp

 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/CodeGen/ParallelCG.h"
 #include "llvm/CodeGen/RuntimeLibcalls.h"
 #include "llvm/Config/config.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/DiagnosticPrinter.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Verifier.h"
cl::opt<bool> LTODiscardValueNames(
     "lto-discard-value-names",
     cl::desc("Strip names from Value during LTO (other than GlobalValue)."),
 #ifdef NDEBUG
     cl::init(true),
 #else
     cl::init(false),
 #endif
     cl::Hidden);
+
+cl::opt<bool> LTOStripInvalidDebugInfo(
+    "lto-strip-invalid-debug-info",
+    cl::desc("Strip invalid debug info metadata during LTO instead of aborting."),
+#ifdef NDEBUG
+    cl::init(true),
+#else
+    cl::init(false),
+#endif
+    cl::Hidden);
 }
 
 LTOCodeGenerator::LTOCodeGenerator(LLVMContext &Context)
     : Context(Context), MergedModule(new Module("ld-temp.o", Context)),
       TheLinker(new Linker(*MergedModule)) {
   Context.setDiscardValueNames(LTODiscardValueNames);
   Context.enableDebugTypeODRUniquing();
   initializeLTOPasses();

 }
 
 void LTOCodeGenerator::verifyMergedModuleOnce() {
   // Only run on the first call.
   if (HasVerifiedInput)
     return;
   HasVerifiedInput = true;
 
+  if (LTOStripInvalidDebugInfo) {
+    bool BrokenDebugInfo = false;
+    if (verifyModule(*MergedModule, &dbgs(), &BrokenDebugInfo))
+      report_fatal_error("Broken module found, compilation aborted!");
+    if (BrokenDebugInfo) {
+      emitWarning("Invalid debug info found, debug info will be stripped");
+      StripDebugInfo(*MergedModule);
+    }
+  }
   if (verifyModule(*MergedModule, &dbgs()))
     report_fatal_error("Broken module found, compilation aborted!");
 }
 
 /// Optimize merged modules using various IPO passes
 bool LTOCodeGenerator::optimize(bool DisableVerify, bool DisableInline,
                                 bool DisableGVNLoadPRE,
                                 bool DisableVectorization) {

 }
 
 void LTOCodeGenerator::emitError(const std::string &ErrMsg) {
   if (DiagHandler)
     (*DiagHandler)(LTO_DS_ERROR, ErrMsg.c_str(), DiagContext);
   else
     Context.diagnose(LTODiagnosticInfo(ErrMsg));
 }
+
+void LTOCodeGenerator::emitWarning(const std::string &WarnMsg) {
+  if (DiagHandler)
+    (*DiagHandler)(LTO_DS_WARNING, WarnMsg.c_str(), DiagContext);
+  else
+    Context.diagnose(LTODiagnosticInfo(WarnMsg, DS_Warning));
+}

test/LTO/X86/strip-debug-info.ll

+; RUN: not llvm-lto -lto-strip-invalid-debug-info=false \
+; RUN:     -o %t.o %S/Inputs/strip-debug-info.bc 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-ERR
+; RUN: llvm-lto -lto-strip-invalid-debug-info=true -exported-symbol _foo \
+; RUN:     -o %t.o %S/Inputs/strip-debug-info.bc 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-WARN
+; RUN: llvm-nm %t.o | FileCheck %s 
+
+; CHECK-ERR: Broken module found, compilation aborted
+; CHECK-WARN: Invalid debug info found, debug info will be stripped
+; CHECK: foo
+define void @foo() {
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = !DIFile(filename: "broken", directory: "")