Re: [PATCH] msvcp120: Fix _Xtime_diff_to_millis2 overflow behavior.

[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.
> @@ -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