Page MenuHomePhabricator

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

Authored by pirama on Nov 5 2018, 2:52 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

pirama created this revision.Nov 5 2018, 2:52 PM
pirama updated this revision to Diff 172674.Nov 5 2018, 4:07 PM

Fix commit message.

tejohnson added inline comments.Nov 6 2018, 6:39 AM
lib/LTO/LTO.cpp
285 ↗(On Diff #172674)

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

294 ↗(On Diff #172674)

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 added inline comments.Nov 6 2018, 9:23 AM
lib/LTO/LTO.cpp
294 ↗(On Diff #172674)

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.

353 ↗(On Diff #172674)

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 ↗(On Diff #172674)

This is the check I wanted to eliminate.

pirama updated this revision to Diff 172852.Nov 6 2018, 2:10 PM

Rename thinLTOResolveWeakForLinker* to thinLTOResolvePrevailing*

pirama marked 4 inline comments as done.Nov 6 2018, 2:13 PM
pirama added inline comments.
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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.

smeenai added a subscriber: smeenai.Nov 6 2018, 6:08 PM
tejohnson added inline comments.Nov 6 2018, 7:00 PM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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 added inline comments.Nov 6 2018, 10:14 PM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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 ↗(On Diff #172852)

The InternalLinkage set in thinLTOInternalizeAndPromoteGUID is read here.

942 ↗(On Diff #172852)

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 added inline comments.Nov 7 2018, 7:04 AM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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 added inline comments.Nov 7 2018, 11:26 AM
lib/LTO/LTO.cpp
353 ↗(On Diff #172674)

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 added inline comments.Nov 7 2018, 1:19 PM
lib/LTO/LTO.cpp
318 ↗(On Diff #172852)

Please update the comment.

353 ↗(On Diff #172674)

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
891 ↗(On Diff #172852)

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

pirama updated this revision to Diff 173123.Nov 8 2018, 12:06 AM

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

tejohnson accepted this revision.Nov 8 2018, 6:25 AM

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

lib/LTO/LTO.cpp
318 ↗(On Diff #172852)

This comment still needs a fix.

lib/LTO/ThinLTOCodeGenerator.cpp
503 ↗(On Diff #173123)

Update comment

678 ↗(On Diff #173123)

Comment needs update.

This revision is now accepted and ready to land.Nov 8 2018, 6:25 AM
pirama updated this revision to Diff 173201.Nov 8 2018, 11:05 AM

Fix a few more comments.

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

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

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.

pirama retitled this revision from [LTO] Drop non-prevailing definitions for non-local linkage types to [LTO] Drop non-prevailing definitions only if linkage is not local or appending.Nov 8 2018, 11:10 AM
pirama edited the summary of this revision. (Show Details)

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.

This revision was automatically updated to reflect the committed changes.
void reopened this revision.Nov 29 2018, 5:32 PM
void added a subscriber: 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?

This revision is now accepted and ready to land.Nov 29 2018, 5:32 PM

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 added a comment.Nov 29 2018, 5:49 PM

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 added a comment.Nov 29 2018, 5:52 PM

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.

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.

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?

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.

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?

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 added a comment.Nov 30 2018, 11:09 AM

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.

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 added a comment.Nov 30 2018, 11:13 AM

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!