llvm dev - Feb 2018 - ThinLTO and linkonce_odr + unnamed_addr

Rafael, another question for you. IRLinker currently takes min visibility for
the symbol (lib/Linker/LinkModules.cpp:120). Should it take the max visibility?
At least that is what ld64 is doing and is somewhat related to this change
because I want to make sure the behavior is consistent if we mark more stuff as
hidden.

Steven
> On Feb 9, 2018, at 12:04 PM, Steven Wu via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> I agree with your analysis. I will do the GlobalOpts part as well. The
current fix in thinLTO is still good to have for the input that doesn’t have
this optimization and I will argue to keep it.
> 
> Steven
> 
>> On Feb 9, 2018, at 11:49 AM, Rafael Avila de Espindola
<rafael.espindola at gmail.com> wrote:
>> 
>> Steven Wu <stevenwu at apple.com> writes:
>> 
>>> Add Rafael here. Because he basically ask me the same question in
my commit.
>>> 
>>> I wonder why clang choose to do unnamed_addr instead of just do
visibility hidden. If linkonce_odr + unnamed_addr is just hidden, is the only
difference linkonce_odr + unnamed_addr not in the symbol table?
>> 
>> A local symbol being in the symbol table is mostly an implementation
>> detail.
>> 
>> Not marking a linkonce_odr + *global* unnamed_addr hidden is a missing
>> optimization on non-MachO.
>> 
>> Cheers,
>> Rafael
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Steven Wu <stevenwu at apple.com> writes:
> Rafael, another question for you. IRLinker currently takes min visibility
for the symbol (lib/Linker/LinkModules.cpp:120). Should it take the max
visibility? At least that is what ld64 is doing and is somewhat related to this
change because I want to make sure the behavior is consistent if we mark more
stuff as hidden.
No, it should take the min visibility to be consistent with the ELF
spec. The spec says

 If different visibility attributes are specified for distinct
 references to or definitions of a symbol, the most constraining
 visibility attribute must be propagated to the resolving symbol in the
 linked object.

The reason the ELF spec mandates the minimum is that a .o having a
hidden symbol foo means that that file can access foo in a way that is
only valid under the assumption it is local. It not might change the pic
register before doing a call for example.

Cheers,
Rafael

Interesting. Now I understand the full picture. For MachO, linker is picking
based on atoms so it picks the atom with more visibility which doesn’t have the
constraint you mentioned. Let me do some more digging. Worst case I will add a
different rule for MachO but it might be fine.

Steven
> On Feb 9, 2018, at 1:40 PM, Rafael Avila de Espindola <rafael.espindola
at gmail.com> wrote:
> 
> Steven Wu <stevenwu at apple.com> writes:
> 
>> Rafael, another question for you. IRLinker currently takes min
visibility for the symbol (lib/Linker/LinkModules.cpp:120). Should it take the
max visibility? At least that is what ld64 is doing and is somewhat related to
this change because I want to make sure the behavior is consistent if we mark
more stuff as hidden.
> 
> No, it should take the min visibility to be consistent with the ELF
> spec. The spec says
> 
> If different visibility attributes are specified for distinct
> references to or definitions of a symbol, the most constraining
> visibility attribute must be propagated to the resolving symbol in the
> linked object.
> 
> The reason the ELF spec mandates the minimum is that a .o having a
> hidden symbol foo means that that file can access foo in a way that is
> only valid under the assumption it is local. It not might change the pic
> register before doing a call for example.
> 
> Cheers,
> Rafael