Re: [review][fixed_string] Andrzej's feedback

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2019-12-05 07:56:41

śr., 4 gru 2019 o 23:47 Gavin Lambert via Boost <boost_at_[hidden]>
napisał(a):

> On 5/12/2019 03:14, Andrzej Krzemienski wrote:
> > Making a resize() over capacity() a precondition violation is a *feature*
> > useful for bug detection and I do not consider it a valid argument that
> > "library will throw exceptions and if you never resize() over capacity()
> > you will never see exceptions or std::abort()". If this is a
> precondition,
> > then I expect of a library to put some BOOST_ASSERT() or
> > _builtin_unreachable() in those paths to enable better bug detection.
>
> I see this argument a lot, and it confuses me.
>
> Perhaps this is my Windows dev background talking (since the analysis
> tools seem more lacking on Windows, despite having a better debugger),
> but in my experience it is vastly easier to find a thrown exception than
> to find "deliberate" UB (including asserts).  And vastly easier to log
> that it unexpectedly occurred in production code in the field, so that
> you can detect and fix it without a debugger attached to the process.
>
> Asserts and unreachables both disappear in release builds, so the
> process ends up continuing to run in some subtly corrupted way -- if
> you're lucky it crashes soon after in an unrelated location that takes
> you weeks to track down the true cause.  If you're unlucky, it runs
> longer, and corrupted some customer data along the way.
>
> Please enlighten me.
>

If the library design were to treat overrunning the capacity as a
programmer bug (which I am not advocating for for fixed_string) then I
imagine the library would use a dedicated macro to indicate the
precondition:

```
void fixed_string<N>::resize(size_type s)
{
  BOOST_FIXED_STRING_PRECONDITION(s <= this->capacity());
  // then do the job
}
```

And you leave the decision what this macro expands to to the user. It will
be a bunch of #if defines. Te library would choose a safe default, like
checking the condition and calling `std::abort()` on failure, but the user
could change it by defining some macros, like
BOOST_FIXED_STRING_THROW_EXCEPTION_ON_PRECONDITION_VIOLATION.

Thus, if the user doesn't have any better idea, she just defines the macro,
and fixed_string will be throwing exceptions on precondition violations.
Everything is "safe", at least under some definition of safety. If on my
platform I cannot use exceptions, I can configure the precondition macro to
call std::abort().

The important point is that I control the behavior only of the
preconditions and only in this library. It does not affect other libraries.
It also does not affect functions that are always guaranteed to throw, such
as fixed_string::at().

Now, if I am doing a test build (or if I can afford doing this in
production), I can compile my program with UB-sanitizer, where the compiler
treats most of the language level UB as a checkable condition, checks it,
and upon a failed check logs a message and aborts. Because compilers are
now required to check most of the UB  when constexpr functions when
evaluated at compile-time, offers the same checks for runtime evaluation
comes practically for free; and compilers like GCC and clang already
provide them. At least one of them also inserts a runtime check and abort
on every occurrence of `__builtin_unreachable()`. So if I define macro
BOOST_FIXED_STRING_PRECONDITION to do `if(!condition)
__builtin_unreachable()` I will get a uniform error message in my test
build if the precondition is ever violated.

Additionally, if I have a static analyzer like Coverity (and I think
Microsoft also offers one), I can make the macro expand to an annotation
that the static analyzer understands and looks for, and then paths that
lead to precondition violation can be exposed statically without even
running the program. This would not be possible if the library only
guaranteed to throw exceptions. You can put any instrumentation code you
want under this macro. But it only works because if the library doesn't
guarantee any particular behavior on precondition violation: it only says
"I leave myself the liberty to do what I choose."

And of course, people in applications where the trade-off between security
and and performance is different (like video games, in internal parts
remote from doing networking) may choose -- after extensive testing has
been done -- to expand the macro to `__builtin_assume() `to check if this
gives them additional performance gain. But this is never done by default,
has to be explicitly opted in, and is really a side bonus to the main
feature which is about safety and controlling how bugs are detected.

Regards,
&rzej;