Re: [PATCH] Use cxx11 abi in versioned namespace

["François Dumont" <frs.dumont@gmail.com>]

On 13/01/23 18:06, Jonathan Wakely wrote:
> 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.
>
Ouch, I did not know.

Maybe there should be a static_assert about this constraint, you would 
have saved a patch review.

This is going to give me a big headache to find a solution to this problem !