[PATCH] D52662: [libc++] Make sure we can build libc++ with -fvisibility=hidden

[Shoaib Meenai via Phabricator]

smeenai added a comment.

In https://reviews.llvm.org/D52662#1249688, @ldionne wrote:

> In https://reviews.llvm.org/D52662#1249649, @smeenai wrote:
>
> > That makes sense. Back when I was looking into this (which is over a year ago now, and sadly I got pulled away into other things before I had a chance to upstream all of it), I found that only some of the missing exports when building with hidden visibility were actually required for `check-cxx` to pass. The rest could be added if you want to preserve ABI compatibility for v1, but hidden visibility very much seems like an ABI v2 thing, so at that point you could also fix the over-exporting.
>
>
> I think this is applicable for ABI v1: those symbols are currently exported by the ABI, and it is just a coincidence that we export them from the dylib.


Fair point. I think it's worth distinguishing between symbols that we actually need to export (for check-cxx or applications or similar) and symbols that just happen to be exported because of the default visibility but don't actually need to be exported. If you're already doing that in this patch, or if there aren't actually any symbols in the first category anymore, then just ignore me :)

> 
> 
>> Internally, we've been building libc++ with hidden visibility for a long time now, and it's been working well. I'm linking some of the issues from when I was working on that; I've lost all context on these by now, but I figured it might save you some trouble if you ran into similar issues:
> 
> Why have you been building libc++ with hidden visibility? What is the advantage of doing that?

For libc++, there's not too much advantage, since (a) it already controls its ABI pretty thoroughly, and (b) it already builds with `-fvisibility-inlines-hidden`, which takes care of a lot of cases. We did find a few symbols that were over-exported, but not enough to make any significant reductions in dynamic symbol table size. Mostly it was a cleanliness thing, where we wanted all our libraries having controlled ABIs and building with hidden visibility. It also helped us with cross-platform compatibility, since Windows always has the equivalent of hidden visibility and you have to explicitly declare your exports (which is a much nicer default IMO).


Repository:
  rCXX libc++

https://reviews.llvm.org/D52662