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

[C++] arrow-decimal-benchmark accesses benchmark data out of bounds #43211

Closed
pitrou opened this issue Jul 10, 2024 · 1 comment
Closed

[C++] arrow-decimal-benchmark accesses benchmark data out of bounds #43211

pitrou opened this issue Jul 10, 2024 · 1 comment

Comments

@pitrou
Copy link
Member

pitrou commented Jul 10, 2024

Describe the bug, including details regarding any error messages, version, and platform.

Some of the decimal benchmarks access their benchmark data in strides, without checking that the accesses fall withing bounds. Example here, where kValueSize (10) is actually not a multiple of 4:

for (int x = 0; x < kValueSize; x += 4) {
auto equal = v1[x] == v2[x];
benchmark::DoNotOptimize(equal);
auto less_than_or_equal = v1[x + 1] <= v2[x + 1];
benchmark::DoNotOptimize(less_than_or_equal);
auto greater_than_or_equal1 = v1[x + 2] >= v2[x + 2];
benchmark::DoNotOptimize(greater_than_or_equal1);
auto greater_than_or_equal2 = v1[x + 3] >= v1[x + 3];
benchmark::DoNotOptimize(greater_than_or_equal2);
}

This manifests in crashes on some of the macOS benchmark machines, and the issue can be reproduced locally using Valgrind:

$ ARROW_DEFAULT_MEMORY_POOL=system valgrind <build dir>/relwithdebinfo/arrow-decimal-benchmark --benchmark_repetitions=6 --benchmark_min_time=0.050000s

Component(s)

C++

@pitrou pitrou self-assigned this Jul 10, 2024
pitrou added a commit to pitrou/arrow that referenced this issue Jul 10, 2024
…ccesses

A side effect is that this will break benchmark history because the previous iterations/s calculation was wrong, even though actual performance is unchanged.
pitrou added a commit that referenced this issue Jul 15, 2024
#43212)

### Rationale for this change

Some of the decimal benchmarks access their benchmark data in strides, without checking that the accesses fall within bounds.

A side effect is that this will break benchmark history because the iterations/s calculation was wrong, even though actual performance is unchanged.

### Are these changes tested?

By the continuous benchmarking jobs.

### Are there any user-facing changes?

No.
* GitHub Issue: #43211

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou pitrou added this to the 18.0.0 milestone Jul 15, 2024
@pitrou
Copy link
Member Author

pitrou commented Jul 15, 2024

Issue resolved by pull request 43212
#43212

@pitrou pitrou closed this as completed Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant