[PATCH] D55404: [libcxx] Support building static library with hidden visibility

[Louis Dionne via Phabricator]

ldionne added a comment.

In D55404#1323611 <https://reviews.llvm.org/D55404#1323611>, @phosek wrote:

> In D55404#1323155 <https://reviews.llvm.org/D55404#1323155>, @ldionne wrote:
>
> > So... the way we planned on tackling the hidden visibility problem is https://reviews.llvm.org/D54810 (which is blocked on a couple of Clang bugs w.r.t. visibility attributes). Would https://reviews.llvm.org/D54810 solve your problem?
> >
> > Also, if your intent is to make _all_ symbols hidden, I don't think this patch achieves it because we still explicitly annotate symbols in the dylib as having default visibility. Am I mistaken?
>
>
> Our use case is static library while D54810 <https://reviews.llvm.org/D54810> seems to focusing on shared case. Specifically, we need to link statically libc++ into various shared libraries and executables (e.g. drivers in Fuchsia) which might co-exist with other shared libraries and we need to ensure that no libc++ symbols are being exported. I suspect this might be also useful for other cases of "private libc++" such as libFuzzer.


Ok, this is the use case I imagined. Basically, you don't want the symbols from the libc++ static library to pollute what's exported by a shared library it might be linked into. That's perfectly reasonable.

> The way this change achieves that is if you set the `LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY` option, it defines `_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS` which should disable all existing annotations and then explicitly sets `-fvisibility=hidden -fvisibility-global-new-delete-hidden` (only for static library, shared library is unaffected). Does this make sense?

Ahhh, this part is what I had missed. I didn't see you were disabling the visibility annotation. In that case everything is indeed hidden. Instead, I think a cleaner approach would be:

1. Build with hidden visibility by default at namespace scope (basically https://reviews.llvm.org/D54810), and

2. generalize https://github.com/llvm-mirror/libcxx/blob/master/CMakeLists.txt#L742 so that we can remove visibility annotations using a CMake option.

This way, we'd avoid adding yet another configuration option and instead leverage something we already (apparently) need and use on Windows. What do you think?

Also, I'd be curious about the relationship between this and the `_LIBCPP_HIDE_FROM_ABI_PER_TU` setting, which gives internal linkage to everything. Right now, it's possible to have both default visibility and internal linkage, but I'm not sure what that even means. Maybe both options should actually be merged under something more general like `_LIBCPP_ALLOW_DISTRIBUTING_STATIC_LIBRARY`, which would hide all symbols _and_ give internal linkage to everything. Not sure.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55404