Bugfix? Restore linking to libmingwthrd #18427

Possibly necessary to get threadsafe stdlib behaviours

#17740 removed this, under the (accidental) false pretence that AC_CHECK_LIB didn'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 -mthreads still links to it).

Seems safest to just add it back in...

[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 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.

Furthermore, we don't have a minimum required version of mingw,

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.

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.

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.

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 -mthreads works correctly with static binaries now, that's probably the best route to go)

At the very least, if removing it, we should have some kind of check for a minimum MinGW version,

I'm not opposed to a adding a minimum-gcc check to enforce the minimum that is in dependencies.md.

OTOH, are any of the compilers that support C++11 and C++11 threads (so can even compile our code at all) affected?

(Alternatively, if someone wants to put in the time to see if -mthreads works correctly with static binaries now, that's probably the best route to go)

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...

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #24391 (build: stop overriding user autoconf flags by fanquake)
• #23969 (build: remove use of TARGET_OS and BUILD_OS by fanquake)

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.

<!--a722867cd34abeea1fadc8d60700f111-->

Gitian builds

Dunno about that, Gentoo has them all separate...

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.

There's also not really a reason to use old mingw libraries.

New ones do have outstanding issues with their build system FWIW.

Needs rebase if still relevant.

Rebased

https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gcc/x86-Options.html:

-mthreads

Support thread-safe exception handling on MinGW. Programs that rely on thread-safe exception handling must compile and link all code with the -mthreads option. When compiling, -mthreads defines -D_MT; when linking, it links in a special thread helper library -lmingwthrd which cleans up per-thread exception-handling data.

All supported compiler version has the -mthreads option.

This PR aims to introduce a precaution in possible cases when the -mthreads option is broken or changed, right?

If that case how one can assure that the libmingwthrd library is linked in a correct order?

Furthermore, we don't have a minimum required version of mingw,

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.

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 -mthreads, so this is a somewhat hacky way to ensure we get the expected linking for threading support.

If we can use -mthreads with all supported versions now, that would probably be a superior fix. But until someone has time to properly evaluate that, this seems to be good enough for now?

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 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".}

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.

What is the status of this? Are you planning on rebasing and responding to review comments?

I can't convince myself that re-linking to mingwthrd is needed.

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: https://github.com/msys2/MINGW-packages/issues/9850

Closing.