This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Reviewers
smeenai
philnik
Group Reviewers
Restricted Project
Restricted Project
Commits
rG2ae52326dab0: [libc++] Towards a simpler extern template story in libc++
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

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
81–84

@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
81–84

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.Jul 25 2021, 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.

@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 updated this revision to Diff 362481.Jul 28 2021, 11:36 AM

Rebase onto main

Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2021, 11:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 418048.Mar 24 2022, 2:30 PM

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.

Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 2:30 PM
ldionne updated this revision to Diff 435211.Jun 8 2022, 9:32 AM

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

philnik accepted this revision as: philnik.Jun 8 2022, 9:38 AM
philnik added a subscriber: 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 accepted this revision as: Restricted Project, Restricted Project.Jun 8 2022, 7:04 PM

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.

This revision is now accepted and ready to land.Jun 8 2022, 7:04 PM
This revision was automatically updated to reflect the committed changes.

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.