util: fix compilation with mingw-w64 7.0.0

fanquake commented at 8:10 am on March 16, 2020: member

Something has changed in the mingw-w64 headers such that we no-longer compile when using 7.0.0.

0util/time.cpp: In function 'std::__cxx11::string FormatISO8601DateTime(int64_t)':
1util/time.cpp:84:9: error: 'gmtime_r' was not declared in this scope
2     if (gmtime_r(&time_val, &ts) == nullptr) {
3         ^~~~~~~~
4util/time.cpp: In function 'std::__cxx11::string FormatISO8601Date(int64_t)':
5util/time.cpp:97:9: error: 'gmtime_r' was not declared in this scope
6     if (gmtime_r(&time_val, &ts) == nullptr) {

Looking at time.h, it seems that gmtime_r() is only available when _POSIX_C_SOURCE is defined. This must have been the case for 6.0.0 (which we compile fine using), but no-longer seems to be for 7.0.0?

I’ve checked that adding -D_POSIX_C_SOURCE=200112L to our compile flags does fix the issue above.

However, an alternative solution seems to be to just use gmtime_s() instead, when compiling with mingw-w64, as gmtime_r() just wraps gmtime_s() anyways.

I’ve tested this change crosss-compiling on Debian Bullseye (mingw-w64 7.0.0) and Buster (mingw-w64 6.0.0).

fanquake added the label Utils/log/libs on Mar 16, 2020

fanquake added the label Needs gitian build on Mar 16, 2020

hebasto commented at 8:46 am on March 16, 2020: member

Concept ACK.

hebasto commented at 8:48 am on March 16, 2020: member

However, an alternative solution seems to be to just use gmtime_s() instead, when compiling with mingw-w64, as gmtime_r() just wraps gmtime_s() anyways.

Does this alternative solution has any downsides?

practicalswift commented at 11:22 am on March 16, 2020: contributor

Concept ACK

DrahtBot commented at 6:16 am on March 17, 2020: member

Gitian builds

File	commit 86623873095f8d73fd28ad323ed3d06e20433176(master)	commit 5794c4e06a4e8c9bff1a4292c23ce4a2e2508eea(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz	`184a2e3f976d1afe...`	`69032278b4fb3194...`
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz	`aa5c3d4eb36052a2...`	`e6af093a3940130b...`
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz	`88a802223576828c...`	`cdcd2eaf388b7995...`
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz	`f5f9101a04efcbc4...`	`7f05a1cb48114e2e...`
bitcoin-0.19.99-osx-unsigned.dmg	`4e7a5cafd7f83e18...`	`8a1c33df35629b92...`
bitcoin-0.19.99-osx64.tar.gz	`c8d48a19665cd75e...`	`8497d3b315cb488e...`
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz	`c12106ddc8c14bb1...`	`f7c7ae68c0e5c8cf...`
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz	`3e860dcefdc35267...`	`f2d979e190f23d7d...`
bitcoin-0.19.99-win64-debug.zip	`967c32a4058a802b...`	`2745f915464f60be...`
bitcoin-0.19.99-win64-setup-unsigned.exe	`8254f4e5a1f09c6a...`	`d5298e31d6527c3e...`
bitcoin-0.19.99-win64.zip	`d5fb1ac9cc809c7c...`	`f08f040863159612...`
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz	`5644bfd3c6efa7ba...`	`3a8ebf737fc31954...`
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz	`0d4ba03883044861...`	`6a94bc2d5266cdf9...`
bitcoin-0.19.99.tar.gz	`3abdc69dfe4d5f23...`	`48d1a69f4afb58a6...`
bitcoin-core-linux-0.20-res.yml	`f3815dcc9396f613...`	`c5c21257a6a86019...`
bitcoin-core-osx-0.20-res.yml	`9e00cf2496e87ee7...`	`e09498c9b06fd794...`
bitcoin-core-win-0.20-res.yml	`acd91b8db1409e99...`	`69ca4551b3c51e2e...`
linux-build.log	`08ce938f565b6375...`	`062edb1ef74dbde8...`
osx-build.log	`c7851524186d3b65...`	`1452c13376551126...`
win-build.log	`3f7133b0888ce7a0...`	`92a02991bb54c1b5...`
bitcoin-core-linux-0.20-res.yml.diff		`0fe8e86861a8f3d9...`
bitcoin-core-osx-0.20-res.yml.diff		`d4bbb2d32700c589...`
bitcoin-core-win-0.20-res.yml.diff		`9970ee113bb8bb83...`
linux-build.log.diff		`50af512810c062fc...`
osx-build.log.diff		`8cfc3a33373d8b5c...`
win-build.log.diff		`78f307674781bbb3...`

DrahtBot removed the label Needs gitian build on Mar 17, 2020

dongcarl commented at 7:29 pm on March 17, 2020: member

Did a bit more digging. Here’s what I found:

This commmit is the one that caused the change in behaviour.

There’s a very short mailing list thread about it here.

My conversation with the author of the commit on IRC:

 015:02 <dongcarl> jacekc: After your "Don't define _POSIX_THREAD_SAFE_FUNCTIONS." patch, which landed in 7.0.0, what is the right way to get `gmtime_r` in my application?
 115:02 * dongcarl waits patiently
 215:04 <jacekc> dongcarl: ideally, I'd suggest to use gmtime_s
 315:04 <dongcarl> jacekc: Why's that?
 415:05 <jacekc> because that's what the platform you target offers
 515:06 <jacekc> but if you prefer mingw wrappers, you may define _POSIX_THREAD_SAFE_FUNCTIONS or _POSIX_C_SOURCE
 615:07 <dongcarl> jacekc: I think I see what you're saying. I'm assuming the mingw wrappers don't offer thread safety, even though the name is gmtime_r?
 715:07 * dongcarl is thankful for the patient response
 815:09 <jacekc> both _s and _r variants offer thread safety
 915:09 <jacekc> _r variant is just an inline wrapper in time.h aroud _s
1015:12 <dongcarl> jacekc: I see. Out of curiosity, what's the difference between defining _POSIX_THREAD_SAFE_FUNCTIONS and _POSIX_C_SOURCE? Feel free to just send me a link to read
1115:14 <jacekc> _POSIX_THREAD_SAFE_FUNCTIONS enables only a few inline functions in time.h, _POSIX_C_SOURCE enables much more (most notably mongw stdio implementation)
1215:15 <jacekc> I don't know if it's documented anywhere, but it's easy to find in the sources
1315:16 <dongcarl> jacekc: Thanks for your help. Very much appreciated and have a tremendous day!
1415:16 <jacekc> you welcome

Here’s some more general use information on _POSIX_C_SOURCE and _POSIX_THREAD_SAFE_FUNCTIONS.

MarcoFalke commented at 8:32 pm on March 17, 2020: member

Looks like this makes the include order of headers more resilient and fixes a bug I was running in to a year ago: https://github.com/bitcoin/bitcoin/pull/16046/files#r285601479

If you feel like, you might sort the includes in the header now:

 0diff --git a/src/util/time.h b/src/util/time.h
 1index 77de1e047d..a8c06ef350 100644
 2--- a/src/util/time.h
 3+++ b/src/util/time.h
 4@@ -6,9 +6,9 @@
 5 #ifndef BITCOIN_UTIL_TIME_H
 6 #define BITCOIN_UTIL_TIME_H
 7 
 8+#include <chrono>
 9 #include <stdint.h>
10 #include <string>
11-#include <chrono>
12 
13 void UninterruptibleSleep(const std::chrono::microseconds& n);
14

dongcarl commented at 10:35 pm on March 17, 2020: member

If I’m thinking about this the right way, your diff implies that previously the #ifdef _MSC_VER would fail, and branch to the #else, which used gmtime_r, which would be fine because it was defined by default in mingw-w64.

If #ifdef _MSC_VER is how we detect if we’re building for windows, and it’s failing, then perhaps there are other places in the codebase where this detection needs to be fixed?

fanquake force-pushed on Mar 18, 2020

fanquake commented at 9:21 am on March 18, 2020: member

If you feel like, you might sort the includes in the header now:

Done.

If #ifdef _MSC_VER is how we detect if we’re building for windows, and it’s failing, then perhaps there are other places in the codebase where this detection needs to be fixed?

I’ve looked into this, and there are some cases where we are using _MSC_VER where we might be running into unexpected behavior. I don’t necessarily want to address that as part of this PR, but will definitely be following up.

MarcoFalke approved

MarcoFalke commented at 1:41 pm on March 18, 2020: member

ACK 1b801e770477e756d67402970694bca73fa0ac7f

cc @sipsorcery

hebasto commented at 1:55 pm on March 18, 2020: member

Something has changed in the mingw-w64 headers such that we no-longer compile when using 7.0.0.

Interesting why master could be built successfully with Ubuntu’s g++-mingw-w64 7.3.0?

fanquake commented at 1:58 pm on March 18, 2020: member

Interesting why master could be built successfully with Ubuntu’s g++-mingw-w64 7.3.0?

The version of mingw-w64 being used is 5.0.0. 7.3.0 is the version of GCC.

hebasto approved

hebasto commented at 5:24 pm on March 18, 2020: member

ACK 1b801e770477e756d67402970694bca73fa0ac7f, tested on Ubuntu 20.04 with mingw-w64 7.0.0.

dongcarl commented at 5:40 pm on March 18, 2020: member

From #bitcoin-builds meeting notes today: We should investigate how to detect features (i.e. check for function availability at configure time), rather than detecting platforms. This might be more robust.

MarcoFalke commented at 5:56 pm on March 18, 2020: member

@dongcarl Is this a general advise/issue or a blocker on this specific patch set?

dongcarl commented at 9:50 pm on March 18, 2020: member

@MarcoFalke I think it’s a blocker while we investigate if feature detection is the right path take going forward, no point merging this then changing right after (unless the release schedule dictates?)

Empact commented at 10:27 pm on March 18, 2020: member

Here’s an alternative: https://github.com/bitcoin/bitcoin/compare/master...Empact:2020-03-gmtime-config

Note I haven’t run it on windows/mingw, and I’m not sure how to get the test to distinguish between MSC-style gmtime_s and C-standard-style gmtime_s - i.e. I doubt the existing casting-based approach.

Edit: and another more succinct alternative: https://github.com/bitcoin/bitcoin/compare/master...Empact:2020-03-gmtime-check-funcs

Inspired by sqlite’s handling of the same question: https://github.com/sqlite/sqlite/blob/118efd162632298bccba21b71934f666e556f594/configure.ac#L111

Although they fall back to gmtime, so it’s not directly comparable.

sipsorcery commented at 8:30 pm on March 19, 2020: member

ACK 1b801e770477e756d67402970694bca73fa0ac7f.

laanwj commented at 3:40 pm on March 25, 2020: member

Using the windows function on windows (instead of mingw’s wrapper) makes sense, imo.

in src/util/time.cpp:81 in 1b801e7704 outdated

77@@ -78,7 +78,7 @@ int64_t GetSystemTimeInSeconds()
78 std::string FormatISO8601DateTime(int64_t nTime) {
79     struct tm ts;
80     time_t time_val = nTime;
81-#ifdef _MSC_VER
82+#if defined(_MSC_VER) || defined(__MINGW64_VERSION_MAJOR)

laanwj commented at 3:40 pm on March 25, 2020:

can’t we change this to #ifdef WIN32 ?

MarcoFalke added the label Waiting for author on Mar 25, 2020

laanwj added this to the milestone 0.20.0 on Mar 25, 2020

dongcarl commented at 1:51 am on April 1, 2020: member

Hmmm, I think perhaps what we want to do is similar to what’s being done here: https://github.com/maru/libmicrohttpd-http2/blob/316af7c06537e524c6ef02bc1d5f59b39af7d1d1/configure.ac#L1033-L1082

It’s feature-based detection, and we’ll have clarity between HAVE_C11_GMTIME_S, HAVE_W32_GMTIME_S, and HAVE_GMTIME_R.

laanwj commented at 10:21 am on April 1, 2020: member

Yeah, that’s another way. I wish this kind of basic functionality was standardized by now. This comes up so often.

If we still want this for 0.20, we need to make a decision here quickly though. I’m sorry for starting a bikeshed :laughing:

dongcarl commented at 5:23 pm on April 1, 2020: member

Yeah I think we should go with @Empact’s https://github.com/bitcoin/bitcoin/compare/master...Empact:2020-03-gmtime-config, it’s feature-based detection, and errors at configure-time if we don’t support the platform instead of error-ing at make-time.

Perhaps the error message should read “both gmtime_r and MSC-style gmtime_s are missing” for disambiguation.

Ping @fanquake

I think we should get this into 0.20, because Ubuntu 20.04 LTS is going to release with mingw-w64 v7.0.0 (also, would help unblock #17595 (review))

fanquake commented at 4:06 am on April 2, 2020: member

Yeah I think we should go with @Empact’s

I’m taking a look at this, and it doesn’t quite work as expected. The AC_MSG_CHECKING output doesn’t actually get printed in the gmtime_s success case. However I think we can use this once that is fixed and messages are adjusted as required.

build: Detect gmtime_* definitions via configure

This improves the portability of the codebase and fixes compilation
with mingw-w64 7.0+.

Co-authored-by: fanquake <fanquake@gmail.com>

a46484c8b3

fanquake force-pushed on Apr 2, 2020

fanquake removed the label Waiting for author on Apr 2, 2020

fanquake commented at 5:50 am on April 2, 2020: member

doesn’t actually get printed in the gmtime_s success case.

Maybe I was just imaging this.. Have pushed up some new changes.

Tested on macOS:

0checking for gmtime_r... yes

mingw-w64 cross compile:

0checking for gmtime_r... no
1checking for gmtime_s... yes

debian:

0checking for gmtime_r... yes

MSVC ✅

Empact commented at 8:54 am on April 2, 2020: member

Note one thing I have not tested with this implementation is how it operates if gmtime_r is not present, but C11-style gmtime_s is.

hebasto commented at 10:33 am on April 2, 2020: member

Tested a46484c8b3715725f5dc0b8ad1bf921880ed9af1 on focal and bionic. Both produce:

0$ CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site ./configure
1...
2checking for gmtime_r... no
3checking for gmtime_s... yes
4...

To make things clear about cross compiling on systems with pre-7.0 mingw-w64 headers, e.g., on bionic: on master the gmtime_r() is used, and with this PR the gmtime_s() is used, right?

laanwj commented at 10:46 am on April 2, 2020: member

ACK a46484c8b3715725f5dc0b8ad1bf921880ed9af1

laanwj merged this on Apr 2, 2020

laanwj closed this on Apr 2, 2020

theuni commented at 3:23 pm on April 2, 2020: member

Post-merge ACK. Thanks for making this test-based, I think this is a nice improvement.

fanquake deleted the branch on Apr 3, 2020

fanquake commented at 12:22 pm on April 3, 2020: member

To make things clear about cross compiling on systems with pre-7.0 mingw-w64 headers, e.g., on bionic: @hebasto the breakdown is:

commit	Debian Buster (mingw-w64 6.0.0)	Debian Bullseye (mingw-w64 7.0.0)
5c1ba3a10a18c6edee20f27f3609ffbbbaa2e8f3~1	gmtime_r()	fails to compile
5c1ba3a10a18c6edee20f27f3609ffbbbaa2e8f3	gmtime_s()	gmtime_s()

random-zebra referenced this in commit 14060430d9 on May 5, 2021

kittywhiskers referenced this in commit 046440f408 on Jul 4, 2021

kittywhiskers referenced this in commit 2bac9b72bb on Jul 4, 2021

kittywhiskers referenced this in commit d47fe88b78 on Jul 9, 2021

kittywhiskers referenced this in commit a6bb3efe93 on Jul 9, 2021

kittywhiskers referenced this in commit e641792088 on Jul 9, 2021

kittywhiskers referenced this in commit e7aeb25560 on Jul 13, 2021

kittywhiskers referenced this in commit a4781155ce on Jul 15, 2021

kittywhiskers referenced this in commit 4ff6055cc1 on Jul 15, 2021

kittywhiskers referenced this in commit e203126f68 on Jul 16, 2021

kittywhiskers referenced this in commit 21ace3c92e on Aug 1, 2021

kittywhiskers referenced this in commit 3b08049172 on Aug 5, 2021

kittywhiskers referenced this in commit 1d5c176943 on Aug 5, 2021

UdjinM6 referenced this in commit 7aebf156e9 on Aug 10, 2021

5tefan referenced this in commit 342903cc96 on Aug 12, 2021

DrahtBot locked this on Feb 15, 2022

util: fix compilation with mingw-w64 7.0.0 #18358

Gitian builds