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

Avoid integer overflow in volk_8ic_x2_multiply_conjugate_16ic corner case #701

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

argilo
Copy link
Member

@argilo argilo commented Nov 10, 2023

While looking for flaky tests with ctest --repeat until-fail:<n>, the qa_volk_8ic_x2_multiply_conjugate_16ic test turned out to be flaky. Failures are rare, but can be induced with the following change to the test framework:

diff --git a/lib/qa_utils.cc b/lib/qa_utils.cc
index 4be7b8a..21a1e34 100644
--- a/lib/qa_utils.cc
+++ b/lib/qa_utils.cc
@@ -99,7 +99,7 @@ void load_random_data(void* data, volk_type_t type, unsigned int n)
             if (type.is_signed) {
                 std::uniform_int_distribution<int16_t> uniform_dist(
                     std::numeric_limits<int8_t>::min(),
-                    std::numeric_limits<int8_t>::max());
+                    -127);
                 for (unsigned int i = 0; i < n; i++)
                     ((int8_t*)data)[i] = uniform_dist(rnd_engine);
             } else {

The root cause is that the corner case aVector[i] = -128-128j, bVector[i] = -128-128j produces an output of 32768+0j, and the real part is too large to fit in a signed 16-bit integer. The SIMD protokernels use saturation arithmetic (producing 32767+0j, while the generic protokernel (and tail processing in the other protokernels) overflows and produces -32768+0j. To fix the problem, I've switched to saturation arithmetic everywhere, which seems more appropriate for a DSP context, and avoids Undefined Behaviour.

Only the real part of the result needs to be clamped, because the imaginary part cannot overflow.

…case

Signed-off-by: Clayton Smith <argilo@gmail.com>
@argilo argilo added the bug label Nov 10, 2023
@argilo
Copy link
Member Author

argilo commented Nov 10, 2023

The test failure is due to #663.

Copy link
Contributor

@jdemel jdemel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jdemel jdemel merged commit 76c421e into gnuradio:main Dec 1, 2023
32 of 33 checks passed
@argilo argilo deleted the fix-8ic-multiply-conjugate branch December 2, 2023 16:47
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Avoid integer overflow in volk_8ic_x2_multiply_conjugate_16ic corner case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants