[libc++] Add _LIBCPP_TEMPLATE_VIS to __vector_base_common

[thomasanderson]

Summary

I built libc++ using gcc with -fvisibility=hidden, but when linking my app against the resulting libc++.so, I got this error:

../../buildtools/third_party/libc++/trunk/include/vector:964: error: undefined reference to 'std::1::vector_base_common<true>::__throw_length_error() const'

While this issue is also fixed by D35388, this CL tries to match the behavior to __string_base_common:
https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/include/string;307970$560

[smeenai]

This doesn't look right to me. _LIBCPP_TEMPLATE_VIS expands to __attribute__((__type_visibility__("default"))) on compilers where __type_visibility__ is supported; i.e., its intent is to only export the typeinfo and vtable. gcc doesn't support this attribute, so we'll use __visibility__ instead and this change will appear to fix your issue, but it's not conceptually correct.

All the types you're seeing link errors against have extern template instantiation declarations in the libc++ headers (and the corresponding instantiation definitions in the library itself). I think we may have a bug in our definition of _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS. Can you try applying P8003 and let me know if that fixes the link errors?

[thomasanderson]

In D35326#808181, @smeenai wrote:

This doesn't look right to me. _LIBCPP_TEMPLATE_VIS expands to __attribute__((__type_visibility__("default"))) on compilers where __type_visibility__ is supported; i.e., its intent is to only export the typeinfo and vtable. gcc doesn't support this attribute, so we'll use __visibility__ instead and this change will appear to fix your issue, but it's not conceptually correct.

All the types you're seeing link errors against have extern template instantiation declarations in the libc++ headers (and the corresponding instantiation definitions in the library itself). I think we may have a bug in our definition of _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS. Can you try applying P8003 and let me know if that fixes the link errors?

That patch fixes the vector case but unfortunately operator+ for string is still not getting exported:
../../test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:580: error: undefined reference to 'std::1::basic_string<char, std::1::char_traits<char>, std::1::allocator<char> > std::1::operator+<char, std::1::char_traits<char>, std::1::allocator<char> >(char const*, std::1::basic_string<char, std::1::char_traits<char>, std::__1::allocator<char> > const&)'

[smeenai]

Does P8004 fix operator+?

[thomasanderson]

In D35326#808618, @smeenai wrote:

Does P8004 fix operator+?

Yes it does :)

[smeenai]

Perfect. I'll throw up some patches.

[smeenai]

Hmm, what version of gcc are you using? https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst;307951$86-90 suggests this may have changed recently.

[thomasanderson]

In D35326#808713, @smeenai wrote:

Hmm, what version of gcc are you using? https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst;307951$86-90 suggests this may have changed recently.

I'm using 4.8.4

[smeenai]

Hmm, that's pretty ancient (and I would be surprised if that changed in newer versions). Lemme play with this some more.

[thomasanderson]

Also, string has a class almost identical to __vector_base_common:
https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/include/string;307970$560

But currently string has _LIBCPP_TEMPLATE_VIS and vector does not. Maybe at least one of them is wrong?

[smeenai]

r307966 fixes string operator+, and D35388 fixes __vector_base_common.

[thomasanderson]

In D35326#808870, @smeenai wrote:

r307966 fixes string operator+, and D35388 fixes __vector_base_common.

Awesome, thanks for the quick fixes!

[smeenai]

In D35326#808855, @thomasanderson wrote:

Also, string has a class almost identical to __vector_base_common:
https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/include/string;307970$560

But currently string has _LIBCPP_TEMPLATE_VIS and vector does not. Maybe at least one of them is wrong?

Hmm.

I feel that __string_base_common shouldn't be marked _LIBCPP_TEMPLATE_VIS. In the clang world, _LIBCPP_TEMPLATE_VIS marks the vtable and typeinfo with default visibility. The class won't have a vtable since it doesn't have any virtual functions. Typeinfo visibility should only matter if you're throwing types across dynamic library boundaries, but since __string_base_common is an internal type, that shouldn't be an issue anyway. @EricWF can weigh in on that when he sees this though.

[smeenai]

In D35326#808884, @thomasanderson wrote:

In D35326#808870, @smeenai wrote:

r307966 fixes string operator+, and D35388 fixes __vector_base_common.

Awesome, thanks for the quick fixes!

I'm just glad someone else is also attempting to use libc++ with hidden visibility :)

[smeenai]

@thomasanderson you could re-upload this diff with just the include/vector change, and then @EricWF can decide if that's the right thing to do.

[smeenai]

You probably wanna update the summary. We also generally recommend uploading patches with more context (pass -U9999 to your git commands), though it doesn't make much difference over here.

[thomasanderson]

pinging EricWF
this CL is needed to fix ../../buildtools/third_party/libc++/trunk/include/vector:970: error: undefined reference to 'std::__1::__vector_base_common<true>::__throw_length_error() const'
https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.v8%2Fv8_linux64_gcc_compile_dbg%2F10118%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

include/vector

 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <bool>
-class __vector_base_common
+class _LIBCPP_TEMPLATE_VIS __vector_base_common
 {
 protected:
     _LIBCPP_ALWAYS_INLINE __vector_base_common() {}