Page MenuHomePhabricator

[ThinLTO] Correctly resolve linkonce when importing aliasee
ClosedPublic

Authored by tejohnson on Oct 28 2016, 8:28 AM.

Details

Summary

When we have an aliasee that is linkonce, while we can't convert
the non-prevailing copies to available_externally, we still need to
convert the prevailing copy to weak. If a reference to the aliasee
is exported, not converting a copy to weak will result in undefined
references when the linkonce is removed in its original module.

Add a new test and update existing tests.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 76196.Oct 28 2016, 8:28 AM
tejohnson retitled this revision from to [ThinLTO] Correctly resolve linkonce when importing aliasee.
tejohnson updated this object.
tejohnson added a reviewer: mehdi_amini.
tejohnson added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.Oct 28 2016, 12:19 PM

CC Rafael to hint here.

When we have an aliasee that is linkonce, while we can't convert the non-prevailing copies to available_externally

It is not clear to me why? I understand that the verifier won't like now, but why isn't possible in theory?

Also even if we wouldn't want to relax this in general, for ThinLTO we can always clone the function into an available_externally that would replace the alias.

mehdi_amini added inline comments.Oct 28 2016, 12:23 PM
lib/LTO/LTO.cpp
123 ↗(On Diff #76196)

It is not clear to me right now why is it part of this stage. Conceptually it seems that you are solving a "promotion" issue, and not a "weak resolution" thing.

CC Rafael to hint here.

When we have an aliasee that is linkonce, while we can't convert the non-prevailing copies to available_externally

It is not clear to me why? I understand that the verifier won't like now, but why isn't possible in theory?

Also even if we wouldn't want to relax this in general, for ThinLTO we can always clone the function into an available_externally that would replace the alias.

Yes we could do this, and we've talked about doing this before (see the comment where we compute the GlobalInvolvedWithAlias set just below in llvm::thinLTOResolveWeakForLinkerInIndex). That is a longer term enhancement to handle this limitation of not being able to convert non-prevailing aliasees to available_externally. But note this patch doesn't affect that case, it is rather dealing with a missing case for the prevailing copy.

lib/LTO/LTO.cpp
123 ↗(On Diff #76196)

The code below here converts the prevailing linkonce to weak to ensure one copy is kept - even if there are no other copies that will be converted to available_externally. This is not new and ensures if we export a reference to that linkonce that it will be satisfied at link time (so that particular part of this routine is in fact like a promotion).

The change in this patch is to allow that part to occur (the correctness part) for an aliasee. It was a missing case, and was resulting in some undefs at link time in certain cases (e.g. the unsats davidxl got when trying to link Clang with ThinLTO + PGO last week).

We could either just better document this part of the handling in this routine, or split it out - e.g a separate pass of promoting linkonce->weak when the linkonce is exported, but that seems orthogonal to fixing the existing handling for this specific case (and somewhat redundant with the linkonce->weak that is done here already).

tejohnson added inline comments.Oct 28 2016, 12:43 PM
lib/LTO/LTO.cpp
123 ↗(On Diff #76196)

Note also this routine used to explicitly check for and handle the exported case, but it was removed with the idea that we would always do the linkonce->weak promotion for the prevailing copy (see r274722 and r274784).

mehdi_amini added inline comments.Oct 28 2016, 12:46 PM
lib/LTO/LTO.cpp
123 ↗(On Diff #76196)

We could either just better document this part of the handling in this routine, or split it out - e.g a separate pass of promoting linkonce->weak when the linkonce is exported,

Right, splitting it seems better to me.

but that seems orthogonal to fixing the existing handling for this specific case (and somewhat redundant with the linkonce->weak that is done here already).

Agree.

tejohnson added inline comments.Oct 29 2016, 2:59 PM
lib/LTO/LTO.cpp
123 ↗(On Diff #76196)

Did you have any more comments on the fix in this patch (which fixes a loophole in the current scheme)?

mehdi_amini accepted this revision.Oct 29 2016, 6:11 PM
mehdi_amini edited edge metadata.

Just add a FIXME explaining that there is a correctness part and a compile-time perf part that should be split, and LGTM.

This revision is now accepted and ready to land.Oct 29 2016, 6:11 PM

Just add a FIXME explaining that there is a correctness part and a compile-time perf part that should be split, and LGTM.

Done.

This revision was automatically updated to reflect the committed changes.