This is an archive of the discontinued LLVM Phabricator instance.

CMake: Make most target symbols hidden by default
ClosedPublic

Authored by tstellar on Nov 12 2018, 12:07 PM.

Details

Summary

For builds with LLVM_BUILD_LLVM_DYLIB=ON and BUILD_SHARED_LIBS=OFF
this change makes all symbols in the target specific libraries hidden
by default.

A new macro called LLVM_EXTERNAL_VISIBILITY has been added to mark symbols in these
libraries public, which is mainly needed for the definitions of the
LLVMInitialize* functions.

This patch reduces the number of public symbols in libLLVM.so by about
25%. This should improve load times for the dynamic library and also
make abi checker tools, like abidiff require less memory when analyzing
libLLVM.so

One side-effect of this change is that for builds with
LLVM_BUILD_LLVM_DYLIB=ON and LLVM_LINK_LLVM_DYLIB=ON some unittests that
access symbols that are no longer public will need to be statically linked.

Before and after public symbol counts (using gcc 8.2.1, ld.bfd 2.31.1):
nm before/libLLVM-9svn.so | grep ' [A-Zuvw] ' | wc -l
36221
nm after/libLLVM-9svn.so | grep ' [A-Zuvw] ' | wc -l
26278

Diff Detail

Event Timeline

tstellar created this revision.Nov 12 2018, 12:07 PM

"LLVM_EXPORT" seems like an overly generic name. Maybe a variation on "LLVM_LIBRARY_VISIBILITY" would be more fitting?

Another contender for the "LLVM_EXPORT" name that I'd like to see some day is a macro that exports symbols from Windows DLLs using __declspec( dllexport ) and then we can get rid of this: D18826

A comment should be added in Compiler.h to document the macro.

tstellar updated this revision to Diff 191787.Mar 21 2019, 3:10 PM

Rename macro from LLVM_EXPORT to LLVM_EXTERNAL_VISIBILITY and add documentation
for macro.

Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 3:10 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
tstellar edited the summary of this revision. (Show Details)Mar 21 2019, 3:11 PM
tstellar added a reviewer: rnk.
rnk added a comment.Mar 21 2019, 3:43 PM

I kind of liked LLVM_EXPORT, since it seemed like it would logically grow to expand to dllexport, but this spelling works too. Let's do it, and start taking incremental steps towards actually annotating LLVM's dynamic interface. :)

rnk accepted this revision.Apr 4 2019, 1:44 PM
rnk added a reviewer: hans.

Looks good to me, if Hans also stamps it, I think that's enough approval power for this change.

This revision is now accepted and ready to land.Apr 4 2019, 1:45 PM
hans accepted this revision.Apr 5 2019, 2:42 AM

Looks good to me.

Are there any buildbots that will catch if someone forgets to put an LLVM_EXTERNAL_VISIBILITY somewhere that's needed?

Looks good to me.

Are there any buildbots that will catch if someone forgets to put an LLVM_EXTERNAL_VISIBILITY somewhere that's needed?

I don't think so. At least on linux this would require building with -DLLVM_BUILD_LLVM_DYLIB=ON and -DLLVM_LINK_LLVM_DYLIB=ON and I don't see any bots doing this.

hans added a comment.Apr 9 2019, 12:04 AM

Looks good to me.

Are there any buildbots that will catch if someone forgets to put an LLVM_EXTERNAL_VISIBILITY somewhere that's needed?

I don't think so. At least on linux this would require building with -DLLVM_BUILD_LLVM_DYLIB=ON and -DLLVM_LINK_LLVM_DYLIB=ON and I don't see any bots doing this.

I'm fine with the change itself, but that means it's pretty likely this is going to break sooner or later.

I'm going to commit this today. I have an internal bot set up now, so I can catch when this breaks. I still plan to set up an external bot, I'm just still trying to find the hardware to do this.

This revision was automatically updated to reflect the committed changes.
tstellar reopened this revision.Jun 17 2019, 9:06 PM
This revision is now accepted and ready to land.Jun 17 2019, 9:06 PM
tstellar updated this revision to Diff 205247.Jun 17 2019, 9:08 PM
tstellar edited the summary of this revision. (Show Details)

Add LLVM_EXTERNAL_VISIBILITY to initialize*Pass to try to silence ld
warning on OS X.

tstellar requested review of this revision.Jun 17 2019, 9:09 PM
rnk added inline comments.Jun 18 2019, 10:51 AM
llvm/include/llvm/PassSupport.h
42 ↗(On Diff #205247)

Do we need the same thing here? We instantiate the same template, which I would expect to raise the same warning, if this macro is used from two files that use hidden and default visibility.

62–63 ↗(On Diff #205247)

Is this a speculative fix, or were you able to reproduce the problem and confirm this fixes it?

tstellar updated this revision to Diff 205407.Jun 18 2019, 11:35 AM
tstellar marked an inline comment as done.

Add LLVM_EXTERNAL_VISIBILITY to both INITIALIZE_PASS* macros.

tstellar marked an inline comment as done.Jun 18 2019, 11:38 AM
tstellar added inline comments.
llvm/include/llvm/PassSupport.h
62–63 ↗(On Diff #205247)

This is speculative. I don't have an OS X system to test on. I have tried to reproduce on linux with clang-7, but I don't see any object file differences in files mentioned in the warnings, like X86Targetmachine.cpp.o.

It seems like this warning is triggered when you have a cpp file that uses one of the INITIALIZE_PASS macros and also calls the initialize*Pass function in the same file. My best guess is this is related to inlining decisions in some way, but I still don't know the root cause.

rnk added a subscriber: echristo.Jun 18 2019, 11:52 AM
rnk added inline comments.
llvm/include/llvm/PassSupport.h
62–63 ↗(On Diff #205247)

@echristo, do you want to apply this locally and try to reproduce, since you reported the issue last time around?

@tstellar My worry is that maybe this warning is triggered more broadly by any templates instantiated and called by hidden and default visibility functions. I'd like to nail down when it happens, or disable it entirely, since I'm positive we don't care about being able to override C++ template functions.

llvm/lib/Target/CMakeLists.txt
21 ↗(On Diff #205407)

If the warning does re-occur, I'd suggest working around it by disabling this logic here on Mac. Then we can fix it with smaller patches that aren't too large for phab to display by default.

tstellar marked an inline comment as done.Jul 10 2019, 9:22 AM
tstellar added inline comments.
llvm/include/llvm/PassSupport.h
62–63 ↗(On Diff #205247)

Ok for reference, similar errors were reported in this bug: https://llvm.org/PR31782.

tstellar updated this revision to Diff 209244.Jul 11 2019, 9:09 AM

Rebased and removed the workaround of adding LLVM_EXTERNAL_VISIBLITY
to the void llvm::initializepassNamePass definitions, since this
did not resolve the linker errors on OS X.

Rebased and removed the workaround of adding LLVM_EXTERNAL_VISIBLITY
to the void llvm::initializepassNamePass definitions, since this
did not resolve the linker errors on OS X.

s/errors/warnings/

tstellar updated this revision to Diff 226518.Oct 25 2019, 4:13 PM

Rebase on trunk.

The warnings are still present on OS X. Here is a build log:
https://github.com/tstellar/llvm-project/pull/11/checks?check_run_id=275366813

Rebase on trunk.

The warnings are still present on OS X. Here is a build log:
https://github.com/tstellar/llvm-project/pull/11/checks?check_run_id=275366813

@tstellar Can you add _LIBCPP_INLINE_VISIBILITY to __call_once_proxy and see whether that fixes the warning? I believe that is the correct fix, now that we don't use always_inline anymore to implement _LIBCPP_INLINE_VISIBILITY.

I've added _LIBCPP_INLINE_VISIBILITY to __call_once_proxy locally and everything works, but I don't have your specific patch to test locally.

Rebase on trunk.

The warnings are still present on OS X. Here is a build log:
https://github.com/tstellar/llvm-project/pull/11/checks?check_run_id=275366813

@tstellar Can you add _LIBCPP_INLINE_VISIBILITY to __call_once_proxy and see whether that fixes the warning? I believe that is the correct fix, now that we don't use always_inline anymore to implement _LIBCPP_INLINE_VISIBILITY.

I've added _LIBCPP_INLINE_VISIBILITY to __call_once_proxy locally and everything works, but I don't have your specific patch to test locally.

Note that I've applied that change in 48b7068beca9 regardless because I think it's good, but I'm still curious to know whether it'll fix your problems.

tstellar updated this revision to Diff 229131.Nov 13 2019, 10:06 AM

Rebase patch.

Rebase on trunk.

The warnings are still present on OS X. Here is a build log:
https://github.com/tstellar/llvm-project/pull/11/checks?check_run_id=275366813

@tstellar Can you add _LIBCPP_INLINE_VISIBILITY to __call_once_proxy and see whether that fixes the warning? I believe that is the correct fix, now that we don't use always_inline anymore to implement _LIBCPP_INLINE_VISIBILITY.

I've added _LIBCPP_INLINE_VISIBILITY to __call_once_proxy locally and everything works, but I don't have your specific patch to test locally.

Note that I've applied that change in 48b7068beca9 regardless because I think it's good, but I'm still curious to know whether it'll fix your problems.

I've confirmed that this change does fix the warning. How do we we want to proceed with this patch? The warnings will still be present on existing Mac OS installs. Should we disable this change on Mac OS for now or should we keep it and live with the warnings?

rnk added a comment.Nov 13 2019, 10:34 AM

I've confirmed that this change does fix the warning. How do we we want to proceed with this patch? The warnings will still be present on existing Mac OS installs. Should we disable this change on Mac OS for now or should we keep it and live with the warnings?

I'd be in favor of letting Mac use default visibility for now. One way to make this more palatable would be to have a CMake option that allows users to override the default and get hidden visibility if they really want it.

In D54439#1744353, @rnk wrote:

I've confirmed that this change does fix the warning. How do we we want to proceed with this patch? The warnings will still be present on existing Mac OS installs. Should we disable this change on Mac OS for now or should we keep it and live with the warnings?

I'd be in favor of letting Mac use default visibility for now. One way to make this more palatable would be to have a CMake option that allows users to override the default and get hidden visibility if they really want it.

I'm also in favor of keeping default visibility on Mac. We're trying to cut down on the number of CMake options, so I would prefer not to add an option for this.

nhaehnle removed a subscriber: nhaehnle.Dec 8 2019, 10:29 AM
tstellar updated this revision to Diff 233894.Dec 13 2019, 4:49 PM

Rebase and switch to using default visibility on Mac OS.

Unit tests: pass. 60871 tests passed, 0 failed and 726 were skipped.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or apply this patch.

Build artifacts: console-log.txt, CMakeCache.txt, clang-format.patch, test-results.xml, diff.json

Unit tests: pass. 61303 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rnk accepted this revision.Jan 14 2020, 4:25 PM

lgtm

Let's get the annotations in so we can start to benefit from them.

This revision is now accepted and ready to land.Jan 14 2020, 4:25 PM
This revision was automatically updated to reflect the committed changes.