Enable string_view assertions

[Jonathan Wakely]

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.
-------------- next part --------------
diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
index 1eb4679f67c..3342af3a4f0 100644
--- a/libstdc++-v3/include/bits/c++config
+++ b/libstdc++-v3/include/bits/c++config
@@ -462,6 +462,12 @@ namespace std
 # define __glibcxx_assert(_Condition)
 #endif
 
+#define __glibcxx_assert2(_Condition, _Action)			  \
+  do {									  \
+    if (__builtin_constant_p((_Condition))) { if (!(_Condition)) _Action; \
+    } else { __glibcxx_assert(_Condition); }				  \
+  } while (false)
+
 // Macros for race detectors.
 // _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(A) and
 // _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(A) should be used to explain
diff --git a/libstdc++-v3/include/std/string_view b/libstdc++-v3/include/std/string_view
index 9f39df853e8..a7c74d9d48f 100644
--- a/libstdc++-v3/include/std/string_view
+++ b/libstdc++-v3/include/std/string_view
@@ -169,8 +169,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       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);
+	__glibcxx_assert2(__pos < this->_M_len, __undefined());
 	return *(this->_M_str + __pos);
       }
 
@@ -187,16 +186,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       constexpr const _CharT&
       front() const noexcept
       {
-	// TODO: Assert to restore in a way compatible with the constexpr.
-	// __glibcxx_assert(this->_M_len > 0);
+	__glibcxx_assert2(this->_M_len > 0, __undefined());
 	return *this->_M_str;
       }
 
       constexpr const _CharT&
       back() const noexcept
       {
-	// TODO: Assert to restore in a way compatible with the constexpr.
-	// __glibcxx_assert(this->_M_len > 0);
+	__glibcxx_assert2(this->_M_len > 0, __undefined());
 	return *(this->_M_str + this->_M_len - 1);
       }
 
@@ -209,7 +206,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       constexpr void
       remove_prefix(size_type __n) noexcept
       {
-	__glibcxx_assert(this->_M_len >= __n);
+	__glibcxx_assert2(this->_M_len >= __n, __undefined());
 	this->_M_str += __n;
 	this->_M_len -= __n;
       }
@@ -404,6 +401,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
       
     private:
+      static void
+      __undefined() __attribute__((__error__("undefined behaviour detected")));
 
       static constexpr int
       _S_compare(size_type __n1, size_type __n2) noexcept