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 <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?

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.

Reply via email to