Don't verify cross-imported bitcode in FunctionImporter

[aprantl]

Summary

After landing https://reviews.llvm.org/D33151 I promptly broke the green dragon ThinLTO buildbot[1]. It turns out that the SrcModule in FunctionImporter is in a really inconsistent intermediate state at the point where I ran the Verifier. After poking around a bit I came to the conclusion that it probably isn't even desirable to run the full-module verifier on each cross-imported module: Over the course of building the entire project the same module will be verified over and over again, which is a waste of resources. When we want to lazy-load metadata running a whole-module Verifier is also problematic.

This patch partially reverts D33151 and instead only runs the verifier once on the module after the cross-imports are done.

The downside is that this won't be able to catch verifier issues that will cause crashes during the import itself, but I think that is an acceptable trade-off.

[1] http://green.lab.llvm.org/green/job/clang-stage2-configure-Rthinlto_build/1850/consoleFull

[mehdi_amini]

It turns out that the SrcModule in FunctionImporter is in a really inconsistent intermediate state at the point where I ran the Verifier.

Oh right I forgot about this: lazy-loaded modules, even after materializing metadata, aren't passing the verified. This is super annoying.

After poking around a bit I came to the conclusion that it probably isn't even desirable to run the full-module verifier on each cross-imported module: Over the course of building the entire project the same module will be verified over and over again, which is a waste of resources. When we want to lazy-load metadata running a whole-module Verifier is also problematic.

There is something I'm missing: I thought the important part of the verifier was to be able to drop stalled debug information which is important for backward compatibility? This is the only reason I suggested doing it at this point of the process (before importing)...

[aprantl]

In D33370#759915, @mehdi_amini wrote:

It turns out that the SrcModule in FunctionImporter is in a really inconsistent intermediate state at the point where I ran the Verifier.

Oh right I forgot about this: lazy-loaded modules, even after materializing metadata, aren't passing the verified. This is super annoying.

After poking around a bit I came to the conclusion that it probably isn't even desirable to run the full-module verifier on each cross-imported module: Over the course of building the entire project the same module will be verified over and over again, which is a waste of resources. When we want to lazy-load metadata running a whole-module Verifier is also problematic.

There is something I'm missing: I thought the important part of the verifier was to be able to drop stalled debug information which is important for backward compatibility? This is the only reason I suggested doing it at this point of the process (before importing)...

That is correct*. This why I mentioned that doing it like in this patch will not catch any metadata so egregiously wrong that it would crash the FunctionImporter. If the debug info is structurally sound and can be handled by the FunctionImporter then doing the full verification after importing will still allow us to strip malformed imported debug info.

*) note that it isn't really about backwards-compatibility (we have auto-upgrades for that), but about metadata generated by non-clang frontends. If we have to support every metadata every produced by any frontend and at one point accepted by the verifier, we could effectively never improve the verifier by making it stricter — and historically the verifier was very lax.

[mehdi_amini]

LGTM.

lib/LTO/ThinLTOCodeGenerator.cpp

214 ↗

(On Diff #99631)

Can you mention in the comment that we're doing it in order to strip invalid debug info that could have been imported in the process?

llvm/trunk/include/llvm/Transforms/IPO/FunctionImport.h

public:
   typedef std::function<Expected<std::unique_ptr<Module>>(StringRef Identifier)>
       ModuleLoaderTy;
 
   /// Create a Function Importer.
   FunctionImporter(const ModuleSummaryIndex &Index, ModuleLoaderTy ModuleLoader)
       : Index(Index), ModuleLoader(std::move(ModuleLoader)) {}
 
   /// Import functions in Module \p M based on the supplied import list.
-  Expected<bool>
-  importFunctions(Module &M, const ImportMapTy &ImportList);
+  Expected<bool> importFunctions(Module &M, const ImportMapTy &ImportList);
 
 private:
   /// The summaries index used to trigger importing.
   const ModuleSummaryIndex &Index;
 
   /// Factory function to load a Module for a given identifier
   ModuleLoaderTy ModuleLoader;
 };

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

 #include "llvm/Analysis/ProfileSummaryInfo.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
 #include "llvm/Bitcode/BitcodeWriterPass.h"
 #include "llvm/ExecutionEngine/ObjectMemoryBuffer.h"
 #include "llvm/IR/DiagnosticPrinter.h"
+#include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Mangler.h"
+#include "llvm/IR/Verifier.h"
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/Linker/Linker.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Support/CachePruning.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"

 
 #define DEBUG_TYPE "thinlto"
 
 namespace llvm {
 // Flags -discard-value-names, defined in LTOCodeGenerator.cpp
 extern cl::opt<bool> LTODiscardValueNames;
 extern cl::opt<std::string> LTORemarksFilename;
 extern cl::opt<bool> LTOPassRemarksWithHotness;
+extern cl::opt<bool> LTOStripInvalidDebugInfo;
 }
 
 namespace {
 
 static cl::opt<int>
     ThreadCount("threads", cl::init(llvm::heavyweight_hardware_concurrency()));
 
 // Simple helper to save temporary files for debug.
generateModuleMap(const std::vector<ThinLTOBuffer> &Modules) {
   return ModuleMap;
 }
 
 static void promoteModule(Module &TheModule, const ModuleSummaryIndex &Index) {
   if (renameModuleForThinLTO(TheModule, Index))
     report_fatal_error("renameModuleForThinLTO failed");
 }
 
+namespace {
+class ThinLTODiagnosticInfo : public DiagnosticInfo {
+  const Twine &Msg;
+public:
+  ThinLTODiagnosticInfo(const Twine &DiagMsg,
+                        DiagnosticSeverity Severity = DS_Error)
+      : DiagnosticInfo(DK_Linker, Severity), Msg(DiagMsg) {}
+  void print(DiagnosticPrinter &DP) const override { DP << Msg; }
+};
+}
+
+/// Verify the module and strip broken debug info.
+static void verifyLoadedModule(Module &TheModule) {
+  bool BrokenDebugInfo = false;
+  if (verifyModule(TheModule, &dbgs(),
+                   LTOStripInvalidDebugInfo ? &BrokenDebugInfo : nullptr))
+    report_fatal_error("Broken module found, compilation aborted!");
+  if (BrokenDebugInfo) {
+    TheModule.getContext().diagnose(ThinLTODiagnosticInfo(
+        "Invalid debug info found, debug info will be stripped", DS_Warning));
+    StripDebugInfo(TheModule);
+  }
+}
+
 static std::unique_ptr<Module>
 loadModuleFromBuffer(const MemoryBufferRef &Buffer, LLVMContext &Context,
                      bool Lazy, bool IsImporting) {
   SMDiagnostic Err;
   Expected<std::unique_ptr<Module>> ModuleOrErr =
       Lazy
           ? getLazyBitcodeModule(Buffer, Context,
                                  /* ShouldLazyLoadMetadata */ true, IsImporting)
           : parseBitcodeFile(Buffer, Context);
   if (!ModuleOrErr) {
     handleAllErrors(ModuleOrErr.takeError(), [&](ErrorInfoBase &EIB) {
       SMDiagnostic Err = SMDiagnostic(Buffer.getBufferIdentifier(),
                                       SourceMgr::DK_Error, EIB.message());
       Err.print("ThinLTO", errs());
     });
     report_fatal_error("Can't load module, abort.");
   }
+  if (!Lazy)
+    verifyLoadedModule(*ModuleOrErr.get());
   return std::move(ModuleOrErr.get());
 }
 
 static void
 crossImportIntoModule(Module &TheModule, const ModuleSummaryIndex &Index,
                       StringMap<MemoryBufferRef> &ModuleMap,
                       const FunctionImporter::ImportMapTy &ImportList) {
   auto Loader = [&](StringRef Identifier) {
     return loadModuleFromBuffer(ModuleMap[Identifier], TheModule.getContext(),
                                 /*Lazy=*/true, /*IsImporting*/ true);
   };
 
   FunctionImporter Importer(Index, Loader);
   Expected<bool> Result = Importer.importFunctions(TheModule, ImportList);
   if (!Result) {
     handleAllErrors(Result.takeError(), [&](ErrorInfoBase &EIB) {
       SMDiagnostic Err = SMDiagnostic(TheModule.getModuleIdentifier(),
                                       SourceMgr::DK_Error, EIB.message());
       Err.print("ThinLTO", errs());
     });
     report_fatal_error("importFunctions failed");
   }
+  // Verify again after cross-importing.
+  verifyLoadedModule(TheModule);
 }
 
 static void optimizeModule(Module &TheModule, TargetMachine &TM,
                            unsigned OptLevel, bool Freestanding) {
   // Populate the PassManager
   PassManagerBuilder PMB;
   PMB.LibraryInfo = new TargetLibraryInfoImpl(TM.getTargetTriple());
   if (Freestanding)
     PMB.LibraryInfo->disableAllFunctions();
   PMB.Inliner = createFunctionInliningPass();
   // FIXME: should get it from the bitcode?
   PMB.OptLevel = OptLevel;
   PMB.LoopVectorize = true;
   PMB.SLPVectorize = true;
-  PMB.VerifyInput = true;
+  // Already did this in verifyLoadedModule().
+  PMB.VerifyInput = false;
   PMB.VerifyOutput = false;
 
   legacy::PassManager PM;
 
   // Add the TTI (required to inform the vectorizer about register size for
   // instance)
   PM.add(createTargetTransformInfoWrapperPass(TM.getTargetIRAnalysis()));
 

llvm/trunk/test/LTO/X86/Inputs/strip-debug-info-bar.ll

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12"
+
+define void @bar() !dbg !3 {
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2)
+!2 = !DIFile(filename: "broken", directory: "")
+!3 = distinct !DISubprogram(line: 1000, isDefinition: true)
+

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

+; RUN: llvm-as -disable-verify %s -o %t.bc
+; ---- Full LTO ---------------------------------------------
 ; RUN: not llvm-lto -lto-strip-invalid-debug-info=false \
-; RUN:     -o %t.o %S/Inputs/strip-debug-info.bc 2>&1 | \
+; RUN:     -o %t.o %t.bc 2>&1 | \
 ; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-ERR
 ; RUN: llvm-lto -lto-strip-invalid-debug-info=true \
 ; RUN:     -exported-symbol foo -exported-symbol _foo \
-; RUN:     -o %t.o %S/Inputs/strip-debug-info.bc 2>&1 | \
+; RUN:     -o %t.o %t.bc 2>&1 | \
 ; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-WARN
 ; RUN: llvm-nm %t.o | FileCheck %s 
+; ---- Thin LTO (codegen only) ------------------------------
+; RUN: not llvm-lto -thinlto -thinlto-action=codegen \
+; RUN:     -lto-strip-invalid-debug-info=false \
+; RUN:     %t.bc -disable-verify 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-ERR
+; RUN: llvm-lto -thinlto -thinlto-action=codegen \
+; RUN:     -lto-strip-invalid-debug-info=true \
+; RUN:     %t.bc -disable-verify 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-WARN
+; ---- Thin LTO (optimize, strip main file) -----------------
+; RUN: opt -disable-verify -module-summary %s -o %t.bc
+; RUN: opt -disable-verify -module-summary %S/Inputs/strip-debug-info-bar.ll \
+; RUN:     -o %t2.bc
+; RUN: not llvm-lto -thinlto -thinlto-action=run \
+; RUN:     -lto-strip-invalid-debug-info=false \
+; RUN:     %t.bc -disable-verify 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-ERR
+; RUN: llvm-lto -thinlto -thinlto-action=run \
+; RUN:     -lto-strip-invalid-debug-info=true \
+; RUN:     %t.bc -disable-verify 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-WARN
+; ---- Thin LTO (optimize, strip imported file) -------------
+; RUN: opt -disable-verify -strip-debug -module-summary %t.bc -o %t-stripped.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t-stripped.bc %t2.bc
+; RUN: not llvm-lto -thinlto -thinlto-action=import \
+; RUN:     -thinlto-index=%t.index.bc \
+; RUN:     -lto-strip-invalid-debug-info=false \
+; RUN:     -exported-symbol foo -exported-symbol _foo \
+; RUN:     %t-stripped.bc -disable-verify 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-ERR
+; RUN: llvm-lto -thinlto -thinlto-action=import \
+; RUN:     -lto-strip-invalid-debug-info=true \
+; RUN:     -thinlto-index=%t.index.bc \
+; RUN:     -exported-symbol foo -exported-symbol _foo \
+; RUN:     %t-stripped.bc -disable-verify 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-WARN
 
 ; CHECK-ERR: Broken module found, compilation aborted
 ; CHECK-WARN: Invalid debug info found, debug info will be stripped
+; CHECK-WARN-NOT: Broken module found
 ; CHECK: foo
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12"
+
+declare void @bar()
+
 define void @foo() {
+  call void @bar()
   ret void
 }
 
 !llvm.module.flags = !{!0}
 !llvm.dbg.cu = !{!1}
 
 !0 = !{i32 2, !"Debug Info Version", i32 3}
 !1 = !DIFile(filename: "broken", directory: "")