[PATCH] sso-string@gnu-versioned-namespace [PR83077]

[François Dumont]

Here is a rebased patch following the resize_and_overwrite change.

I confirm that tests are now fixed after the change in tzdb.cc.

I'll prepare a fix for those tests still but preparing also a test to 
detect allocations in the lib.

François

On 17/08/2023 21:44, Jonathan Wakely wrote:
> On Thu, 17 Aug 2023 at 20:37, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On Thu, 17 Aug 2023 at 19:59, Jonathan Wakely <jwakely@redhat.com> wrote:
>>> On Thu, 17 Aug 2023 at 18:40, François Dumont <frs.dumont@gmail.com> wrote:
>>>>
>>>> On 17/08/2023 19:22, Jonathan Wakely wrote:
>>>>> On Sun, 13 Aug 2023 at 14:27, François Dumont via Libstdc++
>>>>> <libstdc++@gcc.gnu.org> wrote:
>>>>>> Here is the fixed patch tested in all 3 modes:
>>>>>>
>>>>>> - _GLIBCXX_USE_DUAL_ABI
>>>>>>
>>>>>> - !_GLIBCXX_USE_DUAL_ABI && !_GLIBCXX_USE_CXX11_ABI
>>>>>>
>>>>>> - !_GLIBCXX_USE_DUAL_ABI && _GLIBCXX_USE_CXX11_ABI
>>>>>>
>>>>>> I don't know what you have in mind for the change below but I wanted to
>>>>>> let you know that I tried to put COW std::basic_string into a nested
>>>>>> __cow namespace when _GLIBCXX_USE_CXX11_ABI. But it had more impact on
>>>>>> string-inst.cc so I preferred the macro substitution approach.
>>>>> I was thinking of implementing the necessary special members functions
>>>>> of __cow_string directly, so they are ABI compatible with the COW
>>>>> std::basic_string but don't actually reuse the code. That would mean
>>>>> we don't need to compile and instantiate the whole COW string just to
>>>>> use a few members from it. But that can be done later, the macro
>>>>> approach seems OK for now.
>>>> You'll see that when cow_string.h is included while
>>>> _GLIBCXX_USE_CXX11_ABI == 1 then I am hiding a big part of the
>>>> basic_string definition. Initially it was to avoid to have to include
>>>> basic_string.tcc but it is also a lot of useless code indeed.
>>>>
>>>>
>>>>>> There are some test failing when !_GLIBCXX_USE_CXX11_ABI that are
>>>>>> unrelated with my changes. I'll propose fixes in coming days.
>>>>> Which tests? I run the entire testsuite with
>>>>> -D_GLIBCXX_USE_CXX11_ABI=0 several times per day and I'm not seeing
>>>>> failures.
>>>>>
>>>>> I'll review the patch ASAP, thanks for working on it.
>>>>>
>>>> So far the only issue I found are in the mode !_GLIBCXX_USE_DUAL_ABI &&
>>>> !_GLIBCXX_USE_CXX11_ABI. They are:
>>>>
>>>> 23_containers/unordered_map/96088.cc
>>>> 23_containers/unordered_multimap/96088.cc
>>>> 23_containers/unordered_multiset/96088.cc
>>>> 23_containers/unordered_set/96088.cc
>>>> ext/debug_allocator/check_new.cc
>>>> ext/malloc_allocator/check_new.cc
>>>> ext/malloc_allocator/deallocate_local.cc
>>>> ext/new_allocator/deallocate_local.cc
>>>> ext/pool_allocator/allocate_chunk.cc
>>>> ext/throw_allocator/deallocate_local.cc
>>> Ah yes, they fail for !USE_DUAL_ABI builds, I wonder why.
>>>
>>> /home/test/src/gcc/libstdc++-v3/testsuite/23_containers/unordered_map/96088.
>>> cc:44: void test01(): Assertion '__gnu_test::counter::count() == 3' failed.
>>> FAIL: 23_containers/unordered_map/96088.cc execution test
>> It's due to this global object in src/c++20/tzdb.cc:
>> 1081        const string tzdata_file = "/tzdata.zi";
>>
>> When the library uses COW strings that requires an allocation before
>> main, which uses the replacement operator new in the tests, which
>> fails to allocate. For example, in 22_locale/locale/cons/12352.cc we
>> have this function used by operator new:
>>
>> int times_to_fail = 0;
>>
>> void* allocate(std::size_t n)
>> {
>>    if (!times_to_fail--)
>>      return 0;
>>
>> The counter is initially zero, so if we try to allocate before it gets
>> set to a non-zero value in test01() then we fail.
>>
>> The test should not assume no allocations before main() begins. The
>> simplest way to do that is with another global that says "we have
>> started testing" e.g.
>>
>> --- a/libstdc++-v3/testsuite/22_locale/locale/cons/12352.cc
>> +++ b/libstdc++-v3/testsuite/22_locale/locale/cons/12352.cc
>> @@ -26,11 +26,12 @@
>>   #include <cstring>
>>   #include <testsuite_hooks.h>
>>
>> +bool tests_started = false;
>>   int times_to_fail = 0;
>>
>>   void* allocate(std::size_t n)
>>   {
>> -  if (!times_to_fail--)
>> +  if (tests_started && !times_to_fail--)
>>       return 0;
>>
>>     void* ret = std::malloc(n ? n : 1);
>> @@ -106,6 +107,8 @@ void operator delete[](void* p, const
>> std::nothrow_t&) throw()
>>   // libstdc++/12352
>>   void test01(int iters)
>>   {
>> +  tests_started = true;
>> +
>>     for (int j = 0; j < iters; ++j)
>>       {
>>         for (int i = 0; i < 100; ++i)
>>
>>
>> This way the replacement operator new doesn't start intentionally
>> failing until we ask it to do so.
> I'll replace the global std::string objects with std::string_view
> objects, so that they don't allocate even if the library only uses COW
> strings.
>
> We should still fix those tests though.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cxx11_gnu-versioned-ns.patch
Type: text/x-patch
Size: 58501 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20230819/a9b77d0e/attachment-0001.bin>