Page MenuHomePhabricator

[libc++] Remove the std::string base class
ClosedPublic

Authored by philnik on Feb 1 2022, 1:15 PM.

Details

Summary

Removing the base class of std::basic_string is not an ABI break, so we can remove any references to it from the header.

Diff Detail

Event Timeline

philnik requested review of this revision.Feb 1 2022, 1:15 PM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 1:15 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

This change breaks ABI stability in the CI, so it needs more work.

libcxx/include/string
686

The comment contradicts the commit message. But the base indeed seems to be empty.

libcxx/src/string.cpp
24–38

Maybe add a comment here that the struct isn't declared in any header anymore and is only provided here to export the symbols for ABI stability. (It's kind of obvious when you think about it, but can't hurt to explain it for future readers.)

ldionne requested changes to this revision.Feb 2 2022, 9:54 AM
ldionne added inline comments.
libcxx/src/string.cpp
41–43

I suggest this instead:

template <bool>
struct __basic_string_common {
    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const;
    _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
};

void __basic_string_common<true>::__throw_length_error() const {
    _VSTD::__throw_length_error("basic_string");
}

void __basic_string_common<true>::__throw_out_of_range() const {
    _VSTD::__throw_out_of_range("basic_string");
}

Also, I suggest we rename _LIBCPP_ABI_NO_BASIC_STRING_BASE_CLASS to _LIBCPP_ABI_DONT_EXPORT_BASIC_STRING_COMMON or something along those lines. Basically, the ABI breaking change here would only be that we're not exporting the functions anymore from the dylib.

This revision now requires changes to proceed.Feb 2 2022, 9:54 AM
philnik updated this revision to Diff 405372.Feb 2 2022, 11:18 AM
philnik marked 3 inline comments as done.
  • Address comments
Mordante accepted this revision as: Mordante.Feb 3 2022, 8:21 AM

LGTM!

ldionne accepted this revision.Feb 3 2022, 1:52 PM

Oooooh, I'm trembling to approve this. If my reasoning on vector_base_common isn't right, this is going to break the whole world (basic_string is like the most used type in the library). But yeah, we've thought it out and it seems legit, so let's go.

This revision is now accepted and ready to land.Feb 3 2022, 1:52 PM

Oooooh, I'm trembling to approve this. If my reasoning on vector_base_common isn't right, this is going to break the whole world (basic_string is like the most used type in the library). But yeah, we've thought it out and it seems legit, so let's go.

What could go wrong? The worst would be that we break thousands of people's software silently :D
Do you cherry-pick the commit onto the release branch?

This revision was automatically updated to reflect the committed changes.
dim added a subscriber: dim.Feb 9 2022, 6:33 AM

FWIW, this breaks compilation of Firefox:

In file included from Unified_cpp_accessible_base1.cpp:20:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/firefox-97.0/accessible/base/TextLeafRange.cpp:11:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/a11y/DocAccessibleParent.h:11:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/ipc/ipdl/_ipdlheaders/mozilla/a11y/PDocAccessibleParent.h:9:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/ipc/ipdl/_ipdlheaders/mozilla/a11y/PDocAccessible.h:24:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/a11y/IPCTypes.h:16:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/GfxMessageUtils.h:30:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/ipc/ProtocolUtils.h:27:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/ipc/MessageChannel.h:23:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/stl_wrappers/stack:64:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/system_wrappers/stack:3:
In file included from /usr/include/c++/v1/stack:105:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/stl_wrappers/deque:64:
In file included from /wrkdirs/share/dim/ports/www/firefox/work/.build/dist/system_wrappers/deque:3:
/usr/include/c++/v1/deque:1854:9: error: call to '__throw_out_of_range' is ambiguous
        _VSTD::__throw_out_of_range("deque");
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/__config:824:15: note: expanded from macro '_VSTD'
#define _VSTD std
              ^
/wrkdirs/share/dim/ports/www/firefox/work/.build/dist/include/mozilla/throw_gcc.h:97:59: note: candidate function
MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_out_of_range(
                                                          ^
/usr/include/c++/v1/stdexcept:264:6: note: candidate function
void __throw_out_of_range(const char*__msg)
     ^

That said, they're doing very naughty things in e.g. https://hg.mozilla.org/mozilla-central/file/tip/memory/mozalloc/throw_gcc.h#l47 :

namespace std {

// NB: user code is not supposed to touch the std:: namespace.  We're
// doing this after careful review because we want to define our own
// exception throwing semantics.  Don't try this at home!

MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_bad_exception(
    void) {
  mozalloc_abort("fatal: STL threw bad_exception");
}

[...]

MOZ_THROW_NORETURN MOZ_THROW_EXPORT MOZ_THROW_INLINE void __throw_out_of_range(
    const char* msg) {
  mozalloc_abort(msg);
}
emaste added a subscriber: emaste.Feb 9 2022, 6:35 AM

FWIW, this breaks compilation of Firefox:
[...]

Thanks for the heads up. I just sent a courtesy email to a Firefox contributor to make them aware of this problem and CC'd you on the thread. Long story short, they are violating the Standard and there's nothing we can do about it -- we can only be nice and give them a heads up, which I did.