[CodeGen] Support bitcode input containing multiple modules

[MaskRay]

Summary

When using -fsplit-lto-unit (explicitly specified or due to using
-fsanitize=cfi/-fwhole-program-vtables), the emitted LLVM IR contains a module
flag metadata "EnableSplitLTOUnit". If a module contains both type metadata
and "EnableSplitLTOUnit", ThinLTOBitcodeWriter.cpp will write two modules
into the bitcode file. Compiling the bitcode (not ThinLTO backend compilation)
will lead to an error due to parseIR requiring a single module.

% clang -flto=thin a.cc -c -o a.bc
% clang -c a.bc
% clang -fsplit-lto-unit -flto=thin a.cc -c -o a.bc
% clang -c a.bc
error: Expected a single module
1 error generated.

There are multiple ways to have just one module in a bitcode file
output: -Xclang -fno-lto-unit, not using features like -fsanitize=cfi,
using -fsanitize=cfi with -fno-split-lto-unit. I think whether a
bitcode input file contains 2 modules (internal implementation strategy)
should not be a criterion to require an additional driver option when
the user seek for a non-LTO compile action.

Let's place the extra module (if present) into CodeGenOptions::LinkBitcodeFiles
(originally for -cc1 -mlink-bitcode-file). Linker::linkModules will link the two
modules together. This patch makes the following commands work:

clang -S -emit-llvm a.bc
clang -S a.bc
clang -c a.bc

[efriedma]

If I follow correctly, this is basically undoing the splitting that was done by the command that produced the bitcode file? I guess that could be useful. But it requires either renaming your object files from the default ".o" to ".bc", or explicitly passing "-x ir"? That seems unintuitive. Maybe it's better to put this behind some explicit flag?

[MaskRay]

In D154923#4491411, @efriedma wrote:

If I follow correctly, this is basically undoing the splitting that was done by the command that produced the bitcode file?

Yes, undoing llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp change that would make the output bitcode file not usable as an input for non-LTO use cases.

I guess that could be useful. But it requires either renaming your object files from the default ".o" to ".bc", or explicitly passing "-x ir"? That seems unintuitive. Maybe it's better to put this behind some explicit flag?

Yes, specify -x ir or let the driver deduce the file type with the predefined extension .bc.

I think this is a less common operation (compiling with LTO but then using as non-LTO), so I think adding another option seems not necessary.

[MaskRay]

In D154923#4491673, @MaskRay wrote:

In D154923#4491411, @efriedma wrote:

If I follow correctly, this is basically undoing the splitting that was done by the command that produced the bitcode file?

Yes, undoing llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp change that would make the output bitcode file not usable as an input for non-LTO use cases.

I guess that could be useful. But it requires either renaming your object files from the default ".o" to ".bc", or explicitly passing "-x ir"? That seems unintuitive. Maybe it's better to put this behind some explicit flag?

Yes, specify -x ir or let the driver deduce the file type with the predefined extension .bc.

I think this is a less common operation (compiling with LTO but then using as non-LTO), so I think adding another option seems not necessary.

Perhaps rephrase my argument:

There are multiple ways to have just one module in a bitcode file output: -Xclang -fno-lto-unit, not using features like -fsanitize=cfi, using -fsanitize=cfi with -fno-split-lto-unit.
I think whether a bitcode input file contains 2 modules (internal implementation strategy) should not be a criterion to require an additional driver option when the user seek for a non-LTO compile action.

[ormris]

This is a good way to resolve the inconsistencies I was looking at in my own review.

I think whether a bitcode input file contains 2 modules (internal implementation strategy) should not be a criterion to require an additional driver option

Agreed. Most users don't know about the internal structure of bitcode modules, so I think it's best to use a consistent interface for all bitcode files.

clang/lib/CodeGen/CodeGenAction.cpp

1174

Perhaps this variable name could be more descriptive. Maybe "FirstM" or something like that?

[MaskRay]

rename a variable

[MaskRay]

rename a variable

[ormris]

LGTM

clang/lib/CodeGen/CodeGenAction.cpp

CodeGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
   return std::move(Result);
 }
 
 std::unique_ptr<llvm::Module>
 CodeGenAction::loadModule(MemoryBufferRef MBRef) {
   CompilerInstance &CI = getCompilerInstance();
   SourceManager &SM = CI.getSourceManager();
 
-  // For ThinLTO backend invocations, ensure that the context
-  // merges types based on ODR identifiers. We also need to read
-  // the correct module out of a multi-module bitcode file.
-  if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty()) {
-    VMContext->enableDebugTypeODRUniquing();
-
-    auto DiagErrors = [&](Error E) -> std::unique_ptr<llvm::Module> {
-      unsigned DiagID =
-          CI.getDiagnostics().getCustomDiagID(DiagnosticsEngine::Error, "%0");
-      handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
-        CI.getDiagnostics().Report(DiagID) << EIB.message();
-      });
-      return {};
-    };
 
+  // For ThinLTO backend invocations, ensure that the context
+  // merges types based on ODR identifiers. We also need to read
+  // the correct module out of a multi-module bitcode file.
+  if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty()) {
+    VMContext->enableDebugTypeODRUniquing();
+
     Expected<std::vector<BitcodeModule>> BMsOrErr = getBitcodeModuleList(MBRef);
     if (!BMsOrErr)
       return DiagErrors(BMsOrErr.takeError());
     BitcodeModule *Bm = llvm::lto::findThinLTOModule(*BMsOrErr);
     // We have nothing to do if the file contains no ThinLTO module. This is
     // possible if ThinLTO compilation was not able to split module. Content of
     // the file was already processed by indexing and will be passed to the
     // linker using merged object file.
     if (!Bm) {
       auto M = std::make_unique<llvm::Module>("empty", *VMContext);
       M->setTargetTriple(CI.getTargetOpts().Triple);
       return M;
     }
     Expected<std::unique_ptr<llvm::Module>> MOrErr =
         Bm->parseModule(*VMContext);
     if (!MOrErr)
       return DiagErrors(MOrErr.takeError());
     return std::move(*MOrErr);
   }
 
   // Load bitcode modules to link with, if we need to.
   if (loadLinkModules(CI))
     return nullptr;
 
+  // Handle textual IR and bitcode file with one single module.
   llvm::SMDiagnostic Err;
   if (std::unique_ptr<llvm::Module> M = parseIR(MBRef, Err, *VMContext))
     return M;
 
+  // If MBRef is a bitcode with multiple modules (e.g., -fsplit-lto-unit
+  // output), place the extra modules (actually only one, a regular LTO module)
+  // into LinkModules as if we are using -mlink-bitcode-file.
+  Expected<std::vector<BitcodeModule>> BMsOrErr = getBitcodeModuleList(MBRef);
+  if (BMsOrErr && BMsOrErr->size()) {
+    std::unique_ptr<llvm::Module> FirstM;
+    for (auto &BM : *BMsOrErr) {
+      Expected<std::unique_ptr<llvm::Module>> MOrErr =
+          BM.parseModule(*VMContext);
+      if (!MOrErr)
+        return DiagErrors(MOrErr.takeError());
+      if (FirstM)
+        LinkModules.push_back({std::move(*MOrErr), /*PropagateAttrs=*/false,
+                               /*Internalize=*/false, /*LinkFlags=*/{}});
+      else
+        FirstM = std::move(*MOrErr);
+    }
+    if (FirstM)
+      return FirstM;
+  }
+  // If BMsOrErr fails, consume the error and use the error message from
+  // parseIR.
+  consumeError(BMsOrErr.takeError());
+
   // Translate from the diagnostic info to the SourceManager location if
   // available.
   // TODO: Unify this with ConvertBackendLocation()
   SourceLocation Loc;
   if (Err.getLineNo() > 0) {
     assert(Err.getColumnNo() >= 0);
     Loc = SM.translateFileLineCol(SM.getFileEntryForID(SM.getMainFileID()),
                                   Err.getLineNo(), Err.getColumnNo() + 1);

clang/test/CodeGen/split-lto-unit-input.cpp

+// REQUIRES: x86-registered-target
+/// When the input is a -fsplit-lto-unit bitcode file, link the regular LTO file like -mlink-bitcode-file.
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm-bc -flto=thin -flto-unit -fsplit-lto-unit %s -o %t.bc
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-obj %t.bc -o %t.o
+// RUN: llvm-nm %t.o | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm %t.bc -o - | FileCheck %s --check-prefix=CHECK-IR
+
+// CHECK:      V _ZTI1A
+// CHECK-NEXT: V _ZTI1B
+// CHECK-NEXT: V _ZTS1A
+// CHECK-NEXT: V _ZTS1B
+// CHECK-NEXT: V _ZTV1A
+// CHECK-NEXT: V _ZTV1B
+
+// CHECK-IR-DAG: _ZTS1B = linkonce_odr constant
+// CHECK-IR-DAG: _ZTS1A = linkonce_odr constant
+// CHECK-IR-DAG: _ZTV1B = linkonce_odr unnamed_addr constant
+// CHECK-IR-DAG: _ZTI1A = linkonce_odr constant
+// CHECK-IR-DAG: _ZTI1B = linkonce_odr constant
+// CHECK-IR-DAG: _ZTV1A = linkonce_odr unnamed_addr constant
+
+struct A {
+  virtual int c(int i) = 0;
+};
+
+struct B : A {
+  virtual int c(int i) { return i; }
+};
+
+int use() {
+  return (new B)->c(0);
+}