[libcxx-commits] [PATCH] D153658: [libc++] Make everything in namespace std have default type visibility and hidden visibility and remove _LIBCPP_ENUM_VIS

Wed Jul 12 08:47:38 PDT 2023

ldionne added inline comments.

================
Comment at: libcxx/include/__config:656-657
 // clang-format off
-#  define _LIBCPP_BEGIN_NAMESPACE_STD namespace std { inline namespace _LIBCPP_ABI_NAMESPACE {
+#  define _LIBCPP_BEGIN_NAMESPACE_STD namespace _LIBCPP_TYPE_VISIBILITY_DEFAULT _LIBCPP_HIDDEN std {                   \
+                               inline namespace _LIBCPP_ABI_NAMESPACE {
 #  define _LIBCPP_END_NAMESPACE_STD }}
----------------
philnik wrote:
> ldionne wrote:
> > This change will have the following non-trivial effects:
> > 1. Everything in namespace `std` will now have default type-visibility. Previously, only the things marked with a visibility macro would get that treatment. Why is that okay? Can this bite users or us?
> > 2. Everything in namespace `std` will now have hidden visibility unless overridden. Previously, that was our intent, but we might not have been doing it 100% consistently so this might actually have an impact on what symbols become exported when folks use our headers. Do we have an idea of which symbols this impacts, and have you thought about the potential user impact of this?
> > 
> > Let's document that in the commit message (and also in a release note if we foresee user impact).
> 1) Every public type is supposed to have default type visibility to make it possible to throw things across library boundaries and have RTTI work (that is required for e.g. `std::any`). While the usefulness is questionable for many types (e.g. `remove_const` and friends), it is technically allowed to get RTTI for all types. For library-internal types, it should be fine, since RTTI won't ever get actually generated for them, making it irrelevant what visibility the symbols have. This will be a user-visible change insofar as it will fix any missing visibility attributes on classes, but I don't think that will have a big impact.
> 
> 2) This should™ only impact global variables and functions which don't have any kind of visibility annotation. Given that almost all functions already have visibility annotations, this shouldn't make much of an impact there. The global variables will be duplicated across libraries with this change, but I don't think that's much of a problem. This should only be noticeable by checking the address of the variable, since any global variables I'm aware of are `constexpr` or at least `const`. Almost nobody does that, since it's not interesting other than checking for strict conformance, so I wouldn't consider that much of a problem either. This also already happens when `-fvisibility=hidden` is used, which is the case for people who care about their ABI surface.
(2) means in particular that we'll give hidden visibility to global objects like `is_const_v` & friends. This means that we no longer export them from our client's ABI surface by default.

This is great since weak symbols at ABI boundaries are costly (load times), however that is technically a behavior change. If we make this change, I would like us to pin that behavior down and also probably get some sort of wording into the standard that says "these `_v` objects are not guaranteed to have a fixed address". It's obviously the intent but we'd need to be explicit about this.

Also, whenever we do this, let's try to do it early in a release cycle to catch issues early on.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153658