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

[Rafael Espíndola]

>> 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.

LGTM, but there is one more "internal hidden"  in
CodeGenCXX/cxx11-thread-local.cpp you need to update now.


>>> 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?

Good point.  It is not clear if a DEBUG dump would be worth it, since
by the end of the patch series we will have better asserting. Up to
you. Patch LGTM.

>>> 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.

Something went wrong when attaching this one:

$ file  ~/Downloads/0005-LTO-Assert-visibility-of-local-linkage-when-mergin-2.patch
/Users/espindola/Downloads/0005-LTO-Assert-visibility-of-local-linkage-when-mergin-2.patch:
AppleSingle encoded Macintosh file


>>> 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"?

Avoiding the asserts is probably good enough if they would fire in an
existing test case. Just note that in the commit.

>>> 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.

No, it is fine. I should be a fairly easy change.

Cheers,
Rafael