[ThinLTO] Perform conservative weak/linkonce resolution in distributed backend case

[tejohnson]

Summary

One tricky aspect of the weak/linkonce resolution in the distributed
backend case occurs if the link involves --start-lib/--end-lib with some
of the object files. Because there are two separate links (the ThinLink
and then the final native link), depending on the intervening importing
and inlining we can get into a situation where the linkonce selected
as prevailing in the ThinLink is no longer linked in by the second link.
The linker will only pull symbols from an archive library, in this
case formed via --start-lib/--end-lib, if there is a strong reference
to a symbol in that library from a library/object listed earlier in the
link, which may no longer be the case after importing etc. Note that
the gold-plugin does not know whether the objects are in a library
formed by --start-lib/--end-lib.

To handle this, under thinlto_index_only (which indicates we will have separate
ThinLTO processes), a new flag to thinLTOResolveWeakForLinkerInIndex and
a change to the isPrevailing callback conservatively ensure that otherwise non-prevailing
linkonce/weak are kept and also converted to weak when exported.

The new thinlto_preserve_nonprevailodr.ll test case ensures this works.

[tejohnson]

As I just noted in D21545, that patch will have the side effect of enabling the linkonce/weak resolution for the distributed backend case. I like the way that patch adds the resolution before invoking the importer, instead of doing it via the FunctionImporter pass as is done here. However, it will expose the bug I am fixing here with the new PreserveNonPrevailing flag. I think I should probably change this patch just to fix that bug and add the new test case, and submit that just after D21545 goes in.

As such, please review the patch ignoring the changes to Transforms/IPO/FunctionImport.cpp, which will go away once D21545 goes in.

[pcc]

include/llvm/LTO/LTO.h

70

Instead of adding this parameter (and adding back isExported), can you simply pass in an isPrevailing that always returns true?

[tejohnson]

include/llvm/LTO/LTO.h

70

Good idea on changing the IsPrevailing callback to avoid passing IsExported. However we still need the flag to avoid converting to available externally in the non exported case. We need to keep link once as link once e.g. when there is a ref in the same module in case it isn't inlined (and the previous prevailing copy is no longer linked in).

[tejohnson]

In D22356#484213, @tejohnson wrote:

As I just noted in D21545, that patch will have the side effect of enabling the linkonce/weak resolution for the distributed backend case. I like the way that patch adds the resolution before invoking the importer, instead of doing it via the FunctionImporter pass as is done here. However, it will expose the bug I am fixing here with the new PreserveNonPrevailing flag. I think I should probably change this patch just to fix that bug and add the new test case, and submit that just after D21545 goes in.

As such, please review the patch ignoring the changes to Transforms/IPO/FunctionImport.cpp, which will go away once D21545 goes in.

Actually, I just realized that I can simulate the effect of D21545 without my FunctionImport pass changes via the right sequence of llvm-lto -thinlto-action invocations. Will rework the patch that way so that it can go in and hopefully fix this issue before D21545 goes in and exposes it.

[pcc]

include/llvm/LTO/LTO.h

70

However we still need the flag to avoid converting to available externally in the non exported case. We need to keep link once as link once e.g. when there is a ref in the same module in case it isn't inlined (and the previous prevailing copy is no longer linked in).

In the non-exported case, aren't we changing the linkage to internal anyway? That was what D21883 was about.

Anyway, I'll wait and see what your updated patch looks like.

[tejohnson]

Rebase and address review comments:

• Fold the isExported check into the isPrevailing callback from

gold-plugin, to avoid needing to pass that callback.

• Remove FunctionImport.cpp changes, since they will be obviated by

D21545.

• Rework test case to use llvm-lto for weak symbol resolution and

importing, to simulate the behavior of the ThinLTO backend invoked by
clang after D21545.

[mehdi_amini]

Right now it is not clear to me why it would be legit (understand: if it should be supported) to perform the second link using static archive.
The first link already selected object from the archives, so we should be able to provide a list of objects to the second/final link.

[tejohnson]

In D22356#487588, @mehdi_amini wrote:

Right now it is not clear to me why it would be legit (understand: if it should be supported) to perform the second link using static archive.
The first link already selected object from the archives, so we should be able to provide a list of objects to the second/final link.

That's a good question and an idea I thought about briefly but discarded for a couple reasons. I was concerned about requiring communication between the ThinLink and final link to build the link line (it would be more difficult to support in a build system, and also seems conceptually more complicated). Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior. But maybe I just need to think through that some more...

[tejohnson]

Added davidxl for thoughts on changing the final link line as Mehdi suggested.

[mehdi_amini]

That's a good question and an idea I thought about briefly but discarded for a couple reasons. I was concerned about requiring communication between the ThinLink and final link to build the link line (it would be more difficult to support in a build system, and also seems conceptually more complicated).

How is the final link invocation computed right now?

Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior.

Your observations about the linker picking different symbols seem to indicate that the --start-lib/--end-lib model is already broken.
If a list of .o on the command line is not enough for relinking, there's gonna be a need for a "linker resolution map" file that drives the linker.

[tejohnson]

In D22356#487784, @mehdi_amini wrote:

That's a good question and an idea I thought about briefly but discarded for a couple reasons. I was concerned about requiring communication between the ThinLink and final link to build the link line (it would be more difficult to support in a build system, and also seems conceptually more complicated).

How is the final link invocation computed right now?

The link line is essentially the same, but with native .o instead of the bitcode .o. (See the new test case for an example)

Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior.

Your observations about the linker picking different symbols seem to indicate that the --start-lib/--end-lib model is already broken.

When you say "is already broken" do you mean even in non-ThinLTO mode? I'm not sure why - it is just like having an archive of the objects between each start/end pair.

If a list of .o on the command line is not enough for relinking, there's gonna be a need for a "linker resolution map" file that drives the linker.

In ThinLTO it is because of the change (between the ThinLink and native object link) in which strong references exist between objects/libraries due to importing and inlining. But I believe with this patch and the follow-on D22467 the importing and symbol resolution is made suitably conservative.

[davidxl]

How about the following solution:

1. in second link, force referencing symbols that are marked as prevailing definitions

or

2. for distributed build mode, do not even do prevailing symbol selection in the first link. The side effect is the same as what this patch does -- slightly increased object file size (not the final binary size).

[mehdi_amini]

In D22356#487800, @tejohnson wrote:

In D22356#487784, @mehdi_amini wrote:

That's a good question and an idea I thought about briefly but discarded for a couple reasons. I was concerned about requiring communication between the ThinLink and final link to build the link line (it would be more difficult to support in a build system, and also seems conceptually more complicated).

How is the final link invocation computed right now?

The link line is essentially the same, but with native .o instead of the bitcode .o. (See the new test case for an example)

The question is: in the presence of static archives, how do you generates --start-lib/--end-lib? This seems to already require some build-system integration?

Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior.

Your observations about the linker picking different symbols seem to indicate that the --start-lib/--end-lib model is already broken.

When you say "is already broken" do you mean even in non-ThinLTO mode? I'm not sure why - it is just like having an archive of the objects between each start/end pair.

I'm only talking about ThinLTO and the two-stage linking, i.e. the second invocation of the linker does not end-up with the same prevailing resolution as the first invocation. Your current patches are working around this deficiency.

If a list of .o on the command line is not enough for relinking, there's gonna be a need for a "linker resolution map" file that drives the linker.

In ThinLTO it is because of the change (between the ThinLink and native object link) in which strong references exist between objects/libraries due to importing and inlining. But I believe with this patch and the follow-on D22467 the importing and symbol resolution is made suitably conservative.

I don't see any justification for --start-lib/--end-lib right now.

[tejohnson]

In D22356#487812, @mehdi_amini wrote:

In D22356#487800, @tejohnson wrote:

In D22356#487784, @mehdi_amini wrote:

That's a good question and an idea I thought about briefly but discarded for a couple reasons. I was concerned about requiring communication between the ThinLink and final link to build the link line (it would be more difficult to support in a build system, and also seems conceptually more complicated).

How is the final link invocation computed right now?

The link line is essentially the same, but with native .o instead of the bitcode .o. (See the new test case for an example)

The question is: in the presence of static archives, how do you generates --start-lib/--end-lib? This seems to already require some build-system integration?

We use --start-lib/--end-lib in all of our links - not regular archive libraries. Yes, a distributed build on regular archive libraries will require build system integration to extract the individual object files first.

Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior.

Your observations about the linker picking different symbols seem to indicate that the --start-lib/--end-lib model is already broken.

When you say "is already broken" do you mean even in non-ThinLTO mode? I'm not sure why - it is just like having an archive of the objects between each start/end pair.

I'm only talking about ThinLTO and the two-stage linking, i.e. the second invocation of the linker does not end-up with the same prevailing resolution as the first invocation. Your current patches are working around this deficiency.

If a list of .o on the command line is not enough for relinking, there's gonna be a need for a "linker resolution map" file that drives the linker.

In ThinLTO it is because of the change (between the ThinLink and native object link) in which strong references exist between objects/libraries due to importing and inlining. But I believe with this patch and the follow-on D22467 the importing and symbol resolution is made suitably conservative.

I don't see any justification for --start-lib/--end-lib right now.

We use --start-lib/--end-lib internally instead of regular objects. So it is not a matter of justification, it is a matter of keeping that working.

[tejohnson]

In D22356#487908, @tejohnson wrote:

In D22356#487812, @mehdi_amini wrote:

I don't see any justification for --start-lib/--end-lib right now.

We use --start-lib/--end-lib internally instead of regular objects. So it is not a matter of justification, it is a matter of keeping that working.

Sorry, I meant "instead of regular archives".

[mehdi_amini]

The link line is essentially the same, but with native .o instead of the bitcode .o. (See the new test case for an example)

The question is: in the presence of static archives, how do you generates --start-lib/--end-lib? This seems to already require some build-system integration?

We use --start-lib/--end-lib in all of our links - not regular archive libraries. Yes, a distributed build on regular archive libraries will require build system integration to extract the individual object files first.

Ok I see, makes sense, I thought you were avoiding re-creating the static library and emulating it with these options.

Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior.

Your observations about the linker picking different symbols seem to indicate that the --start-lib/--end-lib model is already broken.

When you say "is already broken" do you mean even in non-ThinLTO mode? I'm not sure why - it is just like having an archive of the objects between each start/end pair.

I'm only talking about ThinLTO and the two-stage linking, i.e. the second invocation of the linker does not end-up with the same prevailing resolution as the first invocation. Your current patches are working around this deficiency.

If a list of .o on the command line is not enough for relinking, there's gonna be a need for a "linker resolution map" file that drives the linker.

In ThinLTO it is because of the change (between the ThinLink and native object link) in which strong references exist between objects/libraries due to importing and inlining. But I believe with this patch and the follow-on D22467 the importing and symbol resolution is made suitably conservative.

I don't see any justification for --start-lib/--end-lib right now.

We use --start-lib/--end-lib internally instead of regular objects. So it is not a matter of justification, it is a matter of keeping that working.

I don't believe this is relevant: the fact that the first link is taking libraries as an input does not make it a compelling case to use them for the second link. Static libraries or start-lib/end-lib are a specific semantic model, and I believe it is just wrong to pass them to the final link.

The reason is that the first link is performing linker resolution: this decision process carry some specific semantic with archives. After this resolution and the ThinLTO process, there is no reason that makes sense to me right now to repeat this process.

It is possible that it is because I have a different mental model of static archives right now. AFAIK, the semantic difference between plain objects and archive is that an object defined in an archive is loaded and selected by the linker only if one the symbol it defines is referenced.

Keeping the linker semantic with ThinLTO means that the objects and symbols selected during the first link should be the "source of truth": i.e. we don't want a different linker resolution during the second link. Every objects that was selected for the first link should be included in the second link (hence it is wrong to use --start-lib/--end-lib).

Also, the distributed build system probably needs to handle the case where an object in the archive was not selected to be part of the link at all, won't be processed by ThinLTO, and there won't be any object to pass to the final link. I'm not sure how you're handling this with gold right now though.

[tejohnson]

In D22356#487942, @mehdi_amini wrote:

The link line is essentially the same, but with native .o instead of the bitcode .o. (See the new test case for an example)

The question is: in the presence of static archives, how do you generates --start-lib/--end-lib? This seems to already require some build-system integration?

We use --start-lib/--end-lib in all of our links - not regular archive libraries. Yes, a distributed build on regular archive libraries will require build system integration to extract the individual object files first.

Ok I see, makes sense, I thought you were avoiding re-creating the static library and emulating it with these options.

Also I'm not 100% convinced that removing the --start-lib/--end-lib, even if we only include those object files the linker decided to select symbols from, would result in the same linking behavior.

Your observations about the linker picking different symbols seem to indicate that the --start-lib/--end-lib model is already broken.

When you say "is already broken" do you mean even in non-ThinLTO mode? I'm not sure why - it is just like having an archive of the objects between each start/end pair.

I'm only talking about ThinLTO and the two-stage linking, i.e. the second invocation of the linker does not end-up with the same prevailing resolution as the first invocation. Your current patches are working around this deficiency.

If a list of .o on the command line is not enough for relinking, there's gonna be a need for a "linker resolution map" file that drives the linker.

In ThinLTO it is because of the change (between the ThinLink and native object link) in which strong references exist between objects/libraries due to importing and inlining. But I believe with this patch and the follow-on D22467 the importing and symbol resolution is made suitably conservative.

I don't see any justification for --start-lib/--end-lib right now.

We use --start-lib/--end-lib internally instead of regular objects. So it is not a matter of justification, it is a matter of keeping that working.

I don't believe this is relevant: the fact that the first link is taking libraries as an input does not make it a compelling case to use them for the second link. Static libraries or start-lib/end-lib are a specific semantic model, and I believe it is just wrong to pass them to the final link.

The reason is that the first link is performing linker resolution: this decision process carry some specific semantic with archives. After this resolution and the ThinLTO process, there is no reason that makes sense to me right now to repeat this process.

But it shouldn't be a correctness issue to do so.

It is possible that it is because I have a different mental model of static archives right now. AFAIK, the semantic difference between plain objects and archive is that an object defined in an archive is loaded and selected by the linker only if one the symbol it defines is referenced.

That is my understanding as well.

Keeping the linker semantic with ThinLTO means that the objects and symbols selected during the first link should be the "source of truth": i.e. we don't want a different linker resolution during the second link. Every objects that was selected for the first link should be included in the second link (hence it is wrong to use --start-lib/--end-lib).

I disagree that it should be wrong from a correctness point to do the final link with the same options.

I'm not convinced this is a better approach. It makes the build system's job more complicated as noted earlier, and requires it for correctness. I think it is preferable to have the correctness managed by the compiler itself where it is doing the importing and linkonce resolution in the first place, using the necessary conservative behavior in this situation.

Also, the distributed build system probably needs to handle the case where an object in the archive was not selected to be part of the link at all, won't be processed by ThinLTO, and there won't be any object to pass to the final link. I'm not sure how you're handling this with gold right now though.

The gold plugin processes all the bitcode files, and gold simply tells it which symbols will be prevailing and which are preempted. So there is always a resulting object file to pass to the final link.

[mehdi_amini]

The reason is that the first link is performing linker resolution: this decision process carry some specific semantic with archives. After this resolution and the ThinLTO process, there is no reason that makes sense to me right now to repeat this process.

But it shouldn't be a correctness issue to do so.

Cf answer to David by email.

Also, the distributed build system probably needs to handle the case where an object in the archive was not selected to be part of the link at all, won't be processed by ThinLTO, and there won't be any object to pass to the final link. I'm not sure how you're handling this with gold right now though.

The gold plugin processes all the bitcode files, and gold simply tells it which symbols will be prevailing and which are preempted. So there is always a resulting object file to pass to the final link.

I don't have gold to verify, but can you confirm what happens with:

A.cpp:
int main() {}
B.cpp:
void foo() {}

when built with:

clang -c -flto -c A.cpp B.cpp
clang -flto A.o --start-lib B.o --end-lib -Wl,-save-temps

(Not sure what is the right options to save the LTO bitcode file).

Will B.o be sent to LTO? Will it be merged with A.o?
Similarly, repeating the process with ThinLTO, will B.o be part of the index? (That'd be a semantic break).

[tejohnson]

In D22356#487953, @mehdi_amini wrote:

The reason is that the first link is performing linker resolution: this decision process carry some specific semantic with archives. After this resolution and the ThinLTO process, there is no reason that makes sense to me right now to repeat this process.

But it shouldn't be a correctness issue to do so.

Cf answer to David by email.

Yes, as responded there that is a compelling example for doing this via the build system. Abandoning this revision while I work on that solution.

Also, the distributed build system probably needs to handle the case where an object in the archive was not selected to be part of the link at all, won't be processed by ThinLTO, and there won't be any object to pass to the final link. I'm not sure how you're handling this with gold right now though.

The gold plugin processes all the bitcode files, and gold simply tells it which symbols will be prevailing and which are preempted. So there is always a resulting object file to pass to the final link.

I don't have gold to verify, but can you confirm what happens with:

A.cpp:
int main() {}
B.cpp:
void foo() {}

when built with:

clang -c -flto -c A.cpp B.cpp
clang -flto A.o --start-lib B.o --end-lib -Wl,-save-temps

(Not sure what is the right options to save the LTO bitcode file).

Will B.o be sent to LTO? Will it be merged with A.o?
Similarly, repeating the process with ThinLTO, will B.o be part of the index? (That'd be a semantic break).

Previously it was including B.o in the index (with the old binutils I have installed I see that behavior). However, eugenis recently fixed this for the LTO side, via a new callback in gold and support in gold-plugin to skip the resulting files (see https://reviews.llvm.org/rL262676). And I can see that with his changes to the plugin this should avoid putting B.o in the index (will update my binutils shortly to confirm).

include/llvm/LTO/LTO.h

 
 
 /// Resolve Weak and LinkOnce values in the \p Index. Linkage changes recorded
 /// in the index and the ThinLTO backends must apply the changes to the Module
 /// via thinLTOResolveWeakForLinkerModule.
 ///
 /// This is done for correctness (if value exported, ensure we always
 /// emit a copy), and compile-time optimization (allow drop of duplicates).
+///
+/// If \p PreserveNonPrevailing is true, then we conservatively prevent
+/// dropping of non-prevailing copies. 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 thinLTOResolveWeakForLinkerInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
-        recordNewLinkage);
+        recordNewLinkage,
+    bool PreserveNonPrevailing = false);
 
 /// Update the linkages in the given \p Index to mark exported values
 /// as external and non-exported values as internal. The ThinLTO backends
 /// must apply the changes to the Module via thinLTOInternalizeModule.
 void thinLTOInternalizeAndPromoteInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(StringRef, GlobalValue::GUID)> isExported);
 }
 
 #endif

lib/LTO/LTO.cpp

 }
 
 static void thinLTOResolveWeakForLinkerGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
     DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
-        recordNewLinkage) {
+        recordNewLinkage,
+    bool PreserveNonPrevailing) {
   for (auto &S : GVSummaryList) {
     if (GlobalInvolvedWithAlias.count(S.get()))
       continue;
     GlobalValue::LinkageTypes OriginalLinkage = S->linkage();
     if (!GlobalValue::isWeakForLinker(OriginalLinkage))
       continue;
     // We need to emit only one of these. The prevailing module will keep it,
     // but turned into a weak, while the others will drop it when possible.
     if (isPrevailing(GUID, S.get())) {
       if (GlobalValue::isLinkOnceLinkage(OriginalLinkage))
         S->setLinkage(GlobalValue::getWeakLinkage(
             GlobalValue::isLinkOnceODRLinkage(OriginalLinkage)));
     }
     // Alias can't be turned into available_externally.
-    else if (!isa<AliasSummary>(S.get()) &&
+    else if (!PreserveNonPrevailing && !isa<AliasSummary>(S.get()) &&
              (GlobalValue::isLinkOnceODRLinkage(OriginalLinkage) ||
               GlobalValue::isWeakODRLinkage(OriginalLinkage)))
       S->setLinkage(GlobalValue::AvailableExternallyLinkage);
     if (S->linkage() != OriginalLinkage)
       recordNewLinkage(S->modulePath(), GUID, S->linkage());
   }
 }
 
 // Resolve Weak and LinkOnce values in the \p Index.
 //
 // We'd like to drop these functions if they are no longer referenced in the
 // current module. However there is a chance that another module is still
 // referencing them because of the import. We make sure we always emit at least
 // one copy.
 void thinLTOResolveWeakForLinkerInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
-        recordNewLinkage) {
+        recordNewLinkage,
+    bool PreserveNonPrevailingODRs) {
   // We won't optimize the globals that are referenced by an alias for now
   // Ideally we should turn the alias into a global and duplicate the definition
   // when needed.
   DenseSet<GlobalValueSummary *> GlobalInvolvedWithAlias;
   for (auto &I : Index)
     for (auto &S : I.second)
       if (auto AS = dyn_cast<AliasSummary>(S.get()))
         GlobalInvolvedWithAlias.insert(&AS->getAliasee());
 
   for (auto &I : Index)
     thinLTOResolveWeakForLinkerGUID(I.second, I.first, GlobalInvolvedWithAlias,
-                                    isPrevailing, recordNewLinkage);
+                                    isPrevailing, recordNewLinkage,
+                                    PreserveNonPrevailingODRs);
 }
 
 static void thinLTOInternalizeAndPromoteGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
     function_ref<bool(StringRef, GlobalValue::GUID)> isExported) {
   for (auto &S : GVSummaryList) {
     if (isExported(S->modulePath(), GUID)) {
       if (GlobalValue::isLocalLinkage(S->linkage()))

test/tools/gold/X86/Inputs/thinlto_preserve_nonprevailodr-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
+}
+
+define linkonce_odr void @x() {
+  ret void
+}
+
+define linkonce_odr void @f() {
+entry:
+  call void @x()
+  call void @x()
+  call void @x()
+  call void @x()
+  call void @x()
+  ret void
+}

test/tools/gold/X86/Inputs/thinlto_preserve_nonprevailodr-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 (...) @foo()
+  ret void
+}
+
+declare void @foo(...)

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

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

test/tools/gold/X86/thinlto_preserve_nonprevailodr.ll

+; This test ensures that linkonce symbols are converted to weak more
+; aggressively 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. The more aggressive
+; conversion to weak 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-summary %s -o %t.o
+; RUN: opt -module-summary %p/Inputs/thinlto_preserve_nonprevailodr-1.ll -o %t2.o
+; RUN: opt -module-summary %p/Inputs/thinlto_preserve_nonprevailodr-2.ll -o %t3.o
+; RUN: opt -module-summary %p/Inputs/thinlto_preserve_nonprevailodr-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 index-based weak/linkonce symbol resolution that
+; we need to preserve (by converting to weak) any linkonce that are exported,
+; even if they are not the prevailing copy (see below for how this affects f()).
+; Also, -import-instr-limit=4 is used to prevent importing of some of
+; the functions, chiefly the linkonce_odr f().
+; 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 first copy of f() was prevailing and should have been made weak_odr
+; as a result
+; RUN: llvm-dis %t2.opt.bc -o - | FileCheck %s --check-prefix=CHECK-OBJ1
+; CHECK-OBJ1: define weak_odr void @f()
+
+; The use of f() should have been inlined but not its definition, due to the
+; import-instr-limit.
+; RUN: llvm-dis %t3.opt.bc -o - | FileCheck %s --check-prefix=CHECK-OBJ2
+; CHECK-OBJ2: declare void @f()
+
+; The second copy of f() should have been made weak_odr since it is exported
+; and with thinlto-index-only we indicate that linkonce_odr should be preserved.
+; RUN: llvm-dis %t4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-OBJ3
+; CHECK-OBJ3: define weak_odr 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,
+; it needs to see the definition in the subsequent %t4.opt.o library or we
+; would get an undefined reference error. 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 f() is defined and weak in final binary.
+; RUN: llvm-nm %t6 | FileCheck %s --check-prefix=CHECK-FINAL
+; CHECK-FINAL: W f
+
+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

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);
 
-  auto isPrevailing = [&](GlobalValue::GUID GUID, const GlobalValueSummary *S) {
-    const auto &Prevailing = PrevailingCopy.find(GUID);
-    assert(Prevailing != PrevailingCopy.end());
-    return Prevailing->second == S;
-  };
-
   // 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
+           // 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.
   thinLTOResolveWeakForLinkerInIndex(
       CombinedIndex, isPrevailing,
       [](StringRef ModuleIdentifier, GlobalValue::GUID GUID,
-         GlobalValue::LinkageTypes NewLinkage) {});
+         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).
   // Changes are made in the index, consumed in the ThinLTO backends.
   thinLTOInternalizeAndPromoteInIndex(CombinedIndex, isExported);
 
   if (options::thinlto_emit_imports_files && !options::thinlto_index_only)
     message(LDPL_WARNING,