[PATCH] D147832: [libcxx] Introduce clang::lto_visibility_public attribute

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

> You say you build with -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -fvisibility=hidden which AFAICT defeats the purpose of a shared library, since it wouldn't export a single symbol.

My bad, I should've clarified on this. For fuchsia, we build everything with hidden visibility by default and individual binaries need to explicitly mark their symbols with default visibility if they want to be exported. This also means that when a fuchsia binary uses libc++, we need to make sure it doesn't accidentally export any libc++ symbols. Fuchsia binaries can consume libc++ as either a shared lib or a hermetic static lib, but we don't know at compile time how libc++ will eventually be consumed. If used as a shared lib, then symbols will be defined libc++.so. If linked in as a hermetic static lib, then we need to ensure those symbols have hidden visibility. To cover both cases and prevent us from having to compile twice, we just compile everything with `-D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -fvisibility=hidden`.

> It makes our visibility annotations even more complicated. It is already way to complicated, we should avoid making it even worse.

In terms of minimizing extra added visibility annotation complexity, what do you think of this alternative: update `_LIBCPP_TEMPLATE_VIS` and `_LIBCPP_TYPE_VIS`  in `<__config>` such that it can be defined in the compilation step as an argument. If left undefined, the default behavior would what libcxx does now, which is defining the macro according to `_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS`. This way, the change would be less intrusive to libcxx and downstream users can have more flexibility on libc++ class annotations.

> This is a completely untested configuration, which means it will regress (which is already a problem with the current visibility annotations)
> Next to @philnik's issues I have a large issue with the lack of documentation. Our visibility macros are already not very well documented, adding more without any documentation seems like a bad idea. Especially there is no explanation which friend declarations need this markup and which don't.

I'll also be sure to update the next revision with tests and docs. The initial reasoning for the the friend variants is because some attributes like `[[clang::lto_visibility_public]]` can only be attached to class definitions, but not friend declarations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147832