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

Wed Dec 12 09:47:31 PST 2018

phosek added a comment.

In D55404#1327114 <https://reviews.llvm.org/D55404#1327114>, @ldionne wrote:

> In D55404#1326554 <https://reviews.llvm.org/D55404#1326554>, @phosek wrote:
>
> > I'm not sure what you mean, can you elaborate on the last sentence? Without `-fvisibility-global-new-delete-hidden` we would end up with `new` and `delete` symbols being exported from the shared library.
>
>
> Quoting richard in https://reviews.llvm.org/D53787#1282975:
>
> > These symbols really are special. Other symbols are introduced explicitly by a declaration, whereas these are declared implicitly by the compiler. Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program. Other symbols are generally provided in one library and consumed by users of that library, whereas these symbols are typically provided by the main binary and consumed by the libraries that it uses. And so on.
>
> Basically, what I'm saying is that giving hidden visibility to `new` and `delete` seemed like a bad idea to Richard, and I follow his argument: those functions should be overridable by users but that won't work if you give them hidden visibility (right?).

Users can still override `new` and `delete` within the same shared library but not across the binary boundary. That's our intention, our goal is to make each shared library that statically links libc++ hermetic.

This makes me think that the name `LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY` I've chosen for this option may be misleading and causing confusion. I should probably change it to `LIBCXX_HERMETIC_STATIC_LIBRARY` to convey the intention. WDYT?

> The reason why I'm pushing back on this change as-is is that it makes our build logic even more complex than it already is (and it's way too complex as-is). TBH, I'm fine with the use case but I'd like us to find a better solution than just adding yet another build mode with different options. Finding a way to express this in the source would be better (because the source doesn't depend on a build mode), but like you point out it falls short for marking new and delete as hidden (which might not be a good idea, but whatever). Instead of doing visibility like we do today (in the source), perhaps we could instead have an explicit list of symbols that we export from the shared library? We would get rid of all visibility annotations from the sources and would do it externally, all in the build system. I've been told this is what libstdc++ has been doing and I think they're doing better than libc++ in that area.

I agree that this is making the build logic more complicated and I can try to to simplify it a bit more. I also agree that your suggestion would be an improvement, but I think it's orthogonal to what we're trying to achieve. If we changed libc++ to use explicit list, then in the hermetic case the list would be empty but we would still want to use `-fvisibility=hidden -fvisibility-global-new-delete-hidden`, so the only difference compared to current patch would be omitting the `_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS` definition which is still an improvement but fairly minor compared to the rest of this change.

Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55404