This is an archive of the discontinued LLVM Phabricator instance.

ThinLTO: Remove check for multiple modules before applying weak resolutions.
ClosedPublic

Authored by pcc on Jun 30 2016, 6:23 PM.

Details

Summary

This check is not only unnecessary, it can produce the wrong result. If we
are linking a single module and it has an exported linkonce symbol, we need
to promote to weak in order to avoid PR19901-style problems.

Diff Detail

Event Timeline

pcc updated this revision to Diff 62453.Jun 30 2016, 6:23 PM
pcc retitled this revision from to ThinLTO: Remove check for multiple modules before applying weak resolutions..
pcc updated this object.
pcc added reviewers: tejohnson, mehdi_amini.
pcc added a subscriber: llvm-commits.
tejohnson edited edge metadata.Jun 30 2016, 8:03 PM

Is this possible outside of this kind of test case? If there is an exported symbol, then presumably we are linking with another bitcode module (the importing module). In the case of PR19901 the overridden copy was in a native object, but here the importing module must also be bitcode (otherwise we can't actually import the symbol reference and no issue would arise).

mehdi_amini requested changes to this revision.Jul 1 2016, 6:07 AM
mehdi_amini edited edge metadata.

libLTO is handling this differently by adding to llvm.compiler.used, for the same reason as mentioned in another review: linkonce -> weak is (was?) pessimizing codegen on MachO.

This revision now requires changes to proceed.Jul 1 2016, 6:07 AM
pcc added a comment.Jul 1 2016, 11:15 AM

If there is an exported symbol, then presumably we are linking with another bitcode module (the importing module).

I mean exported in the sense of "not internalized". This applies in the case where the symbol is used by the native module, in which case we need to make sure we export it here.

libLTO is handling this differently by adding to llvm.compiler.used, for the same reason as mentioned in another review: linkonce -> weak is (was?) pessimizing codegen on MachO.

Okay, but I'm sure you don't just want this to happen in the case where there is a single ThinLTO module. As the test I am adding in D21915 shows, we are already upgrading to weak.

In D21917#472497, @pcc wrote:

If there is an exported symbol, then presumably we are linking with another bitcode module (the importing module).

I mean exported in the sense of "not internalized". This applies in the case where the symbol is used by the native module, in which case we need to make sure we export it here.

Ah, ok. I was confused by the different uses of the term "exported" in ThinLTO mode. =(

libLTO is handling this differently by adding to llvm.compiler.used, for the same reason as mentioned in another review: linkonce -> weak is (was?) pessimizing codegen on MachO.

Okay, but I'm sure you don't just want this to happen in the case where there is a single ThinLTO module. As the test I am adding in D21915 shows, we are already upgrading to weak.

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

pcc added a comment.Jul 6 2016, 3:36 PM

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

To be more clear, I mean that we're already upgrading to weak in the multiple module case. As far as I'm concerned, that's the most important case here, as presumably if someone is using LTO their program has multiple modules. So I think there's no great harm in upgrading to weak in the single module case.

Probably the most explicit instance of this is linkoncefunc in weak_resolution, which I've now added a promote+internalize test case for.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

That doesn't seem to be necessary. We just need a way to express "auto-hide + keep". There's already a way to do that which is compatible with Mach-O linkers, which is to use linkonce_odr + local_unnamed_addr + llvm.compiler.used. ELF linkers don't care about this (at least unless/until we extend ELF to have an auto-hide bit like Mach-O), so we can just do the same thing there. But that's outside of the scope of what I'm doing here.

pcc updated this revision to Diff 62991.Jul 6 2016, 4:18 PM
pcc edited edge metadata.
  • Refresh
mehdi_amini accepted this revision.Jul 6 2016, 5:56 PM
mehdi_amini edited edge metadata.
In D21917#476048, @pcc wrote:

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

To be more clear, I mean that we're already upgrading to weak in the multiple module case. As far as I'm concerned, that's the most important case here, as presumably if someone is using LTO their program has multiple modules. So I think there's no great harm in upgrading to weak in the single module case.

Probably the most explicit instance of this is linkoncefunc in weak_resolution, which I've now added a promote+internalize test case for.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

That doesn't seem to be necessary.

Agreed.

We just need a way to express "auto-hide + keep". There's already a way to do that which is compatible with Mach-O linkers, which is to use linkonce_odr + local_unnamed_addr + llvm.compiler.used. ELF linkers don't care about this (at least unless/until we extend ELF to have an auto-hide bit like Mach-O), so we can just do the same thing there. But that's outside of the scope of what I'm doing here.

Agreed as well.

Did you already implemented some pre-LTO hiding based on local_unnamed_addr?

This revision is now accepted and ready to land.Jul 6 2016, 5:56 PM
pcc added a comment.Jul 6 2016, 6:46 PM

Did you already implemented some pre-LTO hiding based on local_unnamed_addr?

Yes, lld is already using canBeOmittedFromSymbolTable for that purpose.

This revision was automatically updated to reflect the committed changes.
In D21917#476048, @pcc wrote:

I don't see this in the test cases in D21915 - one of them (alias_import) has multiple modules, and the other (weak_resolution) we are internalizing in the new case added.

To be more clear, I mean that we're already upgrading to weak in the multiple module case. As far as I'm concerned, that's the most important case here, as presumably if someone is using LTO their program has multiple modules. So I think there's no great harm in upgrading to weak in the single module case.

Probably the most explicit instance of this is linkoncefunc in weak_resolution, which I've now added a promote+internalize test case for.

Maybe we need to add a flag to control this behavior, since it seems to need to be different for the different linkers.

That doesn't seem to be necessary.

Agreed.

We just need a way to express "auto-hide + keep". There's already a way to do that which is compatible with Mach-O linkers, which is to use linkonce_odr + local_unnamed_addr + llvm.compiler.used. ELF linkers don't care about this (at least unless/until we extend ELF to have an auto-hide bit like Mach-O), so we can just do the same thing there. But that's outside of the scope of what I'm doing here.

Agreed as well.

Ok SGTM, I didn't realized the Mach-O case was addressed.

In D21917#476048, @pcc wrote:

But that's outside of the scope of what I'm doing here.

Agreed as well.

Ok SGTM, I didn't realized the Mach-O case was addressed.

It is not addressed, but I'll address it more generally at a later point.