[gold] Fix common symbols handling

[eugenis]

Summary

LLVM Gold plugin decides which instance of a common symbol it wants
based on the symbol size in claim_file_hook. If the file that
contains the chosen instance is later dropped from the link, we end
up with an undefined reference.

This change delays this decision until the set of the included files
is known.

[• rafael]

The test passes with the patch reverted.

[eugenis]

I've added a test that always passes with the older gold versions, and fails (w/o this patch) with the ToT gold.

[eugenis]

Something weird happened here. I've accidentally committed this as r263180 instead of D17360. I need to learn arc.

The good news is the bots are happy.
Do you want me to revert, or could we make this a post-commit review?

test/tools/gold/X86/Inputs/start-lib-common.ll

+@x = common global i32 0, align 8

test/tools/gold/X86/start-lib-common.ll

+; Test the case when the preferred (larger / more aligned) version of a common
+; symbol is located in a module that's not included in the link.
+
+; RUN: llvm-as %s -o %t1.o
+; RUN: llvm-as %p/Inputs/start-lib-common.ll -o %t2.o
+
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=emit-llvm \
+; RUN:    -shared %t1.o --start-lib %t2.o --end-lib -o %t3.o
+; RUN: llvm-dis %t3.o -o - | FileCheck %s
+
+@x = common global i32 0, align 4
+
+; ToT gold (as of 03/2016) honors --start-lib/--end-lib, drops %t2.o and ends up
+; with (i32 align 4) symbol.
+; Older gold does not drop %t2.o and ends up with (i32 align 8) symbol. This is
+; incorrect behavior, but this test does not verify this in order to support
+; both old and new gold.
+
+; Check that the common symbol is not dropped completely, which was a regression
+; in r262676.
+; CHECK: @x = common global i32 0

tools/gold/gold-plugin.cpp

struct PluginInputFile {
 
   ld_plugin_input_file &file() { return *File; }
 
   PluginInputFile(PluginInputFile &&RHS) = default;
   PluginInputFile &operator=(PluginInputFile &&RHS) = default;
 };
 
 struct ResolutionInfo {
+  uint64_t CommonSize = 0;
+  unsigned CommonAlign = 0;
   bool IsLinkonceOdr = true;
   bool UnnamedAddr = true;
   GlobalValue::VisibilityTypes Visibility = GlobalValue::DefaultVisibility;
   bool CommonInternal = false;
   bool UseCommon = false;
-  unsigned CommonSize = 0;
-  unsigned CommonAlign = 0;
-  claimed_file *CommonFile = nullptr;
 };
 
 /// Class to own information used by a task or during its cleanup for a
 /// ThinLTO backend instantiation.
 class ThinLTOTaskInfo {
   /// The input file holding the module bitcode read by the ThinLTO task.
   PluginInputFile InputFile;
 
for (auto &Sym : Obj->symbols()) {
     const GlobalValue *GV = Obj->getSymbolGV(Sym.getRawDataRefImpl());
 
     ResolutionInfo &Res = ResInfo[sym.name];
 
     sym.visibility = LDPV_DEFAULT;
     if (GV) {
       Res.UnnamedAddr &= GV->hasUnnamedAddr();
       Res.IsLinkonceOdr &= GV->hasLinkOnceLinkage();
-      if (GV->hasCommonLinkage()) {
-        Res.CommonAlign = std::max(Res.CommonAlign, GV->getAlignment());
-        const DataLayout &DL = GV->getParent()->getDataLayout();
-        uint64_t Size = DL.getTypeAllocSize(GV->getType()->getElementType());
-        if (Size >= Res.CommonSize) {
-          Res.CommonSize = Size;
-          Res.CommonFile = &cf;
-        }
-      }
       Res.Visibility = getMinVisibility(Res.Visibility, GV->getVisibility());
       switch (GV->getVisibility()) {
       case GlobalValue::DefaultVisibility:
         sym.visibility = LDPV_DEFAULT;
         break;
       case GlobalValue::HiddenVisibility:
         sym.visibility = LDPV_HIDDEN;
         break;
if (get_view(F.handle, &View) != LDPS_OK)
     message(LDPL_FATAL, "Failed to get a view of file");
 
   return View;
 }
 
 static std::unique_ptr<FunctionInfoIndex>
 getFunctionIndexForFile(claimed_file &F, ld_plugin_input_file &Info) {
   const void *View = getSymbolsAndView(F);
+  if (!View)
+    return nullptr;
 
   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))
getFunctionIndexForFile(claimed_file &F, ld_plugin_input_file &Info) {
 
   return Obj.takeIndex();
 }
 
 static std::unique_ptr<Module>
 getModuleForFile(LLVMContext &Context, claimed_file &F, const void *View,
                  ld_plugin_input_file &Info, raw_fd_ostream *ApiFile,
                  StringSet<> &Internalize, StringSet<> &Maybe,
-                 std::vector<GlobalValue *> &Keep) {
+                 std::vector<GlobalValue *> &Keep,
+                 StringMap<unsigned> &Realign) {
   MemoryBufferRef BufferRef(StringRef((const char *)View, Info.filesize),
                             Info.name);
   ErrorOr<std::unique_ptr<object::IRObjectFile>> ObjOrErr =
       object::IRObjectFile::create(BufferRef, Context);
 
   if (std::error_code EC = ObjOrErr.getError())
     message(LDPL_FATAL, "Could not read bitcode from file : %s",
             EC.message().c_str());
if (options::thinlto && (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP ||
       Resolution = LDPR_PREVAILING_DEF;
 
     GV->setUnnamedAddr(Res.UnnamedAddr);
     GV->setVisibility(Res.Visibility);
 
     // Override gold's resolution for common symbols. We want the largest
     // one to win.
     if (GV->hasCommonLinkage()) {
-      cast<GlobalVariable>(GV)->setAlignment(Res.CommonAlign);
       if (Resolution == LDPR_PREVAILING_DEF_IRONLY)
         Res.CommonInternal = true;
 
       if (Resolution == LDPR_PREVAILING_DEF_IRONLY ||
           Resolution == LDPR_PREVAILING_DEF)
         Res.UseCommon = true;
 
-      if (Res.CommonFile == &F && Res.UseCommon) {
+      const DataLayout &DL = GV->getParent()->getDataLayout();
+      uint64_t Size = DL.getTypeAllocSize(GV->getType()->getElementType());
+      unsigned Align = GV->getAlignment();
+
+      if (Res.UseCommon && Size >= Res.CommonSize) {
+        // Take GV.
         if (Res.CommonInternal)
           Resolution = LDPR_PREVAILING_DEF_IRONLY;
         else
           Resolution = LDPR_PREVAILING_DEF;
+        cast<GlobalVariable>(GV)->setAlignment(
+            std::max(Res.CommonAlign, Align));
       } else {
+        // Do not take GV, it's smaller than what we already have in the
+        // combined module.
         Resolution = LDPR_PREEMPTED_IR;
+        if (Align > Res.CommonAlign)
+          // Need to raise the alignment though.
+          Realign[Sym.name] = Align;
       }
+
+      Res.CommonSize = std::max(Res.CommonSize, Size);
+      Res.CommonAlign = std::max(Res.CommonAlign, Align);
     }
 
     switch (Resolution) {
     case LDPR_UNKNOWN:
       llvm_unreachable("Unexpected resolution");
 
     case LDPR_RESOLVED_IR:
     case LDPR_RESOLVED_EXEC:

 
 /// Links the module in \p View from file \p F into the combined module
 /// saved in the IRMover \p L. Returns true on error, false on success.
 static bool linkInModule(LLVMContext &Context, IRMover &L, claimed_file &F,
                          const void *View, ld_plugin_input_file &File,
                          raw_fd_ostream *ApiFile, StringSet<> &Internalize,
                          StringSet<> &Maybe) {
   std::vector<GlobalValue *> Keep;
-  std::unique_ptr<Module> M = getModuleForFile(Context, F, View, File, ApiFile,
-                                               Internalize, Maybe, Keep);
+  StringMap<unsigned> Realign;
+  std::unique_ptr<Module> M = getModuleForFile(
+      Context, F, View, File, ApiFile, Internalize, Maybe, Keep, Realign);
   if (!M.get())
     return false;
   if (!options::triple.empty())
     M->setTargetTriple(options::triple.c_str());
   else if (M->getTargetTriple().empty()) {
     M->setTargetTriple(DefaultTriple);
   }
 
-  if (L.move(std::move(M), Keep, [](GlobalValue &, IRMover::ValueAdder) {}))
-    return true;
+  if (!L.move(std::move(M), Keep, [](GlobalValue &, IRMover::ValueAdder) {}))
     return false;
+
+  for (const auto &I : Realign) {
+    GlobalValue *Dst = L.getModule().getNamedValue(I.first());
+    if (!Dst)
+      continue;
+    cast<GlobalVariable>(Dst)->setAlignment(I.second);
+  }
+
+  return true;
 }
 
 /// Perform the ThinLTO backend on a single module, invoking the LTO and codegen
 /// pipelines.
 static void thinLTOBackendTask(claimed_file &F, const void *View,
                                ld_plugin_input_file &File,
                                raw_fd_ostream *ApiFile,
                                const FunctionInfoIndex &CombinedIndex,
static void thinLTOBackends(raw_fd_ostream *ApiFile,
   // on destruction.
   {
     ThreadPool ThinLTOThreadPool(MaxThreads);
     for (claimed_file &F : Modules) {
       // Do all the gold callbacks in the main thread, since gold is not thread
       // safe by default.
       PluginInputFile InputFile(F.handle);
       const void *View = getSymbolsAndView(F);
+      if (!View)
+        continue;
 
       SmallString<128> Filename;
       if (!options::obj_path.empty())
         // Note that openOutputFile will append a unique ID for each task
         Filename = options::obj_path;
       else if (options::TheOutputType == options::OT_SAVE_TEMPS) {
         // Use the input file name so that we get a unique and identifiable
         // output file for each ThinLTO backend task.
static ld_plugin_status allSymbolsReadHook(raw_fd_ostream *ApiFile) {
   std::unique_ptr<Module> Combined(new Module("ld-temp.o", Context));
   IRMover L(*Combined);
 
   StringSet<> Internalize;
   StringSet<> Maybe;
   for (claimed_file &F : Modules) {
     PluginInputFile InputFile(F.handle);
     const void *View = getSymbolsAndView(F);
+    if (!View)
+      continue;
     if (linkInModule(Context, L, F, View, InputFile.file(), ApiFile,
                      Internalize, Maybe))
       message(LDPL_FATAL, "Failed to link module");
   }
 
   for (const auto &Name : Internalize) {
     GlobalValue *GV = Combined->getNamedValue(Name.first());
     if (GV)