Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] After 3583bf3ad8c5, building Firefox results in hidden symbol errors #79027

Open
DimitryAndric opened this issue Jan 22, 2024 · 9 comments
Assignees
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. quality-of-implementation

Comments

@DimitryAndric
Copy link
Collaborator

DimitryAndric commented Jan 22, 2024

I'm investigating a build breakage of Firefox with very recent libc++ (as of llvmorg-18-init-2850-g3583bf3), on FreeBSD 15-CURRENT. What happens is that most of the build goes well, until it tries to link its main shared library, libxul.so:

toolkit/library/build/libxul.so
rm -f libxul.so
clang++ -std=gnu++17 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DLIBICONV_PLUG -isystem /usr/local/include -fno-sized-deallocation -fno-aligned-new -O2 -pipe -O3 -DLIBICONV_PLUG -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing -DLIBICONV_PLUG -isystem /usr/local/include -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -O2 -O3 -fno-omit-frame-pointer -funwind-tables -shared -Wl,-z,defs -Wl,--warn-unresolved-symbols -Wl,--gc-sections -Wl,-h,libxul.so -o libxul.so /wrkdirs/usr/ports/www/firefox/work/.build/toolkit/library/build/libxul_so.list    -pthread -Wl,--as-needed -fstack-protector-strong -fuse-ld=lld -Wl,-z,noexecstack -Wl,-z,text -Wl,-z,relro -Wl,-z,nocopyreloc -Wl,-Bsymbolic-functions -Wl,--build-id=sha1 -fstack-protector-strong -fstack-clash-protection -Wl,-rpath-link,/wrkdirs/usr/ports/www/firefox/work/.build/dist/bin -Wl,-rpath-link,/usr/local/lib  ../../../js/src/build/libjs_static.a ../../../build/pure_virtual/libpure_virtual.a /wrkdirs/usr/ports/www/firefox/work/.build/x86_64-unknown-freebsd/release/libgkrust.a ../../../config/external/gkcodecs/libgkcodecs.so ../../../config/external/lgpllibs/liblgpllibs.so ../../../config/external/sqlite/libmozsqlite3.so ../../../widget/gtk/mozgtk/libmozgtk.so ../../../widget/gtk/mozwayland/libmozwayland.so -Wl,--version-script,libxul.so.symbols  -L/usr/local/lib -licui18n -L/usr/local/lib -licuuc -licudata -laom -ldav1d -lX11 -lXcomposite -lXdamage -lXext -lXfixes -lXrandr -lXrender -lpthread -lffi -lplds4 -lplc4 -lnspr4 -pthread -ldl -L/lib -lz -lm -lnss3 -lsmime3 -lssl3 -lnssutil3 -lfreetype -lfontconfig -lgdk-3 -lharfbuzz -lpangocairo-1.0 -lpango-1.0 -lgtk-3 -latk-1.0 -lcairo -lcairo-gobject -lgdk_pixbuf-2.0 -lglib-2.0 -lintl -lgobject-2.0 -lgio-2.0 -lutil -lpng16 -lwebp -lwebpdemux -lgraphite2 -levent -lvpx -lpixman-1 -ldbus-1 -lxcb-shm -lX11-xcb -lxcb -lXcursor -lXi
ld.lld: error: undefined hidden symbol: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::operator=(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)
>>> referenced by Unified_cpp_xpcom_build0.cpp
>>>               /wrkdirs/usr/ports/www/firefox/work/.build/toolkit/library/build/../../../xpcom/build/Unified_cpp_xpcom_build0.o:(NS_InitXPCOM)
>>> referenced by Unified_cpp_xpcom_build0.cpp
>>>               /wrkdirs/usr/ports/www/firefox/work/.build/toolkit/library/build/../../../xpcom/build/Unified_cpp_xpcom_build0.o:(NS_InitXPCOM)
>>> referenced by DataChannel.cpp
>>>               /wrkdirs/usr/ports/www/firefox/work/.build/toolkit/library/build/../../../netwerk/sctp/datachannel/DataChannel.o:(mozilla::DataChannelConnection::SetSignals(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&))
>>> referenced 275 more times

ld.lld: error: undefined hidden symbol: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::append(char const*, unsigned long)
>>> referenced by Unified_cpp_protocol_http4.cpp
>>>               /wrkdirs/usr/ports/www/firefox/work/.build/toolkit/library/build/../../../netwerk/protocol/http/Unified_cpp_protocol_http4.o:(mozilla::net::nsHttpConnectionInfo::BuildHashKey())
>>> referenced by Unified_cpp_ipc_chromium0.cpp
>>>               /wrkdirs/usr/ports/www/firefox/work/.build/toolkit/library/build/../../../ipc/chromium/Unified_cpp_ipc_chromium0.o:(FilePath::Append(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const)
>>> referenced by Unified_cpp_ipc_chromium0.cpp
>>>               /wrkdirs/usr/ports/www/firefox/work/.build/toolkit/library/build/../../../ipc/chromium/Unified_cpp_ipc_chromium0.o:(base::BuildEnvironmentArray(std::__1::map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>> const&))
>>> referenced 353 more times
... many more of these ...

After much digging, the reason for these symbols being hidden turns out to be a little trick Mozilla uses to limit the amount of visible symbols in libxul.so: the file https://github.com/mozilla/gecko-dev/blob/master/config/gcc_hidden.h which gets included in front of every .cpp file, using -include /wrkdirs/usr/ports/www/firefox/work/firefox-122.0/config/gcc_hidden.h.

The file contains nothing but a #pragma GCC visibility push(hidden) line, so that everything following is hidden, including libc++ headers such as <string>, <vector>, etc.

Before commit 3583bf3, this made no difference since most of the templates used in libc++ headers are prefixed with _LIBCPP_TEMPLATE_VIS, and this expanded to either __attribute__((__type_visibility__("default"))) (for clang) or __attribute__((__visibility__("default"))) (for gcc), which would override the #pragma GCC visibility push(hidden) state.

After commit 3583bf3 however, this no longer happens: _LIBCPP_TEMPLATE_VIS is defined empty if clang is used, and whatever visibility is "active" due to such a pragma is used for most templates in the libc++ headers.

So this example shared library:

#pragma GCC visibility push(hidden)

#include <string>

std::string foo(char const* s, size_t len)
{
  std::string t("bar");
  return t.append(s, len);
}

Will not compile and link anymore with libc++ 18:

$ clang++ -fPIC hidden-string.cpp -o hidden-string.so -shared
ld: error: undefined hidden symbol: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::append(char const*, unsigned long)
>>> referenced by hidden-string.cpp
>>>               /tmp/hidden-string-983b67.o:(foo(char const*, unsigned long))
>>> did you mean: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::append(char const*)
>>> defined in: /lib/libc++.so.1

ld: error: undefined hidden symbol: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::basic_string(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)
>>> referenced by hidden-string.cpp
>>>               /tmp/hidden-string-983b67.o:(foo(char const*, unsigned long))
>>> did you mean: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::basic_string(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)
>>> defined in: /lib/libc++.so.1

ld: error: undefined hidden symbol: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::~basic_string()
>>> referenced by hidden-string.cpp
>>>               /tmp/hidden-string-983b67.o:(foo(char const*, unsigned long))
>>> referenced by hidden-string.cpp
>>>               /tmp/hidden-string-983b67.o:(foo(char const*, unsigned long))

ld: error: undefined hidden symbol: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::__init(char const*, unsigned long)
>>> referenced by hidden-string.cpp
>>>               /tmp/hidden-string-983b67.o:(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::basic_string[abi:se180000]<0>(char const*))
c++: error: linker command failed with exit code 1 (use -v to see invocation)

Now I am aware that Firefox could use -fvisibility=hidden and maybe also -fvisibility-inlines-hidden to achieve the same effect, and that does indeed seem to work, at least for the above simple test case.

But before I go to Mozilla and report this as a bug in their tracker, I would like to have an official word from the libc++ maintainers about whether this whole use case is supposed to work at all.

That is: can you do #pragma GCC visibility push(hidden) (or its clang equivalent, if it exists), and then expect to include any libc++ headers without problems? If so, commit 3583bf3 would have to be adjusted to make it possible, since it no longer works now.

Or: is this kind of messing with visibility around libc++ headers officially unsupported, and would applications attempting to use such methods have to be adjusted?

@DimitryAndric DimitryAndric added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. quality-of-implementation labels Jan 22, 2024
@philnik777
Copy link
Contributor

Yes, this should work. However, you are incorrect that the symbols just uses "whatever is currently active". The explicit instantiation has a _LIBCPP_EXPORTED_FROM_ABI on it and the namespace std has a _LIBCPP_TYPE_VISIBILITY_DEFAULT. This looks to me like a clang bug, especially since you say that using the command line version works fine.

@DimitryAndric
Copy link
Collaborator Author

DimitryAndric commented Jan 22, 2024

Yes, this should work. However, you are incorrect that the symbols just uses "whatever is currently active". The explicit instantiation has a _LIBCPP_EXPORTED_FROM_ABI on it and the namespace std has a _LIBCPP_TYPE_VISIBILITY_DEFAULT. This looks to me like a clang bug, especially since you say that using the command line version works fine.

Hmm, is that so? In <string> I see for instance:

  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s, size_type __n);
  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s);
  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(size_type __n, value_type __c);

in the basic_string class declaration, while the later inline implementation has:

// append

template <class _CharT, class _Traits, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string<_CharT, _Traits, _Allocator>&
basic_string<_CharT, _Traits, _Allocator>::append(const value_type* __s, size_type __n) {
...

I see _LIBCPP_EXPORTED_FROM_ABI only for operator+ and a bunch of non-member functions like the to_string() variants. Am I missing something? :)

@philnik777
Copy link
Contributor

Yes, this should work. However, you are incorrect that the symbols just uses "whatever is currently active". The explicit instantiation has a _LIBCPP_EXPORTED_FROM_ABI on it and the namespace std has a _LIBCPP_TYPE_VISIBILITY_DEFAULT. This looks to me like a clang bug, especially since you say that using the command line version works fine.

Hmm, is that so? In <string> I see for instance:

  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s, size_type __n);
  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(const value_type* __s);
  _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& append(size_type __n, value_type __c);

in the basic_string class declaration, while the later inline implementation has:

// append

template <class _CharT, class _Traits, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string<_CharT, _Traits, _Allocator>&
basic_string<_CharT, _Traits, _Allocator>::append(const value_type* __s, size_type __n) {
...

I see _LIBCPP_EXPORTED_FROM_ABI only for operator+ and a bunch of non-member functions like the to_string() variants. Am I missing something? :)

Yes, you are. The explicit instantiations are listed in __string/extern_template_lists.h. If it wasn't explicitly instantiated, the compiler would have emitted a weak definition for the symbols.

@ldionne
Copy link
Member

ldionne commented Jan 23, 2024

This should work, but I do agree this seems like a Clang bug since we should still be applying the required visibility attribute on everything that we externally instantiate. I think we'll need to reduce your test case.

@DimitryAndric
Copy link
Collaborator Author

I let cvise do some brute-forcing of the preprocessed version of the above testcase, and it came up with the following:

// clang++ -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -shared hidden-append.cpp -o hidden-append-works.so
// clang++ -DPUSH_HIDDEN                                   -fPIC -shared hidden-append.cpp -o hidden-append-fails.so

#ifdef PUSH_HIDDEN
#pragma GCC visibility push(hidden)
#endif

namespace std {
inline namespace __1 {
template <class _Tp> using __make_unsigned_t = __make_unsigned(_Tp);
template <class> struct make_unsigned {
  using type = __make_unsigned_t<long>;
};
template <class> struct char_traits;
template <class> class allocator;
template <class _CharT, class = char_traits<_CharT>, class = allocator<_CharT>>
struct basic_string {
  void append(const char *, make_unsigned<long>::type);
};
} // namespace __1
} // namespace std
void foo(std::basic_string<char> &out, char const *in, unsigned long len) {
  out.append(in, len);
}
  • clang++ -fPIC -shared hidden-append.cpp -o hidden-append.so -> works
  • clang++ -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -shared hidden-append.cpp -o hidden-append.so -> works
  • clang++ -DPUSH_HIDDEN -fPIC -shared hidden-append.cpp -o hidden-append.so -> fails

But I'm not sure if this may be too far reduced?

@MaskRay
Copy link
Member

MaskRay commented Jan 25, 2024

I let cvise do some brute-forcing of the preprocessed version of the above testcase, and it came up with the following:

[...]

  • clang++ -fPIC -shared hidden-append.cpp -o hidden-append.so -> works
  • clang++ -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -shared hidden-append.cpp -o hidden-append.so -> works
  • clang++ -DPUSH_HIDDEN -fPIC -shared hidden-append.cpp -o hidden-append.so -> fails

But I'm not sure if this may be too far reduced?

The linker failure is due to an undefined hidden symbol, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::append(char const*, long) (after demangling).

I think Clang's behavior is correct and matches GCC (g++ -c -DPUSH_HIDDEN -fPIC -shared hidden-append.cpp; remove __make_undefined that GCC doesn't recognize).

In general, -fivisibility=hidden only applies to definitions while #pragma GCC visibility push(hidden) also applies on non-definition declarations.

(Clang's visibility has some minor issues related to template insantiations, https://reviews.llvm.org/D154774, but this is not that case.)

@ldionne
Copy link
Member

ldionne commented Jan 25, 2024

Does this mean that we basically can’t rely on the namespace-level attribute being inherited by declarations inside the namespace because a pragma outside our control could override it?

That seems inconvenient.

@ldionne
Copy link
Member

ldionne commented Jan 25, 2024

I do wonder whether that’s something we want to support. At the end of they day the user is setting a pragma and then including the standard library — to some extent we are doing what the user is asking.

@DimitryAndric
Copy link
Collaborator Author

It's a tricky question. I think that what Mozilla is doing is a Very Big Hammer, so maybe they should opt for something more subtle. On the other hand, they seem to have been getting away with this trick for a very long time, and the obvious reply to a bug on their tracker about this would be: "but libstdc++ has no problems with this" :) (And that is because libstdc++ marks the whole std namespace with __attribute__((__visibility__("default"))), probably.)

On the other hand, there is some text in gcc's Wiki doc about visibility (https://gcc.gnu.org/wiki/Visibility) where they mention:

#pragma GCC visibility is stronger than -fvisibility; it affects extern declarations as well. -fvisibility only affects definitions, so that existing code can be recompiled with minimal changes. This is more true for C than C++; C++ interfaces tend use classes, which are affected by -fvisibility.

I think that "affects extern declarations as well" is the key here, and clang is behaving the same way: using a hidden visibility pragma before including anything is force-hiding everything that not explicitly says it is non-hidden. The biggest hammer, maybe questionable if it is worth supporting.

That said, I am unsure if clang has behaved differently in the past with regards to the -fvisibility and visibility pragmas. After all, #pragma GCC was always a gcc specific extension...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. quality-of-implementation
Projects
None yet
Development

No branches or pull requests

4 participants