[ThinLTO] Avoid backwards references to renamed locals in distributed backend case

[tejohnson]

Summary

Address another issue related to distributed backends and
--start-lib/--end-lib (see D22356 for more background).

We can also run into trouble when importing the prevailing copy of a
weak symbol if it references a local value. The local value will be
promoted and renamed using the module hash of the prevailing copy. If
that prevailing copy is not needed by the final native link, due to
intervening imports/inlines, that renamed copy of the local value may
not be linked in, and any importing module in a library listed later on
the link line will have an undefined reference.

Avoid this via a new PreventBackwardsModuleRefs flag to the importer
which prevents linking in any multiply-defined symbol if it references
a local needing renaming. We pass true for the flag in the
thinlto_index_only case (where we will have distributed backends and a
separate native link process).

I considered a couple of other solutions:

1. Always link in the last copy of a multiply defined symbol. However,

in the non-odr case this may change the program semantics as we would be
importing a different copy than the linker would normally select.

2. Force import the referenced local and as a local copy (don't use the

promoted copy). This is the better long term fix (and I have added a
FIXME to gold-plugin to that effect), but it also relies on doing
importing in the distributed backeds which is not yet there (will come
in via D21545). In that case we would still need to detect this case in
the importer, but force import the referenced locals instead of
preventing the referencing weak/linkonce import. We would also need to
either mark all copies of the referenced local for internalization after
importing, or only those copies in the distributed backend individual index
files that don't also have a singly-defined reference to the source module
(meaning the source module must be after the importing module on the
link line).

Added a new test for this case.

Depends on D22356.

[tejohnson]

See discussion in D22356 - will be addressing this by having ThinLink communicate which files to include to the final link.

include/llvm/Transforms/IPO/FunctionImport.h

 ///
 /// \p ImportLists will be populated with an entry for every Module we are
 /// importing into. This entry is itself a map that can be passed to
 /// FunctionImporter::importFunctions() above (see description there).
 ///
 /// \p ExportLists contains for each Module the set of globals (GUID) that will
 /// be imported by another module, or referenced by such a function. I.e. this
 /// is the set of globals that need to be promoted/renamed appropriately.
+///
+/// \p PreventBackwardsModuleRefs should be set to true if we need to
+/// conservatively prevent creating a reference to a promoted/renamed local
+/// in a module earlier on the link line (by preventing import of any
+/// multiply defined weak linkage function that references such a local).
+/// It is used in the case when the ThinLTO backends will be a separate process,
+/// and the subsequent final native object link might select a different copy as
+/// prevailing.
 void ComputeCrossModuleImport(
     const ModuleSummaryIndex &Index,
     const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
     StringMap<FunctionImporter::ImportMapTy> &ImportLists,
-    StringMap<FunctionImporter::ExportSetTy> &ExportLists);
+    StringMap<FunctionImporter::ExportSetTy> &ExportLists,
+    bool PreventBackwardsModuleRefs = false);
 
 /// Compute all the imports for the given module using the Index.
 ///
 /// \p ImportList will be populated with a map that can be passed to
 /// FunctionImporter::importFunctions() above (see description there).
 void ComputeCrossModuleImportForModule(
     StringRef ModulePath, const ModuleSummaryIndex &Index,
     FunctionImporter::ImportMapTy &ImportList);

lib/Transforms/IPO/FunctionImport.cpp

bool AllCallsCanBeExternallyReferenced = llvm::all_of(
           return canBeExternallyReferenced(Index, Edge.first.getGUID());
         });
     if (!AllCallsCanBeExternallyReferenced)
       return false;
   }
   return true;
 }
 
+// Return true if there are multiple copies of the candidate callee,
+// and it references a local value needing renaming. This is used
+// when we need to conservatively prevent any backwards references, since
+// we don't know which copies of the multiply defined callee are after
+// the importing function, and promoted locals are renamed based on the
+// module hash of the selected copy. (Note we can't simply selecte the
+// last copy which might result in different behavior in the non-odr case.)
+static bool mightIntroduceBackwardsModuleRef(
+    const ModuleSummaryIndex &Index,
+    const GlobalValueSummaryList &CalleeSummaryList,
+    const GlobalValueSummary *S) {
+  auto needsRenaming = [&](GlobalValue::GUID GUID) {
+    auto Summaries = Index.findGlobalValueSummaryList(GUID);
+    if (Summaries == Index.end())
+      return false;
+    if (Summaries->second.size() != 1)
+      // If there are multiple globals with this GUID, then we know it is
+      // not a local symbol, and will not need renaming.
+      return false;
+    return Summaries->second.begin()->get()->needsRenaming();
+  };
+
+  if (CalleeSummaryList.size() > 1) {
+    if (llvm::any_of(S->refs(), [&](const ValueInfo &VI) {
+          return needsRenaming(VI.getGUID());
+        }))
+      return true;
+    if (auto *FuncSummary = dyn_cast<FunctionSummary>(S)) {
+      if (llvm::any_of(FuncSummary->calls(),
+                       [&](const FunctionSummary::EdgeTy &Edge) {
+                         return needsRenaming(Edge.first.getGUID());
+                       }))
+        return true;
+    }
+  }
+  return false;
+}
+
 /// Given a list of possible callee implementation for a call site, select one
 /// that fits the \p Threshold.
 ///
 /// FIXME: select "best" instead of first that fits. But what is "best"?
 /// - The smallest: more likely to be inlined.
 /// - The one with the least outgoing edges (already well optimized).
 /// - One from a module already being imported from in order to reduce the
 ///   number of source modules parsed/linked.
 /// - One that has PGO data attached.
 /// - [insert you fancy metric here]
 static const GlobalValueSummary *
 selectCallee(const ModuleSummaryIndex &Index,
              const GlobalValueSummaryList &CalleeSummaryList,
-             unsigned Threshold) {
+             unsigned Threshold, bool PreventBackwardsModuleRefs) {
   auto It = llvm::find_if(
       CalleeSummaryList,
       [&](const std::unique_ptr<GlobalValueSummary> &SummaryPtr) {
         auto *GVSummary = SummaryPtr.get();
         if (GlobalValue::isInterposableLinkage(GVSummary->linkage()))
           // There is no point in importing these, we can't inline them
           return false;
         if (auto *AS = dyn_cast<AliasSummary>(GVSummary)) {
auto It = llvm::find_if(
         if (!eligibleForImport(Index, *Summary))
           return false;
 
         return true;
       });
   if (It == CalleeSummaryList.end())
     return nullptr;
 
-  return cast<GlobalValueSummary>(It->get());
+  const GlobalValueSummary *S = cast<GlobalValueSummary>(It->get());
+  if (PreventBackwardsModuleRefs &&
+      mightIntroduceBackwardsModuleRef(Index, CalleeSummaryList, S))
+    return nullptr;
+
+  return S;
 }
 
 /// Return the summary for the function \p GUID that fits the \p Threshold, or
 /// null if there's no match.
 static const GlobalValueSummary *selectCallee(GlobalValue::GUID GUID,
                                               unsigned Threshold,
-                                              const ModuleSummaryIndex &Index) {
+                                              const ModuleSummaryIndex &Index,
+                                              bool PreventBackwardsModuleRefs) {
   auto CalleeSummaryList = Index.findGlobalValueSummaryList(GUID);
   if (CalleeSummaryList == Index.end())
     return nullptr; // This function does not have a summary
-  return selectCallee(Index, CalleeSummaryList->second, Threshold);
+  return selectCallee(Index, CalleeSummaryList->second, Threshold,
+                      PreventBackwardsModuleRefs);
 }
 
 /// Mark the global \p GUID as export by module \p ExportModulePath if found in
 /// this module. If it is a GlobalVariable, we also mark any referenced global
 /// in the current module as exported.
 static void exportGlobalInModule(const ModuleSummaryIndex &Index,
                                  StringRef ExportModulePath,
                                  GlobalValue::GUID GUID,
static void exportGlobalInModule(const ModuleSummaryIndex &Index,
   }
 }
 
 using EdgeInfo = std::pair<const FunctionSummary *, unsigned /* Threshold */>;
 
 /// Compute the list of functions to import for a given caller. Mark these
 /// imported functions and the symbols they reference in their source module as
 /// exported from their source module.
-static void computeImportForFunction(
-    const FunctionSummary &Summary, const ModuleSummaryIndex &Index,
-    unsigned Threshold, const GVSummaryMapTy &DefinedGVSummaries,
+static void
+computeImportForFunction(const FunctionSummary &Summary,
+                         const ModuleSummaryIndex &Index, unsigned Threshold,
+                         const GVSummaryMapTy &DefinedGVSummaries,
                          SmallVectorImpl<EdgeInfo> &Worklist,
                          FunctionImporter::ImportMapTy &ImportsForModule,
-    StringMap<FunctionImporter::ExportSetTy> *ExportLists = nullptr) {
+                         StringMap<FunctionImporter::ExportSetTy> *ExportLists,
+                         bool PreventBackwardsModuleRefs) {
   for (auto &Edge : Summary.calls()) {
     auto GUID = Edge.first.getGUID();
     DEBUG(dbgs() << " edge -> " << GUID << " Threshold:" << Threshold << "\n");
 
     if (DefinedGVSummaries.count(GUID)) {
       DEBUG(dbgs() << "ignored! Target already in destination module.\n");
       continue;
     }
 
-    auto *CalleeSummary = selectCallee(GUID, Threshold, Index);
+    auto *CalleeSummary =
+        selectCallee(GUID, Threshold, Index, PreventBackwardsModuleRefs);
     if (!CalleeSummary) {
       DEBUG(dbgs() << "ignored! No qualifying callee with summary found.\n");
       continue;
     }
     // "Resolve" the summary, traversing alias,
     const FunctionSummary *ResolvedCalleeSummary;
     if (isa<AliasSummary>(CalleeSummary)) {
       ResolvedCalleeSummary = cast<FunctionSummary>(

 }
 
 /// Given the list of globals defined in a module, compute the list of imports
 /// as well as the list of "exports", i.e. the list of symbols referenced from
 /// another module (that may require promotion).
 static void ComputeImportForModule(
     const GVSummaryMapTy &DefinedGVSummaries, const ModuleSummaryIndex &Index,
     FunctionImporter::ImportMapTy &ImportsForModule,
-    StringMap<FunctionImporter::ExportSetTy> *ExportLists = nullptr) {
+    StringMap<FunctionImporter::ExportSetTy> *ExportLists = nullptr,
+    bool PreventBackwardsModuleRefs = false) {
   // Worklist contains the list of function imported in this module, for which
   // we will analyse the callees and may import further down the callgraph.
   SmallVector<EdgeInfo, 128> Worklist;
 
   // Populate the worklist with the import for the functions in the current
   // module
   for (auto &GVSummary : DefinedGVSummaries) {
     auto *Summary = GVSummary.second;
     if (auto *AS = dyn_cast<AliasSummary>(Summary))
       Summary = &AS->getAliasee();
     auto *FuncSummary = dyn_cast<FunctionSummary>(Summary);
     if (!FuncSummary)
       // Skip import for global variables
       continue;
     DEBUG(dbgs() << "Initalize import for " << GVSummary.first << "\n");
     computeImportForFunction(*FuncSummary, Index, ImportInstrLimit,
                              DefinedGVSummaries, Worklist, ImportsForModule,
-                             ExportLists);
+                             ExportLists, PreventBackwardsModuleRefs);
   }
 
   while (!Worklist.empty()) {
     auto FuncInfo = Worklist.pop_back_val();
     auto *Summary = FuncInfo.first;
     auto Threshold = FuncInfo.second;
 
     // Process the newly imported functions and add callees to the worklist.
     // Adjust the threshold
     Threshold = Threshold * ImportInstrFactor;
 
     computeImportForFunction(*Summary, Index, Threshold, DefinedGVSummaries,
-                             Worklist, ImportsForModule, ExportLists);
+                             Worklist, ImportsForModule, ExportLists,
+                             PreventBackwardsModuleRefs);
   }
 }
 
 } // anonymous namespace
 
 /// Compute all the import and export for every module using the Index.
 void llvm::ComputeCrossModuleImport(
     const ModuleSummaryIndex &Index,
     const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
     StringMap<FunctionImporter::ImportMapTy> &ImportLists,
-    StringMap<FunctionImporter::ExportSetTy> &ExportLists) {
+    StringMap<FunctionImporter::ExportSetTy> &ExportLists,
+    bool PreventBackwardsModuleRefs) {
   // For each module that has function defined, compute the import/export lists.
   for (auto &DefinedGVSummaries : ModuleToDefinedGVSummaries) {
     auto &ImportsForModule = ImportLists[DefinedGVSummaries.first()];
     DEBUG(dbgs() << "Computing import for Module '"
                  << DefinedGVSummaries.first() << "'\n");
     ComputeImportForModule(DefinedGVSummaries.second, Index, ImportsForModule,
-                           &ExportLists);
+                           &ExportLists, PreventBackwardsModuleRefs);
   }
 
 #ifndef NDEBUG
   DEBUG(dbgs() << "Import/Export lists for " << ImportLists.size()
                << " modules:\n");
   for (auto &ModuleImports : ImportLists) {
     auto ModName = ModuleImports.first();
     auto &Exports = ExportLists[ModName];

test/tools/gold/X86/Inputs/thinlto_preventbackwardsref-1.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @foobar() {
+entry:
+  ret void
+}
+
+define void @baz() {
+entry:
+  call void @f()
+  ret void
+}
+
+@z = internal global i32 0, align 4
+@w = external global i32, align 4
+
+define linkonce_odr void @f() {
+entry:
+  %0 = load i32, i32* @z, align 4
+  store i32 %0, i32* @w, align 4
+  ret void
+}

test/tools/gold/X86/Inputs/thinlto_preventbackwardsref-2.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define linkonce_odr void @x() {
+  ret void
+}
+
+define void @bar() {
+entry:
+  call void @x()
+  call void @x()
+  call void @x()
+  call void @x()
+  call void @x()
+  call void (...) @f()
+  ret void
+}
+
+declare void @f(...)

test/tools/gold/X86/Inputs/thinlto_preventbackwardsref-3.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@z = internal global i32 0, align 4
+@w = external global i32, align 4
+
+define linkonce_odr void @f() {
+entry:
+  %0 = load i32, i32* @z, align 4
+  store i32 %0, i32* @w, align 4
+  ret void
+}

test/tools/gold/X86/thinlto_preventbackwardsref.ll

+; This test ensures that we don't import any multiply-defined weak symbol
+; that references a local needing renaming when we have separate ThinLTO
+; backend processes, and therefore multiple invocations of the gold linker
+; (the ThinLink and then the final native object link). Specifically, when
+; linking within --start-lib/--end-lib which treats the final object files as
+; being in separate libraries, we can run into trouble if the prevailing
+; linkonce from the first link is no longer selected by the linker after
+; importing/inlining, and we don't have a strong (forward) reference causing
+; the linker to pull in the renamed local from that library in the final link.
+; This is needed to avoid undefined references in the final link. See the
+; comments before the two link steps that describe this scenario.
+
+; First generate bitcode with a module summary index for each file
+; RUN: opt -module-hash -module-summary %s -o %t.o
+; RUN: opt -module-hash -module-summary %p/Inputs/thinlto_preventbackwardsref-1.ll -o %t2.o
+; RUN: opt -module-hash -module-summary %p/Inputs/thinlto_preventbackwardsref-2.ll -o %t3.o
+; RUN: opt -module-hash -module-summary %p/Inputs/thinlto_preventbackwardsref-3.ll -o %t4.o
+
+; Next do the ThinLink step, specifying thinlto-index-only so that the gold
+; plugin exits after generating individual indexes. Also, that flag should
+; be used indicate to the importer that we should not import any multiply
+; defined weak (including linkonce) symbol referencing a local which will
+; be renamed.
+; Also, -import-instr-limit=4 is used to prevent importing of some of
+; the functions, chiefly bar() which references and would like to import
+; the linkonce f() which references a local z.
+; Finally, although it doesn't impact the ThinLink step, we use
+; --start-lib/--end-lib to match the final link step further down,
+; where we want archive library linking behavior.
+; Note that because at this point there is a strong reference from
+; %t.o to %t2.o (foobar), all symbols from %t2.o are selected
+; including the linkonce symbols, and therefore the %t2.o copy of f()
+; is prevailing. At this point the %t4.o copy of f() is preempted.
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=thinlto-index-only \
+; RUN:    --plugin-opt=-import-instr-limit=4 \
+; RUN:    -o %t5 \
+; RUN:    %t.o \
+; RUN:    --start-lib %t2.o --end-lib \
+; RUN:    --start-lib %t3.o --end-lib \
+; RUN:    --start-lib %t4.o --end-lib
+
+; Simulate the separate ThinLTO backend processes which will do promotion
+; (including the weak resolutions), function importing, followed by the
+; optimization pipeline including inlining.
+; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t.o.thinlto.bc %t.o -o - | llvm-lto -thinlto-action=import -thinlto-index %t.o.thinlto.bc -thinlto-module-id=%t.o - -o - | opt -O2 -o %t.opt.bc
+; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t2.o.thinlto.bc %t2.o -o - | llvm-lto -thinlto-action=import -thinlto-index %t2.o.thinlto.bc -thinlto-module-id=%t2.o - -o - | opt -O2 -o %t2.opt.bc
+; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t3.o.thinlto.bc %t3.o -o - | llvm-lto -thinlto-action=import -thinlto-index %t3.o.thinlto.bc -thinlto-module-id=%t3.o - -o - | opt -O2 -o %t3.opt.bc
+; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t4.o.thinlto.bc %t4.o -o - | llvm-lto -thinlto-action=import -thinlto-index %t4.o.thinlto.bc -thinlto-module-id=%t4.o - -o - | opt -O2 -o %t4.opt.bc
+
+; The use of f() should not have been imported, since it has multiple copies
+; and contains a reference to a local requiring renaming on export (@z).
+; RUN: llvm-dis %t3.opt.bc -o - | FileCheck %s --check-prefix=CHECK-OBJ
+; CHECK-OBJ: declare void @f(...)
+
+; Generate native object files for the final link
+; RUN: llc %t.opt.bc -filetype=obj -o %t.opt.o
+; RUN: llc %t2.opt.bc -filetype=obj -o %t2.opt.o
+; RUN: llc %t3.opt.bc -filetype=obj -o %t3.opt.o
+; RUN: llc %t4.opt.bc -filetype=obj -o %t4.opt.o
+
+; Final link. Here the --start-lib/--end-lib provoke archive library linking
+; behavior. The relevant effect of this is that the reference of f() in
+; %t3.opt.o will not be resolved to the definition in the earlier %t2.opt.o
+; library, which no longer is selected as prevailing (since the only strong
+; reference to the %t2.opt.o library was inlined into %t.opt.o). Therefore,
+; the promoted/renamed copy of z from %t.opt.o is no longer linked in, and
+; we would end up with an undefined reference if we had imported f() into
+; %t3.opt.o and referenced that copy of the promoted z. Note that the
+; gold-plugin does not know whether --start-lib/--end-lib were used.
+; RUN: %gold \
+; RUN:    -o %t6 \
+; RUN:    %t.opt.o \
+; RUN:    --start-lib %t2.opt.o --end-lib \
+; RUN:    --start-lib %t3.opt.o --end-lib \
+; RUN:    --start-lib %t4.opt.o --end-lib
+
+; Sanity check that bar() is defined in the final binary.
+; RUN: llvm-nm %t6 | FileCheck %s --check-prefix=CHECK-FINAL
+; CHECK-FINAL: T bar
+
+@w = global i32 0, align 4
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @main() {
+entry:
+  call void (...) @foobar()
+  call void (...) @bar()
+  ret i32 0
+}
+
+declare void @bar(...)
+declare void @foobar(...)

tools/gold/gold-plugin.cpp

static ld_plugin_status thinLTOLink(raw_fd_ostream *ApiFile) {
   // Collect for each module the list of function it defines (GUID ->
   // Summary).
   StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>>
       ModuleToDefinedGVSummaries(NextModuleId);
   CombinedIndex.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   StringMap<FunctionImporter::ImportMapTy> ImportLists(NextModuleId);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(NextModuleId);
-  ComputeCrossModuleImport(CombinedIndex, ModuleToDefinedGVSummaries,
-                           ImportLists, ExportLists);
+
+  // Note that in several places below we are conservative in the
+  // thinlto_index_only case, where there will be a separate link process to
+  // link the native objects. Specifically this is because we don't know
+  // whether the object files were from archive libraries (potentially formed
+  // via --start-lib/--end-lib pairs). This is because symbols are not selected
+  // out of (archive) libraries unless there is a strong reference to a symbol
+  // defined in it. When there are multiple copies of a symbol with weak
+  // linkage, and the ThinLink step selects one as prevailing because there
+  // is a strong reference to something in the library, if the strong reference
+  // goes away after importing and inlining the final native link will
+  // no longer link in any symbols from that object. Therefore, we must
+  // be very careful to ensure that there is always a forward reference
+  // to external symbols in the thinlto_index_only case. This has several
+  // implications below:
+  //
+  // 1) Avoid backwards references to promoted local symbols.
+  //    Normally for the --start-lib/--end-lib link to succeed, external
+  //    symbol references from the library must be forward (i.e. satisfied by
+  //    an object file later on the link line. However, this could be violated
+  //    by importing of multiply-defined (weak linkage) symbols that reference
+  //    promoted local values. This is because we may decide to import the
+  //    weak linkage function from a module that was earlier on the link line,
+  //    and any referenced locals will be promoted and renamed using that
+  //    module's hash. Therefore, pass true for PreventBackwardsModuleRefs
+  //    in the call to ComputeCrossModuleImport below to ensure we
+  //    conservatively prevent introducing such backwards references (by
+  //    preventing an import of a multiply defined symbol that references a
+  //    local needing renaming).
+  //    FIXME: This could also be addressed by force-importing the renamed
+  //    local in this case, and ensuring that the imported copy has
+  //    local linkage (once internalization is enabled in the
+  //    thinlto_index_only case).
+  //
+  // 2) Prevent elimination of non-prevailing copies of weak symbols.
+  //    Since as noted above the ThinLink prevailing copy may no longer
+  //    be pulled in by the final native link if all strong references
+  //    go away after importing/inlining, we need to ensure that non-prevailing
+  //    copies stick around. When the non-prevailing copy is exported, we
+  //    preserve it by converting to weak (via the isExported check in
+  //    isPrevailing below). When it isn't exported, we preserve it for
+  //    any remaining references in the same module by passing true
+  //    for PreserveNonPrevailing in the call to
+  //    thinLTOResolveWeakForLinkerInIndex below to prevent their conversion
+  //    to available_externally.
+
+  // See comments above for when we need to pass true for
+  // PreventBackwardsModuleRefs.
+  ComputeCrossModuleImport(
+      CombinedIndex, ModuleToDefinedGVSummaries, ImportLists, ExportLists,
+      /* PreventBackwardsModuleRefs = */ options::thinlto_index_only);
 
   // Callback for internalization, to prevent internalization of symbols
   // that were not candidates initially, and those that are being imported
   // (which introduces new cross references).
   auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
     const auto &ExportList = ExportLists.find(ModuleIdentifier);
     return (ExportList != ExportLists.end() &&
             ExportList->second.count(GUID)) ||
            Preserve.count(GUID);
   };
 
   auto isPrevailing = [&](GlobalValue::GUID GUID, const GlobalValueSummary *S) {
     const auto &Prevailing = PrevailingCopy.find(GUID);
     assert(Prevailing != PrevailingCopy.end());
     return Prevailing->second == S ||
-           // See comments below for why we conservatively return true for
+           // See comments above for why we conservatively return true for
            // exported symbols under thinlto_index_only.
            (options::thinlto_index_only && isExported(S->modulePath(), GUID));
   };
 
-  // Since we don't know whether the object files were from archive
-  // libraries (potentially formed via --start-lib/--end-lib pairs),
-  // we must conservatively ensure that linkonce/weak symbols are
-  // preserved even when they are not prevailing in the thinlto_index_only
-  // case. In that case there will be a separate link process to link the
-  // native objects, and depending on the intervening importing/inlining
-  // the current prevailing copy may no longer be selected out of its
-  // library. This is because symbols are not selected out of (archive)
-  // libraries unless there is a strong reference to a symbol defined in it,
-  // and that strong reference may go away after importing.
-  // Therefore, we pass true for PreserveNonPrevailing when we have
-  // thinlto_index_only, and also conservatively return true for
-  // exported symbols from isPrevailing above.
+  // See comments above for when we need to pass true for PreserveNonPrevailing.
   thinLTOResolveWeakForLinkerInIndex(
       CombinedIndex, isPrevailing,
       [](StringRef ModuleIdentifier, GlobalValue::GUID GUID,
          GlobalValue::LinkageTypes NewLinkage) {},
       /* PreserveNonPrevailing = */ options::thinlto_index_only);
 
   // Use global summary-based analysis to identify symbols that can be
   // internalized (because they aren't exported or preserved as per callback).