-
Notifications
You must be signed in to change notification settings - Fork 419
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
Fixup int overflow in reducers unit tests #3514
Conversation
Clang UBSan reports: core/unit_test/TestReducers.hpp:387:22: runtime error: signed integer overflow: 2038431744 * 3 cannot be represented in type 'int' core/unit_test/TestReducers.hpp:71:64: runtime error: signed integer overflow: 2038431744 * 3 cannot be represented in type 'int' core/unit_test/TestReducers.hpp:189:13: runtime error: signed integer overflow: 2038431744 * 3 cannot be represented in type 'int'
(The unit test produces N random numbers between 1 and 4 and multiply them) |
core/unit_test/TestReducers.hpp
Outdated
@@ -1020,7 +1020,7 @@ struct TestReducers { | |||
|
|||
static void execute_integer() { | |||
test_sum(10001); | |||
test_prod(35); | |||
test_prod(sizeof(Scalar) > 4 ? 35 : 19); // avoid int overflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather branch on the largest representable number such that
std::numeric_limits<Scalar>::max() > std::pow(4,n)
n < std::log(std::numeric_limits<Scalar>::max())/std::log(4);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can expand on the comment but your suggestion is imperfect too because it use knowledge of the fact test_prod
generates random numbers between 1 and 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record std::log(std::numeric_limits<int>::max())/std::log(4)
is 15.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so why is 19 guaranteed to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with me.
Retest this please |
Found along the way while working on #3512
Clang UBSan was reporting