Re: Enable string_view assertions

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 14 March 2018 at 22:52, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> (Resending from a different account, sorry for the duplicate).
>
> On 14/03/18 22:12 +0100, François Dumont wrote:
>>       constexpr const _CharT&
>>       operator[](size_type __pos) const noexcept
>>       {
>>-      // TODO: Assert to restore in a way compatible with the constexpr.
>>-      // __glibcxx_assert(__pos < this->_M_len);
>>+      if (!__builtin_constant_p(__pos))
>>+        __glibcxx_assert(__pos < this->_M_len);
>
> This doesn't do the right thing, because it fails to assert when __pos
> is known at compile-time:
>
>   const std::string_view sv;
>   sv[100];
>
> This will only assert at -O0. As soon as optimization is enabled the
> value is known inside the function, and __builtin_constant_p(__pos)
> is true, and we don't do the range check.
>
> We also don't do the check for constant expressions, when we should be
> able to give a compilation error, not just ignore it!
>
> Enabling the assertions at -O0 and outside constant expressions is
> slightly better than never enabling them at all, but not good enough
> to remove the "TODO" comments. Ideally we want out-of-range accesses
> to be an error in constant expressions, and fail the runtime assertion
> in non-constant expressions.
>
> But using __builtin_constant_p to do the assertion conditionally
> doesn't do that. In PR 78420 either branch is OK: when the comparison
> is known at compile-time, do it directly, otherwise do it via casting.
> Both give the same result.
>
> Here you do "if the result is not known, check it at runtime,
> otherwise don't check at all".
>
> What we should do instead is declare a new member function like this:
>
>   static void
>   __out_of_range() __attribute__((__error__("Index out-of-range")));
>
> Then we can refer to that in the cases where (__pos >= _M_len) is
> known at compile-time:
>
>       constexpr const _CharT&
>       operator[](size_type __pos) const noexcept
>       {
>         if (__builtin_constant_p(__pos >= _M_len))
>           {
>             if (__pos >= _M_len)
>               __out_of_range();
>           }
>         else
>           __glibcxx_assert(__pos < this->_M_len);
>         return *(this->_M_str + __pos);
>       }
>
> Now if we try an out-of-range access in a constant expression we get:
>
> sv.cc:4:23:   in 'constexpr' expansion of
> 's.std::basic_string_view<char>::operator[](1)'
> /home/jwakely/gcc/8/include/c++/8.0.1/string_view:181:22: error: call
> to non-'constexpr' function 'static void
> std::basic_string_view<_CharT, _Traits>::__out_of_range() [with _CharT
> = char; _Traits = std::char_traits<char>]'
>         __out_of_range();
>         ~~~~~~~~~~~~~~^~
>
> In a non-constant expression, with optimization it gives a
> compile-time error because of the __error__ attribute:
>
> In member function 'constexpr const _CharT&
> std::basic_string_view<_CharT,
> _Traits>::operator[](std::basic_string_view<_CharT,
> _Traits>::size_type) const [with _CharT = char; _Traits =
> std::char_traits<char>]',
>     inlined from 'int main()' at sv.cc:9:6:
> /home/jwakely/gcc/8/include/c++/8.0.1/string_view:181:22: error: call
> to 'std::basic_string_view<char>::__out_of_range' declared with
> attribute error: Index out-of-range
>         __out_of_range();
>         ~~~~~~~~~~~~~~^~
>
> Finally, without optimization, you can get a run-time failure with
> _GLIBCXX_ASSERTIONS:
>
> /home/jwakely/gcc/8/include/c++/8.0.1/string_view:178: constexpr const
> _CharT& std::basic_string_view<_CharT,
> _Traits>::operator[](std::basic_string_view<_CharT,
> _Traits>::size_type) const [with _CharT = char; _Traits =
> std::char_traits<char>; std::basic_string_view<_CharT,
> _Traits>::size_type = long unsigned int]: Assertion '__pos <
> this->_M_len' failed.
> Aborted (core dumped)
>
> Without _GLIBCXX_ASSERTIONS and without optimization the code compiles
> and runs, with undefined behaviour.
>
> This seems like the optimal set of checks (I've been working on doing
> this in other types too, so we give compile-time errors when we can
> prove the code has a bug).

Here's one way to generalize this idea. We could potentially replace
most of the lightweight __glibcxx_assert checks with this, to get
zero-overhead static checking at compile-time whenever possible (even
in constexpr functions) and have optional run-time assertions for the
remaining cases.

Attachment: patch.txt
Description: Text document