[PATCH] Ignore/drop visibility for symbols with local linkage
rafael.espindola at gmail.com
Wed May 7 05:31:33 PDT 2014
>> 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)
> 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
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.
More information about the llvm-commits