[PATCH] libstdc++: Add debug assertions to std::list and std::forward_list
François Dumont
frs.dumont@gmail.com
Mon Nov 18 18:32:06 GMT 2024
On 18/11/2024 19:24, François Dumont wrote:
>
> On 16/11/2024 02:18, Jonathan Wakely wrote:
>> On Sat, 16 Nov 2024 at 01:09, Jonathan Wakely <jwakely@redhat.com>
>> wrote:
>>> While working on fancy pointer support for the linked lists I noticed
>>> they didn't have any debug assertions. This adds the obvious non-empty
>>> assertions to front(), back(), pop_front() and pop_back().
>>>
>>> For the pop members, adding an assertion to the underlying function
>>> that
>>> erases a member means it also check erase(end()), which is always
>>> invalid, and erase(begin()) on an empty list. For those erase
>>> members we
>>> can also add a check so that we return without doing anything if the
>>> assertion is disabled, but would have failed had it been enabled.
>>>
>>> libstdc++-v3/ChangeLog:
>>>
>>> * include/bits/forward_list.h (forward_list::front): Add
>>> non-empty assertions.
>>> * include/bits/forward_list.tcc
>>> (_Fwd_list_base::_M_erase_after):
>>> Likewise. Return immediately if argument is invalid.
>>> * include/bits/stl_list.h (list::front, list::back): Add
>>> non-empty assertions.
>>> (list::_M_erase): Likewise. Return immediately if argument is
>>> invalid.
>>> ---
>>>
>>> Tested x86_64-linux.
>>>
>>> As pull request: https://forge.sourceware.org/gcc/gcc-TEST/pulls/26
>>>
>>> libstdc++-v3/include/bits/forward_list.h | 3 +++
>>> libstdc++-v3/include/bits/forward_list.tcc | 6 ++++++
>>> libstdc++-v3/include/bits/stl_list.h | 19 +++++++++++++++++--
>>> 3 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libstdc++-v3/include/bits/forward_list.h
>>> b/libstdc++-v3/include/bits/forward_list.h
>>> index c9238cef96f..3fac657518c 100644
>>> --- a/libstdc++-v3/include/bits/forward_list.h
>>> +++ b/libstdc++-v3/include/bits/forward_list.h
>>> @@ -42,6 +42,7 @@
>>> #include <bits/allocator.h>
>>> #include <ext/alloc_traits.h>
>>> #include <ext/aligned_buffer.h>
>>> +#include <debug/assertions.h>
>>> #if __glibcxx_ranges_to_container // C++ >= 23
>>> # include <bits/ranges_base.h> // ranges::begin, ranges::distance
>>> etc.
>>> # include <bits/ranges_util.h> // ranges::subrange
>>> @@ -884,6 +885,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>> reference
>>> front()
>>> {
>>> + __glibcxx_requires_nonempty();
>>> _Node* __front =
>>> static_cast<_Node*>(this->_M_impl._M_head._M_next);
>>> return *__front->_M_valptr();
>>> }
>>> @@ -896,6 +898,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>> const_reference
>>> front() const
>>> {
>>> + __glibcxx_requires_nonempty();
>>> _Node* __front =
>>> static_cast<_Node*>(this->_M_impl._M_head._M_next);
>>> return *__front->_M_valptr();
>>> }
>>> diff --git a/libstdc++-v3/include/bits/forward_list.tcc
>>> b/libstdc++-v3/include/bits/forward_list.tcc
>>> index 9750c7c0502..50acdb9f26b 100644
>>> --- a/libstdc++-v3/include/bits/forward_list.tcc
>>> +++ b/libstdc++-v3/include/bits/forward_list.tcc
>>> @@ -63,6 +63,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>> _Fwd_list_base<_Tp, _Alloc>::
>>> _M_erase_after(_Fwd_list_node_base* __pos)
>>> {
>>> + if (__pos == nullptr || __pos->_M_next == nullptr)
>>> [[__unlikely__]]
>>> + {
>>> + __glibcxx_assert(__pos != nullptr && __pos->_M_next !=
>>> nullptr);
>>> + return nullptr;
>>> + }
>>> +
>>> _Node* __curr = static_cast<_Node*>(__pos->_M_next);
>>> __pos->_M_next = __curr->_M_next;
>>> _Node_alloc_traits::destroy(_M_get_Node_allocator(),
>>> diff --git a/libstdc++-v3/include/bits/stl_list.h
>>> b/libstdc++-v3/include/bits/stl_list.h
>>> index 7deb04b4bfe..d70ba90b8fa 100644
>>> --- a/libstdc++-v3/include/bits/stl_list.h
>>> +++ b/libstdc++-v3/include/bits/stl_list.h
>>> @@ -59,6 +59,7 @@
>>>
>>> #include <bits/concept_check.h>
>>> #include <ext/alloc_traits.h>
>>> +#include <debug/assertions.h>
>>> #if __cplusplus >= 201103L
>>> #include <initializer_list>
>>> #include <bits/allocated_ptr.h>
>>> @@ -1249,7 +1250,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>>> _GLIBCXX_NODISCARD
>>> reference
>>> front() _GLIBCXX_NOEXCEPT
>>> - { return *begin(); }
>>> + {
>>> + __glibcxx_requires_nonempty();
>>> + return *begin();
>>> + }
>>>
>>> /**
>>> * Returns a read-only (constant) reference to the data at
>>> the first
>>> @@ -1258,7 +1262,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>>> _GLIBCXX_NODISCARD
>>> const_reference
>>> front() const _GLIBCXX_NOEXCEPT
>>> - { return *begin(); }
>>> + {
>>> + __glibcxx_requires_nonempty();
>>> + return *begin();
>>> + }
>>>
>>> /**
>>> * Returns a read/write reference to the data at the last
>>> element
>>> @@ -1268,6 +1275,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>>> reference
>>> back() _GLIBCXX_NOEXCEPT
>>> {
>>> + __glibcxx_requires_nonempty();
>>> iterator __tmp = end();
>>> --__tmp;
>>> return *__tmp;
>>> @@ -1281,6 +1289,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>>> const_reference
>>> back() const _GLIBCXX_NOEXCEPT
>>> {
>>> + __glibcxx_requires_nonempty();
>>> const_iterator __tmp = end();
>>> --__tmp;
>>> return *__tmp;
>>> @@ -2132,6 +2141,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>>> void
>>> _M_erase(iterator __position) _GLIBCXX_NOEXCEPT
>>> {
>>> + if (__builtin_expect(empty(), 0))
>>> + {
>>> + __glibcxx_requires_nonempty();
>>> + return;
>>> + }
>> Hmm, I'm having second thoughts about the "return without doing
>> anything part now.
>> For this simple test:
>>
>> #include <list>
>>
>> int main()
>> {
>> std::list<int> l;
>> l.erase(l.begin());
>> }
>>
>> Currently it crashes (bad), but with -O1 there's a nice warning:
>>
>> /usr/include/c++/14/bits/new_allocator.h:172:33: warning: ‘void
>> operator delete(void*, std::size_t)’ called on unallocated object ‘l’
>> [-Wfree-nonheap-object]
>>
>> And Asan can diagnose it too.
>>
>> Adding an assertion is definitely an improvement, as it avoids the
>> crash . But returning when the assertion is disabled, so that the
>> function is a no-op, means that the warning about freeing a null
>> pointer goes away, because the compiler can see it's never reached.
>> And now Asan can't diagnose it.
>>
>> I think on balance, making it a no-op and avoiding arbitrary UB is
>> better. The warning is only possible in trivial cases where the
>> compiler can see the pointer is definitely null. In more realistic
>> code, there will be no warning, and UB, so turning it into a silent
>> no-op does seem safer. If you want to detect the bug, enable
>> assertions.
>>
>> What do others think? Better to add the assertion but leave the UB
>> present when assertions are disabled, or add the assertion and
>> silently remove the UB when assertions are enabled?
>
> I think it depends on the impact on performances, is there any ?
>
> You're not using the [__unlikely__] attribute to avoid the UB I guess
> but maybe at a cost.
>
On a second thought it rather depends on the call context.
This _M_erase is called from 3 locations, pop_front, pop_back and erase.
For pop_front/pop_back I think the UB should be preserved. For erase,
silently ignoring a erase(begin()) seems more user friendly.
So maybe the assertions should be moved to the public methods and
adapted to this context.
More information about the Libstdc++
mailing list