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.
Details
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG3583bf3ad8c5: [libc++] Make everything in namespace std have default type visibility and…
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/docs/DesignDocs/VisibilityMacros.rst | ||
|---|---|---|
| 1 | We need to document the idea behind this change in this documentation. | |
| libcxx/include/__config | ||
| 717 | 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. | |
| 800–802 | This change will have the following non-trivial effects:
Let's document that in the commit message (and also in a release note if we foresee user impact). | |
Comment Actions
Address comments
| libcxx/include/__config | ||
|---|---|---|
| 800–802 |
| |
| libcxx/include/__config | ||
|---|---|---|
| 800–802 | (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. | |
Comment Actions
Trying to summarize our long discussion just now: I think there are two changes in here.
- 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).
- 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.
Comment Actions
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. | |
Comment Actions
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 | A lot of this is incorrect. -fvisibility-hidden is for when your | |
| libcxx/docs/ReleaseNotes/18.rst | ||
|---|---|---|
| 82 | -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. | |
Comment Actions
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.
Comment Actions
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 .
The commit message needs to be updated!