-D_FILE_OFFSET_BITS=64 also changes the size of time_t to 64 bits (see #3494). This is a bug in mingw: http://sourceforge.net/p/mingw-w64/bugs/279/
Override this by adding -D_USE_32BIT_TIME_T.
According to @theuni: ‘sizeof(time_t) should be 4 for win32. If it’s 8, the headers are buggy and runtime will be unpredictable. At runtime, a 4byte value is stored in an 8byte var, leaving half uninit’d. Using a recent mingw it’s 4 as expected. I’m assuming gitian’s version still has this bug.’
`-D_FILE_OFFSET_BITS=64` also changes the size of time_t to 64 bits.
This is a bug in mingw: http://sourceforge.net/p/mingw-w64/bugs/279/
Override this by adding `-D_USE_32BIT_TIME_T`.
According to @theuni: 'sizeof(time_t) should be 4 for win32. If it's 8,
the headers are buggy and runtime will be unpredictable. At runtime, a
4byte value is stored in an 8byte var, leaving half uninit'd. Using a
recent mingw it's 4 as expected. I'm assuming gitian's version still has
this bug.'
@theuni: @wtogami found out the reason why this issue appears now: with 0.8.x we didn’t define _FILE_OFFSET_BITS on mingw (neither in bitcoin.pro or makefile.mingw)!
This was introduced with the autotools switch.
So that leaves another potential solution: skip _FILE_OFFSET_BITS for win32. No idea if this has any other consequences, but no one ever noticed the lack of it in 0.8.
Let’s please slow down a bit..
The only reasonable way to address this problem (assuming it needs to be addressed) is at the toolchain level, either by self-building mingw or upgrading from precise.
But before going to those lengths, it’s prudent to determine if it’s a real-world problem. It’s a mingw bug, but it’s possible that it’s only exposed by wine’s runtimes.
We don’t define it now either. Autoconf defines it as a result of the large-file test, where it determines what preproc defines are necessary. To explicitly turn it off, you can use ./configure --disable-largefile.
Seeing as tests deem this necessary, as does bdb and the other deps, doing that without examining the consequences would be unwise.
I too doubt that it’s a real-world issue. As I mentioned in the other issue, with newer wine there are no problems, and @diapolo doesn’t seem to suffer from it either (on real windows).
That’s why I closed it in the first place.
When we see actual windows users reporting the time problems, at least we know where to look…
Yea, I was probably a bit hasty in calling for the reopen, sorry about that. I just wasn’t comfortable leaving it resolved as a “wine bug” since the real culprit is our production toolchain.
I’d be ok with closing it and keeping a watchful eye, but I’d rest easier if we added something like (in GetTime()):
0#if _WIN32 && mingw_ver = foo
1int64_t bar = time(0)
2assert(bar constraint)
3#endif
That wouldn’t solve anything, but at least it would lest us know if it manages to happen in practice.
Looking at the disassembly of the binary generated by gitian,
00059aa2f <__Z7GetTimev>:
1 59aa2f: 55 push %ebp
2 59aa30: 89 e5 mov %esp,%ebp
3 ...
4 59aa5a: e8 81 d3 35 00 call 8f7de0 <_time>
5 59aa5f: c9 leave
6 59aa60: c3 ret
7
8008f7de0 <_time>:
9 8f7de0: e9 1b 00 00 00 jmp 8f7e00 <__time32>
10
11008f7e00 <__time32>:
12 8f7e00: a1 10 1b cf 00 mov 0xcf1b10,%eax [relocation for mscvrt.dll:time]
13 8f7e05: ff e0 jmp *%eax
Or:
0GetTime
1 -> time
2 -> __time
3 -> __time32
4 -> [mscvrt.dll] time()
From what I understand time() in msvcrt.dll (legacy) on 32-bit platforms, returns a 32-bit value.
I must conclude that if this works, especially on older windows, it works only by accident.
I have no clue on a solution though, beyond building everything with _USE_32BIT_TIME_T. Windows is driving me crazy.
Wine source for msvcrt.time:
https://github.com/mirrors/wine/blob/master/dlls/msvcrt/time.c#L865
On win32 it returns a 32-bit value, on win64 it returns a 64 bit value.
To explicitly get a 64 bit value it’s possible to use __time64 – however, this doesn’t exist on older versions of msvcrt such as likely that on windows XP.
How about in GetTime():
0#ifdef WIN32
1// Workaround issue where time() sometimes returns only a 32-bit result, even if declared as 64-bit.
2return time(NULL) % 0xFFFFFFFF;
3#else
4return time(NULL);
5#endif
@sipa That would work for this specific instance, but there’s no telling if there are other 32/64 time_t conflicts as well. time_t’s are part of some structures and passed as arguments. Remember that the original crash in #3494 is in some formatting function.
… and that’s even ignoring the other calls to time(), for example in dependency libraries, which may or may not have issues.
Ok, new hypothesis: notice how mingw time.h has a few inlined functions that depend on _USE_32BIT_TIME_T: https://gist.github.com/laanwj/8342674#file-time-h-L233
Well, I found out that the current gitian win32 disables optimization (CXXFLAGS and LDFLAGS are overridden without specifying optimization flags!): This means that those functions do not get inlined.
As bitcoin is built statically, and seemingly not all the dependencies are built with the same time_t size, the two definitions conflict. This explains why util.c gets linked against the wrong definition of time() with the wrong size.
Building with optimization (even -O) should fix the immediate problem. I’m fairly sure of this, as I accidentally fixed it once by adding -g -O and thought it was the debug info that made the problem go away.
(and having different time_t sizes in different dependencies is not nice either… probably something to look at too, as it could cause problems if they’re exchanging time_t’s, but it’s one step…)
Verified: after adding -O2 it works perfectly, both the testcase and bitcoin-qt.exe itself:
0Running 109 test cases...
1
2*** No errors detected
Also the program is a lot smoother and faster.