Enable string_view assertions

Hi

     Following PR 78420 patch I realized that we can use similar 
technique to have assertions in string_view implementations.

     I also rename testsuite files expected to XFAIL as they should have 
a trailing '_neg'. It fixes 4 XPASS when run in debug mode

     Note that I also try to use a static_assert when 
__builtin_constant_v return true but it is not constant enough. Could it 
be a gcc issue ?

     Tested under Linux x86_64 normal and debug modes.

     Ok to commit ? Now ?

François

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

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

On 14 March 2018 at 23:27, Jonathan Wakely wrote:
> 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.

Thinking about this some more, we probably don't want to do this for
most __glibcxx_assert uses, because it's probably rare that we can
statically detect most errors in something like
std::vector::operator[]. I doubt we would catch many bugs that way, as
most bugs would involve non-constant indices and vectors that have
changed size dynamically at run-time.

It *might* be useful  in vector::front, vector::back, string::front,
deque::front etc. to catch bugs where users do:

std::string s;
// ...
someFunction(&s.front(), s.size());

It seems most valuable in constexpr functions (where we definitely
expect constant arguments in many cases) and where run-time arguments
will typically be constants, like in the attached patch for atomic
objects.
diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index a1fadcd8056..0f6783d257c 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -186,9 +186,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     clear(memory_order __m = memory_order_seq_cst) noexcept
     {
       memory_order __b = __m & __memory_order_mask;
-      __glibcxx_assert(__b != memory_order_consume);
-      __glibcxx_assert(__b != memory_order_acquire);
-      __glibcxx_assert(__b != memory_order_acq_rel);
+      __glibcxx_assert2(__b != memory_order_consume, __invalid_memory_order());
+      __glibcxx_assert2(__b != memory_order_acquire, __invalid_memory_order());
+      __glibcxx_assert2(__b != memory_order_acq_rel, __invalid_memory_order());
 
       __atomic_clear (&_M_i, __m);
     }
@@ -197,9 +197,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     clear(memory_order __m = memory_order_seq_cst) volatile noexcept
     {
       memory_order __b = __m & __memory_order_mask;
-      __glibcxx_assert(__b != memory_order_consume);
-      __glibcxx_assert(__b != memory_order_acquire);
-      __glibcxx_assert(__b != memory_order_acq_rel);
+      __glibcxx_assert2(__b != memory_order_consume, __invalid_memory_order());
+      __glibcxx_assert2(__b != memory_order_acquire, __invalid_memory_order());
+      __glibcxx_assert2(__b != memory_order_acq_rel, __invalid_memory_order());
 
       __atomic_clear (&_M_i, __m);
     }
@@ -208,6 +208,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     static constexpr __atomic_flag_data_type
     _S_init(bool __i)
     { return __i ? __GCC_ATOMIC_TEST_AND_SET_TRUEVAL : 0; }
+
+    static void
+    __invalid_memory_order()
+    __attribute__((__error__("invalid memory order for atomic_flag::clear")));
   };
 
 
@@ -367,9 +371,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       store(__int_type __i, memory_order __m = memory_order_seq_cst) noexcept
       {
 	memory_order __b = __m & __memory_order_mask;
-	__glibcxx_assert(__b != memory_order_acquire);
-	__glibcxx_assert(__b != memory_order_acq_rel);
-	__glibcxx_assert(__b != memory_order_consume);
+	__glibcxx_assert2(__b != memory_order_acquire, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b != memory_order_consume, __invalid());
 
 	__atomic_store_n(&_M_i, __i, __m);
       }
@@ -379,9 +383,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	    memory_order __m = memory_order_seq_cst) volatile noexcept
       {
 	memory_order __b = __m & __memory_order_mask;
-	__glibcxx_assert(__b != memory_order_acquire);
-	__glibcxx_assert(__b != memory_order_acq_rel);
-	__glibcxx_assert(__b != memory_order_consume);
+	__glibcxx_assert2(__b != memory_order_acquire, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b != memory_order_consume, __invalid());
 
 	__atomic_store_n(&_M_i, __i, __m);
       }
@@ -390,8 +394,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       load(memory_order __m = memory_order_seq_cst) const noexcept
       {
 	memory_order __b = __m & __memory_order_mask;
-	__glibcxx_assert(__b != memory_order_release);
-	__glibcxx_assert(__b != memory_order_acq_rel);
+	__glibcxx_assert2(__b != memory_order_release, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
 
 	return __atomic_load_n(&_M_i, __m);
       }
@@ -400,8 +404,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       load(memory_order __m = memory_order_seq_cst) const volatile noexcept
       {
 	memory_order __b = __m & __memory_order_mask;
-	__glibcxx_assert(__b != memory_order_release);
-	__glibcxx_assert(__b != memory_order_acq_rel);
+	__glibcxx_assert2(__b != memory_order_release, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
 
 	return __atomic_load_n(&_M_i, __m);
       }
@@ -427,9 +431,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	memory_order __b2 = __m2 & __memory_order_mask;
 	memory_order __b1 = __m1 & __memory_order_mask;
-	__glibcxx_assert(__b2 != memory_order_release);
-	__glibcxx_assert(__b2 != memory_order_acq_rel);
-	__glibcxx_assert(__b2 <= __b1);
+	__glibcxx_assert2(__b2 != memory_order_release, __invalid());
+	__glibcxx_assert2(__b2 != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b2 <= __b1, __invalid());
 
 	return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 1, __m1, __m2);
       }
@@ -441,9 +445,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	memory_order __b2 = __m2 & __memory_order_mask;
 	memory_order __b1 = __m1 & __memory_order_mask;
-	__glibcxx_assert(__b2 != memory_order_release);
-	__glibcxx_assert(__b2 != memory_order_acq_rel);
-	__glibcxx_assert(__b2 <= __b1);
+	__glibcxx_assert2(__b2 != memory_order_release, __invalid());
+	__glibcxx_assert2(__b2 != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b2 <= __b1, __invalid());
 
 	return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 1, __m1, __m2);
       }
@@ -470,9 +474,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	memory_order __b2 = __m2 & __memory_order_mask;
 	memory_order __b1 = __m1 & __memory_order_mask;
-	__glibcxx_assert(__b2 != memory_order_release);
-	__glibcxx_assert(__b2 != memory_order_acq_rel);
-	__glibcxx_assert(__b2 <= __b1);
+	__glibcxx_assert2(__b2 != memory_order_release, __invalid());
+	__glibcxx_assert2(__b2 != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b2 <= __b1, __invalid());
 
 	return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 0, __m1, __m2);
       }
@@ -485,9 +489,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	memory_order __b2 = __m2 & __memory_order_mask;
 	memory_order __b1 = __m1 & __memory_order_mask;
 
-	__glibcxx_assert(__b2 != memory_order_release);
-	__glibcxx_assert(__b2 != memory_order_acq_rel);
-	__glibcxx_assert(__b2 <= __b1);
+	__glibcxx_assert2(__b2 != memory_order_release, __invalid());
+	__glibcxx_assert2(__b2 != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b2 <= __b1, __invalid());
 
 	return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 0, __m1, __m2);
       }
@@ -557,6 +561,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       fetch_xor(__int_type __i,
 		memory_order __m = memory_order_seq_cst) volatile noexcept
       { return __atomic_fetch_xor(&_M_i, __i, __m); }
+
+    private:
+      static void
+      __invalid()
+      __attribute__((__error__("invalid memory order for atomic operation")));
     };
 
 
@@ -684,9 +693,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
         memory_order __b = __m & __memory_order_mask;
 
-	__glibcxx_assert(__b != memory_order_acquire);
-	__glibcxx_assert(__b != memory_order_acq_rel);
-	__glibcxx_assert(__b != memory_order_consume);
+	__glibcxx_assert2(__b != memory_order_acquire, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b != memory_order_consume, __invalid());
 
 	__atomic_store_n(&_M_p, __p, __m);
       }
@@ -696,9 +705,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	    memory_order __m = memory_order_seq_cst) volatile noexcept
       {
 	memory_order __b = __m & __memory_order_mask;
-	__glibcxx_assert(__b != memory_order_acquire);
-	__glibcxx_assert(__b != memory_order_acq_rel);
-	__glibcxx_assert(__b != memory_order_consume);
+	__glibcxx_assert2(__b != memory_order_acquire, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b != memory_order_consume, __invalid());
 
 	__atomic_store_n(&_M_p, __p, __m);
       }
@@ -707,8 +716,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       load(memory_order __m = memory_order_seq_cst) const noexcept
       {
 	memory_order __b = __m & __memory_order_mask;
-	__glibcxx_assert(__b != memory_order_release);
-	__glibcxx_assert(__b != memory_order_acq_rel);
+	__glibcxx_assert2(__b != memory_order_release, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
 
 	return __atomic_load_n(&_M_p, __m);
       }
@@ -717,8 +726,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       load(memory_order __m = memory_order_seq_cst) const volatile noexcept
       {
 	memory_order __b = __m & __memory_order_mask;
-	__glibcxx_assert(__b != memory_order_release);
-	__glibcxx_assert(__b != memory_order_acq_rel);
+	__glibcxx_assert2(__b != memory_order_release, __invalid());
+	__glibcxx_assert2(__b != memory_order_acq_rel, __invalid());
 
 	return __atomic_load_n(&_M_p, __m);
       }
@@ -730,7 +739,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	return __atomic_exchange_n(&_M_p, __p, __m);
       }
 
-
       _GLIBCXX_ALWAYS_INLINE __pointer_type
       exchange(__pointer_type __p,
 	       memory_order __m = memory_order_seq_cst) volatile noexcept
@@ -745,9 +753,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	memory_order __b2 = __m2 & __memory_order_mask;
 	memory_order __b1 = __m1 & __memory_order_mask;
-	__glibcxx_assert(__b2 != memory_order_release);
-	__glibcxx_assert(__b2 != memory_order_acq_rel);
-	__glibcxx_assert(__b2 <= __b1);
+	__glibcxx_assert2(__b2 != memory_order_release, __invalid());
+	__glibcxx_assert2(__b2 != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b2 <= __b1, __invalid());
 
 	return __atomic_compare_exchange_n(&_M_p, &__p1, __p2, 0, __m1, __m2);
       }
@@ -760,9 +768,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	memory_order __b2 = __m2 & __memory_order_mask;
 	memory_order __b1 = __m1 & __memory_order_mask;
 
-	__glibcxx_assert(__b2 != memory_order_release);
-	__glibcxx_assert(__b2 != memory_order_acq_rel);
-	__glibcxx_assert(__b2 <= __b1);
+	__glibcxx_assert2(__b2 != memory_order_release, __invalid());
+	__glibcxx_assert2(__b2 != memory_order_acq_rel, __invalid());
+	__glibcxx_assert2(__b2 <= __b1, __invalid());
 
 	return __atomic_compare_exchange_n(&_M_p, &__p1, __p2, 0, __m1, __m2);
       }
@@ -786,6 +794,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       fetch_sub(ptrdiff_t __d,
 		memory_order __m = memory_order_seq_cst) volatile noexcept
       { return __atomic_fetch_sub(&_M_p, _M_type_size(__d), __m); }
+
+    private:
+      static void
+      __invalid()
+      __attribute__((__error__("invalid memory order for atomic operation")));
     };
 
   // @} group atomics

On 15 March 2018 at 00:18, Jonathan Wakely wrote:
> On 14 March 2018 at 23:27, Jonathan Wakely wrote:
>> 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.
>
> Thinking about this some more, we probably don't want to do this for
> most __glibcxx_assert uses, because it's probably rare that we can
> statically detect most errors in something like
> std::vector::operator[]. I doubt we would catch many bugs that way, as
> most bugs would involve non-constant indices and vectors that have
> changed size dynamically at run-time.
>
> It *might* be useful  in vector::front, vector::back, string::front,
> deque::front etc. to catch bugs where users do:
>
> std::string s;
> // ...
> someFunction(&s.front(), s.size());
>
> It seems most valuable in constexpr functions (where we definitely
> expect constant arguments in many cases) and where run-time arguments
> will typically be constants, like in the attached patch for atomic
> objects.

Those patches aren't quite right. It's OK to make compilation fail
when undefined behaviour is detected in a constant expression, but if
we detect it in other contexts (because optimization means all the
inputs are known at compile-time) we need to use
__attribute((__warning__(""))) not __error__. Those cases are only
undefined if the code is executed, so we can only warn and then fail
at run-time.