On Mon, 7 Jul 2025 at 11:12, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> On Sat, 5 Jul 2025 at 14:03, François Dumont <frs.dum...@gmail.com> wrote:
> >
> > On 01/07/2025 22:51, Jonathan Wakely wrote:
> > > On Mon, 16 Jun 2025 at 18:36, François Dumont <frs.dum...@gmail.com>
> > > wrote:
> > >> I eventually wonder if it is such a big deal to add the new symbols for
> > >> _GLIBCXX_DEBUG mode.
> > > I like this version much more than the one trying to duplicate symbols
> > > with asm.
> > >
> > >
> > >> Here is the patch doing this. It avoids to add many const_cast which is
> > >> what we are trying to achieve here.
> > > I'm still not really sure if this is worth it though - is it fixing a
> > > bug or a correctness problem? (using const_cast is safe if the objects
> > > aren't actually const)
> > >
> > > All the new tests already pass, even without this patch. Are these
> > > just tests for const member functions that we aren't currently testing
> > > at all?
> >
> > Those tests are showing the same UB that you fixed as part of your
> > PR116369 patch but this time with local_iterator. Even if tests are
> > passing without this patch it's still UB before it, do you prefer to
> > remove those tests then ?
>
> Ah OK, so they are showing UB ... it's just that the compiler doesn't
> actually complain about it.
>
> Please make the const containers in those tests global variables,
> instead of local variables inside main(). The compiler won't put local
> variables in ROM so the test would never fail. It might put globals in
> ROM (although not after your patch, because of the mutable members,
> which is why the patch is actually fixing something).
>
>
> >
> > Globally this patch is following your recommendations on PR116369 commit
> > where you were saying:
> >
> > Ideally we would not need the const_cast at all. Instead, the _M_attach
> > member (and everything it calls) should be const-qualified. That would
> > work fine now, because the members that it ends up modifying are
> > mutable. Making that change would require a number of new exports from
> > the shared library, and would require retaining the old non-const
> > member
> > functions (maybe as symbol aliases) for backwards compatibility. That
> > might be worth changing at some point, but isn't done here.
> >
> > In addition to what is said here I made the sequence pointer const too
> > as the added mutable allows that.
> >
> > It was also the occasion to fix some types used in std::forward_list in
> > Debug mode.
> >
> > Do you think it is useless eventually ?
>
> I think it's worth doing, I was just concerned about the __asm__
> solution used in the initial patches.
>
> OK for trunk with the adjusted tests, thanks.
I see a large number of new test failures. It looks like this fix is needed:
--- a/libstdc++-v3/include/debug/forward_list
+++ b/libstdc++-v3/include/debug/forward_list
@@ -144,13 +144,13 @@ namespace __gnu_debug
//std::swap(_M_this()->_M_version, __other._M_version);
_Safe_iterator_base* __this_its = _M_this()->_M_iterators;
_S_swap_aux(__other, __other._M_iterators,
- _M_this(), _M_this()->_M_iterators);
+ *_M_this(), _M_this()->_M_iterators);
_Safe_iterator_base* __this_const_its = _M_this()->_M_const_iterators;
_S_swap_aux(__other, __other._M_const_iterators,
- _M_this(), _M_this()->_M_const_iterators);
- _S_swap_aux(_M_this(), __this_its,
+ *_M_this(), _M_this()->_M_const_iterators);
+ _S_swap_aux(*_M_this(), __this_its,
__other, __other._M_iterators);
- _S_swap_aux(_M_this(), __this_const_its,
+ _S_swap_aux(*_M_this(), __this_const_its,
__other, __other._M_const_iterators);
}
