[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#1249753, @ldionne wrote:

> In https://reviews.llvm.org/D52662#1249706, @smeenai wrote:
>
> > 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 :)
>
>
> I'm not sure how we can ever check that a symbol is exported but not actually needed. Wouldn't this require analyzing the set of all programs linking to `libc++.dylib` in the wild?


Sorry, you're right. Internally I'd based my analysis on the set of symbols needed by check-clang + the set of symbols needed by all the binaries we were building (which is a pretty extensive test case), but of course that wouldn't cover everyone's use cases. There were some symbols which seemed fairly obviously internal/intended to be hidden, but since they've been part of the ABI it's entirely possible someone is using them anyway.

> 
> 
>> 
>> 
>>> 
>>> 
>>>> 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).
> 
> Got it. I agree that Windows is much saner in that respect. In the future, we could actually apply hidden visibility to all of namespace `std` and then only cherry-pick entities that we want to export (and apply a default-visibility attribute to those). However, that requires the ability to explicitly instantiate vtables and RTTI, which we don't have right now (but we could add as an extension to Clang). This would allow us to drop almost all of the visibility annotations we currently have in libc++.

Hmm, why do we need to be able to explicitly instantiate vtables and RTTI ... doesn't the implied emission of those in the object file containing the key function work? I've been aware of a few -dev threads flying back and forth about various visibility issues for libc++, and I admit I haven't been keeping up with them fully, so I apologize if this has already been covered in one of those.


Repository:
  rCXX libc++

https://reviews.llvm.org/D52662