[date_time] int64 overflow bug with subsecond duration
Consider the following code, very simple in nature: static const boost::gregorian::date windows_epoch_base(1601, 1, 1); date_time d(windows_epoch_base, microseconds(12908391903991608)); This should resolve to sometime in early 2010 Now, the problem is that this number is already really big, not that many factors of 10 away from the int64 overflow boundary. microseconds() is really just the subsecond_duration<> template, and on my machine it's being parameterized as subsecond_duration<boost::posix_time::time_duration, 1000000>. The code for the constructor is as follows: explicit subsecond_duration(boost::int64_t ss) : base_duration(0,0,0, ss*traits_type::res_adjust() / frac_of_second) {} res_adjust is also resolving to 1000000. So compiler is doing 12908391903991608*res_adjust first, which causes an overflow. I think it should be doing ss * (traits_type::res_adjust() / frac_of_second) instead. I don't know a whole lot about date_time internals, so I thought I would ask here before just submitting a bug. Is the proposed fix correct? I made the change locally and it does indeed appear to resolve the problem. Zach
On 02/25/2010 06:44 PM, Zachary Turner wrote:
Consider the following code, very simple in nature:
static const boost::gregorian::date windows_epoch_base(1601, 1, 1);
date_time d(windows_epoch_base, microseconds(12908391903991608));
This should resolve to sometime in early 2010
Now, the problem is that this number is already really big, not that many factors of 10 away from the int64 overflow boundary. microseconds() is really just the subsecond_duration<> template, and on my machine it's being parameterized as subsecond_duration<boost::posix_time::time_duration, 1000000>. The code for the constructor is as follows:
explicit subsecond_duration(boost::int64_t ss) : base_duration(0,0,0, ss*traits_type::res_adjust() / frac_of_second) {}
res_adjust is also resolving to 1000000. So compiler is doing 12908391903991608*res_adjust first, which causes an overflow. I think it should be doing ss * (traits_type::res_adjust() / frac_of_second) instead.
I don't know a whole lot about date_time internals, so I thought I would ask here before just submitting a bug. Is the proposed fix correct? I made the change locally and it does indeed appear to resolve the problem.
I think there was a ticket or two about this. The problem with adding the parenthesis is that there's a possible truncation of the division result. This may not be the case for your particular case, but I don't think this is good enough as a general solution.
On Thu, Feb 25, 2010 at 2:41 PM, Andrey Semashev <andrey.semashev@gmail.com>wrote:
On 02/25/2010 06:44 PM, Zachary Turner wrote:
Consider the following code, very simple in nature:
static const boost::gregorian::date windows_epoch_base(1601, 1, 1);
date_time d(windows_epoch_base, microseconds(12908391903991608));
This should resolve to sometime in early 2010
Now, the problem is that this number is already really big, not that many factors of 10 away from the int64 overflow boundary. microseconds() is really just the subsecond_duration<> template, and on my machine it's being parameterized as subsecond_duration<boost::posix_time::time_duration, 1000000>. The code for the constructor is as follows:
explicit subsecond_duration(boost::int64_t ss) : base_duration(0,0,0, ss*traits_type::res_adjust() / frac_of_second) {}
res_adjust is also resolving to 1000000. So compiler is doing 12908391903991608*res_adjust first, which causes an overflow. I think it should be doing ss * (traits_type::res_adjust() / frac_of_second) instead.
I don't know a whole lot about date_time internals, so I thought I would ask here before just submitting a bug. Is the proposed fix correct? I made the change locally and it does indeed appear to resolve the problem.
I think there was a ticket or two about this. The problem with adding the parenthesis is that there's a possible truncation of the division result. This may not be the case for your particular case, but I don't think this is good enough as a general solution.
Yes, actually I thought about that later. Can we just do the division first then? (ss / frac_of_second) * traits_type::res_adjust() This is better, but still not as precise as using the parenthesized version above where the division is performed in floating point. I'm not sure which method is most desirable. Also, why is it using int64_t instead of uint64_t?
On 02/25/2010 11:44 PM, Zachary Turner wrote:
On Thu, Feb 25, 2010 at 2:41 PM, Andrey Semashev <andrey.semashev@gmail.com>wrote:
I think there was a ticket or two about this. The problem with adding the parenthesis is that there's a possible truncation of the division result. This may not be the case for your particular case, but I don't think this is good enough as a general solution.
Yes, actually I thought about that later. Can we just do the division first then?
(ss / frac_of_second) * traits_type::res_adjust()
This is better, but still not as precise as using the parenthesized version above where the division is performed in floating point. I'm not sure which method is most desirable.
I don't think it's any better since both ss and frac_of_second are integrals, so the truncation takes place anyway. What I'm thinking of is introducing a compile-time check to see, whether the truncation will actually take place, and then act accordingly. If it's safe to divide first, then go with the parenthesis version. If it's not, then either go with FP (yuck!) or some int128_t, which apparently would have to be artifical.
Also, why is it using int64_t instead of uint64_t?
Because duration is the result of the subtraction of two time points, I guess. Negative durations are possible.
participants (2)
-
Andrey Semashev -
Zachary Turner