Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unspecified Behavior in Multi-Digit Optimizations #94

Closed
Alexhuszagh opened this issue Aug 21, 2021 · 2 comments · Fixed by #95
Closed

Unspecified Behavior in Multi-Digit Optimizations #94

Alexhuszagh opened this issue Aug 21, 2021 · 2 comments · Fixed by #95

Comments

@Alexhuszagh
Copy link
Contributor

I've noticed a similar issue in fast-float-rust, where checking the bounds of the array produces undefined behavior (in Rust), and similar behavior (unspecified) exists in fast_float.

Quoting the C++14 standard:

If two pointers p and q of the same type point to different objects that are not members of the same object or elements of the same array or to different functions, or if only one of them is null, the results of p<q, p>q, p<=q, and p>=q are unspecified.

Therefore, the following code is unspecified behavior (according to cppreference, it is undefined behavior, however, I am not a language lawyer, so I am unsure of the true semantics).

if ((p + 8 <= pend) && is_made_of_eight_digits_fast(p)) {
i = i * 100000000 + parse_eight_digits_unrolled(p); // in rare cases, this will overflow, but that's ok
p += 8;
if ((p + 8 <= pend) && is_made_of_eight_digits_fast(p)) {
i = i * 100000000 + parse_eight_digits_unrolled(p); // in rare cases, this will overflow, but that's ok
p += 8;

Specifically, since pend is one-past-the-end of the array, the last valid point of comparison, the compiler could optimize this as always being true, since p + 8 <= pend must always be true, an undesirable outcome.

The compliant solution is as follows:

#include <iterator>
...

  if ((std::distance(p, pend) >= 8) && is_made_of_eight_digits_fast(p)) {
    i = i * 100000000 + parse_eight_digits_unrolled(p); // in rare cases, this will overflow, but that's ok
    p += 8;
    if ((std::distance(p, pend) >= 8) && is_made_of_eight_digits_fast(p)) {
      i = i * 100000000 + parse_eight_digits_unrolled(p); // in rare cases, this will overflow, but that's ok
      p += 8;
    }
  }

Another example in parse_decimal also needs to be patched.

@lemire
Copy link
Member

lemire commented Aug 21, 2021

Yes. You are correct, this is (technically) undefined behaviour. Thanks for pointing it out.

@lemire
Copy link
Member

lemire commented Aug 21, 2021

I expect that your fix is correct.

lemire added a commit that referenced this issue Aug 21, 2021
Fixes #94, with unspecified behavior in pointer comparisons.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants