Skip to content

fix valgrind issues with long double module test#9709

Merged
oranagra merged 3 commits intoredis:unstablefrom
oranagra:fix_valgrind_test_issues
Nov 1, 2021
Merged

fix valgrind issues with long double module test#9709
oranagra merged 3 commits intoredis:unstablefrom
oranagra:fix_valgrind_test_issues

Conversation

@oranagra
Copy link
Member

@oranagra oranagra commented Oct 31, 2021

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
%.17Lg which is able to represent this value without adding any digits due to precision loss.

In the long double, 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)

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:

*** [err]: RM_ReplyWithLongDouble: a float reply in tests/unit/moduleapi/reply.tcl
Expected '3.141' to be equal to '3.14100000000000001' (context: type eval line 2 cmd {assert_equal 3.141 [r rw.longdouble 3.141]} proc ::test)

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

@oranagra oranagra requested a review from yossigo October 31, 2021 10:24
@oranagra
Copy link
Member Author

@yossigo can you please test it for me on ARM (32 / 64)?
i see we do use that level of precision in the scripting tests, but i wanna be sure it passes.

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.
@oranagra oranagra merged commit f1f3cce into redis:unstable Nov 1, 2021
@oranagra oranagra deleted the fix_valgrind_test_issues branch November 1, 2021 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants