fix valgrind issues with long double module test#9709
Merged
oranagra merged 3 commits intoredis:unstablefrom Nov 1, 2021
oranagra:fix_valgrind_test_issues
Merged
fix valgrind issues with long double module test#9709oranagra merged 3 commits intoredis:unstablefrom oranagra:fix_valgrind_test_issues
oranagra merged 3 commits intoredis:unstablefrom
oranagra:fix_valgrind_test_issues
Conversation
Member
Author
|
@yossigo can you please test it for me on ARM (32 / 64)? |
First, since we use %.17Lf in ld2string it preserves 17 significant digits, rather than 17 digit after the decimal point (like in %.17Lg). So to make these similar, i use value lower than 1 (no digits left of the period) Secondly we need to choose numbers that are possible to represent in 64 bit floating point, since valgrind and ARM32 don't have larger long doubles. thirdly, we have the same issue with TCL (no long doubles) so we read raw protocol in that test.
yossigo
approved these changes
Nov 1, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The module test in reply.tcl was introduced by #8521 but didn't run until recently (see #9639)
and then it started failing with valgrind.
This is because valgrind uses 64 bit long double (unlike most other platforms that have at least 80 bits)
But besides valgrind, the tests where also incompatible with ARM32, which also uses 64 bit long doubles.
We now use appropriate value to avoid issues with either valgrind or ARM32
In all the double tests, i use 3.141, which is safe since since addReplyDouble uses
%.17Lgwhich is able to represent this value without adding any digits due to precision loss.In the long double, since we use
%.17Lfin ld2string, it preserves 17 significantdigits, rather than 17 digit after the decimal point (like in
%.17Lg).So to make these similar, i use value lower than 1 (no digits left of
the period)
Lastly, we have the same issue with TCL (no long doubles) so we read
raw protocol in that test.
Note that the only error before this fix (in both valgrind and ARM32 is this:
so the changes to debug.c and scripting.tcl aren't really needed, but i consider them a cleanup
(i.e. scripting.c validated a different constant than the one that's sent to it from debug.c).
Another unrelated change is to add the RESP version to the repeated tests in reply.tcl