Re: [PATCH] Use cxx11 abi in versioned namespace

[Jonathan Wakely <jwakely@redhat.com>]

On Fri, 13 Jan 2023 at 16:33, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Mon, 5 Dec 2022 at 21:14, François Dumont via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
> >
> > I just rebased this patch.
> >
> > All good apart from the to_chars/from_chars symbols issue.
> >
> > François
> >
> >
> > On 11/10/22 19:28, François Dumont wrote:
> > > Hi
> > >
> > >     Now that pretty printer is fixed (once patch validated) I'd like
> > > to propose this patch again.
> > >
> > >     Note that I'am adding a check on pretty printer with a std::any on
> > > a std::wstring. I did so because of the FIXME in printers.py which is
> > > dealing with 'std::string' explicitely. Looks like in my case, where
> > > there is no 'std::string' but just a 'std::__8::string' we do not need
> > > the workaround.
> > >
> > >     Once again I am attaching also the version namespace bump patch as
> > > I think that adopting the cxx11 abi in this mode is a good enough
> > > reason to bump it. If you agress let me know if I should squash the
> > > commits before pushing.
>
> Yes, I think this change would justify bumping the version.
>
> > >
> > >     libstdc++: [_GLIBCXX_INLINE_VERSION] Use cxx11 abi
> > >
> > >     Use cxx11 abi when activating versioned namespace mode.
> > >
> > >     libstdcxx-v3/ChangeLog:
> > >
> > >             * acinclude.m4 [GLIBCXX_ENABLE_LIBSTDCXX_DUAL_ABI]:
> > > Default to "new" libstdcxx abi.
> > >             * config/locale/dragonfly/monetary_members.cc
> > > [!_GLIBCXX_USE_DUAL_ABI]: Define money_base
> > >             members.
> > >             * config/locale/generic/monetary_members.cc
> > > [!_GLIBCXX_USE_DUAL_ABI]: Likewise.
> > >             * config/locale/gnu/monetary_members.cc
> > > [!_GLIBCXX_USE_DUAL_ABI]: Likewise.
> > >             * config/locale/gnu/numeric_members.cc
> > >             [!_GLIBCXX_USE_DUAL_ABI](__narrow_multibyte_chars): Define.
> > >             * configure: Regenerate.
> > >             * include/bits/c++config
> > >             [_GLIBCXX_INLINE_VERSION](_GLIBCXX_NAMESPACE_CXX11,
> > > _GLIBCXX_BEGIN_NAMESPACE_CXX11): Define
> > >             empty.
> > > [_GLIBCXX_INLINE_VERSION](_GLIBCXX_END_NAMESPACE_CXX11,
> > > _GLIBCXX_DEFAULT_ABI_TAG): Likewise.
> > >             * python/libstdcxx/v6/printers.py
> > >             (StdStringPrinter::__init__): Set self.new_string to True
> > > when std::__8::basic_string type is
> > >             found.
> > >             * src/Makefile.am
> > > [ENABLE_SYMVERS_GNU_NAMESPACE](ldbl_alt128_compat_sources): Define empty.
> > >             * src/Makefile.in: Regenerate.
> > >             * src/c++11/Makefile.am (cxx11_abi_sources): Rename into...
> > >             (dual_abi_sources): ...this, new. Also move several
> > > sources to...
> > >             (sources): ...this.
> > >             (extra_string_inst_sources): Move several sources to...
> > >             (inst_sources): ...this.
>
> I don't understand this part. Moving those files to sources and
> inst_sources will mean they are always compiled, right? But we don't
> want them compiled for --disable-libstdcxx-dual-abi
>
> In those files you've changed the #if conditions so they are empty if
> the dual ABI is disabled, but why do they need to be compiled at all?
> This isn't clear from the patch or the description or the changelog.
>
>
> > >             * src/c++11/Makefile.in: Regenerate.
> > >             * src/c++11/cow-fstream-inst.cc [_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions.
> > >             * src/c++11/cow-locale_init.cc [_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions.
> > >             * src/c++11/cow-sstream-inst.cc [_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions.
> > >             * src/c++11/cow-stdexcept.cc
> > > [_GLIBCXX_USE_CXX11_ABI](error_category::_M_message):
> > >             Skip definition.
> > >             [_GLIBCXX_USE_CXX11_ABI]: Skip Transaction Memory TS
> > > definitions.
> > >             * src/c++11/cow-string-inst.cc [_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions.
> > >             * src/c++11/cow-string-io-inst.cc
> > > [_GLIBCXX_USE_CXX11_ABI]: Skip definitions.
> > >             * src/c++11/cow-wstring-inst.cc [_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions.
> > >             * src/c++11/cow-wstring-io-inst.cc
> > > [_GLIBCXX_USE_CXX11_ABI]: Skip definitions.
> > >             * src/c++11/cxx11-hash_tr1.cc [!_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions.
> > >             * src/c++11/cxx11-ios_failure.cc
> > > [!_GLIBCXX_USE_CXX11_ABI]: Skip definitions.
> > >             [!_GLIBCXX_USE_DUAL_ABI] (__ios_failure): Remove.
>
> For this file I think your changes make sense, because the definitions
> of the gcc4-compatible and cxx11 ABI are different, we're not just
> compiling it twice.
>
>
> > >             * src/c++11/cxx11-locale-inst.cc: Cleanup, just include
> > > locale-inst.cc.
> > >             * src/c++11/cxx11-stdexcept.cc [!_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__cow_string): Remove.
> > >             * src/c++11/cxx11-wlocale-inst.cc
> > > [!_GLIBCXX_USE_CXX11_ABI]: Skip definitions.
> > >             * src/c++11/fstream-inst.cc [!_GLIBCXX_USE_CXX11_ABI]:
> > > Skip definitions
> > >             * src/c++11/locale-inst-numeric.h
> > > [!_GLIBCXX_USE_DUAL_ABI](std::use_facet<num_get<>>,
> > > std::use_facet<num_put<>>): Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](std::has_facet<num_get<>>,
> > > std::has_facet<num_put<>>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](std::num_get<C,
> > > istreambuf_iterator<C>>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](std::num_put<C,
> > > ostreambuf_iterator<C>>): Instantiate.
> > >             * src/c++11/locale-inst.cc [!_GLIBCXX_USE_DUAL_ABI]: Build
> > > only when configured
> > >             _GLIBCXX_USE_CXX11_ABI is equal to currently built abi.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__moneypunct_cache<C, false>):
> > > Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__moneypunct_cache<C, true>):
> > > Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__numpunct_cache<C>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__timepunct<C>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__timepunct_cache<C>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](time_put<C,
> > > ostreambuf_iterator<C>>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](time_put_byname<C,
> > > ostreambuf_iterator<C>>): Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](__ctype_abstract_base<C>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](ctype_byname<C>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__codecvt_abstract_base<C, char,
> > > mbstate_t>): Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](codecvt_byname<C, char,
> > > mbstate_t>): Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](use_facet<ctype<C>>(const locale&)):
> > > Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](use_facet<codecvt<C, char,
> > > mbstate_t>>(const locale&)): Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](use_facet<__timepunct<C>>(const locale&)):
> > > Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](use_facet<time_put<C>>(const locale&)):
> > > Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](has_facet<ctype<C>>(const locale&)):
> > > Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](has_facet<codecvt<C, char,
> > > mbstate_t>>(const locale&)): Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](has_facet<__timepunct<C>>(const locale&)):
> > > Instantiate.
> > > [!_GLIBCXX_USE_DUAL_ABI](has_facet<time_put<C>>(const locale&)):
> > > Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__add_grouping<C>): Define.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__pad<C, char_traits<C>>):
> > > Instantiate.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__int_to_char(C*, unsigned long,
> > > const C*, ios_base::fmtflags, bool)):
> > >             Define.
> > >             [!_GLIBCXX_USE_DUAL_ABI](__int_to_char(C*, unsigned long
> > > long, const C*, ios_base::fmtflags, bool)):
> > >             Define.
> > >             * src/c++11/cxx11-wlocale-inst.cc
> > > [!_GLIBCXX_USE_CXX11_ABI]: Skip definitions.
> > >             * src/c++98/Makefile.am (cxx11_abi_sources): Remove,
> > > unique cow-istream-string.cc entry move to...
> > >             (inst_sources): ...this.
>
> Why?

I know all this ENABLE_DUAL_ABI stuff is a horrible mess, and I should
really try to clean it up, and it's impressive you made this work at
all. But I think the solution you've found makes the mess even worse,
unfortunately.

Another problem is here:

--- a/libstdc++-v3/src/c++98/stdexcept.cc
+++ b/libstdc++-v3/src/c++98/stdexcept.cc
@@ -26,7 +26,7 @@
 // ISO C++ 14882: 19.1  Exception classes
 //

-// All exception classes still use the classic COW std::string.
+// In dual abi all exception classes still use the classic COW std::string.
 #define _GLIBCXX_USE_CXX11_ABI 0

Exceptions must have a non-throwing copy constructor, which is why
they still need to use a reference-counted string. If we disable the
dual ABI and only build the cxx11 string, we need to figure out a way
to compile enough of the COW string to keep exceptions working.