Re: [PATCH] msvcp120: Fix _Xtime_diff_to_millis2 overflow behavior.
On 10/11/17 17:07, Stefan Dösinger wrote:
+ {0x7FFFFFFFFFFFFFFF, 0, 0x7FFFFFFFFFFFFFFD, 0, 0}, /* Not an overflow */ Defining LONGLONG constants this way is not portable.
@@ -410,7 +410,13 @@ MSVCRT_long __cdecl _Xtime_diff_to_millis2(const xtime *t1, const xtime *t2) diff_nsec = t1->nsec - t2->nsec; ret = diff_sec * MILLISEC_PER_SEC + (diff_nsec + NANOSEC_PER_MILLISEC - 1) / NANOSEC_PER_MILLISEC; - return ret > 0 ? ret : 0; + + /* This function does not return negative numbers, except when they get created through + * an integer overflow. */ + if (diff_sec > ((LONGLONG)INT_MAX + NANOSEC_PER_MILLISEC - 1) / NANOSEC_PER_MILLISEC) + return ret; + else + return ret > 0 ? ret : 0; } It looks like the function should be treating negative time differences in special way. Except of that it seems to be ignoring overflows. I think that following code is much easier to read. Is something like this working for you? MSVCRT_long __cdecl _Xtime_diff_to_millis2(const xtime *t1, const xtime *t2) { LONGLONG diff_sec, diff_nsec;
TRACE("(%p, %p)\n", t1, t2); diff_sec = t1->sec - t2->sec; diff_nsec = t1->nsec - t2->nsec; diff_sec += diff_nsec / NANOSEC_PER_SEC; diff_nsec %= NANOSEC_PER_SEC; if (diff_nsec < 0) { diff_sec -= 1; diff_nsec += NANOSEC_PER_SEC; } if (diff_sec<0 || (diff_sec==0 && diff_nsec<0)) return 0; return diff_sec * MILLISEC_PER_SEC + (diff_nsec + NANOSEC_PER_MILLISEC - 1) / NANOSEC_PER_MILLISEC; } Thanks, Piotr
Am 2017-10-12 um 11:32 schrieb Piotr Caban:
On 10/11/17 17:07, Stefan Dösinger wrote:
+ {0x7FFFFFFFFFFFFFFF, 0, 0x7FFFFFFFFFFFFFFD, 0, 0}, /* Not an overflow */ Defining LONGLONG constants this way is not portable. Any better way? Does (LONGLONG)0x7FFFFFFFFFFFFFFF work, or do I need something more elaborate? The information I can find online points to C99 :-\ .
It looks like the function should be treating negative time differences in special way. Except of that it seems to be ignoring overflows. I think that following code is much easier to read. Is something like this working for you? Your code looks more elegant than mine for sure, and I believe it should work. I'll double check it with WGC to be on the safe side.
As I've mentioned in the patch description I think native uses 128 bit integers, probably through https://msdn.microsoft.com/en-us/library/windows/desktop/hh802931(v=vs.85).a... or https://msdn.microsoft.com/en-us/library/windows/desktop/82cxdw50(v=vs.85).a... . Unfortunately neither function is currently implemented in Wine. The MS headers suggest that they are msvc compiler intrinsics.
On 12 Oct 2017, at 20:07, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
Am 2017-10-12 um 11:32 schrieb Piotr Caban:
On 10/11/17 17:07, Stefan Dösinger wrote:
+ {0x7FFFFFFFFFFFFFFF, 0, 0x7FFFFFFFFFFFFFFD, 0, 0}, /* Not an overflow */ Defining LONGLONG constants this way is not portable. Any better way? Does (LONGLONG)0x7FFFFFFFFFFFFFFF work, or do I need something more elaborate? The information I can find online points to C99 :-\ . You can use ((LONGLONG)0x7fffffff << 32) | 0xffffffff or _I64_MAX here.
It looks like the function should be treating negative time differences in special way. Except of that it seems to be ignoring overflows. I think that following code is much easier to read. Is something like this working for you? Your code looks more elegant than mine for sure, and I believe it should work. I'll double check it with WGC to be on the safe side.
As I've mentioned in the patch description I think native uses 128 bit integers, probably through https://msdn.microsoft.com/en-us/library/windows/desktop/hh802931(v=vs.85).a... or https://msdn.microsoft.com/en-us/library/windows/desktop/82cxdw50(v=vs.85).a... . Unfortunately neither function is currently implemented in Wine. The MS headers suggest that they are msvc compiler intrinsics. I don’t see why 128-bit operations should be needed here.
Thanks, Piotr
Am 2017-10-13 um 09:46 schrieb Piotr Caban:
It looks like the function should be treating negative time differences in special way. Except of that it seems to be ignoring overflows. I think that following code is much easier to read. Is something like this working for you? Your code looks more elegant than mine for sure, and I believe it should work. I'll double check it with WGC to be on the safe side. Your patch works for my tests and WGC. You forgot to define NANOSEC_PER_SEC though.
I'd propose that you send your patch and I'll resubmit my tests with the LONGLONG constant fix. Does that work for you?
I don’t see why 128-bit operations should be needed here. I'm not saying they are necessary, as your code obviously shows. It's just my guess of how Microsoft ended up with this behavior.
On 10/17/17 16:58, Stefan Dösinger wrote:
Am 2017-10-13 um 09:46 schrieb Piotr Caban:
It looks like the function should be treating negative time differences in special way. Except of that it seems to be ignoring overflows. I think that following code is much easier to read. Is something like this working for you? Your code looks more elegant than mine for sure, and I believe it should work. I'll double check it with WGC to be on the safe side. Your patch works for my tests and WGC. You forgot to define NANOSEC_PER_SEC though.
I'd propose that you send your patch and I'll resubmit my tests with the LONGLONG constant fix. Does that work for you? Yes, I've sent the patch.
Thanks, Piotr
participants (2)
-
Piotr Caban -
Stefan Dösinger