Bug fix for PR#43703

[rprichard]

Summary

https://PR43703 - std::steady_clock::now() frequently overflows on Windows

I'm putting this up for review because I'm not seeing an easy way to test this - and because I don't have a windows box handy.

[rprichard]

I tested this patch, and it appears to fix the overflow problem.

(FWIW, I tested using https://gist.github.com/rprichard/bcff3b3fb0451207dc9f31ccbab938df on Wine and on Windows 10. For both targets, the performance counter frequency was 10MHz, so counter.QuadPart * nano::den had previously wrapped around about every 1845 seconds.)

[ldionne]

In D69314#1834674, @rprichard wrote:

I tested this patch, and it appears to fix the overflow problem.

(FWIW, I tested using https://gist.github.com/rprichard/bcff3b3fb0451207dc9f31ccbab938df on Wine and on Windows 10. For both targets, the performance counter frequency was 10MHz, so counter.QuadPart * nano::den had previously wrapped around about every 1845 seconds.)

@rprichard Thanks for testing this on Windows. Do you think it's possible to make that test program run for a shorter amount of time so that it can be added to our test suite (i.e. do you think the test would still be relevant if we made it run for a shorter amount of time)?

[ldionne]

(requesting changes so it shows up in the review queue properly)

[rprichard]

In D69314#1941495, @ldionne wrote:

@rprichard Thanks for testing this on Windows. Do you think it's possible to make that test program run for a shorter amount of time so that it can be added to our test suite (i.e. do you think the test would still be relevant if we made it run for a shorter amount of time)?

I suppose a test would only need to sleep for a second or two total to exercise the new part1-vs-part2 logic. I'm not sure how much of the existing test would still be relevant.

Can the computation be split out from the Query calls, though? If we had an internal (Freq, Counter) -> time_point function, we could test that quickly.

I think this function assumes that the performance counter frequency is less than about 2**63 / 10**9, about 9.22GHz. It was 10MHz on the two targets I saw, but comments on the Internet suggest that it's sometimes 3MHz. I think this assumption is OK.

[rprichard]

A version of this fix was already merged in D93456.

libcxx/src/chrono.cpp

 //    If the function fails, the return value is zero. <snip>
 //    On systems that run Windows XP or later, the function will always succeed 
 //      and will thus never return zero.
+//  The same comment is made for QueryPerformanceCounter at
+//    https://docs.microsoft.com/en-us/windows/win32/api/profileapi/nf-profileapi-queryperformancecounter
 
 static LARGE_INTEGER
 __QueryPerformanceFrequency()
 {
-  LARGE_INTEGER val;
-  (void) QueryPerformanceFrequency(&val);
-  return val;
+    LARGE_INTEGER val;
+    (void) QueryPerformanceFrequency(&val);
+    return val;
+}
+
+static LARGE_INTEGER
+__QueryPerformanceCounter()
+{
+    LARGE_INTEGER val;
+    (void) QueryPerformanceCounter(&val);
+    return val;
 }
 
 steady_clock::time_point
 steady_clock::now() _NOEXCEPT
 {
-  static const LARGE_INTEGER freq = __QueryPerformanceFrequency();
+    static const LARGE_INTEGER freq    = __QueryPerformanceFrequency();
+           const LARGE_INTEGER counter = __QueryPerformanceCounter();
+
+//  Do the multiplication in two parts to prevent overflow (See PR#43703)
+    const LONGLONG part1 = (counter.QuadPart / freq.QuadPart) * nano::den;
+    const LONGLONG part2 = (counter.QuadPart % freq.QuadPart) * nano::den / freq.QuadPart;
 
-  LARGE_INTEGER counter;
-  (void) QueryPerformanceCounter(&counter);
-  return time_point(duration(counter.QuadPart * nano::den / freq.QuadPart));
+    return time_point(duration(part1 + part2));
 }
 
 #elif defined(CLOCK_MONOTONIC)