HomePhabricator

[libc++] Explicitly enumerate std::string external instantiations.
Concern Raised61bd19206f61

Authored by EricWF on Thu, Jan 9, 12:50 PM.

Description

[libc++] Explicitly enumerate std::string external instantiations.

The external instantiation of std::string is a problem for libc++.

Additions and removals of inline functions in string can cause ABI
breakages, including introducing new symbols.

This patch aims to:
  (1) Make clear which functions are explicitly instatiated.
  (2) Prevent new functions from being accidentally instantiated.
  (3) Allow a migration path for adding or removing functions from the
  explicit instantiation over time.

Although this new formulation is uglier, it is preferable from a
maintainability and readability standpoint because it explicitly
enumerates the functions we've chosen to expose in our ABI. Changing
this list is non-trivial and requires thought and planning.

(3) is achieved by making it possible to control the extern template declaration
separately from it's definition. Meaning we could add a new definition to
the dylib, wait for it to roll out, then add the extern template
declaration to the header. Similarly, we could remove existing extern
template declarations while still keeping the definition to prevent ABI
breakages.

Event Timeline

shafik added a subscriber: shafik.EditedThu, Jan 9, 5:25 PM

Eric, this is breaking the green dragon build bots for LLDB, see the first log here: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/5581/

We are still investigating but we may end up reverting if we can't come up with a quick fix.

Note, the tests are only failing for the modules build.

Hi, it seems that this commit also introduces libc++ headers that are incompatible with gcc 9.2:

[6883/56315] CXX user-x64-gcc.shlib/obj/system/ulib/fit/fit._sources.scope.cc.o
FAILED: user-x64-gcc.shlib/obj/system/ulib/fit/fit._sources.scope.cc.o
/b/s/w/ir/cache/goma/client/gomacc  ../../prebuilt/third_party/gcc/linux-x64/bin/x86_64-elf-g++ -MD -MF user-x64-gcc.shlib/obj/system/ulib/fit/fit._sources.scope.cc.o.d -o user-x64-gcc.shlib/obj/system/ulib/fit/fit._sources.scope.cc.o -DTOOLCHAIN_VERSION=jLZP6M4T00X5FDngWQkqwPzmQ5ztsfkdXK3yzCkzJIEC -D__Fuchsia__ -DZX_ASSERT_LEVEL=2 -DWITH_FRAME_POINTERS=1 -D_ALL_SOURCE -I/b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1 -I../../zircon/system/public -I../../zircon/system/ulib/fit/include -fno-common -fasynchronous-unwind-tables -mcx16 -march=x86-64 -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -O2 -g3 -Wall -Wextra -Wno-unused-parameter -Wno-address-of-packed-member -Wno-nonnull-compare -Wno-format-truncation -fno-omit-frame-pointer -ffunction-sections -fdata-sections -Wimplicit-fallthrough -fvisibility=hidden -Werror -Wno-error=deprecated-declarations -fPIC -idirafter ../../zircon/third_party/ulib/musl/include -Wno-maybe-uninitialized -std=c++17 -Wconversion -Wno-sign-conversion -Wextra-semi -Wno-deprecated-copy -ftemplate-backtrace-limit=0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -c ../../zircon/system/ulib/fit/scope.cc
In file included from /b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/atomic:549,
from ../../zircon/system/ulib/fit/include/lib/fit/scope.h:10,
from ../../zircon/system/ulib/fit/scope.cc:5:
/b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/string:1679:1: error: ambiguous template specialization '__ct <>' for 'std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> >::basic_string(const std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> >&)'
1679 | _LIBCPP_STRING_EXTERN_TEMPLATE_LIST(_LIBCPP_EXTERN_TEMPLATE, char)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/system_error:149,
from /b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/__mutex_base:15,
from /b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/mutex:190,
from ../../zircon/system/ulib/fit/include/lib/fit/scope.h:11,
from ../../zircon/system/ulib/fit/scope.cc:5:
/b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/string:799:5: note: candidates are: 'std::__2::basic_string<_CharT, _Traits, _Allocator>::basic_string(const std::__2::basic_string<_CharT, _Traits, _Allocator>&) [with _CharT = char; _Traits = std::__2::char_traits<char>; _Allocator = std::__2::allocator<char>]'
799 |     basic_string(const basic_string& __str);
|     ^~~~~~~~~~~~
/b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/string:853:18: note:                 'template<class _Tp, class> std::__2::basic_string<_CharT, _Traits, _Allocator>::basic_string(const _Tp&) [with _Tp = _Tp; <template-parameter-2-2> = <template-parameter-1-2>; _CharT = char; _Traits = std::__2::char_traits<char>; _Allocator = std::__2::allocator<char>]'
853 |         explicit basic_string(const _Tp& __t);
|                  ^~~~~~~~~~~~
In file included from /b/s/w/ir/k/fuchsia/prebuilt/third_party/clang/linux-x64/include/c++/v1/atomic:549,
from ../../zircon/system/ulib/fit/include/lib/fit/scope.h:10,
from ../../zircon/system/ulib/fit/scope.cc:5:

Ran a bisect to confirm this. Would you mind taking a look? Thanks.

mgorny raised a concern with this commit.Sat, Jan 11, 7:13 AM
mgorny added a subscriber: mgorny.
This commit now has outstanding concerns.Sat, Jan 11, 7:13 AM

There are also some test failures (link errors caused by undefined references) on the ARM/AArch64 bots caused by this, so I'm going to revert it.

http://lab.llvm.org:8011/builders/libcxx-libcxxabi-libunwind-aarch64-linux/builds/2628

FWIW we solved the LLDB problems.

hans added a subscriber: hans.Tue, Jan 14, 3:49 AM

This change also caused AppleClang 9.0.0.9000037 to crash while trying to build libcxx, which is problematic for Chromium because that's the version we use for bootstrapping clang/libcxx etc. on Mac.

Build log here: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8891668841083277648/+/steps/package_clang/0/stdout

Please reply on the libc++-commits email before reverting next time. These comments are not easily noticeable.

I've recommitted the libc++ changes. I believe each of the build failures mentioned here has been addressed.
Please inform me if you still hit errors.

hans added a comment.Thu, Jan 16, 12:47 AM

I've recommitted the libc++ changes. I believe each of the build failures mentioned here has been addressed.
Please inform me if you still hit errors.

Looks good on our Mac builder. Thanks! (https://ci.chromium.org/p/chromium/builders/try/mac_upload_clang/941 which built 44560762c62d72a103bdceff49ffa70451efd5f8)