Bugfix? Restore linking to libmingwthrd #18427
Conversation
|
[03:10:58] <fanquake> I just checked that binaries produced with and without -lmingwthrd are basically identical. I see 5 bytes difference, and that's the git commit and what looks like 2 timestamps. So at least gitian isn't affected |
There was a problem hiding this comment.
There was some discussion in IRC.
In 22b6398a8acf17e6687375c414fae832888de53a (Jun 2009) the contents of mingwthrd_mt.c which becomes libmingwthrd.a lib was changed to:
/* As _CRT_MT is getting defined in libgcc when using shared version, or it is getting defined by startup code itself,
this library is a dummy version for supporting the link library for gcc's option -mthreads. As we support TLS-cleanup
even without specifying this library, this library is deprecated and just kept for compatibility. */
int _CRT_MT_OLD = 1;I've re-tested building with and without -lmingwthrd and I'm not seeing any significance difference in the binaries produced.
configure.ac
Outdated
| @@ -487,6 +487,7 @@ case $host in | |||
| use_pkgconfig=no | |||
|
|
|||
| TARGET_OS=windows | |||
| AC_CHECK_LIB([mingwthrd],[main],, AC_MSG_ERROR(libmingwthrd missing)) | |||
There was a problem hiding this comment.
If we're going to add this back, I think there should at least be a comment stating why it's here.
From what I can gather, if you're using the oldest version of GCC we are meant to support (4.8), then the oldest version of mingw you would be able to use is v3. |
2e1e4c9 to
7fe4967
Compare
If this isn't a problem with any of the compilers that are realistically used to build bitcoin core, I tend towards NACK. We shouldn't keep old compatibility cruft around forever. |
It's still linked by current MinGW. Even if it does nothing, it is incorrect to omit it. At the very least, if removing it, we should have some kind of check for a minimum MinGW version, which is probably more complex than just doing the right thing here... (Alternatively, if someone wants to put in the time to see if |
I'm not opposed to a adding a minimum-gcc check to enforce the minimum that is in OTOH, are any of the compilers that support C++11 and C++11 threads (so can even compile our code at all) affected?
Yes, if that's the general suggestion, it would be good to add that, preferable to manually linking against a threading library. |
|
AFAIK this is related to the mingw runtime libraries, NOT the GCC version. |
|
Sure, but the mingw toolchain distributions tend to be monolithic, packaging a certain gcc/c++ version with a certain version of the library. |
|
Dunno about that, Gentoo has them all separate... |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Gitian builds
|
Even with that, the range of libc versions that can be used with a certain gcc/c++ version tends to be restricted. There's also not really a reason to use old mingw libraries. E.g. for glibc we target an old version because of compatibility reasons, but newer mingw do support old windows versions. And with that, using old windows versions for Bitcoin Core is a bad idea anyway as they're all unmaintained. |
New ones do have outstanding issues with their build system FWIW. |
|
Needs rebase if still relevant. |
7fe4967 to
570ac85
Compare
|
Rebased |
Possibly necessary to get threadsafe stdlib behaviours
|
https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gcc/x86-Options.html:
All supported compiler version has the This PR aims to introduce a precaution in possible cases when the If that case how one can assure that the
Could we use https://doc.qt.io/qt-5.15/windows.html to define a minimum required version of mingw? |
|
My understanding is that for some reason (I forget), we don't/can't use If we can use |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
There was a problem hiding this comment.
I can't convince myself that re-linking to mingwthrd is needed. I realise that adding this was the solution to an issue ~10 years ago, however I my current understanding is that this workaround was likely only needed for mingw, and that using mingw-w64, and only supporting 64 bit windows, has obsoleted any need for it.
GCC and mingw-w64 don't make trying to understand this easy. Both seem to be full of hacks / workarounds for each other, or just incorrect documentation. i.e mingw-w64 has the following related commentary:
/* mingw.org's version macros: these make gcc to define
MINGW32_SUPPORTS_MT_EH and to use the _CRT_MT global
and the __mingwthr_key_dtor() function from the MinGW
CRT in its private gthr-win32.h header. */However GCC hasn't cared about __MINGW32_MAJOR_VERSION or __MINGW32_MINOR_VERSION since 2009, where it started defining MINGW32_SUPPORTS_MT_EH unconditionally for _WIN32. See this commit.
| @@ -667,6 +673,7 @@ case $host in | |||
| AC_MSG_ERROR([windres not found]) | |||
| fi | |||
|
|
|||
| dnl As with above, _MT is required and normally defined by -mthreads | |||
There was a problem hiding this comment.
As with above, _MT is required
Can you explain what it's required for? I see that is is defined by GCC when using -mthreads. I also see that it's defined in mingw-w64.
However I can't seem to figure out what uses it. Nothing in our code, or dependency code seems to check whether it's defined, and/or modify it's behaviour. A Guix build run with/without it makes no difference.
I think if we are going add a comment about something being required, ideally we can mention why.
| @@ -638,6 +638,12 @@ AC_ARG_WITH([daemon], | |||
| case $host in | |||
| *mingw*) | |||
| TARGET_OS=windows | |||
|
|
|||
| dnl Some versions of MinGW may require libmingwthrd linked for a thread-safe stdlib implementation | |||
| dnl Normally, this is automatically linked by GCC's -mthreads flag, but for some reason (lost in the dust of time), that doesn't work when static linking | |||
There was a problem hiding this comment.
but for some reason (lost in the dust of time)
I really don't think adding comments like this are valuable. It's more wording that doesn't explain anything to the reader, now, or in the future. This also seems like something that could just be tested, to (re-)determine why it doesn't work, rather than adding a comment to say we don't know why. "Some versions" is also ambiguous, and I'm fairly certain it can only be mingw-w64 1.x.
|
What is the status of this? Are you planning on rebasing and responding to review comments? |
Also we explicitly require the POSIX threads implementation of mingw-w64. I'm quite sure mingwthrd is for win32-style threading in the old MinGW. See also: msys2/MINGW-packages#9850 Closing. |
Possibly necessary to get threadsafe stdlib behaviours
#17740 removed this, under the (accidental) false pretence that
AC_CHECK_LIBdidn't actually cause linkage (it does).It appears this hasn't been necessary since mingw 2.0 released in 2011 turned it into a dummy library, but CVE-2012-1910 occurred in 2012. Furthermore, we don't have a minimum required version of mingw, and it's possible future releases might use it again (GCC
-mthreadsstill links to it).Seems safest to just add it back in...