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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D153658#4612457 <https://reviews.llvm.org/D153658#4612457>, @EricWF wrote:

> In D153658#4599282 <https://reviews.llvm.org/D153658#4599282>, @ldionne wrote:
>
>> This LGTM with passing CI, updated commit message and the release note.
>>
>> For most people who compile with `-fvisibility=default` (or nothing at all), this is a no-op since everything in the library already had default visibility. For users who build with `-fvisibility=hidden`, there used to be a bunch of types that we forgot to apply `type_visibility(default)` or `visibility(default)` to, and those would previously be hidden. After this change, their type_visibility would become `default` (as it should) on Clang (but not on GCC).
>
> I don't think this change was thought through.
>
> [...]
>
> Based on the comments in this change, it's not clear to me you considered the most common use cases for `-fvisibility=hidden`.

We did definitely think through this change and consider the effects this would have on shared libraries. It's not impossible that we made a mistake somewhere, but at least we discussed it for hours and I still think that this patch is definitely correct (and the behavior we want). Let's talk to figure this out.

> - This is a major issue for people who build and ship their own shared libraries.

I think this is actually the behavior they want/need for their code to be correct, see below.

> - Iterating over long symbol lists is an issue, and can cause really yucky hash collisions in the dynamic linker loader.
> - This isn't just about types. It's about functions. And now every single weak function template that's instantiated as part of a users library build

`__type_visibility__` only affects the visibility of the RTTI and the vtable of the type, doesn't it? I don't think it has any effect on the functions inside the namespace (otherwise yes this would be a completely different can of worms). I checked on Godbolt and it seemed to confirm this: https://godbolt.org/z/z315e79cf

> Could you explain to me what affect you believe this change has on other shared libraries?

As explained in https://reviews.llvm.org/D153658#4599282:

> For most people who compile with `-fvisibility=default` (or nothing at all), this is a no-op since everything in the library already had default visibility. For users who build with `-fvisibility=hidden`, there used to be a bunch of types that we forgot to apply `type_visibility(default)` or `visibility(default)` to, and those would previously be hidden. After this change, their `type_visibility` would become default (as it should) on Clang (but not on GCC).

I think we agree that this is a no-op for people building with nothing at all or with `-fvisibility=default`, since they already export everything.

Let's consider users of `-fvisibility=hidden` (let's say they are building a shared library). For those, there used to be a bunch of types that we did not apply any visibility attribute to (I'll argue that this is just an oversight). As a result, the RTTI of these types would **not** be exported from the user's dylib. For example, let's say someone had `filter_view<V, Pred> view{...}` in their code. Since we don't have any visibility markup on that type (oops, I think that one is on me), that type's vtable+RTTI would get hidden visibility if people are building with `-fvisibility=hidden`. Do we agree on that?

If so, then do we also agree that if they did something like `throw view;` from within the dylib boundary and then someone tried to catch it as `catch (std::filter_view<V, Pred>)` from outside the dylib boundary, it wouldn't work? That's because the RTTI attached to the `filter_view` is local to the `-fvisibility=hidden` dylib, and it's never exported from it. This shouldn't be a common use case, however something much more common would be e.g. passing a `std::any` containing a `std::filter_view` across an ABI boundary built with `-fvisibility=hidden` and then trying to retrieve it on the other side, or anything else that relates to RTTI.

This is basically a bug, right? If we used `_LIBCPP_TEMPLATE_VIS` and the other `_FOO_VIS` with 100% consistency, this would not be an issue but then applying `__type_visibility__` to the namespace would be a no-op. But since we don't apply `_FOO_VIS` with 100% consistency, this basically fixes a bug where some types have incorrect visibility. Yes, it will slightly increase the number of symbols exported on dylib boundaries, but the symbols that are newly exported are going to be vtables/RTTIs of types that **should always have been exported** for correctness.

Let's see where/if we actually disagree or if I said something untrue above. We definitely considered the issue very closely and actually spent hours discussing this, and I explicitly requested to delay this change to after the release (and to land it early in the 18 release cycle) because I know how tricky these types of changes can be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153658