Only set LLVM_EXTERNAL_VISIBILITY when building libLLVM dylib

[benlangmuir]

Summary

Updated:
When building LLVM static libraries, we should not make symbols more visible than CMAKE_CXX_VISIBILITY_PRESET, since the goal may be to have a purely hidden llvm embedded in another library. Instead, we only define LLVM_EXTERNAL_VISIBILITY for the dynamic library build (when LLVM_BUILD_LLVM_DYLIB=YES).

Original Summary:
Add an option LLVM_ENABLE_VISIBILITY_MACROS (default ON) to control whether or not to honor the LLVM_*_VISIBILITY macros, or whether to use the normal visibility. This enables the configuration -DLLVM_ENABLE_VISIBILITY_MACROS=0 -DCMAKE_CXX_VISIBILITY_PRESET=hidden to stop exporting any symbols, which is useful when linking LLVM statically into a library that does not want to expose any LLVM symbols.

[lhames]

Adding a few of the commenters from https://reviews.llvm.org/D54439.

FWIW This change makes sense to me, but I haven't spent much time poking around in our build system.

It feels like we should have a way of doing this already, but maybe linking more than one copy of LLVM is rare enough that the issue hasn't come up before?

[benlangmuir]

but maybe linking more than one copy of LLVM is rare enough that the issue hasn't come up before?

It also improves dead code stripping; with this change I was able to save several MB in a library that didn't need all these previously exported symbols.

[rnk]

+@tstellar @serge-sans-paille for the recent LLVM_EXTERNAL_VISIBILITY changes
+@compnerd @MaskRay for our discussions about the shared library build

I think there is a valid use case for a static, fully hidden visibility build of LLVM. I don't think this is the absolute cleanest way to support that use case, but it works and I can live with it.

Part of me wants to say that we should make these visibility macros conditional on the existing LLVM_ENABLE_DYLIB (sp?) option. If the user is statically linking LLVM, we should drop these annotations and allow the global visibility preset to take precedence.

llvm/include/llvm/Config/llvm-config.h.cmake

98

Unrelated, but this isn't namespaced, and it should not live in the llvm-config header... I'll try to blame that and chase it down... Looks like it is D88467

[benlangmuir]

Part of me wants to say that we should make these visibility macros conditional on the existing LLVM_ENABLE_DYLIB (sp?) option. If the user is statically linking LLVM, we should drop these annotations and allow the global visibility preset to take precedence.

This approach would work fine for my use case and I'm happy to go this way if that's the consensus. The reason I went with the separate option is that I could imagine an alternative use case where you're "wrapping" llvm into your own dylib, in which case you might have the same reasons to use these options as LLVM's own dylib build does.

[benlangmuir]

Ping, does anyone have any feedback on this?

[ldionne]

I don't have a horse in this race since this is LLVM and not libc++, however from a user perspective, it would feel more ergonomic to have a single setting like LLVM_VISIBILITY=normal|hidden. Or something like LLVM_HIDDEN_VISIBILITY=ON (by default it's OFF, which means default visibility is used).

Heck, thinking about it, why does LLVM even override what's decided by CMAKE_CXX_VISIBILITY_PRESET? It seems to me that:

• LLVM_LIBRARY_VISIBILITY should always be hidden, and
• LLVM_EXTERNAL_VISIBILITY should be whatever CMAKE_CXX_VISIBILITY_PRESET says

Just my .02

[dexonsmith]

In D113610#3125742, @benlangmuir wrote:

Part of me wants to say that we should make these visibility macros conditional on the existing LLVM_ENABLE_DYLIB (sp?) option. If the user is statically linking LLVM, we should drop these annotations and allow the global visibility preset to take precedence.

This approach would work fine for my use case and I'm happy to go this way if that's the consensus. The reason I went with the separate option is that I could imagine an alternative use case where you're "wrapping" llvm into your own dylib, in which case you might have the same reasons to use these options as LLVM's own dylib build does.

My guess would be that the alternative use case you're proposing is pretty rare, in which case @rnk's suggestion SGTM. Later, if someone comes across it, we can add the flag to override the default (which would still depend on shared vs. static).

[benlangmuir]

In D113610#3183892, @ldionne wrote:

I don't have a horse in this race since this is LLVM and not libc++, however from a user perspective, it would feel more ergonomic to have a single setting like LLVM_VISIBILITY=normal|hidden. Or something like LLVM_HIDDEN_VISIBILITY=ON (by default it's OFF, which means default visibility is used).

Heck, thinking about it, why does LLVM even override what's decided by CMAKE_CXX_VISIBILITY_PRESET? It seems to me that:

• LLVM_LIBRARY_VISIBILITY should always be hidden, and
• LLVM_EXTERNAL_VISIBILITY should be whatever CMAKE_CXX_VISIBILITY_PRESET says

I guess the issue is that LLVM is setting CMAKE_CXX_VISIBILITY_PRESET=hidden itself in lib/Targets/CMakeLists.txt and it needs to get back to "default" for these particular symbols.

In D113610#3183924, @dexonsmith wrote:

My guess would be that the alternative use case you're proposing is pretty rare, in which case @rnk's suggestion SGTM. Later, if someone comes across it, we can add the flag to override the default (which would still depend on shared vs. static).

@dexonsmith @rnk do you have an opinion on whether we should disable both macros, or only LLVM_EXTERNAL_VISIBILITY in that case? I hadn't really thought about it before, but LLVM_LIBRARY_VISIBILITY (hidden) might still make sense to keep in the static build since it's marked on symbols that are library-local either way.

[dexonsmith]

In D113610#3183993, @benlangmuir wrote:

In D113610#3183924, @dexonsmith wrote:

My guess would be that the alternative use case you're proposing is pretty rare, in which case @rnk's suggestion SGTM. Later, if someone comes across it, we can add the flag to override the default (which would still depend on shared vs. static).

@dexonsmith @rnk do you have an opinion on whether we should disable both macros, or only LLVM_EXTERNAL_VISIBILITY in that case? I hadn't really thought about it before, but LLVM_LIBRARY_VISIBILITY (hidden) might still make sense to keep in the static build since it's marked on symbols that are library-local either way.

Yup, seems reasonable. If/when there's an option, could probably call it LLVM_ENABLE_EXTERNAL_VISIBILITY, defaulting to "off" for static / "on" for shared.

[benlangmuir]

Updated to set LLVM_EXTERNAL_VISIBILITY only when building the LLVM dylib.

[benlangmuir]

Since the new approach is no longer "opt-in", I tested:

• Default build (static, LLVMInitialize* is external)
• -DCMAKE_CXX_VISIBILITY_PRESET=hidden (static, LLVMInitialize* is hidden -- this is the new thing)
• -DBUILD_SHARED_LIBS=YES (dynamic, LLVMInitialize* is external due to not overriding visibility)
• -LLVM_LINK_LLVM_DYLIB=YES (dynamic, LLVMInitialize* is external due to LLVM_EXTERNAL_VISIBILITY)

[rnk]

I think this looks good, and my reading of the other comments is that other reviewers are on board with this too.

[MaskRay]

LLVM_BUILD_LLVM_DYLIB is a cmake variable, not a C/C++ macro. You may need to change include/llvm/Config/llvm-config.h.cmake

[rnk]

In D113610#3190673, @MaskRay wrote:

LLVM_BUILD_LLVM_DYLIB is a cmake variable, not a C/C++ macro. You may need to change include/llvm/Config/llvm-config.h.cmake

I believe that is included already, if I'm not mistaken.

[MaskRay]

This comment was removed by MaskRay.

[mehdi_amini]

Reverted in e0aa2ea661fb

See the bot here: https://lab.llvm.org/buildbot/#/builders/61/builds/18785
Let us know if you need assistance with this!

[benlangmuir]

@mehdi_amini thanks!

[benlangmuir]

@mehdi_amini I'm seeing the same failure output in the llvm.c test even with my changes reverted and a clean build. Any idea what I might be missing? I am building with the following arguments -- they're about as close as I could get to that bot without having CUDA.

$ cmake ../../llvm-project/llvm \
   -DLLVM_BUILD_EXAMPLES=ON \
   '-DLLVM_TARGETS_TO_BUILD=host;NVPTX' \
   -DLLVM_ENABLE_PROJECTS=mlir \
   -DMLIR_INCLUDE_INTEGRATION_TESTS=ON \
   -DBUILD_SHARED_LIBS=ON \
   -DCMAKE_BUILD_TYPE=Release \
   -DLLVM_ENABLE_ASSERTIONS=ON \
   '-DLLVM_LIT_ARGS=-v -vv' \
   -GNinja

Failure:

FAIL: MLIR :: CAPI/llvm.c (2 of 1232)
******************** TEST 'MLIR :: CAPI/llvm.c' FAILED ********************
Script:
--
: 'RUN: at line 10';   /Users/blangmuir/src/build-llvm/build-mlir-shared/bin/mlir-capi-llvm-test 2>&1 | /Users/blangmuir/src/build-llvm/build-mlir-shared/bin/FileCheck /Users/blangmuir/src/llvm-project/mlir/test/CAPI/llvm.c
--
Exit Code: 1

Command Output (stderr):
--
+ : 'RUN: at line 10'
+ /Users/blangmuir/src/build-llvm/build-mlir-shared/bin/mlir-capi-llvm-test
+ /Users/blangmuir/src/build-llvm/build-mlir-shared/bin/FileCheck /Users/blangmuir/src/llvm-project/mlir/test/CAPI/llvm.c
/Users/blangmuir/src/llvm-project/mlir/test/CAPI/llvm.c:45:12: error: CHECK: expected string not found in input
 // CHECK: !llvm.void: 1
           ^
<stdin>:3:21: note: scanning from here
!llvm.ptr<i32, 4>: 1
                    ^
<stdin>:4:42: note: possible intended match here
LLVM ERROR: can't create type 'mlir::LLVM::LLVMVoidType' because storage uniquer isn't initialized: the dialect was likely not loaded, or the type wasn't added with addTypes<...>() in the Dialect::initialize() method.
                                         ^

[mehdi_amini]

That's interesting, what is your host configuration?

[benlangmuir]

@mehdi_amini x86_64 macOS

[benlangmuir]

Never mind, I got it working on Linux. BUILD_SHARED_LIBS of MLIR doesn't seem to work correctly on macOS. New patch with a fix: https://reviews.llvm.org/D115825

[mehdi_amini]

I tried this config on my Mac and ninja check-mlir failed with lot of failures actually:

Unsupported:  91
Passed     : 313
Failed     : 828

I suspect that BUILD_SHARED_LIBS never worked on MacOS here...

(sorry I typed this and didn't hit send before).

[mehdi_amini]

llvm/include/llvm/Support/Compiler.h

133

If I understand correctly, the -DBUILD_SHARED_LIBS=YES will use this "else" and not define LLVM_EXTERNAL_VISIBILITY.
I'm a bit puzzled why is this correct?
I would expect that BUILD_SHARED_LIBS requires at least as much visibility as LLVM_BUILD_LLVM_DYLIB (more in some cases).

That is: AFAIU we're making the unit of visibility smaller with BUILD_SHARED_LIBS (many small shared object) compared to LLVM_BUILD_LLVM_DYLIB: if a symbol foo from libA is used in libB, it does not need to be externally visible if we link the static libA and libB together in a single dylib, but if libA and libB are each individual dylibs then libA needs to make foo visible.

[benlangmuir]

llvm/include/llvm/Support/Compiler.h

133

BUILD_SHARED_LIBS=YES doesn't work with -fvisibility=hidden in LLVM, so it seemed misleading to define this macro for that configuration when it wasn't really exercised there. That being said, I'm not opposed to changing it just for MLIR or other downstreams, so I'll give that a shot.

llvm/include/llvm/Config/llvm-config.h.cmake

 
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
 /* Define if LLVM was built with a dependency to the tensorflow compiler */
 #cmakedefine LLVM_HAVE_TF_AOT
 
 /* Define to 1 if you have the <sysexits.h> header file. */
 #cmakedefine HAVE_SYSEXITS_H ${HAVE_SYSEXITS_H}
 
 /* Define to 1 to enable the experimental new pass manager by default */
 #cmakedefine01 LLVM_ENABLE_NEW_PASS_MANAGER
 
 /* Define if the xar_open() function is supported on this platform. */
 #cmakedefine LLVM_HAVE_LIBXAR ${LLVM_HAVE_LIBXAR}
 
+/* Define if building libLLVM shared library */
+#cmakedefine LLVM_BUILD_LLVM_DYLIB
+
 #endif

llvm/include/llvm/Support/Compiler.h

 
 /// LLVM_LIBRARY_VISIBILITY - If a class marked with this attribute is linked
 /// into a shared library, then the class should be private to the library and
 /// not accessible from outside it.  Can also be used to mark variables and
 /// functions, making them private to any shared library they are linked into.
 /// On PE/COFF targets, library visibility is the default, so this isn't needed.
 ///
 /// LLVM_EXTERNAL_VISIBILITY - classes, functions, and variables marked with
-/// this attribute will be made public and visible outside of any shared library
-/// they are linked in to.
+/// this attribute will be made public and visible outside the libLLVM dynamic
+/// library.
 #if __has_attribute(visibility) && !defined(__MINGW32__) &&                    \
     !defined(__CYGWIN__) && !defined(_WIN32)
 #define LLVM_LIBRARY_VISIBILITY __attribute__ ((visibility("hidden")))
-#define LLVM_EXTERNAL_VISIBILITY __attribute__ ((visibility("default")))
+#if defined(LLVM_BUILD_LLVM_DYLIB)
+#define LLVM_EXTERNAL_VISIBILITY __attribute__((visibility("default")))
 #else
+#define LLVM_EXTERNAL_VISIBILITY
+#endif
+#else
 #define LLVM_LIBRARY_VISIBILITY
 #define LLVM_EXTERNAL_VISIBILITY
 #endif
 
 #if defined(__GNUC__)
 #define LLVM_PREFETCH(addr, rw, locality) __builtin_prefetch(addr, rw, locality)
 #else
 #define LLVM_PREFETCH(addr, rw, locality)