object_with_zone.system_clock_impl_min fails (undefined behavior) #881
Comments
|
It's weird. The error line is https://github.com/msgpack/msgpack-c/blob/cpp-3.3.0/test/object_with_zone.cpp#L1031 The function is TEST(object_with_zone, system_clock_impl_min)
{
std::chrono::system_clock::time_point v(std::chrono::system_clock::time_point::min());
msgpack::zone z;
msgpack::object obj(v, z);
EXPECT_TRUE(obj.as<std::chrono::system_clock::time_point>() == v);
}I'm interested in what happens if the temporary variable TEST(object_with_zone, system_clock_impl_min)
{
std::chrono::system_clock::time_point v(std::chrono::system_clock::time_point::min());
msgpack::zone z;
msgpack::object obj(v, z);
std::chrono::system_clock::time_point v2 = obj.as<std::chrono::system_clock::time_point>();
EXPECT_TRUE(v2 == v);
}If the error is disappeared, I guess that there is some macro and |
That doesn't help. However, both of these changes do make the test pass: EXPECT_EQ(obj.as<std::chrono::system_clock::time_point>(), v);EXPECT_EQ(v2, v);I'm re-building 3.0.1 since that successfully built when it was uploaded to Debian to see if that now fails. If so, that would seem to be a good indication that this is a compiler problem. This is the compiler I'm running: I'll also try with clang and gcc-10. |
|
Inserting the following code to v2 introduced one might help to get more information. auto nsec = std::chrono::duration_cast<std::chrono::nanoseconds>(v2 - v);
auto sec = std::chrono::duration_cast<std::chrono::seconds>(v2 - v);
std::cout << nsec.count() << std::endl;
std::cout << sec.count() << std::endl; |
|
With those changes, the failure goes away, even when keeping the original Building with This seems pretty clearly to be a compiler issue. I'll try to narrow down what optimization is causing the problem and report it to gcc. |
|
I think that the issue is related to optimizer too. |
|
The issue is signed overflow in the test: |
jamessan
changed the title
|
Running the test suite with |
jamessan
changed the title
|
In the attachment you uploaded, two types of issues are mentioned:
It can be fixed by checking the null pointer.
I have not found a solution. I just guessed that the problem was caused by the compiler, so I need to spend some time to study it. |
|
@ygj6 , thank you for analyzing and fixing. I've checked the rest problem. I inserted The error is reported in but not reported in . In addition, the error is only reported only system_clock. steady_clock and high_resolution_clock don't report errors. I don't understand why the difference happens, so far. |
template <typename Clock>
struct as<std::chrono::time_point<Clock>> {
typename std::chrono::time_point<Clock> operator()(msgpack::object const& o) const {
using duration_t = typename std::chrono::time_point<Clock>::duration;
if(o.type != msgpack::type::EXT) { throw msgpack::type_error(); }
if(o.via.ext.type() != -1) { throw msgpack::type_error(); }
std::chrono::time_point<Clock> tp;
switch(o.via.ext.size) {
case 4: {
uint32_t sec;
_msgpack_load32(uint32_t, o.via.ext.data(), &sec);
tp += std::chrono::seconds(sec);
} break;
case 8: {
uint64_t value;
_msgpack_load64(uint64_t, o.via.ext.data(), &value);
uint32_t nanosec = static_cast<uint32_t>(value >> 34);
uint64_t sec = value & 0x00000003ffffffffLL;
tp += std::chrono::duration_cast<duration_t>(
std::chrono::nanoseconds(nanosec));
tp += std::chrono::seconds(sec);
} break;
case 12: {
uint32_t nanosec;
_msgpack_load32(uint32_t, o.via.ext.data(), &nanosec);
int64_t sec;
_msgpack_load64(int64_t, o.via.ext.data() + 4, &sec);
tp += std::chrono::duration_cast<duration_t>(
std::chrono::nanoseconds(nanosec));
std::cout << __LINE__ << std::endl; // Line 58
tp += std::chrono::seconds(sec);
std::cout << __LINE__ << std::endl; // Line 60
} break;
default:
throw msgpack::type_error();
}
return tp;
}
};
template <typename Clock>
struct convert<std::chrono::time_point<Clock>> {
msgpack::object const& operator()(msgpack::object const& o, std::chrono::time_point<Clock>& v) const {
using duration_t = typename std::chrono::time_point<Clock>::duration;
if(o.type != msgpack::type::EXT) { throw msgpack::type_error(); }
if(o.via.ext.type() != -1) { throw msgpack::type_error(); }
std::chrono::time_point<Clock> tp;
switch(o.via.ext.size) {
case 4: {
uint32_t sec;
_msgpack_load32(uint32_t, o.via.ext.data(), &sec);
tp += std::chrono::seconds(sec);
v = tp;
} break;
case 8: {
uint64_t value;
_msgpack_load64(uint64_t, o.via.ext.data(), &value);
uint32_t nanosec = static_cast<uint32_t>(value >> 34);
uint64_t sec = value & 0x00000003ffffffffLL;
tp += std::chrono::duration_cast<duration_t>(
std::chrono::nanoseconds(nanosec));
tp += std::chrono::seconds(sec);
v = tp;
} break;
case 12: {
uint32_t nanosec;
_msgpack_load32(uint32_t, o.via.ext.data(), &nanosec);
int64_t sec;
_msgpack_load64(int64_t, o.via.ext.data() + 4, &sec);
tp += std::chrono::duration_cast<duration_t>(
std::chrono::nanoseconds(nanosec));
std::cout << __LINE__ << std::endl; // Line 100
tp += std::chrono::seconds(sec);
std::cout << __LINE__ << std::endl; // Line 102
v = tp;
} break;
default:
throw msgpack::type_error();
}
return o;
}
}; |
|
The result was:
I suspect that the first one is false positive. So I re-ordered as follwos: TEST(MSGPACK_CHRONO, system_clock_impl_min)
{
std::chrono::system_clock::time_point val1(std::chrono::system_clock::time_point::min());
msgpack::sbuffer sbuf;
msgpack::pack(sbuf, val1);
msgpack::object_handle oh =
msgpack::unpack(sbuf.data(), sbuf.size());
// convert first
std::chrono::system_clock::time_point val3;
oh.get().convert(val3);
EXPECT_EQ(val1, val3);
// then as
std::chrono::system_clock::time_point val2 = oh.get().as<std::chrono::system_clock::time_point>();
EXPECT_EQ(val1, val2);
}Then I got very interesting result:
If I `#if 0"ed system clock test case, #if 0
TEST(MSGPACK_CHRONO, system_clock_impl_min)
{
std::chrono::system_clock::time_point val1(std::chrono::system_clock::time_point::min());
msgpack::sbuffer sbuf;
msgpack::pack(sbuf, val1);
msgpack::object_handle oh =
msgpack::unpack(sbuf.data(), sbuf.size());
std::chrono::system_clock::time_point val3;
oh.get().convert(val3);
EXPECT_EQ(val1, val3);
std::chrono::system_clock::time_point val2 = oh.get().as<std::chrono::system_clock::time_point>();
EXPECT_EQ(val1, val2);
}
#endifthen
the first one (steady clock, as) starts reporting the error. It seems that the error is reported the first one only. I think that it is not msgpack-c implementation problem. @jamessan, what do you think? |
|
I noticed that there is a possibiliy that the first one is true positive and the rests are false negative. |
|
I wrote the code that reproduce the same issue. output |
|
|
libstdc++ chrono template<typename _ToDur, typename _CF, typename _CR>
struct __duration_cast_impl<_ToDur, _CF, _CR, false, true>
{
template<typename _Rep, typename _Period>
static constexpr _ToDur
__cast(const duration<_Rep, _Period>& __d)
{
typedef typename _ToDur::rep __to_rep;
return _ToDur(static_cast<__to_rep>(
static_cast<_CR>(__d.count()) * static_cast<_CR>(_CF::num))); // error happens here
}
};libc++ chrono template <class _FromDuration, class _ToDuration, class _Period>
struct __duration_cast<_FromDuration, _ToDuration, _Period, false, true>
{
_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
_ToDuration operator()(const _FromDuration& __fd) const
{
typedef typename common_type<typename _ToDuration::rep, typename _FromDuration::rep, intmax_t>::type _Ct;
return _ToDuration(static_cast<typename _ToDuration::rep>(
static_cast<_Ct>(__fd.count()) * static_cast<_Ct>(_Period::num)));
}
};It seems that libc++ defines I'm not 100% sure but that difference makes the error. |
|
I guess that the following decrement code is something wrong. |
|
This is msgpack TimeStamp spec: |
|
It seems that signed integer overflow message output once is not an essential problem. MessagePack TimeStamp format is similar to std::timespec (since C++17) According to the following Q&A, std::chrono::time_point cannot express complete data of std::timespec. Consider the test case again: TEST(MSGPACK_CHRONO, system_clock_impl_min)
{
std::chrono::system_clock::time_point val1(std::chrono::system_clock::time_point::min());
msgpack::sbuffer sbuf;
msgpack::pack(sbuf, val1); // maybe some data lost here
msgpack::object_handle oh =
msgpack::unpack(sbuf.data(), sbuf.size()); // losing accuracy is acceptable but need to avoid signed integer overflow
std::chrono::system_clock::time_point val3;
oh.get().convert(val3); // losing accuracy is acceptable but need to avoid signed integer overflow
EXPECT_EQ(val1, val3);
std::chrono::system_clock::time_point val2 = oh.get().as<std::chrono::system_clock::time_point>();
EXPECT_EQ(val1, val2);
}I guess that std::chrono::time_point doesn't have much bytes to express packed data. Maybe it is related comment #886 (comment) mentioned by @LuisAyuso . I think that we need to fix the library code to avoid signed integer overflow and accepting data accuracy loss. I'm not sure how to fix it. Any ideas? |
|
I'm not sure that avoiding signed integer overflow is user(msgpack-c) responsibility or chrono(libstdc++) responsibility. I reported https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95992 . If it is user (msgpack-c) responsibility, how to check the value? |
|
I got a comment https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95992#c1 According to the comment, It is NOT a libstdc++ bug. msgpack-c/include/msgpack/v1/adaptor/cpp11/chrono.hpp Lines 107 to 126 in 4629583 When packing, if the value is std::chrono::system_clock::time_point::min(), then
and convert/as process, from these values:
got
with signed integeroverflow. |
|
I'm digging into timestamp (msgpack spec) manipulation in msgpack-c. It seems that |
|
Finally, I fixed the chrono problem. #893 min() count is negative. Let's say min is -10.4 second. When do as/convert, time_point is default constructed. Let's say the value is tp. In order to fix it, Then nanosecond part becomes nanosecond - 1,000,000,000. Finally, add second part and nanosecond part to tp. |
|
Thanks! |
The Debian armhf build of 3.3.0 is failing the mentioned test.
Since it's using
EXPECT_TRUE()instead ofEXPECT_EQ(), there's not much diagnostic information in the build log (although there are a lot of warnings in the build). I was able to reproduce the issue on one of our porter systems, and changed the assertion toEXPECT_EQ(obj.as<std::chrono::system_clock::time_point>(), v);I was hoping that would give more clarity about why the test failed, but instead the test passes whenever I run with that change.
If it helps, I could provide the annotated assembly that's generated for that test function.
The text was updated successfully, but these errors were encountered: