[PATCH] libstdc++: Add debug assertions to std::list and std::forward_list

[Marc Glisse]

On Sat, 16 Nov 2024, Jonathan Wakely wrote:

>>        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 all seems related to the current flamewar^Wdiscussion on the 
C++ reflectors. A precondition is violated, and the question is what we 
should do about it.

The ASAN regression looks bad. With special code protected by the relevant 
macros if necessary, it would be good if it still gave a diagnostic.

As a personal opinion, I am not convinced that no-op is a good default 
behavior. I am fine with UB if I am not in a debug or hardened mode. But 
*if* we are going to the trouble of testing the precondition, stopping the 
program (trap) or raising an exception seems less surprising. Ignoring the 
operation and continuing, which can have unpredictable behavior since the 
logic of the program has failed already at this point, looks like 
something that should only happen if the user explicitly asked for a 
no-fail mode that tries its best to continue even when asked to execute 
nonsense.

(things would obviously be different if the standard standardized the 
no-op behavior on these specific functions)

-- 
Marc Glisse