Page MenuHomePhabricator

Only set LLVM_EXTERNAL_VISIBILITY when building libLLVM dylib
ClosedPublic

Authored by benlangmuir on Nov 10 2021, 2:01 PM.

Details

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.

Diff Detail

Event Timeline

benlangmuir created this revision.Nov 10 2021, 2:01 PM
benlangmuir requested review of this revision.Nov 10 2021, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 2:01 PM

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?

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.

+@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

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.

Ping, does anyone have any feedback on this?

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

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

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.

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.

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 retitled this revision from [cmake] Add option LLVM_ENABLE_VISIBILITY_MACROS to Only set LLVM_EXTERNAL_VISIBILITY when building libLLVM dylib.
benlangmuir edited the summary of this revision. (Show Details)

Updated to set LLVM_EXTERNAL_VISIBILITY only when building the LLVM dylib.

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 accepted this revision.Dec 13 2021, 2:50 PM

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

This revision is now accepted and ready to land.Dec 13 2021, 2:50 PM

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 added a comment.Dec 13 2021, 3:26 PM

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.

This comment was removed by MaskRay.
MaskRay accepted this revision.Dec 13 2021, 3:36 PM

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!

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

That's interesting, what is your host configuration?

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

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 added inline comments.Dec 15 2021, 1:09 PM
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 added inline comments.Dec 15 2021, 1:24 PM
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.