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

[Jonathan Wakely <jwakely@redhat.com>]

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.