On Mon, 18 Nov 2024 at 19:10, Marc Glisse <marc.gli...@inria.fr> wrote:
>
> 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.
Yes, that should be doable, and preferable to silently ignoring the bug.
> 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.
Some users definitely want no-fail modes, but I like your suggestion
of requiring explicit opt-in.
Ideally we'd have a single syntax that works for all cases, something like:
if (!__precondition(pos != end())
return;
In hardened modes __precondition would either return true or
abort/trap. In no-fail mode it would check and return false if the
precondition is violated.
But I think we'd need other forms too, e.g. "Asan is enabled and will
detect this one anyway so just return true without checking". That
wouldn't be applicable for all precondition checks, so we'd need to
pass flags to the __precondition function, or have more than one
function. That needs more investigation and is not going to happen for
GCC 15.
> (things would obviously be different if the standard standardized the
> no-op behavior on these specific functions)
I'm actually thinking about writing a proposal to do exactly that, in
select places. There are some preconditions that could be changed from
"get this right, or you have UB" to "erroneously returns with no
effects" or similar wording. The C++26 notion of erroneous behaviour
means that we are allowed to diagnose (abort, trap, call a violation
handler etc) in checking modes with assertions enabled, but if we
don't diagnose it then the behaviour has to be well-defined. So no UB.
I think some of the standard library's preconditions could be turned
from UB to EB with very little loss of performance.
See also https://isocpp.org/files/papers/P3471R1.html for another
approach to making precondition checks more reliable. I am very
strongly in favour of the P3471 direction.
