-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
<chrono>: make system_clock::now()
and file_clock
aware of Windows leap second handling
#1588
<chrono>: make system_clock::now()
and file_clock
aware of Windows leap second handling
#1588
Conversation
I'm working on something similar, and so far it's the best I've been able to come up with. I think it might be necessary to go to the registry to get the leap second information for the conversions between
I'd say the second, " |
c934590
to
119d474
Compare
FILETIME
accordingly
@BillyONeal I kept the old behavior of |
Unfortunately I don't remember enough about the details; I believe the only important thing is that everything that touches "struct xtime" must remain exactly how |
Windows 10 1809 and Windows Server 2019 introduced support for leap seconds (enabled by default, could be disabled system-wide). When leap seconds are enabled, leap seconds after July 2018 are included in the tick count of `FILETIME`. This is different from the old behavior that `FILETIME` doesn't count leap seconds. In the future, the same `FILETIME` value could represent different time points a few seconds apart depending on whether leap second support is available and enabled. This commit implements the core algorithm for conversion between `file_time` and UTC date/time components. It calls Windows `FileTimeToSystemTime` or `SystemTimeToFileTime` to determine the interpretation of `FILETIME` by the underlying system.
119d474
to
b925e16
Compare
FILETIME
accordinglysystem_time::now()
and file_clock
aware of Windows leap second handling
system_time::now()
and file_clock
aware of Windows leap second handlingsystem_clock::now()
and file_clock
aware of Windows leap second handling
- Return `enum class` instead of `int` error code from `extern C` functions. - Add more `[[nodiscard]]`. - Remove `_STL_INTERNAL_CHECK` in import lib code, where the checks are never performed. Also modified the fallback behavior for unknown leap second smearing.
stl/src/xtime0.cpp
Outdated
// This must be as small as possible, because its contents are | ||
// injected into the msvcprt.lib and msvcprtd.lib import libraries. | ||
// Do not include or define anything else here. | ||
// In particular, basic_string must not be included here. | ||
|
||
#include <algorithm> | ||
#include <chrono> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<algorithm>
and <chrono>
are large headers - and <chrono>
drags in <string>
via <format>
. (There are only 2 import libs, for release and debug, but 5 possible IDL settings, and the import lib is effectively statically linked to user code, hence the comment warning about this hazard.) Can we reduce xtime0.cpp
to dragging in only the essential WinAPI machinery, and any simple types/enums it needs to wrap those APIs, much like filesystem.cpp
does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed #include <algorithm>
and #include <chrono>
from xtime0.cpp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a partial video as part of our video code review for the week -- I still need to look at xtime0.h
, xtime0.cpp
, and the added test.
stl/inc/chrono
Outdated
return time_point(duration(_Xtime_get_ticks())); | ||
constexpr long long __std_fs_file_time_epoch_adjustment = 0x19DB1DED53E8000LL; // TRANSITION, ABI | ||
|
||
const duration _File_time{_Xtime_get_ticks() + __std_fs_file_time_epoch_adjustment}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I'm unsure of how _Xtime_get_ticks()
is affected by the leap-seconds-related changes in different OS versions. Specifically this function contains
FILETIME ft;
__crtGetSystemTimePreciseAsFileTime(&ft);
where __crtGetSystemTimePreciseAsFileTime(&ft)
wraps either GetSystemTimeAsFileTime
or GetSystemTimePreciseAsFileTime
. I notice that neither of these functions is listed as an "affected API" in this blog. It would make sense to me that they would be affected in similar ways, but I just want to doublecheck / ask for some clarity here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That blog is written in a "FILETIME
centric view". The listed "affected APIs" are those whose behavior would change in different OS versions even when the inputs/clocks represent time points with the same FILETIME
value on each corresponding machine. GetSystemTimeAsFileTime
and GetSystemTimePreciseAsFileTime
return the current FILETIME
, which by definition doesn't change for the same FILETIME
, but would map to different time points in the UTC time scale.
stl/inc/chrono
Outdated
if (_Utc._Second < 60) { | ||
return time_point{_Ymd + _Hms + (_File_time - _File_seconds)}; | ||
} else { | ||
// positive leap second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "// positive leap second
" made me wonder how/whether we need to handle potential future negative leap seconds. I didn't see negative leap seconds referenced in either Dan's blogpost or Daniel's blogpost so I'm wondering if there is any documentation for how a negative leap second would be represented in the Windows API/structs/*?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any documentation explicitly mentioning the handling of negative leap seconds by Windows APIs. My experiments showed that FileTimeToSystemTime
jumps from 23:59:58.999 to 00:00:00.000 and that SystemTimeToFileTime
rejects 23:59:59.xxx deleted by a fake negative leap second, whether or not PROCESS_LEAP_SECOND_INFO_FLAG_ENABLE_SIXTY_SECOND
is set for the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code for handling negative leap seconds:
Lines 102 to 109 in 7373b98
if (_St.wSecond == 59) { | |
--_St.wSecond; | |
if (FILETIME _Prev_sec_ft; SystemTimeToFileTime(&_St, &_Prev_sec_ft) != FALSE) { | |
// negative leap second | |
const long long _Prev_sec_ticks = _File_time_to_ticks(_Prev_sec_ft); | |
return {._Ticks = _Prev_sec_ticks + _Ticks_per_sec, ._Ec = _Nonexistent}; | |
} | |
} |
Co-authored-by: Miya Natsuhara <[email protected]>
Conflicts: stl/inc/chrono
Apologies for the delay here - is this ready for another review? |
Not yet. I'm working on |
Hi @statementreply - we talked about this at the weekly maintainer meeting, as your PR is now the oldest active PR 🎂 🥳. As no significant changes have been made here in the last year and a half, and the branch has steadily gotten out of date relative to However, we remain open to making changes in this area, if you (or anyone else) would like to open a new PR after investigating what needs to be done. Smaller PRs addressing individual areas would be preferred (but if changes are strongly connected, then of course a larger PR is necessary). |
…#3681) Part of #1588. `_Xtime_get_ticks` and `system_clock::now` would no longer be equal after a future leap second when #1520 is eventually fixed. This PR changes the clock used in `_To_timespec64_sys_10_day_clamped` to be consistent with code that consumes its result, e.g. `target` in https://github.com/microsoft/STL/blob/091cad2eaaa5bc25873eb7261cae57ab123592f3/stl/src/cond.cpp#L72-L80
Windows 10 1809 and Windows Server 2019 introduced support for leap seconds (enabled by default, could be disabled system-wide). When leap seconds are enabled, leap seconds after July 2018 are included in the tick count of
FILETIME
. This is different from the old behavior thatFILETIME
doesn't count leap seconds. In the future, the sameFILETIME
value could represent different time points a few seconds apart depending on whether leap second support is available and enabled.This PR performs conversion between
FILETIME
and UTC by calling WindowsFileTimeToSystemTime
orSystemTimeToFileTime
to determine the interpretation ofFILETIME
by the underlying system. The following functions, which depend on interpretation ofFILETIME
, are modified to correctly handleFILETIME
:system_clock::now()
file_clock
conversionsfile_clock
I/OThis PR also modifies
_To_xtime_10_day_clamped
so that it no longer incorrectly callsystem_clock::now()
.@BillyONeal pointed out (on Discord) that this PR could break (already somewhat broken) timed wait ABI.
_To_xtime_10_day_clamped
, used bycondition_variable::wait_for
,this_thread::sleep_for
, etc., callssystem_clock::now()
internally to compute a value interpreted asGetSystemTimeAsFileTime + offset
by another component. My understanding is that if the linker picks the old version of_To_xtime_10_day_clamped
and the new version ofsystem_clock::now()
from different object files, all those wait functions could wait for a few seconds too short (assuming positive leap seconds) after future leap seconds.STL/stl/inc/chrono
Lines 6525 to 6547 in 65eb507
Fixes #1822. Fixes #1520.