Skip to content

<__msvc_chrono.hpp>: Use _Xtime_get_ticks in _To_timespec64_sys_10_day_clamped for consistency - #3681

Merged
Casey Carter (CaseyCarter) merged 1 commit into
microsoft:mainfrom
statementreply:to_timespec64_sys_10_day_clamped
May 18, 2023
Merged

<__msvc_chrono.hpp>: Use _Xtime_get_ticks in _To_timespec64_sys_10_day_clamped for consistency#3681
Casey Carter (CaseyCarter) merged 1 commit into
microsoft:mainfrom
statementreply:to_timespec64_sys_10_day_clamped

Conversation

@statementreply

@statementreply statementreply commented May 2, 2023

Copy link
Copy Markdown
Contributor

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

STL/stl/src/cond.cpp

Lines 72 to 80 in 091cad2

_timespec64 now;
_Timespec64_get_sys(&now);
_Mtx_clear_owner(mtx);
if (!cond->_get_cv()->wait_for(cs, _Xtime_diff_to_millis2(target, &now))) { // report timeout
_Timespec64_get_sys(&now);
if (_Xtime_diff_to_millis2(target, &now) == 0) {
res = _Thrd_timedout;
}
}

Billy O'Neal (@BillyONeal) pointed out (on Discord) that this PR could break (already somewhat broken) timed wait ABI:

_To_xtime_10_day_clamped, used by condition_variable::wait_for, this_thread::sleep_for, etc., calls system_clock::now() internally to compute a value interpreted as GetSystemTimeAsFileTime + 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 of system_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.

@StephanTLavavej

Copy link
Copy Markdown
Member

Looks good to me - no behavioral change now, and the consistency argument for the future is convincing. Thanks!

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added enhancement Something can be improved and removed bug Something isn't working labels May 12, 2023
@StephanTLavavej

Copy link
Copy Markdown
Member

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@CaseyCarter

Copy link
Copy Markdown
Contributor

Thanks for fixing this future bug before we could write it! 🕦 🕐 🕙 🕥 🕚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chrono C++20 chrono enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants