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.
+
this->_M_dec_size(1);
__position._M_node->_M_unhook();
_Node* __n = static_cast<_Node*>(__position._M_node);
--
2.47.0
