[libc++] Give extern templates default visibility on gcc

[smeenai]

Summary

Contrary to the current visibility macro documentation, it appears that
gcc does handle visibility attribute on extern templates correctly, e.g.
https://godbolt.org/g/EejuV7. We need this so that extern template
instantiations of classes not marked _LIBCPP_TEMPLATE_VIS (e.g.
__vector_base_common) are correctly exported with gcc when building with
hidden visibility.

[thomasanderson]

Friendly ping for @EricWF

[thomasanderson]

Hm.. is there anyone else who might be able to review?

[EricWF]

Hmm. So the documentation is wrong, but not "fully wrong". If you simply try applying this patch and compiling with GCC you'll still encounter a bunch of instances of warning: type attributes ignored after type is already defined [-Wattributes].

The warning occurs when the class template specialization named in the extern template declaration has already been defined (possibly by an implicit instantiation). Ex. https://godbolt.org/g/DLgxhe

#define VIS(Str) __attribute__((__visibility__(Str)))
template <class T> class VIS("hidden") T { void foo(); };
T<int> t; // cause definition of T<int>. 
template <> void T<float>::foo() {} // causes definition?
extern template class VIS("default") T<void>; // OK not defined yet.
extern template class VIS("default") T<int>: // Not OK, T<int> is already defined

This case occurs multiple times in <locale> where the extern template declaration occurs after we declare an explicit specialization of a member function. It appears we need to do more to fix this than simply enable the visibility macro with GCC.
Likely either by ensuring the extern template decl occurs before anything that causes an implicit instantiation (but I'm not sure if that's allow, maybe member specializations have to occur first?), or by adding yet another macro to specially handle the <locale> case et al. with GCC.

[smeenai]

Good point. It looks like clang actually ignores the attributes in that case as well; it just doesn't warn you about it :D

Take a look at https://godbolt.org/g/CJTD6c to see what I mean. Note the .hidden c<int>::f() in both the gcc and clang outputs.

[smeenai]

Filed https://bugs.llvm.org/show_bug.cgi?id=34614 about the silent attribute ignoring.

[EricWF]

Adding @ldionne as an observer, since he's knee-deep in the visibility mess right now.

After re-evaluating, I think I was being overly cautious the last time I looked at this. I think this patch should be acceptable ABI wise. But I would appreciate a second opinion.

[ldionne]

ABI-wise, I think this change is OK. Indeed, if __type_visibility__ is not supported, we're already marking the base template as __visibility__("default"), so marking the extern template declaration with __visibility__("default") is not a problem. If __type_visibility__ is supported, then there's no change in behavior. So I think there is no change in behavior either way, and I'm actually wondering why @thomasanderson wants this change in if the behavior is always the same. Perhaps I am missing something?

As a diversion, I'm confused by the very fact that we need to apply _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS to the _extern template declaration_. I would have expected instead that, if anything, we need to specify the visibility on the _explicit instantiation_ in the dylib.

[thomasanderson]

In D35388#1167305, @ldionne wrote:

ABI-wise, I think this change is OK. Indeed, if __type_visibility__ is not supported, we're already marking the base template as __visibility__("default"), so marking the extern template declaration with __visibility__("default") is not a problem. If __type_visibility__ is supported, then there's no change in behavior. So I think there is no change in behavior either way, and I'm actually wondering why @thomasanderson wants this change in if the behavior is always the same. Perhaps I am missing something?

It's been a while, but IIRC this fixes the Chromium build with gcc and libc++, since std::vector_base_common<true>::__throw_length_error() wasn't getting exported otherwise.

See the comment in the Chromium source code:
https://cs.chromium.org/chromium/src/buildtools/third_party/libc%2B%2B/BUILD.gn?rcl=0dd5c6f980d22be96b728155249df2da355989d9&l=88

And also this CL https://reviews.llvm.org/D35326

As a diversion, I'm confused by the very fact that we need to apply _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS to the _extern template declaration_. I would have expected instead that, if anything, we need to specify the visibility on the _explicit instantiation_ in the dylib.

[ldionne]

In D35388#1167315, @thomasanderson wrote:

In D35388#1167305, @ldionne wrote:

ABI-wise, I think this change is OK. Indeed, if __type_visibility__ is not supported, we're already marking the base template as __visibility__("default"), so marking the extern template declaration with __visibility__("default") is not a problem. If __type_visibility__ is supported, then there's no change in behavior. So I think there is no change in behavior either way, and I'm actually wondering why @thomasanderson wants this change in if the behavior is always the same. Perhaps I am missing something?

It's been a while, but IIRC this fixes the Chromium build with gcc and libc++, since std::vector_base_common<true>::__throw_length_error() wasn't getting exported otherwise.

Ah, well of course! That's because I assumed that _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS was always used on extern template declarations where the base template had been declared with _LIBCPP_TEMPLATE_VIS, which isn't true for __vector_base_common.

See the comment in the Chromium source code:
https://cs.chromium.org/chromium/src/buildtools/third_party/libc%2B%2B/BUILD.gn?rcl=0dd5c6f980d22be96b728155249df2da355989d9&l=88

And also this CL https://reviews.llvm.org/D35326

I would think that both this change and https://reviews.llvm.org/D35326 are good.

[thakis]

I would think that both this change and https://reviews.llvm.org/D35326 are good.

What's the status here and in D35326? We still have a workaround in chromium's build files pointing at these two changes.

[smeenai]

Sorry, I seem to have somehow blanked out on this diff entirely. Is it still relevant?

[thomasanderson]

We appear to still be carrying a patch for this in Chromium, so I think the issue still stands.
https://source.chromium.org/chromium/chromium/src/+/master:buildtools/third_party/libc++/BUILD.gn;drc=21272efa27e69622c6d174f29e4a73f1a6088cfc;l=135

[ldionne]

Can you rebase onto main and re-upload the diff? It will trigger CI. If the CI passes, this is good to go.

[smeenai]

Rebase

[smeenai]

It's been a long time since I've contributed to libc++, and the pre-merge CI setup is a massive improvement over what we had before. A huge kudos to everyone who made it possible :)

This is still showing up as Needs Review. I'm not sure if that's because of @EricWF's prior "Needs Revision" or because it needs to be explicitly accepted as the libc++ group (or some combination of the two).

[ldionne]

In D35388#2493696, @smeenai wrote:

It's been a long time since I've contributed to libc++, and the pre-merge CI setup is a massive improvement over what we had before. A huge kudos to everyone who made it possible :)

I'm glad this is helpful! It's a huge time saver for me so far.

This is still showing up as Needs Review. I'm not sure if that's because of @EricWF's prior "Needs Revision" or because it needs to be explicitly accepted as the libc++ group (or some combination of the two).

It must be because of Eric's "Needs Revision", because I accepted as libc++. I'll remove Eric as a reviewer to fix this (Eric, if you see this, it's not against you!).

libcxx/docs/DesignDocs/VisibilityMacros.rst

**_LIBCPP_EXTERN_TEMPLATE_TYPE_VIS**
   Mark the member functions, typeinfo, and vtable of the type named in
   a `_LIBCPP_EXTERN_TEMPLATE` declaration as being exported by the libc++ library.
   This attribute must be specified on all extern class template declarations.
 
   This macro is used to override the `_LIBCPP_TEMPLATE_VIS` attribute
   specified on the primary template and to export the member functions produced
   by the explicit instantiation in the dylib.
 
-  **GCC Behavior**: GCC ignores visibility attributes applied the type in
-  extern template declarations and applying an attribute results in a warning.
-  However since `_LIBCPP_TEMPLATE_VIS` is the same as
-  `__attribute__((visibility("default"))` the visibility is already correct.
-  The macro has an empty definition with GCC.
-
   **Windows Behavior**: `extern template` and `dllexport` are fundamentally
   incompatible *on a class template* on Windows; the former suppresses
   instantiation, while the latter forces it. Specifying both on the same
   declaration makes the class template be instantiated, which is not desirable
   inside headers. This macro therefore expands to `dllimport` outside of libc++
   but nothing inside of it (rather than expanding to `dllexport`); instead, the
   explicit instantiations themselves are marked as exported. Note that this
   applies *only* to extern *class* templates. Extern *function* templates obey

libcxx/include/__config

 #  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) && __has_attribute(__type_visibility__)
 #    define _LIBCPP_ENUM_VIS __attribute__ ((__type_visibility__("default")))
 #  else
 #    define _LIBCPP_ENUM_VIS
 #  endif
 #endif
 
 #ifndef _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
-#  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) && __has_attribute(__type_visibility__)
+#  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 #    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __attribute__ ((__visibility__("default")))
 #  else
 #    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
 #  endif
 #endif
 
 #ifndef _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 #define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS