Fix timestamps in Unix.stat on Windows#1057
Conversation
|
@damiendoligez - reviews notwithstanding, I'd like this to be a replacement of #861 for 4.04, 4.05 and trunk, please? I wanted to try to put in a regression test for this, but I can't think of a sane way of altering the system clock during CI testing?! |
Sounds reasonable. Can we have a review from @nojb for example? I think we'll have to do without a regression test. |
|
@dra27 : I admire your tenacity and encyclopedic knowledge of all things Windows. Truly I do. Yet, I wonder if there could be a simpler solution than this. Correct me if I'm wrong:
Do we really need to go through (year, month, day, hour, min, sec) representation, with leap years, leap seconds probably, time zones, DST, and all that crap? Isn't it just a simple linear relation y = ax + b? |
|
A quick google search based on Xavier's remark yields this page: Also note that you should do the computation in floating-point because that's what we are returning, and we want sub-second precision. |
|
@xavierleroy - seeing the wood for the trees, I think! My (our) former concern has been to reimplement as little of the CRT as humanly possible. Circumventing calling the CRT function here is certainly simpler, what it isn't is necessarily equivalent - the UCRT's uses _mktime. That said, looking at their code, I think they suffer from the same TZ-related problem that I've fixed here (perhaps I should tell Project Zero.........). So I think any questions of divergence don't matter - I'll amend the code. @damiendoligez - I'll make sure the conversion back from 100ns to s is floating point, yes. |
556e214 to
3a13bca
Compare
|
@dra27 OK for me. Thanks ! |
|
Excellent! Please could either/both of @xavierleroy and @damiendoligez review? I would like to keep the first three commits in the history, so please don't please don't merge at once - I'll squash the last 4 commits together first. |
xavierleroy
left a comment
There was a problem hiding this comment.
Looks very good overall. Two minor remarks.
otherlibs/win32unix/stat.c
Outdated
| { | ||
| SYSTEMTIME sys; | ||
| FILETIME local; | ||
| unsigned __int64 utime = *((unsigned __int64*)time); |
There was a problem hiding this comment.
MSDN says somewhere "Do not cast a pointer to a FILETIME structure to either a ULARGE_INTEGER* or __int64* value because it can cause alignment faults on 64-bit Windows." and recommends to copy through a ULARGE_INTEGER structure instead.
There was a problem hiding this comment.
It does indeed - it goes to show why you shouldn't try to code when you're supposed to be in bed :-/ It's not totally clear to me from the documentation whether this is an Itanium problem (so definitely irrelevant) or an x86-64 problem but, either way, this is hardly a performance-critical piece of code!
otherlibs/win32unix/stat.c
Outdated
| /* There are 11644473600000 seconds since 1 January 1601 (the NT Epoch) and | ||
| * 1 January 1970 (the Unix Epoch). FILETIME is measured in 100ns ticks. | ||
| */ | ||
| *result = (utime - 116444736000000000ULL); |
There was a problem hiding this comment.
Do old versions of MSVC understand the ISO C99 suffix ULL ? Do we care about those versions?
There was a problem hiding this comment.
Another good point 😊
A museum curator did this before elsewhere: https://github.com/ocaml/ocaml/blame/trunk/byterun/ints.c#L504-L510
18ca868 to
397a7b6
Compare
|
OK, this is finally ready, via a long diversion into creating a test case. @xavierleroy's two comments should be dealt with - for the integer literals, it made sense to hoist the macro already in byterun so that it can be used anywhere. The test case started out as a simple idea (!) based on some reading originally for the stack overflow structured exception handler. The code in fakeclock.c partially implements a per-process clock on Windows. I think the principle might end up being useful in testing elsewhere and possibly worth spinning out into a more full implementation, so for the time being I'm basically parking it in the testsuite! It does ensure that this doesn't accidentally creep in again, but in a somewhat contrived manner... |
|
I'll rebase this one when my Windows 10 VM allows me to login (one seems to be spend most of the time working with Windows Insider Builds looking at updates screens....) but can I make a bit of noise on the key changes from this PR? I would like to put the test case commit on trunk, but I don't think that's necessary for 4.04 or 4.05, so I'd cherry-pick all except that and I'd quite like to get that in! |
otherlibs/win32unix/stat.c
Outdated
| *result = _mktime64(&stamp); | ||
| } | ||
| if (utime.QuadPart) { | ||
| /* There are 11644473600000 seconds since 1 January 1601 (the NT Epoch) and |
There was a problem hiding this comment.
"since" should be "between"
|
OK for merge to trunk and cherry-pick (without test) to 4.04 and 4.05. |
397a7b6 to
a7f6a82
Compare
|
Rebased for trunk. Committed on to 4.04 in a230e30 and 4.05 in 4afea74 (without the test) Comments on the testsuite change appreciated, but otherwise this is ready to go on trunk too. I'd appreciate a rebase rather than a squash as I'd like to keep the history of the other fix before Xavier's suggestion. |
a7f6a82 to
25b74c4
Compare
25b74c4 to
06ff38e
Compare
This is largely to park some work for running tests with a different date/time to the system clock, because this may be useful in the future. Patching the system clock is easy, unfortunately the test ends up rather contrived as only the process, not kernel functions, notice the change, so other local time functions have to be patched as well.
The Win32 API used treats DST as a timezone offset rather than a bias, resulting in all timestamps being an hour out when the system date is during DST.
The Microsoft CRT interprets the TZ environment variable, which can affect the conversion of Windows UTC SYSTEMTIME structs to __time64_t with _mktime64 as the Windows API uses the system's timezone setting which could differ from the TZ variable (especially if a native executable is invoked from within Cygwin).
_mkgmtime64 is _mktime64, but assumes UTC.
With gratitude to the boss for spotting this apparently non-obvious manoeuvre!
06ff38e to
6c7af40
Compare
|
@dra27 Please add a Changes entry |
|
@mshinwell - a quirk of the multi-branch process means that the Changes entry for this GPR is already in trunk (it was part of 4.04.1). I've added the label instead. It was waiting on feedback with the testsuite change and should probably be rebased again before merging. |
|
@dra27 I'm not sure we have anyone else who understands the details of the fake clock anywhere near as well as you. The general idea seems reasonable to me and the implementation suitably distasteful. Here's a question though: are you sure that the fake clock isn't masking another bug by itself containing an error relating to the DST offset? Assuming it doesn't contain such an error, I think it's ok to merge this. |
|
@mshinwell - the fake clock is considerably more likely just to fail to change the clock (so you just get the normal behaviour for the test); I'm confident with it, yes. |
- OCaml 4.04.1 and newer move the definition of INT64_LITERAL into config/m.h. Add the corresponding definition for these compilers at configure time. (See ocaml/ocaml#1057) - OCaml 4.04.2 needs either getuid() and friends or secure_getenv(). Add a stub for the latter and define availability in config/s.h.
Co-authored-by: Cuihtlauac ALVARADO <cuihtmlauac@tarides.com>
This is a continuation of MPR#7385 and #861.
The use of two different branches of code is ugly, but I offer some points in support of doing it this way:
_mkgmtime64is much safer, so it seems unfortunate not to use this for both 64-bit ports and the Microsoft 32-bit C compilers which support it.TZand_tzsetis template code which will be refactored to solve MPR#7489._mkgmtime64function in msvcrt.dll introduced in Windows Vista. I don't (yet) have a copy of 64-bit Windows XP or Windows Server 2003 x64 to confirm it absolutely, but Windows XP x86-64 was released 4 years after Windows XP x86, which is probably why mingw64 permits the function as all known 64-bit versions of msvcrt.dll should have it.