[PATCH] Ignore/drop visibility for symbols with local linkage
Duncan P. N. Exon Smith
dexonsmith at apple.com
Wed May 7 16:12:12 PDT 2014
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.
>>>> 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.
>>>> 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.
More information about the llvm-commits