Regression testing / Reproducability: Fix MacOS and Linux differences for rand() and qsort()#9574
Regression testing / Reproducability: Fix MacOS and Linux differences for rand() and qsort()#9574fredowski wants to merge 6 commits into
Conversation
rand() produces different sequences on glibc from linux versus Apple libc. mt19937 produces the same sequence on all platforms. This affects 26 tests in gpl and 3 tests in rsz plus the python tests which test the same. This reduces the regression test failures on MacOS from 60 to 11. Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
The new platform deterministic mt19937 random number generator produces new reference files. This commit contains the updated golden reference files. Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
abc uses rand() in various places. The glibc version and the apple version of rand() produce different sequences. This produces different results on MacOS and Linux. This patch provides rand() and qsort() for the MacOS build to achieve the same results. As this version mirrors the glibc behaviour, the golden regression files can remain untouched. This reduces the regression failures from 11 to 4 - mainly for rmp module tests. Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
|
Note The number of changes in this pull request is too large for Gemini Code Assist to generate a review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * Simplified implementation of the merge sort algorithm used by glibc. | ||
| */ | ||
|
|
||
| #include <stdlib.h> |
There was a problem hiding this comment.
warning: inclusion of deprecated C++ header 'stdlib.h'; consider using 'cstdlib' instead [modernize-deprecated-headers]
| #include <stdlib.h> | |
| #include <cstdlib> |
| */ | ||
|
|
||
| #include <stdlib.h> | ||
| #include <string.h> |
There was a problem hiding this comment.
warning: inclusion of deprecated C++ header 'string.h'; consider using 'cstring' instead [modernize-deprecated-headers]
| #include <string.h> | |
| #include <cstring> |
| #include <string.h> | ||
|
|
||
| /* Threshold below which we use insertion sort */ | ||
| #define INSERTION_THRESHOLD 4 |
There was a problem hiding this comment.
warning: macro 'INSERTION_THRESHOLD' defines an integral constant; prefer an enum instead [modernize-macro-to-enum]
#define INSERTION_THRESHOLD 4
^|
|
||
| /* Threshold below which we use insertion sort */ | ||
| #define INSERTION_THRESHOLD 4 | ||
|
|
There was a problem hiding this comment.
warning: replace macro with enum [modernize-macro-to-enum]
| enum { | |
| INSERTION_THRESHOLD = 4}; | |
| * TYPE_3: degree 31, separation 3 | ||
| */ | ||
|
|
||
| #define DEG_3 31 |
There was a problem hiding this comment.
warning: macro 'DEG_3' defines an integral constant; prefer an enum instead [modernize-macro-to-enum]
#define DEG_3 31
^| _init_state(seed); | ||
| } | ||
|
|
||
| int rand(void) |
There was a problem hiding this comment.
warning: function 'rand' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| int rand(void) | |
| static int rand(void) |
| _init_state(seed); | ||
| } | ||
|
|
||
| int rand(void) |
There was a problem hiding this comment.
warning: no header providing "rand" is directly included [misc-include-cleaner]
int rand(void)
^| _init_state(seed); | ||
| } | ||
|
|
||
| int rand(void) |
There was a problem hiding this comment.
warning: redundant void argument list in function definition [modernize-redundant-void-arg]
| int rand(void) | |
| int rand() |
| int rand(void) | ||
| { | ||
| if (!_initialized) | ||
| _init_state(1); |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| _init_state(1); | |
| if (!_initialized) { | |
| _init_state(1); | |
| } |
| if (!_initialized) | ||
| _init_state(1); | ||
|
|
||
| unsigned int result = _state[_fptr] + _state[_rptr]; |
There was a problem hiding this comment.
warning: no header providing "result" is directly included [misc-include-cleaner]
unsigned int result = _state[_fptr] + _state[_rptr];
^…ange Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
| unsigned int result = _state[_fptr] + _state[_rptr]; | ||
| _state[_fptr] = result; | ||
|
|
||
| int ret = (int)((result >> 1) & 0x7fffffffU); |
There was a problem hiding this comment.
warning: no header providing "ret" is directly included [misc-include-cleaner]
int ret = (int)((result >> 1) & 0x7fffffffU);
^|
|
||
| _fptr++; | ||
| if (_fptr >= DEG_3) | ||
| _fptr = 0; |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| _fptr = 0; | |
| if (_fptr >= DEG_3) { | |
| _fptr = 0; | |
| } |
| _fptr = 0; | ||
| _rptr++; | ||
| if (_rptr >= DEG_3) | ||
| _rptr = 0; |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| _rptr = 0; | |
| if (_rptr >= DEG_3) { | |
| _rptr = 0; | |
| } |
| return ret; | ||
| } | ||
|
|
||
| int rand_r(unsigned int *seedp) |
There was a problem hiding this comment.
warning: function 'rand_r' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| int rand_r(unsigned int *seedp) | |
| static int rand_r(unsigned int *seedp) |
I ran clang-tidy on the two new files. No functional change. Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
| char *b = (char *)base; | ||
| char *tmp = (char *)malloc(size); | ||
| if (!tmp) | ||
| return; |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| return; | |
| if (!tmp) { | |
| return; | |
| } |
| dst += size; | ||
| } | ||
| if (n1 > 0) | ||
| memcpy(dst, src1, n1 * size); |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| memcpy(dst, src1, n1 * size); | |
| if (n1 > 0) { | |
| memcpy(dst, src1, n1 * size); | |
| } |
| if (n1 > 0) | ||
| memcpy(dst, src1, n1 * size); | ||
| if (n2 > 0) | ||
| memcpy(dst, src2, n2 * size); |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| memcpy(dst, src2, n2 * size); | |
| if (n2 > 0) { | |
| memcpy(dst, src2, n2 * size); | |
| } |
| void qsort(void *base, size_t nmemb, size_t size, | ||
| int (*compar)(const void *, const void *)) { | ||
| if (nmemb < 2) | ||
| return; |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| return; | |
| if (nmemb < 2) { | |
| return; | |
| } |
| * TYPE_3: degree 31, separation 3 | ||
| */ | ||
|
|
||
| #define DEG_3 31 |
There was a problem hiding this comment.
warning: macro 'DEG_3' defines an integral constant; prefer an enum instead [modernize-macro-to-enum]
#define DEG_3 31
^|
|
||
| void srand(unsigned int seed) { _init_state(seed); } | ||
|
|
||
| int rand(void) { |
There was a problem hiding this comment.
warning: function 'rand' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| int rand(void) { | |
| static int rand(void) { |
|
|
||
| void srand(unsigned int seed) { _init_state(seed); } | ||
|
|
||
| int rand(void) { |
There was a problem hiding this comment.
warning: no header providing "rand" is directly included [misc-include-cleaner]
int rand(void) {
^|
|
||
| void srand(unsigned int seed) { _init_state(seed); } | ||
|
|
||
| int rand(void) { |
There was a problem hiding this comment.
warning: redundant void argument list in function definition [modernize-redundant-void-arg]
| int rand(void) { | |
| int rand() { |
|
|
||
| int rand(void) { | ||
| if (!_initialized) | ||
| _init_state(1); |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| _init_state(1); | |
| if (!_initialized) { | |
| _init_state(1); | |
| } |
| if (!_initialized) | ||
| _init_state(1); | ||
|
|
||
| unsigned int result = _state[_fptr] + _state[_rptr]; |
There was a problem hiding this comment.
warning: no header providing "result" is directly included [misc-include-cleaner]
unsigned int result = _state[_fptr] + _state[_rptr];
^Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
| unsigned int result = _state[_fptr] + _state[_rptr]; | ||
| _state[_fptr] = result; | ||
|
|
||
| int ret = (int)((result >> 1) & 0x7fffffffU); |
There was a problem hiding this comment.
warning: no header providing "ret" is directly included [misc-include-cleaner]
int ret = (int)((result >> 1) & 0x7fffffffU);
^|
|
||
| _fptr++; | ||
| if (_fptr >= DEG_3) | ||
| _fptr = 0; |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| _fptr = 0; | |
| if (_fptr >= DEG_3) { | |
| _fptr = 0; | |
| } |
| _fptr = 0; | ||
| _rptr++; | ||
| if (_rptr >= DEG_3) | ||
| _rptr = 0; |
There was a problem hiding this comment.
warning: statement should be inside braces [google-readability-braces-around-statements]
| _rptr = 0; | |
| if (_rptr >= DEG_3) { | |
| _rptr = 0; | |
| } |
| return ret; | ||
| } | ||
|
|
||
| int rand_r(unsigned int *seedp) { |
There was a problem hiding this comment.
warning: function 'rand_r' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| int rand_r(unsigned int *seedp) { | |
| static int rand_r(unsigned int *seedp) { |
|
Instead of pulling LGPL code, could we just stub these function with the c++ version std::mt1997 etc. or std::stable_sort |
|
Yes this exploded in clang-tidy, google code hints and friends. I will remove the last part with rand() and qsort() and just do a new pr. |
2 participants
As already discussed in #9480 the regression yields different results on MacOS and on Linux. Today around 60 tests fail on MacOS when running a part of the regression in bazel with
bazelisk test src/...One major reason are different implementations of rand() and qsort() between glibc and the apple functions. qsort() gives different results for equal elements. This PR addresses this problem by
I tested this on MacOS and debian 12 x86_64. On MacOS the failed tests are reduced to 5. On linux x86_64 there are no regression failures.
I think these changes have no impact on actual function but only on regression and reproducability. You can see the small differences in the updated golden files.