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

[Rafael Espíndola]

On 1 May 2014 17:01, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> When symbols have local linkage (i.e., `internal` or `private`), a
> visibility of `hidden` or `protected` doesn't make much sense.
>
> These patches change LLVM and clang to ignore and drop non-default
> visibility for such symbols.

To summarize a discussion we had on IRC: ELF can represent this as a
side effect that visibility is an independent field in the binary
(just like llvm IR). The advantage of disallowing the combination in
IR is that it removes a foot gun where code can easily handle a
internal hidden symbol as if it was just a global hidden.

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

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

> 0003 fixes -internalize to drop visibility.

Please also test all cases (global variables, aliases and functions).
LGTM with that.

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


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

This should be an assert I think. If the symbol is internal, we renamed it.

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


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?

> 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 :-(

Cheers,
Rafael