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

32-bit safety in big random number #143

Merged
merged 1 commit into from Dec 5, 2019
Merged

32-bit safety in big random number #143

merged 1 commit into from Dec 5, 2019

Conversation

@barak
Copy link

barak commented Dec 5, 2019

This patch fixes a compile-time warning and test case failure on
32-bit architectures, including i386.

cd /<<PKGBUILDDIR>>/obj-i686-linux-gnu/tests && /usr/bin/c++   -I/<<PKGBUILDDIR>>/include  -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fopenmp -Wall -Wpedantic -Wunused-parameter   -std=gnu++11 -o CMakeFiles/ensmallen_tests.dir/cmaes_test.cpp.o -c /<<PKGBUILDDIR>>/tests/cmaes_test.cpp
/<<PKGBUILDDIR>>/tests/callbacks_test.cpp: In function ‘void ____C_A_T_C_H____T_E_S_T____46()’:
/<<PKGBUILDDIR>>/tests/callbacks_test.cpp:397:28: warning: unsigned conversion from ‘long long int’ to ‘size_t’ {aka ‘unsigned int’} changes value from ‘10000000000’ to ‘1410065408’ [-Woverflow]
  397 |   StandardSGD s(0.0003, 1, 10000000000, -10);
  |                            ^~~~~~~~~~~
/<<PKGBUILDDIR>>/tests/callbacks_test.cpp: In function ‘void ____C_A_T_C_H____T_E_S_T____54()’:
/<<PKGBUILDDIR>>/tests/callbacks_test.cpp:473:28: warning: unsigned conversion from ‘long long int’ to ‘size_t’ {aka ‘unsigned int’} changes value from ‘10000000000’ to ‘1410065408’ [-Woverflow]
  473 |   StandardSGD s(0.0003, 1, 10000000000, -100, true);
  |                            ^~~~~~~~~~~

...

Test project /<<PKGBUILDDIR>>/obj-i686-linux-gnu
Start 1: ensmallen_tests
1/1 Test #1: ensmallen_tests ..................***Failed  5163.40 sec

The SGD maxIterations parameter to the SGD routine is stored in a
size_t. Arguably inappropriate; it belongs in something guaranteed to
be at least 64 bits, perhaps long long int.

Be that as it may, a test routine was passing a number meant to be so
large it would not be exceeded, to test a mechanism that should
terminate the optimization early. The large number chosen was
10000000000, ten billion, which exceeds the maximum value of an
unsigned 32-bit integer of 4 billion plus change.

This patch changes the maxIterations parameter passed to two billion,
2000000000, which is both sufficiently large for the purposes here and
safely below the maximum value for a 32-bit signed integer.

Barak A. Pearlmutter
This patch fixes a compile-time warning and test case failure on
32-bit architectures, including i386.

    cd /<<PKGBUILDDIR>>/obj-i686-linux-gnu/tests && /usr/bin/c++   -I/<<PKGBUILDDIR>>/include  -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fopenmp -Wall -Wpedantic -Wunused-parameter   -std=gnu++11 -o CMakeFiles/ensmallen_tests.dir/cmaes_test.cpp.o -c /<<PKGBUILDDIR>>/tests/cmaes_test.cpp
    /<<PKGBUILDDIR>>/tests/callbacks_test.cpp: In function ‘void ____C_A_T_C_H____T_E_S_T____46()’:
    /<<PKGBUILDDIR>>/tests/callbacks_test.cpp:397:28: warning: unsigned conversion from ‘long long int’ to ‘size_t’ {aka ‘unsigned int’} changes value from ‘10000000000’ to ‘1410065408’ [-Woverflow]
      397 |   StandardSGD s(0.0003, 1, 10000000000, -10);
	  |                            ^~~~~~~~~~~
    /<<PKGBUILDDIR>>/tests/callbacks_test.cpp: In function ‘void ____C_A_T_C_H____T_E_S_T____54()’:
    /<<PKGBUILDDIR>>/tests/callbacks_test.cpp:473:28: warning: unsigned conversion from ‘long long int’ to ‘size_t’ {aka ‘unsigned int’} changes value from ‘10000000000’ to ‘1410065408’ [-Woverflow]
      473 |   StandardSGD s(0.0003, 1, 10000000000, -100, true);
	  |                            ^~~~~~~~~~~

    ...

    Test project /<<PKGBUILDDIR>>/obj-i686-linux-gnu
	Start 1: ensmallen_tests
    1/1 Test #1: ensmallen_tests ..................***Failed  5163.40 sec

The SGD maxIterations parameter to the SGD routine is stored in a
size_t. Arguably inappropriate; it belongs in something guaranteed to
be at least 64 bits, perhaps long long int.

Be that as it may, a test routine was passing a number meant to be so
large it would not be exceeded, to test a mechanism that should
terminate the optimization early. The large number chosen was
10000000000, ten billion, which exceeds the maximum value of an
unsigned 32-bit integer of 4 billion plus change.

This patch changes the maxIterations parameter passed to two billion,
2000000000, which is both sufficiently large for the purposes here and
safely below the maximum value for a 32-bit signed integer.
@rcurtin
rcurtin approved these changes Dec 5, 2019
Copy link
Member

rcurtin left a comment

Looks good to me, thanks for the fix! We used to have some i386 build machines but don't anymore. I think it's fine to leave the maximum iterations as a size_t---really the goal of 10B iterations was just to be "really big but not unlimited". :)

@zoq
zoq approved these changes Dec 5, 2019
Copy link
Member

zoq left a comment

Thanks, looks good to me.

@zoq zoq merged commit d70b12a into mlpack:master Dec 5, 2019
2 checks passed
2 checks passed
Static Code Analysis Checks Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq zoq removed the s: needs review label Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.