flamenco/tests: fix double normal classification

[anarcheuz]

Tested by differential fuzzing with C++'s std::isnormal(). Without the fix we would reject normal double such as 0x93da8e that comes from the fuzzer (ie. lower coverage) whereas std::normal() would accept it. -ffast-math prevents us from using std::isnormal().

Note: We are purposely not rejecting 0, because it is an interesting value from a fuzzing perspective. With @ripatel-fd we suggest to change the harnesses to accept 0 as a valid fuzzing value.

[mjain-jump]

This function seems like it would mistakenly classify 0 as a normal number. Could you please double check that? If so then this line can be removed, or we should add another case that explicitly checks for 0 (for neatness)

[topointon-jump]

@anarcheuz it would be good to add a comment here explaining that we are explicitly allowing zero, and the reason.

[ptaffet-jump]

Based on my reading of what I find on the Internet, 0 is not a subnormal number

[ripatel-fd]

@ptaffet-jump That is true, but the C/C++ definition of "isnormal" is "not a NaN, not infinity, not subnormal, and not zero". It was my mistake to not put the "is_zero" check into fd_double_is_normal.

[topointon-jump]

Looks good, could we not merge this until Agave also accepts 0?

[topointon-jump]

@anarcheuz it would be good to add a comment here explaining that we are explicitly allowing zero, and the reason.

[ptaffet-jump]

Would you also mind changing the commit message? How about flamenco/tests: fix double normal classification.

[anarcheuz]

Would you also mind changing the commit message? How about flamenco/tests: fix double normal classification.

Done.

[anarcheuz]

Looks good, could we not merge this until Agave also accepts 0?

@mjain-jump I think you mentioned that Agave accepts 0 (ie. not causing mismatch)?

Other than that, 0 is a good "magic" value for fuzzing. If Agave is also not checking for 0 we should not constrain our harness to only accept normal floating points. Especially since this check only exists in our harness and is not in production code.

[mjain-jump]

Looks good, could we not merge this until Agave also accepts 0?

@mjain-jump I think you mentioned that Agave accepts 0 (ie. not causing mismatch)?

Other than that, 0 is a good "magic" value for fuzzing. If Agave is also not checking for 0 we should not constrain our harness to only accept normal floating points. Especially since this check only exists in our harness and is not in production code.

Agave indeed seems to be accepting 0.