Enable string_view assertions

[Jonathan Wakely]

(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).



>--- /dev/null
>+++ b/libstdc++-v3/testsuite/21_strings/basic_string_view/element_access/char/2_neg.cc
>@@ -0,0 +1,30 @@
>+// { dg-do run { xfail *-*-* } }
>+// { dg-options "-std=gnu++17 -O0" }
>+// { dg-require-debug-mode "" }

Instead of requiring debug mode we could just define
_GLIBCXX_ASSERTIONS.

We should also add tests for the compile-time errors, both in and out
of constant expressions.