[PATCH] Ignore/drop visibility for symbols with local linkage

[Duncan P. N. Exon Smith]

Thanks for the review!

Comments inline, and new patches attached.

On 2014-May-05, at 07:17, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

> On 1 May 2014 17:01, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 0001 is a code cleanup before 0002, which updates clang to stop
>> generating `internal hidden`.  I checked every call to
>> `setVisibility()` -- the rest were already doing the right thing.
> 
> Cleanup LGTM.

r207978

> Is patch 2 missing a testcase? There are two code changes and one test.

This was implicitly tested by the asserts added in 0007, but I've
nevertheless updated the patch with explicit tests.

>> 0003 fixes -internalize to drop visibility.
> 
> Please also test all cases (global variables, aliases and functions).
> LGTM with that.

r207979.

>> 0004 fixes LTOModule.cpp to prefer internal over hidden and to give
>> symbols with appending linkage "default" scope.  I don't have a
>> testcase for this.  (How do I test the output of LTOModule.cpp?)
> 
> You should be able to test it with llvm-lto. It is a simple "test
> linker" that uses lib/LTO.

It's not clear to me how to do this.  The only functionality change
is what the linker sees from its accessors (what's the scope of this
symbol?) when LTO is advertising its symbols.  I.e., the resulting
binary dumped from LTO is unchanged.  The only place llvm-lto uses
this API is at line 132 of llvm-lto.cpp:

    lto_symbol_attributes Attrs = Module->getSymbolAttributes(I);
    unsigned Scope = Attrs & LTO_SYMBOL_SCOPE_MASK;
    if (Scope != LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN)
      KeptDSOSyms.push_back(Name);

I don't think my change will affect this logic, so it's currently
unobservable in llvm-lto.

Or are you saying I should add dumping capabilities to llvm-lto so
that I can see what `Module->getSymbolAttributes()` returns?

>> 0005 clarifies `ModuleLinker::getLinkageResult()` with no
>> functionality change.
> 
> This should be an assert I think. If the symbol is internal, we renamed it.

Good call; the updated patch changes it that way.

>> 0006 updates the assembler and bitcode readers to reject and auto-
>> upgrade (respectively) non-default visibility on symbols with local
>> linkage, and updates LangRef.
> 
> +    return Error(LinkageLoc, "local linkage must have default visibility");
> 
> Symbol with local linkage...

Updated the error messages to include "symbol with".

> This LGTM, but needs to wait for the other patches. You can check in
> the test changes that just replace "hidden internal" with "internal"
> if you want.
> 
>> 0007 adds asserts to `setLinkage()` and `setVisibility()`, and
>> updates `makeVisible()` in ExtractGV.cpp, which was setting them out
>> of order.
> 
> Can you test the change in ExtractGV.cpp?

The change in ExtractGV.cpp has no functionality change, except that
it avoids the newly added asserts.  There are checked in tests that
already cover this.  Should I separate that change to a prior
patch/commit that specifically says "no functionality change"?

>> My checkout of llvm+clang passes make check-all.  Are there other
>> projects I should check before committing 0007?
>> 
> 
> We will need to update dragonegg, but I can look into it once my
> previous dragonegg patch gets reviewed :-(

Boo.  Let me know if you want me to hold off committing 0007, which
adds the asserts.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-CodeGen-Don-t-set-hidden-visibility-on-symbols-wit-2.patch
Type: application/octet-stream
Size: 5591 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1418f160/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-LTO-Check-local-linkage-first-2.patch
Type: application/octet-stream
Size: 2153 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1418f160/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-LTO-Assert-visibility-of-local-linkage-when-mergin-2.patch
Type: application/applefile
Size: 123 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1418f160/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-IR-Don-t-allow-non-default-visibility-on-local-lin-2.patch
Type: application/octet-stream
Size: 23603 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1418f160/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-GlobalValue-Assert-symbols-with-local-linkage-have-2.patch
Type: application/octet-stream
Size: 2206 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1418f160/attachment-0003.obj>