[PATCH] D113610: [cmake] Add option LLVM_ENABLE_VISIBILITY_MACROS

Thu Dec 9 14:43:29 PST 2021

benlangmuir added a comment.

In D113610#3183892 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113610/new/

https://reviews.llvm.org/D113610