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

[ldionne]

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.

[ldionne]

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]

libcxx/docs/ReleaseNotes.rst

60–63

@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

344

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]

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

libcxx/docs/ReleaseNotes.rst

60–63

Thanks for that, please see my reply.

libcxx/include/__locale

344

do you agree?

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

[ldionne]

@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]

In D103960#2808574, @ldionne wrote:

@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.

[ldionne]

In D103960#2903608, @smeenai wrote:

In D103960#2808574, @ldionne wrote:

@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.

No worries, and awesome, I'm quite happy to make this simplification.

I'll need to fix a few things with the debug mode first though, since we currently assume that we can run debug mode tests against the non-debug-mode-built dylib, which doesn't work.

[ldionne]

Rebase onto main

[ldionne]

Rebase onto main. We still haven't fixed the underlying issues with the debug mode (it requires the dylib to have been built with support for the debug mode), but I'm curious to see what fails, if anything.

[ldionne]

Rebase onto main. Now that the debug mode is configure-time only, I think this should work.

[philnik]

I really like this. I've got just one question: Why are the extern declarations for string different than for every other extern template class declaration?

[ldionne]

Poke CI.

[ldionne]

In D103960#3567171, @philnik wrote:

I really like this. I've got just one question: Why are the extern declarations for string different than for every other extern template class declaration?

Do you mean why do we use macros for them? It's just that there's a really long list of methods to declare, so using macros avoids repeating them. However, TBH it's not clear to me that it's actually better, since this sort of duplication isn't super harmful anyways. I think I'd be OK with changing that and repeating the list of instantiated methods in a separate patch.

[philnik]

In D103960#3568852, @ldionne wrote:

In D103960#3567171, @philnik wrote:

I really like this. I've got just one question: Why are the extern declarations for string different than for every other extern template class declaration?

Do you mean why do we use macros for them? It's just that there's a really long list of methods to declare, so using macros avoids repeating them. However, TBH it's not clear to me that it's actually better, since this sort of duplication isn't super harmful anyways. I think I'd be OK with changing that and repeating the list of instantiated methods in a separate patch.

I mean why aren't we using extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS basic_string<char, char_traits<char>, allocator<char>> etc.? It's quite surprising IMO that string is the one class where we don't do that. Just to try it out I removed the explicit list and replaced it with the normal extern template stuff. It turns out that __init_copy_ctor_external, __erase_external_with_move, __assign_external and __assign_short aren't marked as _LIBCPP_HIDE_FROM_ABI but are also not exported from the dylib. Other than __assign_short I think these are intended to go into the dylib. Wheter they are or aren't supposed to go into the dylib doesn't really matter right now. My point is that the explicit list causes inconsistencies and doesn't seem to have any actual uses. It's of course possible that I'm missing something.'that makes the explicit lists super useful.