This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make everything in namespace std have default type visibility and hidden visibility and remove _LIBCPP_ENUM_VIS
ClosedPublic

Authored by philnik on Jun 23 2023, 12:00 PM.

Details

Summary

This avoids having to add _LIBCPP_ENUM_VIS, since that is handled through type_visibility and GCC always makes the visibility of enums default. It also fixes and missing _LIBCPP_EXPORTED_FROM_ABI on classes when using Clang.

Diff Detail

Event Timeline

philnik created this revision.Jun 23 2023, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 12:00 PM
philnik requested review of this revision.Jun 23 2023, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 12:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Jun 28 2023, 8:33 AM
libcxx/docs/DesignDocs/VisibilityMacros.rst
1

We need to document the idea behind this change in this documentation.

libcxx/include/__config
716

Can you add a comment explaining why this is necessary? IIUC, this is only necessary because GCC doesn't implement __type_visibility__ -- otherwise we would be able to drop this macro entirely.

799–801

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).

philnik updated this revision to Diff 535502.Jun 28 2023, 1:28 PM
philnik marked 2 inline comments as done.

Address comments

libcxx/include/__config
799–801
  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.
  1. 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.
ldionne added inline comments.Jul 12 2023, 8:47 AM
libcxx/include/__config
799–801

(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.

Trying to summarize our long discussion just now: I think there are two changes in here.

  1. Apply type_visibility(default) on namespace std. This fixes an existing issue where we most likely are missing some type_visibility(default) on types, which means that some types might not work properly with std::any and anything related to RTTI, basically (which includes exceptions). This change does have a change of behavior, but it makes things "less strict" in a way, i.e. more code will work than today. Applying type_visibility(default) on the namespace also has the side effect that _LIBCPP_ENUM_VIS is not required anymore, so we can clean that up if we want (potentially even as a separate patch, IDK).
  2. The second change is applying _LIBCPP_HIDDEN on namespace std. This will have the effect that we stop exporting some functions that we basically forgot to mark as hidden, which we arguably want. However, we also have to decide what to do with a bunch of other entities, such as the _v variable templates. For those, we have to decide whether we want to keep exporting them or not, and that requires guidance from the Committee (especially since WG21 went through the trouble of making these inline variables). If the conclusion is that we need to keep exporting them, then we can either drop this change, or mark namespace std as hidden but then also apply an explicit default visibility to the things we want to export. Those are different approaches and we can discuss them on a patch that tackles just that side of the problem. Also note that we don't necessarily have to wait for WG21 to apply hidden to the namespace. The WG21 guidance is only required to know whether we could drop the manual EXPORT_FROM_ABI annotation from these _v entities if we made namespace std have hidden visibility.
philnik updated this revision to Diff 551274.Aug 17 2023, 2:27 PM
  • Rebase
  • Only add type_visibility("default") to the namespace
ldionne accepted this revision.Aug 18 2023, 9:04 AM

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).

libcxx/.clang-format
25

The commit message needs to be updated!

libcxx/docs/DesignDocs/VisibilityMacros.rst
3

Release note:

This release of libc++ added missing visibility annotations on some types in the library. Users compiling with -fvisbility=hidden may notice that additional type infos from libc++ are being exported from their ABI. This is the correct behavior in almost all cases since exporting the RTTI is required for these types to work properly with dynamic_cast, exceptions and other mechanisms. However, if you intend to use libc++ purely as an internal implementation detail (i.e. you use libc++ as a static archive and never export libc++ symbols from your ABI) and you notice changes to your exported symbols list, then this means that you were not properly preventing libc++ symbols from being part of your ABI.

This revision is now accepted and ready to land.Aug 18 2023, 9:04 AM
philnik edited the summary of this revision. (Show Details)Aug 18 2023, 1:44 PM
philnik updated this revision to Diff 551629.Aug 18 2023, 1:49 PM
philnik marked 4 inline comments as done.
philnik edited the summary of this revision. (Show Details)

Address comments

EricWF added a subscriber: EricWF.Aug 23 2023, 7:37 PM

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.

  • This is a major issue for people who build and ship their own shared libraries.
  • 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

Based on the comments in this change, it's not clear to me you considered the most common use cases for -fvisibility=hidden.
Could you explain to me what affect you believe this change has on other shared libraries?

libcxx/docs/ReleaseNotes/18.rst
82 ↗(On Diff #551791)

A lot of this is incorrect.

-fvisibility-hidden is for when your

EricWF added inline comments.Aug 23 2023, 7:39 PM
libcxx/docs/ReleaseNotes/18.rst
82 ↗(On Diff #551791)

-fvisibility=default causes all symbols to be exported from any dylib they're emitted in (and for templates, that's every TU).

Dynamic cast and friends only matter when you're trying to do it across shared library boundaries, which is very rare. Otherwise having duplicate vtables/type infos isn't a big deal.

Further, I don't understand what ". However, if you intend to use libc++ purely as an

internal implementation detail (i.e. you use libc++ as a static archive and never export libc++ symbols from your ABI)
and you notice changes to your exported symbols list, then this means that you were not properly preventing libc++
symbols from being part of your ABI." is actually talking about.

Address comments
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.

This is easy to test. Was that done?

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.

I have to apologize for my previous comment.

When I was initially testing this change, I made some unknown mistake which make it appear as though there had been a 3x symbol increase in a test shared library.
However, when I went to reproduce, I couldn't replicate the result and indeed there was almost no movement in the amount of symbols.

I'm sorry. I spoke too soon and too strongly. I will work to improve my communication .