Page MenuHomePhabricator

CMake: Make most target symbols hidden by default
Needs ReviewPublic

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

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

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/