[LTO] Drop non-prevailing definitions only if linkage is not local or appending

[pirama]

Summary

This fixes PR 37422

In ELF, non-weak symbols can also be non-prevailing. In this particular
PR, the __llvm_profile_* symbols are non-prevailing but weren't getting
dropped - causing multiply-defined errors with lld.

Also add a test, strong_non_prevailing.ll, to ensure that multiple
copies of a strong symbol are dropped.

To fix the test regressions exposed by this fix,

• do not mark prevailing copies for symbols with 'appending' linkage.

There's no one prevailing copy for such symbols.

• fix the prevailing version in dead-strip-fulllto.ll
• explicitly pass exported symbols to llvm-lto in fumcimport.ll and

funcimport_var.ll

[pirama]

Fix commit message.

[tejohnson]

lib/LTO/LTO.cpp

285

The thinLTOResolveWeakForLinker* routines need a new name with this change. Maybe thinLTOResolvePrevailing*?

294–297

You mentioned in the PR that you get test failures if you guard against appending linkage symbols here. I'd like to understand why. With the old code we would not have resolved appending linkage symbols either.

[pirama]

lib/LTO/LTO.cpp

294–297

Adding the guard here was fine. But, per your suggstion, I replaced the guard with the change to ThinLTOCodeGenerator.cpp:computePrevailingCopies that skips adding prevailing symbols for appending-linkage symbols.

I'll leave a comment at the lines that tripped the tests.

356–359

If I skip appending-linkage summaries here, so I can instead get rid of the AppendingLinkage check in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule (line 914), I get some test failures.

My reasoning was that this line sets linkage to Internal for summaries of appending-linkage symbols, which then gets propagated to the actual symbol in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule. Not setting the linkage here will obviate the need for that check.

I just realized this line is slightly different from my prior test - I must not have synced recently. I'll recreate this scenario and report on the tests that failed.

lib/Transforms/IPO/FunctionImport.cpp

915

This is the check I wanted to eliminate.

[pirama]

Rename thinLTOResolveWeakForLinker* to thinLTOResolvePrevailing*

[pirama]

lib/LTO/LTO.cpp

356–359

On top of the current patch, if I apply the diff below:

diff --git a/lib/LTO/LTO.cpp b/lib/LTO/LTO.cpp
index 2c3c7359dc0..347f1a56a25 100644
--- a/lib/LTO/LTO.cpp
+++ b/lib/LTO/LTO.cpp
@@ -350,7 +350,8 @@ static void thinLTOInternalizeAndPromoteGUID(
       if (GlobalValue::isLocalLinkage(S->linkage()))
         S->setLinkage(GlobalValue::ExternalLinkage);
     } else if (EnableLTOInternalization &&
-               !GlobalValue::isLocalLinkage(S->linkage()))
+               !GlobalValue::isLocalLinkage(S->linkage()) &&
+               !GlobalValue::isAppendingLinkage(S->linkage()))
       S->setLinkage(GlobalValue::InternalLinkage);
   }
 }
diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
index 5816633db97..f7c380c8a09 100644
--- a/lib/Transforms/IPO/FunctionImport.cpp
+++ b/lib/Transforms/IPO/FunctionImport.cpp
@@ -912,7 +912,6 @@ void llvm::thinLTOResolveWeakForLinkerModule(
     }
 
     if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
-        GlobalValue::isAppendingLinkage(GV.getLinkage()) ||
         // In case it was dead and already converted to declaration.
         GV.isDeclaration())
       return;

the following tests fail:

LLVM :: ThinLTO/X86/module_asm2.ll                                                    
LLVM :: tools/gold/X86/comdat.ll                                                      
LLVM :: tools/gold/X86/visibility.ll                                                  
cfi-devirt-lld-thinlto-x86_64 :: cross-dso/stats.cpp                                  
cfi-devirt-thinlto-i386 :: cross-dso/stats.cpp                                        
cfi-devirt-thinlto-newpm-i386 :: cross-dso/stats.cpp                                  
cfi-devirt-thinlto-newpm-x86_64 :: cross-dso/stats.cpp                                
cfi-devirt-thinlto-x86_64 :: cross-dso/stats.cpp                                      
cfi-standalone-lld-thinlto-x86_64 :: cross-dso/stats.cpp                              
cfi-standalone-lld-thinlto-x86_64 :: stats.cpp                                        
cfi-standalone-thinlto-i386 :: cross-dso/stats.cpp                                    
cfi-standalone-thinlto-i386 :: stats.cpp                                              
cfi-standalone-thinlto-newpm-i386 :: cross-dso/stats.cpp                              
cfi-standalone-thinlto-newpm-i386 :: stats.cpp                                        
cfi-standalone-thinlto-newpm-x86_64 :: cross-dso/stats.cpp                            
cfi-standalone-thinlto-newpm-x86_64 :: stats.cpp                                      
cfi-standalone-thinlto-x86_64 :: cross-dso/stats.cpp                                  
cfi-standalone-thinlto-x86_64 :: stats.cpp

If the diff above is preferable over the current patch, I'll look further into why the tests break.

[tejohnson]

lib/LTO/LTO.cpp

356–359

I'd like to understand what is causing the appending linkage variable to be internalized if you don't guard against appending linkage in the thinLTOResolveWeakForLinkerModule. At HEAD, we skip appending linkage in the thinLTOResolveWeakForLinker.* routines since it isn't weak linkage, and don't need to guard against them in the thinLTOInternalizeAndPromote* either.

With your change to computePrevailingCopies, any appending linkage should be prevailing, since they won't be in the map, and thinLTOResolveWeakForLinkerGUID should theoretically not do anything since they aren't LinkOnceLinkage - which should be the same net result as the code at HEAD for those variables. Why is it now being eventually marked internal in the index whereas before it apparently wasn't?

[pirama]

lib/LTO/LTO.cpp

356–359

The reason is that in thinLTOInternalizeAndPromoteGUID, in the lines around the current comment, the Summary is marked as having InternalLinkage. Subsequently, in thinLTOResolveWeakForLinkerGUID , the Summary's linkage is read from the DefinedGlobals parameter. In that function, NewLinkage is InternalLinkage and gets set for the symbol itself.

lib/Transforms/IPO/FunctionImport.cpp

898

The InternalLinkage set in thinLTOInternalizeAndPromoteGUID is read here.

941

If I don't have the guard for AppendingLinkage earlier in this function, control flow reaches this line and the variable's linkage gets set to InternalLinkage.

[tejohnson]

lib/LTO/LTO.cpp

356–359

I see - at HEAD the marking of the InternalLinkage in the summary has no effect on these appending variables since thinLTOInternalizeModule invokes llvm::internalizeModule which in turn special cases these @llvm.* special variables.

My preference then would be to make a change in the internalization handling, rather than work around this with a chance in the weak resolution (now prevailing) handling. So let's try to understand what is causing the failures you noted with the above patch applied. I looked at the first 3 test cases:

LLVM :: ThinLTO/X86/module_asm2.ll

This one has @llvm*.used appending variables. What is the failure mode when you apply your patch?

LLVM :: tools/gold/X86/comdat.ll
LLVM :: tools/gold/X86/visibility.ll

The above two don't have any appending variables, so I'm not sure why they are affected. These 2 tests fail without a recent version of gold with a bugfix - are they passing for you without the above patch applied?

[pirama]

lib/LTO/LTO.cpp

356–359

module_asm2.ll fails the check on %t.0.1. Symbol b, the one added to llvm.used no longer gets exported.

../llvm/test/ThinLTO/X86/module_asm2.ll:37:12: error: NM0-DAG: expected string not found in input
; NM0-DAG: d b
           ^
<stdin>:1:2: note: scanning from here
 U b
 ^

The tools/gold/* failures are due to the unpatched ld.gold. I knew that but forgot to remove it when I made the comment - sorry.

[tejohnson]

lib/LTO/LTO.cpp

321

Please update the comment.

356–359

Thanks for the info. I decided to apply the patch and take a look rather than keep asking for more info. =) It turns out this time it is the new LTO API (via llvm-lto2) that is doing the wrong thing, because as I see now, the linker doesn't know anything about the @llvm.*used appending linkage variables and doesn't resolve them. We could do something like what I had suggested for the old LTO API (i.e. your change to ThinLTOCodeGenerator.cpp), but it would require a little bit more work there because of the way the map is set up.

As a result, I am now thinking it is better just to special case the appending linkage in the thinLTO*InGUID routines (just as we are for local linkage) - sorry for the churn. Along with a comment in both places that the guard is necessary because the linker doesn't resolve locals and appending linkage values.

With that change the appending linkage guard in thinLTOResolveWeakForLinkerModule can be removed, along with the change I had suggested to ThinLTOCodeGenerator.cpp.

Here's my updated patch with the source changes (tests are clean):

diff --git a/include/llvm/LTO/LTO.h b/include/llvm/LTO/LTO.h
index 7d6beab6b44..e9db6276cee 100644

• a/include/llvm/LTO/LTO.h

+++ b/include/llvm/LTO/LTO.h
@@ -46,7 +46,7 @@ class raw_pwrite_stream;
/
/ This is done for correctness (if value exported, ensure we always
/// emit a copy), and compile-time optimization (allow drop of duplicates).
-void thinLTOResolveWeakForLinkerInIndex(
+void thinLTOResolvePrevailingInIndex(

ModuleSummaryIndex &Index,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
    isPrevailing,

diff --git a/lib/LTO/LTO.cpp b/lib/LTO/LTO.cpp
index 2726b6785ed..b60d7a1f813 100644

• a/lib/LTO/LTO.cpp

+++ b/lib/LTO/LTO.cpp
@@ -282,7 +282,7 @@ static void computeCacheKey(

Key = toHex(Hasher.result());

}

-static void thinLTOResolveWeakForLinkerGUID(
+static void thinLTOResolvePrevailingGUID(

GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>

@@ -291,7 +291,10 @@ static void thinLTOResolveWeakForLinkerGUID(

      recordNewLinkage) {
for (auto &S : GVSummaryList) {
  GlobalValue::LinkageTypes OriginalLinkage = S->linkage();

• if (!GlobalValue::isWeakForLinker(OriginalLinkage))

+ Ignore local and appending linkage values since the linker
+ doesn't resolve them.
+ if (GlobalValue::isLocalLinkage(OriginalLinkage) ||
+ GlobalValue::isAppendingLinkage(S->linkage()))

  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.

@@ -321,7 +324,7 @@ static void thinLTOResolveWeakForLinkerGUID(
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 llvm::thinLTOResolveWeakForLinkerInIndex(
+void llvm::thinLTOResolvePrevailingInIndex(

ModuleSummaryIndex &Index,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
    isPrevailing,

@@ -337,7 +340,7 @@ void llvm::thinLTOResolveWeakForLinkerInIndex(

      GlobalInvolvedWithAlias.insert(&AS->getAliasee());
 
for (auto &I : Index)

• thinLTOResolveWeakForLinkerGUID(I.second.SummaryList, I.first,

+ thinLTOResolvePrevailingGUID(I.second.SummaryList, I.first,

GlobalInvolvedWithAlias, isPrevailing,
recordNewLinkage);

}
@@ -350,7 +353,10 @@ static void thinLTOInternalizeAndPromoteGUID(

  if (GlobalValue::isLocalLinkage(S->linkage()))
    S->setLinkage(GlobalValue::ExternalLinkage);
} else if (EnableLTOInternalization &&

• !GlobalValue::isLocalLinkage(S->linkage()))

+ Ignore local and appending linkage values since the linker
+ doesn't resolve them.
+ !GlobalValue::isLocalLinkage(S->linkage()) &&
+ !GlobalValue::isAppendingLinkage(S->linkage()))

    S->setLinkage(GlobalValue::InternalLinkage);
}

}
@@ -1205,7 +1211,7 @@ Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache) {

                            GlobalValue::LinkageTypes NewLinkage) {
  ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
};

• thinLTOResolveWeakForLinkerInIndex(ThinLTO.CombinedIndex, isPrevailing,

+ thinLTOResolvePrevailingInIndex(ThinLTO.CombinedIndex, isPrevailing,

                                   recordNewLinkage);
 
std::unique_ptr<ThinBackendProc> BackendProc =

diff --git a/lib/LTO/ThinLTOCodeGenerator.cpp b/lib/LTO/ThinLTOCodeGenerator.cpp
index 9500b2ded70..e8743a25e6c 100644

• a/lib/LTO/ThinLTOCodeGenerator.cpp

+++ b/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -527,7 +527,7 @@ static void resolveWeakForLinkerInIndex(

  ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
};

• thinLTOResolveWeakForLinkerInIndex(Index, isPrevailing, recordNewLinkage);

+ thinLTOResolvePrevailingInIndex(Index, isPrevailing, recordNewLinkage);
}

// Initialize the TargetMachine builder for a given Triple
diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
index 31531beea5e..b4b16713fe0 100644

• a/lib/Transforms/IPO/FunctionImport.cpp

+++ b/lib/Transforms/IPO/FunctionImport.cpp
@@ -921,7 +921,9 @@ void llvm::thinLTOResolveWeakForLinkerModule(

  return;
}

• if (!GlobalValue::isWeakForLinker(GV.getLinkage()))

+ if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
+ // In case it was dead and already converted to declaration.
+ GV.isDeclaration())

  return;
// Check for a non-prevailing def that has interposable linkage
// (e.g. non-odr weak or linkonce). In that case we can't simply

lib/Transforms/IPO/FunctionImport.cpp

890–891

Please rename consistently with the index-based routines in LTO.cpp (and adjust comment).

[pirama]

Update based on review comments. Hopefully I did the WeakForLinker->Prevailing
name changes in all the right places.

[tejohnson]

LGTM, just a few comments that need to be updated before submit. Thanks!

lib/LTO/LTO.cpp

321

This comment still needs a fix.

lib/LTO/ThinLTOCodeGenerator.cpp

503

Update comment

678

Comment needs update.

[pirama]

Fix a few more comments.

[pirama]

Thanks for the review Teresa. I'm running the tests again after a rebase and will commit this after the tests pass.

[smeenai]

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

[pirama]

In D54125#1291931, @smeenai wrote:

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

I fixed it in the commit message, but Phabricator seems to showing the commit message from the first patch. I'll manually update the summary.

[smeenai]

In D54125#1291935, @pirama wrote:

In D54125#1291931, @smeenai wrote:

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

I fixed it in the commit message, but Phabricator seems to showing the commit message from the first patch. I'll manually update the summary.

Yeah, Phabricator doesn't update the title or summary from the commit (after the initial diff creation) unless you pass --verbatim to --arc-diff. Doing so also restores the reviewers, subscribers, etc. from the original commit message, so it can end up dropping people who added themselves later.

[void]

Hi @piramam,

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

[pirama]

In D54125#1313836, @void wrote:

Hi @piramam,

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

This patch fixed a longstanding break with ThinLTO + PGO + LLD. If the current issue is easy to fix, I'd prefer to fix forward rather than revert this patch. Can you share details on what fails - error message or crash?

But ultimately, this is @tejohnson's call to revert or fix forward.

[void]

In D54125#1313855, @pirama wrote:

In D54125#1313836, @void wrote:

Hi @piramam,

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

This patch fixed a longstanding break with ThinLTO + PGO + LLD. If the current issue is easy to fix, I'd prefer to fix forward rather than revert this patch. Can you share details on what fails - error message or crash?

But ultimately, this is @tejohnson's call to revert or fix forward.

That's fine with me. I have an example that I'll attach here. If it's going to take too long, then maybe revert? But it's up to you and @tejohnson.

[void]

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

softirq.o271 KBDownload

irqdesc.o131 KBDownload

[tejohnson]

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

[tejohnson]

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

[tejohnson]

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

[tejohnson]

In D54125#1314823, @tejohnson wrote:

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

[tejohnson]

In D54125#1314824, @tejohnson wrote:

In D54125#1314823, @tejohnson wrote:

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

Actually - as Peter suggested there, this is in fact fixed by a more recent version of gold. The gold I use with llvm regression tests (GNU gold (GNU Binutils 2.30.51.20180214) 1.16) fixes this issue:

softirq.o: early_irq_init: PREEMPTED_IR
irqdesc.o: early_irq_init: PREVAILING_DEF_REG

$ nm test-object.o | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001a0 W arch_early_irq_init
0000000000000190 W arch_probe_nr_irqs
$

My installed version of gold (1.15) has the bug. So please do use a more recent version of gold to fix this issue.

[void]

In D54125#1314824, @tejohnson wrote:

In D54125#1314823, @tejohnson wrote:

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

I didn't report it as I was told that regular (non-thin) LTO + weak symbols didn't work. I defaulted to ThinLTO, which was working but I suppose the bug was there and just hidden like you mentioned.

[tejohnson]

In D54125#1314993, @void wrote:

In D54125#1314824, @tejohnson wrote:

In D54125#1314823, @tejohnson wrote:

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

I didn't report it as I was told that regular (non-thin) LTO + weak symbols didn't work. I defaulted to ThinLTO, which was working but I suppose the bug was there and just hidden like you mentioned.

It has been fixed in binutils anyway - see my follow on comment, you just need to use a more recent fixed version of gold.

[void]

In D54125#1314825, @tejohnson wrote:

In D54125#1314824, @tejohnson wrote:

In D54125#1314823, @tejohnson wrote:

In D54125#1314702, @tejohnson wrote:

In D54125#1314634, @tejohnson wrote:

In D54125#1313868, @void wrote:

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

Actually - as Peter suggested there, this is in fact fixed by a more recent version of gold. The gold I use with llvm regression tests (GNU gold (GNU Binutils 2.30.51.20180214) 1.16) fixes this issue:

softirq.o: early_irq_init: PREEMPTED_IR
irqdesc.o: early_irq_init: PREVAILING_DEF_REG

$ nm test-object.o | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001a0 W arch_early_irq_init
0000000000000190 W arch_probe_nr_irqs
$

My installed version of gold (1.15) has the bug. So please do use a more recent version of gold to fix this issue.

That's the one I'm using. Let me see if it's possible to move to 1.16 (or maybe even lld). Thanks for your help!

include/llvm/LTO/LTO.h

 class LLVMContext;
 class MemoryBufferRef;
 class Module;
 class Target;
 class raw_pwrite_stream;
 
 /// 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.
+/// via thinLTOResolvePrevailingInModule.
 ///
 /// This is done for correctness (if value exported, ensure we always
 /// emit a copy), and compile-time optimization (allow drop of duplicates).
-void thinLTOResolveWeakForLinkerInIndex(
+void thinLTOResolvePrevailingInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
         recordNewLinkage);
 
 /// Update the linkages in the given \p Index to mark exported values
 /// as external and non-exported values as internal. The ThinLTO backends

include/llvm/Transforms/IPO/FunctionImport.h

void gatherImportedSummariesForModule(
     const FunctionImporter::ImportMapTy &ImportList,
     std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex);
 
 /// Emit into \p OutputFilename the files module \p ModulePath will import from.
 std::error_code EmitImportsFiles(
     StringRef ModulePath, StringRef OutputFilename,
     const std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex);
 
-/// Resolve WeakForLinker values in \p TheModule based on the information
+/// Resolve prevailing symbol linkages in \p TheModule based on the information
 /// recorded in the summaries during global summary-based analysis.
-void thinLTOResolveWeakForLinkerModule(Module &TheModule,
-                                       const GVSummaryMapTy &DefinedGlobals);
+void thinLTOResolvePrevailingInModule(Module &TheModule,
 
 /// Internalize \p TheModule based on the information recorded in the summaries
 /// during global summary-based analysis.
 void thinLTOInternalizeModule(Module &TheModule,
                               const GVSummaryMapTy &DefinedGlobals);
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_IPO_FUNCTIONIMPORT_H

lib/LTO/LTO.cpp

if (FileOrErr) {
           Hasher.update(FileOrErr.get()->getBuffer());
       }
     }
   }
 
   Key = toHex(Hasher.result());
 }
 
-static void thinLTOResolveWeakForLinkerGUID(
+static void thinLTOResolvePrevailingGUID(
     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) {
   for (auto &S : GVSummaryList) {
     GlobalValue::LinkageTypes OriginalLinkage = S->linkage();
-    if (!GlobalValue::isWeakForLinker(OriginalLinkage))
+    // Ignore local and appending linkage values since the linker
+    // doesn't resolve them.
+    if (GlobalValue::isLocalLinkage(OriginalLinkage) ||
+        GlobalValue::isAppendingLinkage(S->linkage()))
       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.
     // This is both a compile-time optimization and a correctness
     // transformation. This is necessary for correctness when we have exported
     // a reference - we need to convert the linkonce to weak to
     // ensure a copy is kept to satisfy the exported reference.
     // FIXME: We may want to split the compile time and correctness
     // aspects into separate routines.
     if (isPrevailing(GUID, S.get())) {
       if (GlobalValue::isLinkOnceLinkage(OriginalLinkage))
         S->setLinkage(GlobalValue::getWeakLinkage(
             GlobalValue::isLinkOnceODRLinkage(OriginalLinkage)));
     }
     // Alias and aliasee can't be turned into available_externally.
     else if (!isa<AliasSummary>(S.get()) &&
              !GlobalInvolvedWithAlias.count(S.get()))
       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 llvm::thinLTOResolveWeakForLinkerInIndex(
+void llvm::thinLTOResolvePrevailingInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
     function_ref<void(StringRef, GlobalValue::GUID, GlobalValue::LinkageTypes)>
         recordNewLinkage) {
   // 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.SummaryList)
       if (auto AS = dyn_cast<AliasSummary>(S.get()))
         GlobalInvolvedWithAlias.insert(&AS->getAliasee());
 
   for (auto &I : Index)
-    thinLTOResolveWeakForLinkerGUID(I.second.SummaryList, I.first,
-                                    GlobalInvolvedWithAlias, isPrevailing,
-                                    recordNewLinkage);
+    thinLTOResolvePrevailingGUID(I.second.SummaryList, I.first,
 }
 
 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()))
         S->setLinkage(GlobalValue::ExternalLinkage);
     } else if (EnableLTOInternalization &&
-               !GlobalValue::isLocalLinkage(S->linkage()))
+               // Ignore local and appending linkage values since the linker
+               // doesn't resolve them.
+               !GlobalValue::isLocalLinkage(S->linkage()) &&
+               !GlobalValue::isAppendingLinkage(S->linkage()))
       S->setLinkage(GlobalValue::InternalLinkage);
   }
 }
 
 // Update the linkages in the given \p Index to mark exported values
 // as external and non-exported values as internal.
 void llvm::thinLTOInternalizeAndPromoteInIndex(
     ModuleSummaryIndex &Index,
auto isPrevailing = [&](GlobalValue::GUID GUID,
                           const GlobalValueSummary *S) {
     return ThinLTO.PrevailingModuleForGUID[GUID] == S->modulePath();
   };
   auto recordNewLinkage = [&](StringRef ModuleIdentifier,
                               GlobalValue::GUID GUID,
                               GlobalValue::LinkageTypes NewLinkage) {
     ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
   };
-  thinLTOResolveWeakForLinkerInIndex(ThinLTO.CombinedIndex, isPrevailing,
-                                     recordNewLinkage);
+  thinLTOResolvePrevailingInIndex(ThinLTO.CombinedIndex, isPrevailing,
 
   std::unique_ptr<ThinBackendProc> BackendProc =
       ThinLTO.Backend(Conf, ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
                       AddStream, Cache);
 
   // Tasks 0 through ParallelCodeGenParallelismLevel-1 are reserved for combined
   // module and parallel code generation partitions.
   unsigned Task = RegularLTO.ParallelCodeGenParallelismLevel;

lib/LTO/LTOBackend.cpp

Error lto::thinBackend(Config &Conf, unsigned Task, AddStreamFn AddStream,
 
   if (Conf.PreOptModuleHook && !Conf.PreOptModuleHook(Task, Mod))
     return finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
 
   renameModuleForThinLTO(Mod, CombinedIndex);
 
   dropDeadSymbols(Mod, DefinedGlobals, CombinedIndex);
 
-  thinLTOResolveWeakForLinkerModule(Mod, DefinedGlobals);
+  thinLTOResolvePrevailingInModule(Mod, DefinedGlobals);
 
   if (Conf.PostPromoteModuleHook && !Conf.PostPromoteModuleHook(Task, Mod))
     return finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
 
   if (!DefinedGlobals.empty())
     thinLTOInternalizeModule(Mod, DefinedGlobals);
 
   if (Conf.PostInternalizeModuleHook &&

lib/LTO/ThinLTOCodeGenerator.cpp

ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
                      bool Freestanding, unsigned OptLevel, unsigned count) {
 
   // "Benchmark"-like optimization: single-source case
   bool SingleModule = (ModuleMap.size() == 1);
 
   if (!SingleModule) {
     promoteModule(TheModule, Index);
 
-    // Apply summary-based LinkOnce/Weak resolution decisions.
-    thinLTOResolveWeakForLinkerModule(TheModule, DefinedGlobals);
+    // Apply summary-based prevailing-symbol resolution decisions.
+    thinLTOResolvePrevailingInModule(TheModule, DefinedGlobals);
 
     // Save temps: after promotion.
     saveTempBitcode(TheModule, SaveTempsDir, count, ".1.promoted.bc");
   }
 
   // Be friendly and don't nuke totally the module when the client didn't
   // supply anything to preserve.
   if (!ExportList.empty() || !GUIDPreservedSymbols.empty()) {
SmallVector<char, 128> OutputBuffer;
       WriteBitcodeToFile(TheModule, OS, true, &Index);
     }
     return make_unique<SmallVectorMemoryBuffer>(std::move(OutputBuffer));
   }
 
   return codegenModule(TheModule, TM);
 }
 
 /// Resolve LinkOnce/Weak symbols. Record resolutions in the \p ResolvedODR map
 /// for caching, and in the \p Index for application during the ThinLTO
 /// backends. This is needed for correctness for exported symbols (ensure
 /// at least one copy kept) and a compile-time optimization (to drop duplicate
 /// copies when possible).
-static void resolveWeakForLinkerInIndex(
+static void resolvePrevailingInIndex(
     ModuleSummaryIndex &Index,
     StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>>
         &ResolvedODR) {
 
   DenseMap<GlobalValue::GUID, const GlobalValueSummary *> PrevailingCopy;
   computePrevailingCopies(Index, PrevailingCopy);
 
   auto isPrevailing = [&](GlobalValue::GUID GUID, const GlobalValueSummary *S) {
     const auto &Prevailing = PrevailingCopy.find(GUID);
     // Not in map means that there was only one copy, which must be prevailing.
     if (Prevailing == PrevailingCopy.end())
       return true;
     return Prevailing->second == S;
   };
 
   auto recordNewLinkage = [&](StringRef ModuleIdentifier,
                               GlobalValue::GUID GUID,
                               GlobalValue::LinkageTypes NewLinkage) {
     ResolvedODR[ModuleIdentifier][GUID] = NewLinkage;
   };
 
-  thinLTOResolveWeakForLinkerInIndex(Index, isPrevailing, recordNewLinkage);
+  thinLTOResolvePrevailingInIndex(Index, isPrevailing, recordNewLinkage);
 }
 
 // Initialize the TargetMachine builder for a given Triple
 static void initTMBuilder(TargetMachineBuilder &TMBuilder,
                           const Triple &TheTriple) {
   // Set a default CPU for Darwin triples (copied from LTOCodeGenerator).
   // FIXME this looks pretty terrible...
   if (TMBuilder.MCpu.empty() && TheTriple.isOSDarwin()) {
void ThinLTOCodeGenerator::promote(Module &TheModule,
   computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
 
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists);
 
   // Resolve LinkOnce/Weak symbols.
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
-  resolveWeakForLinkerInIndex(Index, ResolvedODR);
+  resolvePrevailingInIndex(Index, ResolvedODR);
 
-  thinLTOResolveWeakForLinkerModule(
+  thinLTOResolvePrevailingInModule(
       TheModule, ModuleToDefinedGVSummaries[ModuleIdentifier]);
 
   // Promote the exported values in the index, so that they are promoted
   // in the module.
   internalizeAndPromoteInIndex(ExportLists, GUIDPreservedSymbols, Index);
 
   promoteModule(TheModule, Index);
 }
ComputeCrossModuleImport(*Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists);
 
   // We use a std::map here to be able to have a defined ordering when
   // producing a hash for the cache entry.
   // FIXME: we should be able to compute the caching hash for the entry based
   // on the index, and nuke this map.
   StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
 
-  // Resolve LinkOnce/Weak symbols, this has to be computed early because it
+  // Resolve prevailing symbols, this has to be computed early because it
   // impacts the caching.
-  resolveWeakForLinkerInIndex(*Index, ResolvedODR);
+  resolvePrevailingInIndex(*Index, ResolvedODR);
 
   // 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.
   internalizeAndPromoteInIndex(ExportLists, GUIDPreservedSymbols, *Index);
 
   // Make sure that every module has an entry in the ExportLists, ImportList,
   // GVSummary and ResolvedODR maps to enable threaded access to these maps

lib/Transforms/IPO/FunctionImport.cpp

else
                              GV.getType()->getAddressSpace());
     NewGV->takeName(&GV);
     GV.replaceAllUsesWith(NewGV);
     return false;
   }
   return true;
 }
 
-/// Fixup WeakForLinker linkages in \p TheModule based on summary analysis.
-void llvm::thinLTOResolveWeakForLinkerModule(
+/// Fixup prevailing symbol linkages in \p TheModule based on summary analysis.
+void llvm::thinLTOResolvePrevailingInModule(
     Module &TheModule, const GVSummaryMapTy &DefinedGlobals) {
   auto updateLinkage = [&](GlobalValue &GV) {
     // See if the global summary analysis computed a new resolved linkage.
     const auto &GS = DefinedGlobals.find(GV.getGUID());
     if (GS == DefinedGlobals.end())
       return;
     auto NewLinkage = GS->second->linkage();
     if (NewLinkage == GV.getLinkage())
       return;
 
     // Switch the linkage to weakany if asked for, e.g. we do this for
     // linker redefined symbols (via --wrap or --defsym).
     // We record that the visibility should be changed here in `addThinLTO`
     // as we need access to the resolution vectors for each input file in
     // order to find which symbols have been redefined.
     // We may consider reorganizing this code and moving the linkage recording
-    // somewhere else, e.g. in thinLTOResolveWeakForLinkerInIndex.
+    // somewhere else, e.g. in thinLTOResolvePrevailingInIndex.
     if (NewLinkage == GlobalValue::WeakAnyLinkage) {
       GV.setLinkage(NewLinkage);
       return;
     }
 
-    if (!GlobalValue::isWeakForLinker(GV.getLinkage()))
+    if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
+        // In case it was dead and already converted to declaration.
+        GV.isDeclaration())
       return;
     // Check for a non-prevailing def that has interposable linkage
     // (e.g. non-odr weak or linkonce). In that case we can't simply
     // convert to available_externally, since it would lose the
     // interposable property and possibly get inlined. Simply drop
     // the definition in that case.
     if (GlobalValue::isAvailableExternallyLinkage(NewLinkage) &&
         GlobalValue::isInterposableLinkage(GV.getLinkage())) {
       if (!convertToDeclaration(GV))
         // FIXME: Change this to collect replaced GVs and later erase
-        // them from the parent module once thinLTOResolveWeakForLinkerGUID is
+        // them from the parent module once thinLTOResolvePrevailingGUID is
         // changed to enable this for aliases.
         llvm_unreachable("Expected GV to be converted");
     } else {
       // If the original symbols has global unnamed addr and linkonce_odr linkage,
       // it should be an auto hide symbol. Add hidden visibility to the symbol to
       // preserve the property.
       if (GV.hasLinkOnceODRLinkage() && GV.hasGlobalUnnamedAddr() &&
           NewLinkage == GlobalValue::WeakODRLinkage)
         GV.setVisibility(GlobalValue::HiddenVisibility);
 
       LLVM_DEBUG(dbgs() << "ODR fixing up linkage for `" << GV.getName()
                         << "` from " << GV.getLinkage() << " to " << NewLinkage
                         << "\n");
       GV.setLinkage(NewLinkage);
     }
     // Remove declarations from comdats, including available_externally
     // as this is a declaration for the linker, and will be dropped eventually.
     // It is illegal for comdats to contain declarations.
     auto *GO = dyn_cast_or_null<GlobalObject>(&GV);
     if (GO && GO->isDeclarationForLinker() && GO->hasComdat())
       GO->setComdat(nullptr);
   };

test/LTO/Resolution/X86/dead-strip-fulllto.ll

 ; RUN: opt -module-summary -o %t %s
 ; RUN: opt -module-summary -o %t2 %S/Inputs/dead-strip-fulllto.ll
 
-; RUN: llvm-lto2 run %t -r %t,main,px -r %t,live1,p -r %t,live2,p -r %t,dead2,p \
-; RUN:               %t2 -r %t2,live1, -r %t2,live2, -r %t2,dead1,p -r %t2,dead2, -r %t2,odr, \
+; RUN: llvm-lto2 run %t -r %t,main,px -r %t,live1, -r %t,live2,p -r %t,dead2,p \
+; RUN:               %t2 -r %t2,live1,p -r %t2,live2, -r %t2,dead1,p -r %t2,dead2, -r %t2,odr, \
 ; RUN: -save-temps -o %t3
 ; RUN: llvm-nm %t3.0 | FileCheck --check-prefix=FULL %s
 ; RUN: llvm-nm %t3.1 | FileCheck --check-prefix=THIN %s
 
-; RUN: llvm-lto2 run %t -r %t,main,px -r %t,live1,p -r %t,live2,p -r %t,dead2,p \
-; RUN:               %t2 -r %t2,live1, -r %t2,live2, -r %t2,dead1,p -r %t2,dead2, -r %t2,odr, \
+; RUN: llvm-lto2 run %t -r %t,main,px -r %t,live1, -r %t,live2,p -r %t,dead2,p \
+; RUN:               %t2 -r %t2,live1,p -r %t2,live2, -r %t2,dead1,p -r %t2,dead2, -r %t2,odr, \
 ; RUN: -save-temps -o %t3 -O0
 ; RUN: llvm-nm %t3.0 | FileCheck --check-prefix=FULL %s
 ; RUN: llvm-nm %t3.1 | FileCheck --check-prefix=THIN %s
 
 ; FULL-NOT: dead
 ; FULL: U live1
 ; FULL: T live2
 ; FULL: T main

test/ThinLTO/X86/Inputs/strong_non_prevailing.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__llvm_profile_filename = comdat any
+
+@__llvm_profile_filename = constant [19 x i8] c"default_%m.profraw\00", comdat

test/ThinLTO/X86/funcimport.ll

 ; RUN: llvm-lto -thinlto-action=optimize %t2.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=OPTIMIZED
 ; OPTIMIZED: define i32 @main()
 
 ; Verify that the codegen run
 ; RUN: llvm-lto -thinlto-action=codegen %t2.bc -o - | llvm-nm -o - | FileCheck %s --check-prefix=CODEGEN
 ; CODEGEN: T _main
 
 ; Verify that all run together
-; RUN: llvm-lto -thinlto-action=run %t2.bc  %t.bc
+; RUN: llvm-lto -thinlto-action=run %t2.bc  %t.bc  -exported-symbol=_main
 ; RUN: llvm-nm -o - < %t.bc.thinlto.o | FileCheck %s --check-prefix=ALL
 ; RUN: llvm-nm -o - < %t2.bc.thinlto.o | FileCheck %s --check-prefix=ALL2
 ; ALL: T _callfuncptr
 ; ALL2: T _main
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 

test/ThinLTO/X86/strong_non_prevailing.ll

+; RUN: opt -module-summary %s -o %t.bc
+; RUN: opt -module-summary %p/Inputs/strong_non_prevailing.ll -o %t2.bc
+
+; RUN: llvm-lto -thinlto-action=run %t.bc %t2.bc -exported-symbol=__llvm_profile_filename
+; RUN: llvm-nm -o - < %t.bc.thinlto.o | FileCheck %s --check-prefix=EXPORTED
+; RUN: llvm-nm -o - < %t2.bc.thinlto.o 2>&1 | FileCheck %s --check-prefix=NOT_EXPORTED
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__llvm_profile_filename = comdat any
+
+@__llvm_profile_filename = constant [19 x i8] c"default_%m.profraw\00", comdat
+
+; EXPORTED: N __llvm_profile_filename
+; NOT_EXPORTED-NOT: N __llvm_profile_filename

test/Transforms/FunctionImport/funcimport_var.ll

 ; This test makes sure a static var is not selected as a callee target
 ; (which will crash compilation).
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/funcimport_var2.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -thinlto-action=thinlink -o %t3 %t.bc %t2.bc
 ; RUN: llvm-lto -thinlto -thinlto-action=import -thinlto-index=%t3 %t.bc %t2.bc
-; RUN: llvm-lto -thinlto -thinlto-action=run %t.bc %t2.bc
+; RUN: llvm-lto -thinlto -thinlto-action=run %t.bc %t2.bc -exported-symbol=_Z4LinkPKcS0_
 ; RUN: llvm-nm %t.bc.thinlto.o | FileCheck %s
 ; RUN: llvm-lto2 run %t.bc %t2.bc -o %t.out \
 ; RUN:   -r %t.bc,_Z4LinkPKcS0_,plx \
 ; RUN:   -r %t.bc,link,l \
 ; RUN:   -r %t2.bc,get_link,plx
 ; RUN: llvm-nm %t.out.1 | FileCheck %s
 ; CHECK: U link