Repository navigation
Unpacking float32 does not preserve signaling NaN, unlike float64 #1087
Description
Describe the bug
float32/float64 NaNs have a quiet vs. signaling state. This state is encoded using a couple bits in the significand.
Unpacking a float64 signaling NaN using msgpack-c 6.0.0 preserves the quiet vs. signaling state. However, unpacking a float32 signaling NaN always results in a quiet NaN even if the packed value is a signaling NaN. The behavior is inconsistent between float32 and float64.
To Reproduce
static bool is_quiet_nan(double nan_val) {
uint64_t bit_pattern = reinterpret_cast<uint64_t&>(nan_val);
int is_quiet_bit_index = DBL_MANT_DIG - 2;
return (bit_pattern >> is_quiet_bit_index) & 1;
}
TEST(MSGPACKC, simple_buffer_float_signaling_nan)
{
if (!numeric_limits<float>::has_signaling_NaN) {
return;
}
float val = numeric_limits<float>::signaling_NaN();
msgpack_sbuffer sbuf;
msgpack_sbuffer_init(&sbuf);
msgpack_packer pk;
msgpack_packer_init(&pk, &sbuf, msgpack_sbuffer_write);
msgpack_pack_float(&pk, val);
msgpack_zone z;
msgpack_zone_init(&z, 2048);
msgpack_object obj;
msgpack_unpack_return ret =
msgpack_unpack(sbuf.data, sbuf.size, NULL, &z, &obj);
ASSERT_EQ(MSGPACK_UNPACK_SUCCESS, ret);
ASSERT_EQ(MSGPACK_OBJECT_FLOAT32, obj.type);
EXPECT_TRUE(isnan(obj.via.f64));
EXPECT_FALSE(is_quiet_nan(obj.via.f64));
#if defined(MSGPACK_USE_LEGACY_NAME_AS_FLOAT)
EXPECT_TRUE(isnan(obj.via.dec));
EXPECT_FALSE(is_quiet_nan(obj.via.dec));
#endif // MSGPACK_USE_LEGACY_NAME_AS_FLOAT
msgpack_zone_destroy(&z);
msgpack_sbuffer_destroy(&sbuf);
}
TEST(MSGPACKC, simple_buffer_double_signaling_nan)
{
if (!numeric_limits<double>::has_signaling_NaN) {
return;
}
double val = numeric_limits<double>::signaling_NaN();
msgpack_sbuffer sbuf;
msgpack_sbuffer_init(&sbuf);
msgpack_packer pk;
msgpack_packer_init(&pk, &sbuf, msgpack_sbuffer_write);
msgpack_pack_double(&pk, val);
msgpack_zone z;
msgpack_zone_init(&z, 2048);
msgpack_object obj;
msgpack_unpack_return ret =
msgpack_unpack(sbuf.data, sbuf.size, NULL, &z, &obj);
ASSERT_EQ(MSGPACK_UNPACK_SUCCESS, ret);
ASSERT_EQ(MSGPACK_OBJECT_FLOAT64, obj.type);
ASSERT_EQ(MSGPACK_OBJECT_FLOAT, obj.type);
#if defined(MSGPACK_USE_LEGACY_NAME_AS_FLOAT)
ASSERT_EQ(MSGPACK_OBJECT_DOUBLE, obj.type);
#endif // MSGPACK_USE_LEGACY_NAME_AS_FLOAT
EXPECT_TRUE(isnan(obj.via.f64));
EXPECT_FALSE(is_quiet_nan(obj.via.f64));
#if defined(MSGPACK_USE_LEGACY_NAME_AS_FLOAT)
EXPECT_TRUE(isnan(obj.via.dec));
EXPECT_FALSE(is_quiet_nan(obj.via.dec));
#endif // MSGPACK_USE_LEGACY_NAME_AS_FLOAT
msgpack_zone_destroy(&z);
msgpack_sbuffer_destroy(&sbuf);
}Expected behavior
Either both tests should pass or both tests should fail. Currently, simple_buffer_double_signaling_nan passes but simple_buffer_float_signaling_nan fails.
Root cause
The unpacker returns float32 values as double. This conversion implicitly causes the float32 signaling NaN to become a float64 quiet NaN.
Possible solution
The unpacker could return float32 values as float instead of converting them to double. This might be a breaking change because callers may need to update their usage to check a different msgpack_object_union field. However, in addition to resolving this issue, it could also be more convenient for callers because they wouldn’t need to cast the double field back to float.
Activity
redboltz commentedon Aug 27, 2023
Thank you for reporting the issue.
I got confused. The code seems that C not C++. But your link to Root cause is C++ code.
Now C and C++ are completely different branch and different implementation.
Which one has the isssue C or C++, or both?
It seems that code is for C.
redboltz commentedon Aug 27, 2023
I study about signaling/quiet NaN. Now I understand the issue.
It is common in C and C++.
I consider fixing the issue with minimal code change.
This code demonstrates the issue (signaling NaN becomes quiet NaN when convert from float to double), and another possible fix.
https://godbolt.org/z/Yehe6GMGG
Your link indicates the issue is caused by the following function. I agree.
When I apply the minimal fix, then the code would be as follows:
What do you think ?
Bitimage print version: https://godbolt.org/z/rqaPx4b34
redboltz commentedon Aug 28, 2023
I ran my poc code on windows/msvc2019.
Here is the result:
Linux result:
float signaling NaN
*1
It seems that only Fraction != 0 is defined.
So there are many variations.
I think that your solution, introducing float type to object union, is better than my approach.
However, there is still problem.
similarlly
*2
I'm still not sure how to distinguish quiet or signal.
I think that the following code is not portable. Works well on Linux but doesn't work on Windows:
redboltz commentedon Aug 28, 2023
Perhaps msgpack-c could have a database of double/float signaling/quiet NaN bit representation on common operating systems, like Linux, macosx, Windows.
If they doesn't have conflicts, then unpack function could treat them well.
Conflict means OS-A quiet NaN bit representation is the same as OS-B signaling NaN bit representation.
Is there any information about quiet/signaling NaN bit repserentation ?
redboltz commentedon Aug 28, 2023
I testes on msvc2022 too.
Then got the same result as msvc2019.
I found a document:
https://learn.microsoft.com/en-us/cpp/build/ieee-floating-point-representation?view=msvc-170#nan---not-a-number
It seems that the document and result are not mached.
Fixed msgpack#1087 for C++.
Fixed msgpack#1087 for C++.
Fixed msgpack#1087 for C++.
Fixed msgpack#1087 for C++.
redboltz commentedon Aug 28, 2023
I wrote a PR to fix this issue on C++.
Then some of CI doesn't pass. They are 32bit built using
-m32option.So I've checked the behavior using godbolt.
32bit
https://godbolt.org/z/8ETYd3Gz4
64bit
https://godbolt.org/z/aa8Wxoaao
64 bit is expected behavior. The left most bit of Fraction(Significand) indicates quiet(1) or signaling(0).
But 32 bit is both 1. But the 2nd left most bit seems to indicate quiet(0) or signaling(1).
It is very inconsintent for me.
Am I missing something?
My printing way or reinterrupt_cast would be undefined behavior ??
redboltz commentedon Aug 28, 2023
https://en.wikipedia.org/wiki/IEEE_754#2019
Perhaps the encoding rule for signaling/quiet NaN is very recently decided. If it is, there are various kinds of encoding...
So msgpack-c pack/unpack using its platform one. So introducing f32 is good.
How do I write test for that ...
try rising signal ??
redboltz commentedon Aug 28, 2023
I checked signal using fetestexcept.
signaling NaN with floating operation results:
fumoboy007 commentedon Aug 28, 2023
Thanks for looking into this @redboltz!
I think the Windows issue you encountered may just be an issue with the Windows C++ standard library implementation: https://developercommunity.visualstudio.com/t/stdnumeric-limitssignaling-nan-returns-quiet-nan/155064. Perhaps a floating point operation that generates an sNAN still has the correct bit pattern even if
numeric_limits::signaling_NaN()does not?Is the 32-bit CPU issue you encountered the same as the above Windows issue?
Your solution of clearing the
is_quietbit seems like it should work well. That being said, introducing anf32field seems like the cleanest solution to me.fumoboy007 commentedon Aug 28, 2023
Actually, the Windows issue is a 32-bit CPU issue, not the other way around! More details here.
From what I understood, if a function returns an sNAN, it will always get converted to a qNAN (and raise a floating point exception) because the calling conventions on x86 specify that floating point values are returned in the x87 registers, which are inside the FPU. Hence,
numeric_limits::signaling_NaN()always returns a qNAN on x86.