On Thu, 17 Aug 2023 at 20:44, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> On Thu, 17 Aug 2023 at 20:37, Jonathan Wakely <jwak...@redhat.com> wrote:
> >
> > On Thu, 17 Aug 2023 at 19:59, Jonathan Wakely <jwak...@redhat.com> wrote:
> > >
> > > On Thu, 17 Aug 2023 at 18:40, François Dumont <frs.dum...@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++
> > > > > <libstd...@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.
Done at r14-3309-gd82a85b6161cbe
>
> We should still fix those tests though.
