On 14 March 2018 at 22:52, Jonathan Wakely <jwakely....@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
