On Mon, 18 Nov 2024 at 18:32, François Dumont <frs.dum...@gmail.com> wrote: > > > 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.
I used __builtin_expect instead of [[__unlikely__]] because that's valid in C++98 (GCC and recent versions of Clang now allow [[attr]] in C++98 too, so at some point we could switch to using those attributes everywhere). There will be a non-zero cost for correct code, because of an extra check. But std::list is not a high-performance container anyway, and avoiding UB might be worth the overhead. > > > 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. Do you mean no assertions? > 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. Yes, I came to the same conclusion. I have a simpler version of the patch which only adds nonempty assertions to front() and back(). I'll work on adding erase and splice assertions separately (and maybe not for GCC 15).
