On Sat, 16 Nov 2024, 01:21 Jonathan Wakely, <jwak...@redhat.com> 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? > ^^ *disabled* ! I clearly need to get more sleep... > > > > > > + > > this->_M_dec_size(1); > > __position._M_node->_M_unhook(); > > _Node* __n = static_cast<_Node*>(__position._M_node); > > -- > > 2.47.0 > > > >
