ThinLTO: Verify bitcode before lauching the ThinLTOCodeGenerator.

[aprantl]

Summary

This patch invokes the Verifier on each module after loading, analogously to what regular LTO is doing. Besides diagnosing malformed bitcode, this allows it to strip out invalid debug info from bitcode produced by older, more buggy versions of LLVM. The new code is pretty much the same as in LTOCodeGenerator.cpp.

rdar://problem/31233625

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

186 ↗

(On Diff #98837)

How does it work with lazy-loaded modules?

223 ↗

(On Diff #98837)

We're gonna reverify here, aren't we?

[aprantl]

Interesting. The linker invocation I was debugging was using the

if (CodeGenOnly) {
  // Perform only parallel codegen and return.
 ...

codepath, which doesn't initializer the PM the same way.

[mehdi_amini]

The codegen-only path also does not exercise the cross-module importing.

[aprantl]

This turns out to be quite ugly.
There are two different code paths, by their llvm-lto options:
-thinlto-action=run uses PassManagerBuilder and sets VerifyInput=true, which causes a VerifierLegacyPass to be scheduled
-thinlto-action=codegen (which mimics llc) doesn't.
The obvious correct fix for -thinlto-action=codegen is to also schedule a Verifier pass. That is easy — but wait.

The VerifierLegacyPass pass doesn't work at all like one might expect. The crux is that VerifierLegacyPass is a FunctionPass, which implements runOnFunction for function-level verification (this part is fine) and does all of its module-level verification in doFinalization(), but doFinalization runs when the PassManager is done with running the entire pipeline, so the per-module verifications are running *after* the bugs happen that they are supposed to prevent.

I think that the right solution for this is to turn VerifierLegacyPass into a ModulePass so the per-module verification can be scheduled to run first.

That is not all, however. There is also the problem that the verifier wants to assert on invalid debug info, which makes this behavior untestable in an asserts build. I will also have to thread a flag to disable the assertion into the VerifierLegacyPass so we can write a testcase for it.

Back to the drawing board...

[mehdi_amini]

What about disabling the verification as part of the pass pipeline and just do the manual call to the verifier on loading?

The thing that is messy is that we want to avoid verifying all the buffers on every cross-module import, but I'm not sure about the debug-info dropping?

[aprantl]

Yes I think you are right. There are too many places that assume the Verifier is as function pass.
The updated patch disabled input verification in the pass manager builder and manually verifies the input.

[mehdi_amini]

(+@pcc)
Someone should take care of making sure this is also handled by the new LTO API.

lib/LTO/ThinLTOCodeGenerator.cpp

440 ↗

(On Diff #99078)

The name seems misleading: the module hasn't been "merged" here (no importing happened at this point).

470 ↗

(On Diff #99078)

How costly is it to do it two times?
I guess there is not much choice any way with respect to debug info....

Do we have testing for this?

[aprantl]

What an entangled mess. This patch adds a boatload of testcases including the case where a function with broken debug info is cross-module-imported into a verified correct module. It also avoids re-verifying the module as much as possible.

[aprantl]

lib/LTO/ThinLTOCodeGenerator.cpp

470 ↗

(On Diff #99078)

The verifier is not terribly expensive, but it does visit every entity in the module once.

[mehdi_amini]

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

335 ↗

(On Diff #99190)

This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.

lib/LTO/ThinLTOCodeGenerator.cpp

190 ↗

(On Diff #99190)

I'm still not sure how this plays with importing: what happens here with a lazy-loaded module?

I currently think we need to verify here only non-lazy-loaded modules (i.e. main modules). And implement another verification after materialization, but before importing in FunctionImporter.cpp

214 ↗

(On Diff #99190)

That shouldn't be necessary if we checked every module at load time.

[aprantl]

This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.

In the attached version I removed the flag. This will cause the

void ThinLTOCodeGenerator::run() // CodeGenOnly=true

path to verify the module twice. I'm not particularly concerned by this though and it keeps the code simpler.

I also implemented the suggestion to verify in FunctionImporter.

Thanks!

[aprantl]

Fix typos.

[aprantl]

lib/LTO/ThinLTOCodeGenerator.cpp

186 ↗

(On Diff #98837)

How can I simulate a lazy-loaded module using llvm-lto? I would like to add a test for this.

[mehdi_amini]

In D33151#756783, @aprantl wrote:

This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.

In the attached version I removed the flag. This will cause the

void ThinLTOCodeGenerator::run() // CodeGenOnly=true

path to verify the module twice. I'm not particularly concerned by this though and it keeps the code simpler.

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

[aprantl]

In D33151#756788, @mehdi_amini wrote:

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Because llvm-lto invokes codegen() directly after loading the module itself (wihtout going through ThinLTOCodeGenerator for the loading):

void codegen() {
  if (InputFilenames.size() != 1 && !OutputFilename.empty())
    report_fatal_error("Can't handle a single output filename and multiple "
                       "input files, do not provide an output filename and "
                       "the output files will be suffixed from the input "
                       "ones.");
  if (!ThinLTOIndex.empty())
    errs() << "Warning: -thinlto-index ignored for codegen stage";

  for (auto &Filename : InputFilenames) {
    LLVMContext Ctx;
    auto TheModule = loadModule(Filename, Ctx);

    auto Buffer = ThinGenerator.codegen(*TheModule);

[mehdi_amini]

In D33151#756793, @aprantl wrote:

In D33151#756788, @mehdi_amini wrote:

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Because llvm-lto invokes codegen() directly after loading the module itself (wihtout going through ThinLTOCodeGenerator for the loading):

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

lib/LTO/ThinLTOCodeGenerator.cpp

186 ↗

(On Diff #98837)

Anything that do cross-module importing will do lazy-loading. So two modules and one function calling another function in the other module. I think test/ThinLTO/X86/funcimport.ll should have such an example.

[aprantl]

In D33151#756801, @mehdi_amini wrote:

In D33151#756793, @aprantl wrote:

In D33151#756788, @mehdi_amini wrote:

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Because llvm-lto invokes codegen() directly after loading the module itself (wihtout going through ThinLTOCodeGenerator for the loading):

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

lib/LTO/ThinLTOCodeGenerator.cpp

186 ↗

(On Diff #98837)

If that is so then my cross-module import tests (the last two llvm-lto invocations) are already testing this. The lazy-loaded modules will use the PostProcess verifier in FunctionImport.

[aprantl]

In D33151#756851, @aprantl wrote:

In D33151#756801, @mehdi_amini wrote:

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

Thanks for the offer! Please let me know if you won't get to it. If you can just quickly sketch what needs to be done I can do the grunt work, too.

[mehdi_amini]

In D33151#757534, @aprantl wrote:

In D33151#756851, @aprantl wrote:

In D33151#756801, @mehdi_amini wrote:

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

Thanks for the offer! Please let me know if you won't get to it. If you can just quickly sketch what needs to be done I can do the grunt work, too.

It should be fairly straightforward, just mimicking the C API: it should be only replacing the call to codegen() with two calls to setCodeGenOnly(true) and then run() (that's the best case, but I haven't looked yet, the code may not be setup to do it so easily).

[aprantl]

Depending on https://reviews.llvm.org/D33360, get rid of the verifier invocation in codegen()

[mehdi_amini]

LGTM.

include/llvm/Transforms/IPO/FunctionImport.h

58 ↗

(On Diff #99583)

What about renaming PostProcess to PostBitcodeLoading? It seems more accurate to me.

lib/LTO/ThinLTOCodeGenerator.cpp

205 ↗

(On Diff #99583)

Just curious: are the { } pair required?

[aprantl]

Thanks for the review!

include/llvm/Transforms/IPO/FunctionImport.h

58 ↗

(On Diff #99583)

Makes sense!

lib/LTO/ThinLTOCodeGenerator.cpp

205 ↗

(On Diff #99583)

Yes, otherwise we are getting an error: no viable conversion from 'void (llvm::Module &)' to 'Optional<std::function<void (Module &)> >'

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,
+      Optional<std::function<void(Module &)>> PostBitcodeLoading = None);
 
 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);
+  Expected<bool> Result =
+      Importer.importFunctions(TheModule, ImportList, {verifyLoadedModule});
   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");
   }
 }
 
 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/lib/Transforms/IPO/FunctionImport.cpp

void llvm::thinLTOInternalizeModule(Module &TheModule,
   // based on the index, rather than invoking internalizeModule.
   llvm::internalizeModule(TheModule, MustPreserveGV);
 }
 
 // Automatically import functions in Module \p DestModule based on the summaries
 // index.
 //
 Expected<bool> FunctionImporter::importFunctions(
-    Module &DestModule, const FunctionImporter::ImportMapTy &ImportList) {
+    Module &DestModule, const FunctionImporter::ImportMapTy &ImportList,
+    Optional<std::function<void(Module &)>> PostBitcodeLoading) {
   DEBUG(dbgs() << "Starting import for Module "
                << DestModule.getModuleIdentifier() << "\n");
   unsigned ImportedCount = 0;
 
   IRMover Mover(DestModule);
   // Do the actual import of functions now, one Module at a time
   std::set<StringRef> ModuleNameOrderedList;
   for (auto &FunctionsToImportPerModule : ImportList) {
#endif
           return std::move(Err);
         GlobalsToImport.insert(&GA);
       }
     }
 
     // Upgrade debug info after we're done materializing all the globals and we
     // have loaded all the required metadata!
     UpgradeDebugInfo(*SrcModule);
+    if (PostBitcodeLoading)
+      (*PostBitcodeLoading)(*SrcModule);
 
     // Link in the specified functions.
     if (renameModuleForThinLTO(*SrcModule, Index, &GlobalsToImport))
       return true;
 
     if (PrintImports) {
       for (const auto *GV : GlobalsToImport)
         dbgs() << DestModule.getSourceFileName() << ": Import " << GV->getName()

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: "")