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

[Duncan P. N. Exon Smith]

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

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

Heh, thanks for pointing that out.  r208258.

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

...

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

I left it out.  r208261.

>>>> 0005 clarifies `ModuleLinker::getLinkageResult()` with no
>>>> functionality change.

...

> The stronger version of the assert LGTM.

r208262.

>>>> 0006 updates the assembler and bitcode readers to reject and auto-
>>>> upgrade (respectively) non-default visibility on symbols with local
>>>> linkage, and updates LangRef.

...

>>> This LGTM, but needs to wait for the other patches.

r208263.

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

r208264.