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

[Louis Dionne via Phabricator]

ldionne added a comment.

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

> In D55404#1323828 <https://reviews.llvm.org/D55404#1323828>, @ldionne wrote:
>
> > 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?
>
>
> In general that sounds fine me, problem with generalizing https://github.com/llvm-mirror/libcxx/blob/master/CMakeLists.txt#L742 is that we also want to make sure that `new` and `delete` operators are hidden as well. Those operators may be implicitly declared by Clang with default visibility which defeats `-fvisibility=hidden` or explicit annotations which is why we've introduced `-fvisibility-global-new-delete-hidden`which is being set in this change as well. I don't know if that's something that would affect Windows, and in general I don't know if it's something that should be enabled automatically for static libc++?


I was going to say "why didn't you just fix `-fvisibility=hidden` to do the right thing for `new` and `delete`"? But then I read the discussion in https://reviews.llvm.org/D53787. However, after reading that, I'm strongly questioning whether you actually want `new` and `delete` to have hidden visibility. And if you actually do, it seems to me like the arguments in https://reviews.llvm.org/D53787 are somewhat defeated, no?

> 
> 
>> 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.
> 
> Do we even need `_LIBCPP_HIDE_FROM_ABI_PER_TU` now that we have `__attribute__((__exclude_from_explicit_instantiation))`? The only case I can think of are older versions of Clang or other compilers that don't support this attribute.

Yes, we do (at least in theory). `_LIBCPP_HIDE_FROM_ABI_PER_TU` makes sure that inline functions are not ODR-merged across translation units or static libraries, since we don't know whether they are token-by-token equivalent (for example if one TU has an older definition of the function and the other TU has a newer definition of the function). `exclude_from_explicit_instantiation` was added to workaround a completely different problem, which is that we were promising that some functions were defined in the dylib/static library when they really were not defined in the dylib/static library, which would cause link errors.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55404