(I don't understand the details so apologies if this is a silly question, but) The MSDN documentation indicates that the TZ environment variable may influence the result of these functions. The fact that TZ would influence the result of Unix.gettimeofday was reported as a bug in MPR#6671, is there a risk of a similar issue popping up here?

It should not happen with the proposed workaround, since we would not be doing any timezone conversion.

We are running OCaml under Cygwin, so the TZ may be involved somehow. I will try to find out more.

This looks good to me.

@dra27 Do you have an opinion on this topic?

I need more time to be able to look at this properly - I've got a feeling that the implementation I did was designed for compatibility with the Microsoft C implementation (the code for that is available in any paid for distro of Visual Studio prior to 2015 and in all 2015 distros). I think the idea was that I only enhanced fields (so some fields are populated, such as st_ino which had fixed values and so would never have been used before), I didn't change any. More anon...

An update on this issue: this patch does not compile on mingw (maybe only in 32 bits, did not check) as it does not include the function mkgmtime64. I close this PR since I do not see a way around this. However, the bug remains.

I think that fixing for the MSVC ports would already be useful, even if having a different behavior between MSVC and Mingw is not great.

OK, I will add the necessary preprocessor macros and update the patch.

OK, sorry for the long delay on my input here. I have no idea how this slipped through when I developed stat - perhaps it was a combination of my working in a VM (which has a habit of resetting the system clock to the host) or that I developed this while in the Netherlands and got confused from my usual lazy comfort of living in UTC+0!

So, what is wrong with the code is the use of the wrong API function - it's step 2 which is the problem. FileTimeToLocalFileTime does not take DST into account so, giving my own time zone of GMT/BST, during BST it converts a time to UTC+1 literally (i.e. it treats BST as a timezone, rather than a bias). But this is not correct. It means, for example, that if you are trying to convert the stamp 12-Feb-2017 20:43:00 on a computer in BST then FileTimeToLocalFileTime will give 12-Feb-2017 21:43:00 UTC+1 which is correct, but not what you want. All the remaining functions work as intended - they've simply been given the wrong hour.

The correct sequence - noted in MSDN (brief blush for not reading the docs properly...) is FileTimeToSystemTime followed by SystemTimeToTzSpecificLocalTime which does take DST into account and so gives 12-Feb-2017 20:43:00 BST. This fixes the problem:

--- a/otherlibs/win32unix/stat.c
+++ b/otherlibs/win32unix/stat.c
@@ -118,11 +119,11 @@ static value stat_aux(int use_64, __int64 st_ino, struct _stat64 *buf)
 static int convert_time(FILETIME* time, __time64_t* result, __time64_t def)
 {
   SYSTEMTIME sys;
-  FILETIME local;
+  SYSTEMTIME local;

   if (time->dwLowDateTime || time->dwHighDateTime) {
-    if (!FileTimeToLocalFileTime(time, &local) ||
-        !FileTimeToSystemTime(&local, &sys))
+    if (!FileTimeToSystemTime(time, &sys) ||
+        !SystemTimeToTzSpecificLocalTime(NULL, &sys, &local))
     {
       win32_maperr(GetLastError());
       return 0;

The issue with mingw32 is because it only permits the binary API of Visual Studio 6.0 (the full and reasonable explanation is here). It does this with good reason - _mkgmtime64 is not available in msvcrt.dll until Windows Vista. I'm not convinced that this single issue is a good enough reason to drop Windows 2000 and Windows XP completely.

Using _mkgmtime64 is better, though, because _mktime64 can be affected by the TZ environment variable so for my patch above to be safe we'd need to ensure its unset and call _tzset or SystemTimeToTzSpecificLocalTime and _mktime64 could assume different time zones.

My feeling is that both versions should go in - the additional safety offered by _mkgmtime64 should be taken advantage of where possible and the _mktime64 version only used for compatibility.

I've proposed #1057 for this.

Thanks @dra27 for the thorough explanation! Closing this to concentrate the discussion at #1057.