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

176

How does it work with lazy-loaded modules?

202–203

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

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

470

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

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

166

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

189

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

176

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

176

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

176

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

180

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

180

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

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"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Support/VCSRevision.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/FunctionImport.h"
 #include "llvm/Transforms/IPO/Internalize.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "llvm/Transforms/ObjCARC.h"
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
+#include "llvm/Transforms/ObjCARC.h"
 
 #include <numeric>
 
 using namespace llvm;
 
 #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.
loadModuleFromBuffer(const MemoryBufferRef &Buffer, LLVMContext &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.");
   }
   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");
   }
 }
 
 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 verifyMergedModule().
+  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()));
 
for (auto &Entry : PreservedSymbols) {
     StringRef Name = Entry.first();
     if (TheTriple.isOSBinFormatMachO() && Name.size() > 0 && Name[0] == '_')
       Name = Name.drop_front();
     GUIDPreservedSymbols.insert(GlobalValue::getGUID(Name));
   }
   return GUIDPreservedSymbols;
 }
 
+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 verifyMergedModule(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);
+  }
+}
+
 std::unique_ptr<MemoryBuffer> codegenModule(Module &TheModule,
                                             TargetMachine &TM) {
   SmallVector<char, 128> OutputBuffer;
 
   // CodeGen
   {
     raw_svector_ostream OS(OutputBuffer);
     legacy::PassManager PM;
ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
                      const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
                      const GVSummaryMapTy &DefinedGlobals,
                      const ThinLTOCodeGenerator::CachingOptions &CacheOptions,
                      bool DisableCodeGen, StringRef SaveTempsDir,
                      bool Freestanding, unsigned OptLevel, unsigned count) {
 
   // "Benchmark"-like optimization: single-source case
   bool SingleModule = (ModuleMap.size() == 1);
+  verifyMergedModule(TheModule);
 
   if (!SingleModule) {
     promoteModule(TheModule, Index);
 
     // Apply summary-based LinkOnce/Weak resolution decisions.
     thinLTOResolveWeakForLinkerModule(TheModule, DefinedGlobals);
 
     // Save temps: after promotion.
ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
 
   if (!SingleModule) {
     crossImportIntoModule(TheModule, Index, ModuleMap, ImportList);
 
     // Save temps: after cross-module import.
     saveTempBitcode(TheModule, SaveTempsDir, count, ".3.imported.bc");
   }
 
+  // Verify again to catch all cross-imports.
+  verifyMergedModule(TheModule);
   optimizeModule(TheModule, TM, OptLevel, Freestanding);
 
   saveTempBitcode(TheModule, SaveTempsDir, count, ".4.opt.bc");
 
   if (DisableCodeGen) {
     // Configured to stop before CodeGen, serialize the bitcode and return.
     SmallVector<char, 128> OutputBuffer;
     {
void ThinLTOCodeGenerator::optimize(Module &TheModule) {
   // Optimize now
   optimizeModule(TheModule, *TMBuilder.create(), OptLevel, Freestanding);
 }
 
 /**
  * Perform ThinLTO CodeGen.
  */
 std::unique_ptr<MemoryBuffer> ThinLTOCodeGenerator::codegen(Module &TheModule) {
+  verifyMergedModule(TheModule);
   initTMBuilder(TMBuilder, Triple(TheModule.getTargetTriple()));
   return codegenModule(TheModule, *TMBuilder.create());
 }
 
 /// Write out the generated object file, either from CacheEntryPath or from
 /// OutputBuffer, preferring hard-link when possible.
 /// Returns the path to the generated file in SavedObjectsDirectoryPath.
 static std::string writeGeneratedObject(int count, StringRef CacheEntryPath,

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 \
 ; RUN:     -exported-symbol foo -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 
+; RUN: not llvm-lto -thinlto -thinlto-action=codegen \
+; RUN:     -lto-strip-invalid-debug-info=false \
+; RUN:     %S/Inputs/strip-debug-info.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:     %S/Inputs/strip-debug-info.bc -disable-verify 2>&1 | \
+; RUN:     FileCheck %s -allow-empty -check-prefix=CHECK-WARN
+; RUN: not llvm-lto -thinlto -thinlto-action=run \
+; RUN:     -lto-strip-invalid-debug-info=false \
+; RUN:     %S/Inputs/strip-debug-info.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:     %S/Inputs/strip-debug-info.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"
+
 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: "")