Pretty: fix Undefined Behavior with NaN floats

[niooss-ledger]

When printing CBOR data containing NaN, function convertToUint64 does an undefined behavior. This can be reproduced using test cases from tests/parser/data.cpp:

$ make CC='clang -fsanitize=undefined'
$ printf "\xfb\x7f\xf8\0\0\0\0\0\0" | ./bin/cbordump
src/cborpretty.c:171:17: runtime error: nan is outside the range of representable values of type 'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/cborpretty.c:171:17 in
nan

$ printf "\xf9\x7e\x00" | ./bin/cbordump
src/cborpretty.c:171:17: runtime error: nan is outside the range of representable values of type 'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/cborpretty.c:171:17 in
nan

Fix this by checking whether the value to convert is not NaN.

[thiagomacieira]

Also looks ok, but a simpler change is to invert the comparison below:

    supremum = -2.0 * INT64_MIN;     /* -2 * (- 2^63) == 2^64 */
    if (!(v < supremum))
        return false;       /* out of range or NaN */

[niooss-ledger]

In my humble opinion, both if (isnan(v)) and if (!(v < supremum)) work for me. I modified the PR to use the second option. Could you please take a look?

[thiagomacieira]

In my humble opinion, both if (isnan(v)) and if (!(v < supremum)) work for me. I modified the PR to use the second option. Could you please take a look?

Done. Since this is tiny CBOR, I'd prefer the one that generates less code, even if slightly less expressive. Compilers ought to have been able to tell that an isnan and range comparison can be implemented in a single go... but most don't.
https://gcc.godbolt.org/z/69Thje93Y - compiled with -O3, Clang is the only one to do it. Nothing changes with -Os and similar. (I can barely read https://cross.godbolt.org/z/WfPrYb99Y). The "JA" and "SETA" instructions operate if CF = ZF = 0, which UCOMISD will not set if it's NaN. So it's showing the NaN check for this assembly as GCC generated is unnecessary.

Meanwhile, in https://gcc.godbolt.org/z/qPK8oehhW with the code I suggested, all four compilers generated optimal code. The "B" (or "C") condition is if CF = 1... wait, that doesn't look right.

Testing in https://gcc.godbolt.org/z/da3KqbbWr: the three compilers that inlined the function say "NaN" is in range.

Ok, this didn't work. Please change back to your original code. Sorry about the noise!

Time to file a missed optimisation report with GCC.

[thiagomacieira]

Sorry, this didn't work. Please change back to your original idea.

[thiagomacieira]

Meanwhile, in https://gcc.godbolt.org/z/qPK8oehhW with the code I suggested, all four compilers generated optimal code. The "B" (or "C") condition is if CF = 1... wait, that doesn't look right.

Testing in https://gcc.godbolt.org/z/da3KqbbWr: the three compilers that inlined the function say "NaN" is in range.

Ok, this didn't work. Please change back to your original code. Sorry about the noise!

Time to file a missed optimisation report with GCC.

Oops, we do want an out of range.
https://gcc.godbolt.org/z/qnn4E7T9h - isnan + range
https://gcc.godbolt.org/z/qzE8djjhz - range

Or, more specifically, what we actually want:
https://gcc.godbolt.org/z/4GEGjTzT5 - original proposal
https://gcc.godbolt.org/z/WMW58rfsE - this way
They're both the same.