Page MenuHomePhabricator

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

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

Details

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

Repository
rL LLVM

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

canAutoHide? (same elsewhere)

lib/Transforms/IPO/FunctionImport.cpp
984 ↗(On Diff #191914)

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

Will do.

lib/Transforms/IPO/FunctionImport.cpp
984 ↗(On Diff #191914)

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.Mar 27 2019, 9:19 PM

Address comments

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

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

ping - @pcc any more comments or can this fix go in now?

Ping - ok if I submit this bugfix?

pcc added a comment.May 10 2019, 9:56 AM

I think there might still be a problem where we're dropping the distinction between visible to other translation units and visible outside the linkage unit. This is related to the distinction between LDPR_PREVAILING_DEF_IRONLY and LDPR_PREVAILING_DEF_IRONLY_EXP in the gold plugin interface. Let me look into it a bit more and get back to you.

pcc accepted this revision.May 10 2019, 10:33 AM

LGTM

Sorry for the long delay in reviewing this.

In D59709#1498204, @pcc wrote:

I think there might still be a problem where we're dropping the distinction between visible to other translation units and visible outside the linkage unit. This is related to the distinction between LDPR_PREVAILING_DEF_IRONLY and LDPR_PREVAILING_DEF_IRONLY_EXP in the gold plugin interface. Let me look into it a bit more and get back to you.

I don't think this is a problem. After having reminded myself about how things work, I realise that the linkers already keep track of this distinction and reflect it in the VisibleToRegularObj field.

lib/Transforms/IPO/FunctionImport.cpp
984 ↗(On Diff #191914)

Okay, I missed that you were checking for global unnamed_addr when setting the flag to begin with.

This revision is now accepted and ready to land.May 10 2019, 10:33 AM
This revision was automatically updated to reflect the committed changes.