[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882
--- Comment #8 from Jonathan Wakely ---
(In reply to Tom de Vries from comment #6)
> I was not able to find out if char_traits::find is similar in that aspect,
The effects of std::char_traits::find(p, n, c) are specified in terms of
[p,p+n) and that has an implicit requirement that [p,p+n) is a valid range,
which means p+n must be reachable from p. If p+n overflows a pointer, or if p
does not point to an array of n elements, then it's an invalid range and so
undefined.
> but if so, then char_traits::find ("foobar", (size_t)-1, 'a') would
> both be well-defined and trigger the branch.
The warning doesn't say your code is undefined or ill-formed, it just says that
-1zu is larger than the largest possible object, which GCC considers to be
dubious and worth warning about.
[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882
--- Comment #7 from Tom de Vries ---
(In reply to Jonathan Wakely from comment #5)
> But what problem would we be solving? The false-positive middle end warning
> was fixed years ago by Aldy improving VRP. Are there other false
> -Wstringop-overread warnings that this would help? I hope those would also
> be fixed in the right place, not like this.
My intention here was to make the code more robust, but given that it comes at
a cost, I agree it's not a good trade-off.
Having said that, we could approach this from the other side, and look at
substr, and do:
...
diff --git a/libstdc++-v3/include/std/string_view
b/libstdc++-v3/include/std/string_view
index 735d46f2de7..40d2d91a353 100644
--- a/libstdc++-v3/include/std/string_view
+++ b/libstdc++-v3/include/std/string_view
@@ -339,7 +339,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
[[nodiscard]]
constexpr basic_string_view
- substr(size_type __pos = 0, size_type __n = npos) const noexcept(false)
+ substr(size_type __pos = 0, size_type __n =
++#if __PTRDIFF_MAX__ < __SIZE_MAX__
+/* The standard mandates npos == (size_t)-1 here, but since this
+ is used internally only, using __PTRDIFF_MAX__ shouldn't make a
+ difference in semantics, but it could help range analysis. */
+__PTRDIFF_MAX__
+#else
+npos
+#endif
+) const noexcept(false)
{
__pos = std::__sv_check(size(), __pos, "basic_string_view::substr");
const size_type __rlen = std::min(__n, _M_len - __pos);
...
which wouldn't come at a cost.
But yeah, I'm not sure it's worth the trouble.
[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882
--- Comment #6 from Tom de Vries ---
(In reply to Jonathan Wakely from comment #1)
> This function is performance critical,
I can understand that, though I'd expect the bit that actually iterates to be
performance critical.
> adding a branch to deal with a
> condition that's impossible in correct code doesn't seem desirable.
>
As pointed out in PR124905, it's well-defined to do:
...
memchr ("foobar", 'a', (size_t)-1)
...
I was not able to find out if char_traits::find is similar in that aspect, but
if so, then char_traits::find ("foobar", (size_t)-1, 'a') would both be
well-defined and trigger the branch.
> Maybe __n &= __PTRDIFF_MAX__ would be better, although still redundant in
> correct code.
Agreed.
[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882 --- Comment #5 from Jonathan Wakely --- But what problem would we be solving? The false-positive middle end warning was fixed years ago by Aldy improving VRP. Are there other false -Wstringop-overread warnings that this would help? I hope those would also be fixed in the right place, not like this.
[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882 --- Comment #4 from Jonathan Wakely --- Yeah that would make more sense.
[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882 --- Comment #3 from Jakub Jelinek --- (In reply to Jakub Jelinek from comment #2) > So perhaps [[__assume__(__n < (size_t) __PTRDIFF_MAX__)]]; instead? <= I mean.
[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #2 from Jakub Jelinek --- So perhaps [[__assume__(__n < (size_t) __PTRDIFF_MAX__)]]; instead?
[Bug libstdc++/124882] limit memchr count in char_traits::find
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=124882 --- Comment #1 from Jonathan Wakely --- This function is performance critical, adding a branch to deal with a condition that's impossible in correct code doesn't seem desirable. Maybe __n &= __PTRDIFF_MAX__ would be better, although still redundant in correct code.
