Page MenuHomePhabricator

[libc++] Towards a simpler extern template story in libc++
Needs ReviewPublic

Authored by ldionne on Jun 9 2021, 6:01 AM.

Details

Reviewers
smeenai
Group Reviewers
Restricted Project
Summary

The flexibility around extern template instantiation declarations in
libc++ result in a very complicated model, especially when support for
slightly different configurations (like the debug mode or assertions
in the dylib) are taken into account. That results in unexpected bugs
like http://llvm.org/PR50534 (and there have been multiple similar
bugs in the past, notably around the debug mode).

This patch gets rid of the _LIBCPP_DISABLE_EXTERN_TEMPLATE knob, which
I don't think is fundamental. Indeed, the motivation for that knob was to
avoid taking a dependency on the library, however that can be done better
by linking against the static library instead. And in fact, some parts of
the headers will always depend on things defined in the library, which
defeats the original goal of _LIBCPP_DISABLE_EXTERN_TEMPLATE.

Diff Detail

Unit TestsFailed

TimeTest
970 mslibcxx CI C++03 > libcxx-trunk-shared.libcxx/strings/basic_string/string_modifiers::insert_iter_char_db1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/libcxx/test/libcxx/strings/basic.string/string.modifiers/insert_iter_char_db1.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp.exe
940 mslibcxx CI C++11 > libcxx-trunk-shared.libcxx/strings/basic_string/string_modifiers::insert_iter_char_db1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/strings/basic.string/string.modifiers/insert_iter_char_db1.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx11/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx11/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx11/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx11/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp.exe
1,030 mslibcxx CI C++14 > libcxx-trunk-shared.libcxx/strings/basic_string/string_modifiers::insert_iter_char_db1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/da883eb33258-1/llvm-project/libcxx-ci/libcxx/test/libcxx/strings/basic.string/string.modifiers/insert_iter_char_db1.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/da883eb33258-1/llvm-project/libcxx-ci/build/generic-cxx14/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/da883eb33258-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++14 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/da883eb33258-1/llvm-project/libcxx-ci/build/generic-cxx14/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/da883eb33258-1/llvm-project/libcxx-ci/build/generic-cxx14/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/da883eb33258-1/llvm-project/libcxx-ci/build/generic-cxx14/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/da883eb33258-1/llvm-project/libcxx-ci/build/generic-cxx14/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp.exe
1,050 mslibcxx CI C++17 > libcxx-trunk-shared.libcxx/strings/basic_string/string_modifiers::insert_iter_char_db1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/libcxx/test/libcxx/strings/basic.string/string.modifiers/insert_iter_char_db1.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx17/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++17 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx17/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx17/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx17/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/b1fbd69d24e0-1/llvm-project/libcxx-ci/build/generic-cxx17/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp.exe
1,370 mslibcxx CI C++20 > libcxx-trunk-shared.libcxx/strings/basic_string/string_modifiers::insert_iter_char_db1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/libcxx/test/libcxx/strings/basic.string/string.modifiers/insert_iter_char_db1.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx20/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++20 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx20/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx20/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/75028752f437-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/libcxx/strings/basic.string/string.modifiers/Output/insert_iter_char_db1.pass.cpp.dir/t.tmp.exe
View Full Test Results (21 Failed)

Event Timeline

ldionne created this revision.Jun 9 2021, 6:01 AM
ldionne requested review of this revision.Jun 9 2021, 6:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 6:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a comment.EditedJun 9 2021, 6:39 AM

This patch does have the problem that if we want to be able to enable the debugging mode, the library has to have been built with the debug mode enabled, which wasn't the case previously (but instead we had blatant ODR issues that might just not have bitten anyone so far).

Quuxplusone added inline comments.
libcxx/docs/ReleaseNotes.rst
69–72

@ldionne wrote:

does have the problem that if we want to be able to enable the debugging mode, the library has to have been built with the debug mode enabled, which wasn't the case previously

It was always the case for debug iterators. I wrote an email to the libcxx-dev mailing list on 2021-05-01 and still haven't seen any reply to it.

libcxx/include/__locale
343

This PR basically LGTM. I do want to raise the question of greppability: The old macros let you grep -l _LIBCPP_EXTERN_TEMPLATE to find these sneaky declarations. The new code makes you grep -l extern.template instead. This seems fine to me; do you agree?

FYI, some places already use extern template (for function templates only):

libcxx/include/locale:extern template _LIBCPP_FUNC_VIS time_base::dateorder __time_get_storage<_CharT>::__do_date_order() const; \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS __time_get_storage<_CharT>::__time_get_storage(const char*); \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS __time_get_storage<_CharT>::__time_get_storage(const string&); \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS void __time_get_storage<_CharT>::init(const ctype<_CharT>&); \
libcxx/include/locale:extern template _LIBCPP_FUNC_VIS __time_get_storage<_CharT>::string_type __time_get_storage<_CharT>::__analyze(char, const ctype<_CharT>&); \
ldionne updated this revision to Diff 350946.Jun 9 2021, 10:34 AM
ldionne marked 2 inline comments as done.
ldionne edited the summary of this revision. (Show Details)

Rebase onto main. This is now unrelated to fixing http://llvm.org/PR50534.

libcxx/docs/ReleaseNotes.rst
69–72

Thanks for that, please see my reply.

libcxx/include/__locale
343

do you agree?

Yes, I think that's fine (and in fact a lot more straightforward than before).

@smeenai What do you think about this (since you added the knob in the first place)? Do you agree with my assessment that the goal of avoiding a dependency on the library isn't met by _LIBCPP_EXTERN_TEMPLATE alone?

smeenai accepted this revision.Sun, Jul 25, 10:05 PM

@smeenai What do you think about this (since you added the knob in the first place)? Do you agree with my assessment that the goal of avoiding a dependency on the library isn't met by _LIBCPP_EXTERN_TEMPLATE alone?

Sorry, this got buried in my inbox!

This looks like a great simplification to me. The work that originally needed this knob is defunct, and we were actually bitten by it in another place (something with the iostream static initializers got messed up IIRC). Doing a static link if you don't want the library dependency (and making it hermetic if you want it to be self-contained) SGTM.