Port the strip-invalid-debuginfo logic to the legacy verifier pass, too.

[aprantl]

Summary

Since r268966 the modern Verifier pass defaults to stripping invalid debug info
in nonasserts builds.  This patch ports this behavior back to the legacy
Verifier pass as well.  The primary motivation is that the clang frontend
accepts bitcode files as input but is still using the legacy pass pipeline.

Background: 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/26448800>

[aprantl]

Fixed a logic error that would have prevented the assertion from firing at all.

[mehdi_amini]

I didn't know there was a modern and a "legacy" one. Is it suppose to go away? Could we just nuke it and move clang to the new one?

[mehdi_amini]

Since r268966 the modern Verifier pass defaults to stripping invalid debug info in nonasserts builds. This patch ports this behavior back to the legacy Verifier pass as well.

This sentence makes me expect a difference between release and assert builds, and I don't spot it in this patch.

[mehdi_amini]

lib/IR/Verifier.cpp

4475

It is not clear to me why new VerifierLegacyPass() has a different behavior with respect to debug info compared to new VerifierLegacyPass(true) or new VerifierLegacyPass(false) (these last two will have the same behavior in face of debug info.

[aprantl]

In D20629#439834, @joker.eph wrote:

I didn't know there was a modern and a "legacy" one. Is it suppose to go away? Could we just nuke it and move clang to the new one?

This is certainly the intended long-term prospect, but that would mean rewriting the majority of clang's BackendUtil, which I don't feel qualified for at the moment. It may not even be possible because not all passes have been ported to the new pass manager.

[aprantl]

In D20629#439835, @joker.eph wrote:

Since r268966 the modern Verifier pass defaults to stripping invalid debug info in nonasserts builds. This patch ports this behavior back to the legacy Verifier pass as well.

This sentence makes me expect a difference between release and assert builds, and I don't spot it in this patch.

The difference is this assert:

assert(!V.hasBrokenDebugInfo() && "Module contains invalid debug info");

lib/IR/Verifier.cpp

4475

Good catch, they probably shouldn't.

[mehdi_amini]

In D20629#439844, @aprantl wrote:

In D20629#439834, @joker.eph wrote:

I didn't know there was a modern and a "legacy" one. Is it suppose to go away? Could we just nuke it and move clang to the new one?

This is certainly the intended long-term prospect, but that would mean rewriting the majority of clang's BackendUtil, which I don't feel qualified for at the moment. It may not even be possible because not all passes have been ported to the new pass manager.

Oh I missed the fact that it was involving the new pass manager, makes sense.

[aprantl]

Made the default constructor match the explicit one.

[mehdi_amini]

LGTM

[aprantl]

Thanks! Committed as r270768.

lib/IR/Verifier.cpp

bool llvm::verifyModule(const Module &M, raw_ostream *OS,
   return Broken;
 }
 
 namespace {
 struct VerifierLegacyPass : public FunctionPass {
   static char ID;
 
   Verifier V;
-  bool FatalErrors;
+  bool FatalErrors = true;
 
   VerifierLegacyPass()
       : FunctionPass(ID),
-        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/true),
-        FatalErrors(true) {
+        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/true) {
     initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry());
   }
   explicit VerifierLegacyPass(bool FatalErrors)
       : FunctionPass(ID),
-        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/true),
+        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/false),
         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;
   }
 
   bool doFinalization(Module &M) override {
-    if (!V.verify(M) && FatalErrors)
+    bool HasErrors = !V.verify(M);
+    if (FatalErrors) {
+      if (HasErrors)
         report_fatal_error("Broken module found, compilation aborted!");
+      assert(!V.hasBrokenDebugInfo() && "Module contains invalid debug info");
+    }
 
+    // Strip broken debug info.
+    if (V.hasBrokenDebugInfo()) {
+      DiagnosticInfoIgnoringInvalidDebugMetadata DiagInvalid(M);
+      M.getContext().diagnose(DiagInvalid);
+      if (!StripDebugInfo(M))
+        report_fatal_error("Failed to strip malformed debug info");
+    }
     return false;
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
   }
 };
 }

unittests/IR/VerifierTest.cpp

 //===- llvm/unittest/IR/VerifierTest.cpp - Verifier unit tests --*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/IR/Verifier.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Module.h"
+#include "llvm/IR/Verifier.h"
 #include "gtest/gtest.h"
 
 namespace llvm {
 namespace {
 
 TEST(VerifierTest, Branch_i1) {
   LLVMContext C;
   Module M("M", C);
TEST(VerifierTest, StripInvalidDebugInfo) {
   MPM.addPass(VerifierPass(false));
   ModuleAnalysisManager MAM(true);
   MAM.registerPass([&] { return VerifierAnalysis(); });
   MPM.run(M, MAM);
   EXPECT_FALSE(verifyModule(M));
 }
 #endif
 
+TEST(VerifierTest, StripInvalidDebugInfoLegacy) {
+  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));
+
+  legacy::PassManager Passes;
+  Passes.add(createVerifierPass(false));
+  Passes.run(M);
+  EXPECT_FALSE(verifyModule(M));
+}
+
 } // end anonymous namespace
 } // end namespace llvm