On Sat, 16 Nov 2024 at 01:09, Jonathan Wakely <jwak...@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?
> +
> this->_M_dec_size(1);
> __position._M_node->_M_unhook();
> _Node* __n = static_cast<_Node*>(__position._M_node);
> --
> 2.47.0
>
