Page MenuHomePhabricator

[ThinLTO] Auto-hide prevailing linkonce_odr only when all copies eligible
Needs ReviewPublic

Authored by tejohnson on Mar 22 2019, 11:32 AM.

Details

Reviewers
pcc
Summary

We hit undefined references building with ThinLTO when one source file
contained explicit instantiations of a template method (weak_odr) but
there were also implicit instantiations in another file (linkonce_odr),
and the latter was the prevailing copy. In this case the symbol was
marked hidden when the prevailing linkonce_odr copy was promoted to
weak_odr. It led to unsats when the resulting shared library was linked
with other code that contained a reference (expecting to be resolved due
to the explicit instantiation).

Add a CanAutoHide flag to the GV summary to allow the thin link to
identify when all copies are eligible for auto-hiding (because they were
all originally linkonce_odr global unnamed addr), and only do the
auto-hide in that case.

Most of the changes here are due to plumbing the new flag through the
bitcode and llvm assembly, and resulting test changes. I augmented the
existing auto-hide test to check for this situation.

Diff Detail

Event Timeline

tejohnson created this revision.Mar 22 2019, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 11:32 AM

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

If that is the case, would it be a problem for non-LTO link as well? ld64 always prefers the symbol that is not autohide so I don't think it will run into this situation.

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

If that is the case, would it be a problem for non-LTO link as well? ld64 always prefers the symbol that is not autohide so I don't think it will run into this situation.

pcc@ looked at this and found that ELF semantics picks the most hidden visibility. However, without ThinLTO we aren't marking any of these as hidden, so it isn't an issue.

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

Interesting, I made that change! But I somehow remembered I got pushed back because of ELF semantics and end up implementing that in ld64 instead. Let me dig up some context and get back to you.

If that is the case, would it be a problem for non-LTO link as well? ld64 always prefers the symbol that is not autohide so I don't think it will run into this situation.

pcc@ looked at this and found that ELF semantics picks the most hidden visibility. However, without ThinLTO we aren't marking any of these as hidden, so it isn't an issue.

I see, so if ELF linker doesn't have this autohide optimization, it should be fine in this specific case (but I thought there are some ELF linker actually does this).

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

Interesting, I made that change! But I somehow remembered I got pushed back because of ELF semantics and end up implementing that in ld64 instead. Let me dig up some context and get back to you.

Are you thinking of https://reviews.llvm.org/D43361?

If that is the case, would it be a problem for non-LTO link as well? ld64 always prefers the symbol that is not autohide so I don't think it will run into this situation.

pcc@ looked at this and found that ELF semantics picks the most hidden visibility. However, without ThinLTO we aren't marking any of these as hidden, so it isn't an issue.

I see, so if ELF linker doesn't have this autohide optimization, it should be fine in this specific case (but I thought there are some ELF linker actually does this).

@pcc might have some info here. But I don't think so. We just can't mark hidden without the global visibility of all symbols, as we have here with ThinLTO (because of ELF picking the most hidden one).

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

Interesting, I made that change! But I somehow remembered I got pushed back because of ELF semantics and end up implementing that in ld64 instead. Let me dig up some context and get back to you.

Are you thinking of https://reviews.llvm.org/D43361?

That is exactly what I was thinking.

If that is the case, would it be a problem for non-LTO link as well? ld64 always prefers the symbol that is not autohide so I don't think it will run into this situation.

pcc@ looked at this and found that ELF semantics picks the most hidden visibility. However, without ThinLTO we aren't marking any of these as hidden, so it isn't an issue.

I see, so if ELF linker doesn't have this autohide optimization, it should be fine in this specific case (but I thought there are some ELF linker actually does this).

@pcc might have some info here. But I don't think so. We just can't mark hidden without the global visibility of all symbols, as we have here with ThinLTO (because of ELF picking the most hidden one).

If this is really linker semantics difference, would it be easier if just make the visibility hidden change in D43130 be target specific?

pcc added a comment.Mar 22 2019, 3:49 PM

Does this has the correct behaviour in the case where the explicit instantiation is in a regular object file and the implicit instantiation is in bitcode?

@pcc might have some info here. But I don't think so. We just can't mark hidden without the global visibility of all symbols, as we have here with ThinLTO (because of ELF picking the most hidden one).

ELF linkers don't do auto-hide because it would require extending ELF to store the auto-hide bit (or information from which it can be recovered) somewhere. We have extended ELF with an address-significance table which tells us whether symbols are (local_)unnamed_addr, but we aren't keeping track of whether symbols are linkonce_odr.

If this is really linker semantics difference, would it be easier if just make the visibility hidden change in D43130 be target specific?

That would mean losing the auto hide optimization on ELF. I don't think we should do that.

include/llvm/IR/ModuleSummaryIndex.h
202

canAutoHide? (same elsewhere)

lib/Transforms/IPO/FunctionImport.cpp
984

Can't it also be local_unnamed_addr?

Thanks for explanation. Then the current approach in this patch LGTM. Thanks!

tejohnson marked 2 inline comments as done.Mar 22 2019, 9:10 PM
In D59709#1440303, @pcc wrote:

Does this has the correct behaviour in the case where the explicit instantiation is in a regular object file and the implicit instantiation is in bitcode?

Hmm, no. Nor will it work when the explicit instantiation is in a bitcode file without a summary. I can fix these cases by utilizing the VisibleOutsideSummary flag on the GlobalResolution to conservatively set CanAutoHide=false when that is set.

@pcc might have some info here. But I don't think so. We just can't mark hidden without the global visibility of all symbols, as we have here with ThinLTO (because of ELF picking the most hidden one).

ELF linkers don't do auto-hide because it would require extending ELF to store the auto-hide bit (or information from which it can be recovered) somewhere. We have extended ELF with an address-significance table which tells us whether symbols are (local_)unnamed_addr, but we aren't keeping track of whether symbols are linkonce_odr.

If this is really linker semantics difference, would it be easier if just make the visibility hidden change in D43130 be target specific?

That would mean losing the auto hide optimization on ELF. I don't think we should do that.

include/llvm/IR/ModuleSummaryIndex.h
202

Will do.

lib/Transforms/IPO/FunctionImport.cpp
984

I don't know but I wanted to maintain the status quo on that - we only did this in the global unnamed addr case before. If it can be changed to local_unnamed_addr that should probably be done as a follow up.

In D59709#1440303, @pcc wrote:

Does this has the correct behaviour in the case where the explicit instantiation is in a regular object file and the implicit instantiation is in bitcode?

Hmm, no. Nor will it work when the explicit instantiation is in a bitcode file without a summary. I can fix these cases by utilizing the VisibleOutsideSummary flag on the GlobalResolution to conservatively set CanAutoHide=false when that is set.

The GUIDPreservedSymbols set already has the set of GUIDs that are visible outside the summary, so I just made a change to pass that down and check it as well. Added a case to the test that checks for this situation (one module without summary).

tejohnson updated this revision to Diff 192560.Wed, Mar 27, 9:19 PM

Address comments

tejohnson updated this revision to Diff 192561.Wed, Mar 27, 9:39 PM

Added a gold test so that we can test the native object case too.