[PATCH] libstdc++: Make debug iterator pointer sequence const [PR116369]

[François Dumont]

Now fixed as trivial, I set you as author.

François

On 08/07/2025 13:55, Jonathan Wakely wrote:
> On Mon, 7 Jul 2025 at 11:12, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On Sat, 5 Jul 2025 at 14:03, François Dumont <frs.dumont@gmail.com> wrote:
>>> On 01/07/2025 22:51, Jonathan Wakely wrote:
>>>> On Mon, 16 Jun 2025 at 18:36, François Dumont <frs.dumont@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);
>      }
>