[ThinLTO] Handle bitcode without function summary sections gracefully

[tejohnson]

Summary

Several fixes to the handling of bitcode files without function summary
sections so that they are skipped during ThinLTO processing in llvm-lto
and the gold plugin when appropriate instead of aborting.

1 Don't assert when trying to add a FunctionInfo that doesn't have

a summary attached.

2 Skip FunctionInfo structures that don't have attached function summary

sections when trying to create the combined function summary.

3 In both llvm-lto and gold-plugin, check whether a bitcode file has

a function summary section before trying to parse the index, and skip
the bitcode file if it does not.

4 Fix hasFunctionSummaryInMemBuffer in BitcodeReader, which had a bug

where we returned to early while looking for the summary section.

Also added llvm-lto and gold-plugin based tests for cases where we
don't have function summaries in the bitcode file. I verified that
either the first couple fixes described above are enough to avoid the
crashes, or fixes 1,3,4. But have combined them all here for added
robustness.

[mehdi_amini]

LGTM, thanks.

include/llvm/IR/FunctionInfo.h

public:
 
   /// Get the list of function info objects for a given function.
   const FunctionInfoList &getFunctionInfoList(StringRef FuncName) {
     return FunctionMap[FuncName];
   }
 
   /// Add a function info for a function of the given name.
   void addFunctionInfo(StringRef FuncName, std::unique_ptr<FunctionInfo> Info) {
-    if (ExportingModule) {
-      assert(Info->functionSummary());
+    // Update the HasExportedFunctions flag, but only if we had a function
+    // summary (i.e. we aren't parsing them lazily or have a bitcode file
+    // without a function summary section).
+    if (ExportingModule && Info->functionSummary()) {
       if (ExportingModule->getModuleIdentifier() ==
           Info->functionSummary()->modulePath())
         HasExportedFunctions = true;
     }
     FunctionMap[FuncName].push_back(std::move(Info));
   }
 
   /// Iterator to allow writer to walk through table during emission.

lib/Bitcode/Reader/BitcodeReader.cpp

while (1) {
     switch (Entry.Kind) {
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
       return std::error_code();
 
     case BitstreamEntry::SubBlock:
       if (CheckFuncSummaryPresenceOnly) {
-        if (Entry.ID == bitc::FUNCTION_SUMMARY_BLOCK_ID)
+        if (Entry.ID == bitc::FUNCTION_SUMMARY_BLOCK_ID) {
           SeenFuncSummary = true;
-        if (Stream.SkipBlock())
-          return error("Invalid record");
           // No need to parse the rest since we found the summary.
           return std::error_code();
         }
+        if (Stream.SkipBlock())
+          return error("Invalid record");
+        continue;
+      }
       switch (Entry.ID) {
       default: // Skip unknown content.
         if (Stream.SkipBlock())
           return error("Invalid record");
         break;
       case bitc::BLOCKINFO_BLOCK_ID:
         // Need to parse these to get abbrev ids (e.g. for VST)
         if (Stream.ReadBlockInfoBlock())

lib/IR/FunctionInfo.cpp

for (auto &OtherFuncInfoLists : *Other) {
     std::string FuncName = OtherFuncInfoLists.getKey();
     FunctionInfoList &List = OtherFuncInfoLists.second;
 
     // Assert that the func info list only has one entry, since we shouldn't
     // have duplicate names within a single per-module index.
     assert(List.size() == 1);
     std::unique_ptr<FunctionInfo> Info = std::move(List.front());
 
+    // Skip if there was no function summary section.
+    if (!Info->functionSummary())
+      continue;
+
     // Add the module path string ref for this module if we haven't already
     // saved a reference to it.
     if (ModPath.empty())
       ModPath =
           addModulePath(Info->functionSummary()->modulePath(), NextModuleId);
     else
       assert(ModPath == Info->functionSummary()->modulePath() &&
              "Each module in the combined map should have a unique ID");

test/Linker/funcimport.ll

+; First ensure that the ThinLTO handling in llvm-link and llvm-lto handles
+; bitcode without function summary sections gracefully.
+; RUN: llvm-as %s -o %t.bc
+; RUN: llvm-as %p/Inputs/funcimport.ll -o %t2.bc
+; RUN: llvm-link %t.bc -functionindex=%t.bc -S
+; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
+
 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: llvm-as -function-summary %s -o %t.bc
 ; RUN: llvm-as -function-summary %p/Inputs/funcimport.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
 
 ; Ensure statics are promoted/renamed correctly from this file (all but
 ; constant variable need promotion).
 ; RUN: llvm-link %t.bc -functionindex=%t3.thinlto.bc -S | FileCheck %s --check-prefix=EXPORTSTATIC

test/tools/gold/X86/thinlto.ll

+; First ensure that the ThinLTO handling in the gold plugin handles
+; bitcode without function summary sections gracefully.
+; RUN: llvm-as %s -o %t.o
+; RUN: llvm-as %p/Inputs/thinlto.ll -o %t2.o
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    -shared %t.o %t2.o -o %t3
+
 ; RUN: llvm-as -function-summary %s -o %t.o
 ; RUN: llvm-as -function-summary %p/Inputs/thinlto.ll -o %t2.o
 
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:    --plugin-opt=thinlto \
 ; RUN:    -shared %t.o %t2.o -o %t3
 ; RUN: llvm-bcanalyzer -dump %t3.thinlto.bc | FileCheck %s --check-prefix=COMBINED
 ; RUN: not test -e %t3

tools/gold/gold-plugin.cpp

if (get_symbols(F.handle, F.syms.size(), &F.syms[0]) != LDPS_OK)
     message(LDPL_FATAL, "Failed to get symbol information");
 
   const void *View;
   if (get_view(F.handle, &View) != LDPS_OK)
     message(LDPL_FATAL, "Failed to get a view of file");
 
   MemoryBufferRef BufferRef(StringRef((const char *)View, Info.filesize),
                             Info.name);
+
+  // Don't bother trying to build an index if there is no summary information
+  // in this bitcode file.
+  if (!object::FunctionIndexObjectFile::hasFunctionSummaryInMemBuffer(
+          BufferRef, diagnosticHandler))
+    return std::unique_ptr<FunctionInfoIndex>(nullptr);
+
   ErrorOr<std::unique_ptr<object::FunctionIndexObjectFile>> ObjOrErr =
       object::FunctionIndexObjectFile::create(BufferRef, diagnosticHandler);
 
   if (std::error_code EC = ObjOrErr.getError())
     message(LDPL_FATAL, "Could not read function index bitcode from file : %s",
             EC.message().c_str());
 
   object::FunctionIndexObjectFile &Obj = **ObjOrErr;
if (options::thinlto) {
     uint64_t NextModuleId = 0;
     for (claimed_file &F : Modules) {
       ld_plugin_input_file File;
       if (get_input_file(F.handle, &File) != LDPS_OK)
         message(LDPL_FATAL, "Failed to get file information");
 
       std::unique_ptr<FunctionInfoIndex> Index =
           getFunctionIndexForFile(F, File);
+
+      // Skip files without a function summary.
+      if (!Index)
+        continue;
+
       CombinedIndex.mergeFrom(std::move(Index), ++NextModuleId);
     }
 
     std::error_code EC;
     raw_fd_ostream OS(output_name + ".thinlto.bc", EC,
                       sys::fs::OpenFlags::F_None);
     if (EC)
       message(LDPL_FATAL, "Unable to open %s.thinlto.bc for writing: %s",

tools/llvm-lto/llvm-lto.cpp

for (auto &Filename : InputFilenames) {
     for (int I = 0, E = Module->getSymbolCount(); I != E; ++I)
       outs() << Module->getSymbolName(I) << "\n";
   }
   return 0;
 }
 
 /// Parse the function index out of an IR file and return the function
 /// index object if found, or nullptr if not.
-static std::unique_ptr<FunctionInfoIndex>
-getFunctionIndexForFile(StringRef Path, std::string &Error,
+static ErrorOr<std::unique_ptr<FunctionInfoIndex>>
+getFunctionIndexForFile(StringRef Path,
                         DiagnosticHandlerFunction DiagnosticHandler) {
   std::unique_ptr<MemoryBuffer> Buffer;
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getFile(Path);
-  if (std::error_code EC = BufferOrErr.getError()) {
-    Error = EC.message();
-    return nullptr;
-  }
+  if (std::error_code EC = BufferOrErr.getError())
+    return EC;
   Buffer = std::move(BufferOrErr.get());
+
+  // Don't bother trying to build an index if there is no summary information
+  // in this bitcode file.
+  if (!object::FunctionIndexObjectFile::hasFunctionSummaryInMemBuffer(
+          Buffer->getMemBufferRef(), DiagnosticHandler))
+    return std::unique_ptr<FunctionInfoIndex>(nullptr);
+
   ErrorOr<std::unique_ptr<object::FunctionIndexObjectFile>> ObjOrErr =
       object::FunctionIndexObjectFile::create(Buffer->getMemBufferRef(),
                                               DiagnosticHandler);
-  if (std::error_code EC = ObjOrErr.getError()) {
-    Error = EC.message();
-    return nullptr;
-  }
+  if (std::error_code EC = ObjOrErr.getError())
+    return EC;
   return (*ObjOrErr)->takeIndex();
 }
 
 /// Create a combined index file from the input IR files and write it.
 ///
 /// This is meant to enable testing of ThinLTO combined index generation,
 /// currently available via the gold plugin via -thinlto.
 static int createCombinedFunctionIndex(StringRef Command) {
   FunctionInfoIndex CombinedIndex;
   uint64_t NextModuleId = 0;
   for (auto &Filename : InputFilenames) {
-    std::string Error;
-    std::unique_ptr<FunctionInfoIndex> Index =
-        getFunctionIndexForFile(Filename, Error, diagnosticHandler);
-    if (!Index) {
+    ErrorOr<std::unique_ptr<FunctionInfoIndex>> IndexOrErr =
+        getFunctionIndexForFile(Filename, diagnosticHandler);
+    if (std::error_code EC = IndexOrErr.getError()) {
+      std::string Error = EC.message();
       errs() << Command << ": error loading file '" << Filename
              << "': " << Error << "\n";
       return 1;
     }
+    std::unique_ptr<FunctionInfoIndex> Index = std::move(IndexOrErr.get());
+    // Skip files without a function summary.
+    if (!Index)
+      continue;
     CombinedIndex.mergeFrom(std::move(Index), ++NextModuleId);
   }
   std::error_code EC;
   assert(!OutputFilename.empty());
   raw_fd_ostream OS(OutputFilename + ".thinlto.bc", EC,
                     sys::fs::OpenFlags::F_None);
   if (EC) {
     errs() << Command << ": error opening the file '" << OutputFilename