thread_local for MinGW-w64 builds
#30099
The assumption in the commit 188ca75e5fe4837d16241446558c7566912f67b2 about the broken thread_local implementation in GCC was misguided because the initial failure was not due to GCC, but a bug in the Wine
runtime, as evidenced, for example, here:
Consequently, it is safe to re-enable thread_local support for MinGW-w64 builds.
Fixes one item from #29952.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
The previously mentioned erroneous behavior is not an issue anymore
Erm, as of what version? Or what fix? We need more info here to be able to have any confidence.
Erm, as of what version?
I used this test case on platforms that support C++20.
… which means, starting from gcc 9.3 (Ubuntu 20.04) and onwards.
Or what fix?
It is hard to say because https://github.com/bitcoin/bitcoin/commit/188ca75e5fe4837d16241446558c7566912f67b2 refers to the mentioned test case only. There was no links to any upstream issues.
We need more info here to be able to have any confidence.
At this moment, there are no evidences that the test case fails for any supported platform.
It is hard to say because https://github.com/bitcoin/bitcoin/commit/188ca75e5fe4837d16241446558c7566912f67b2 refers to the mentioned test case only. There was no links to any upstream issues.
What did you find in the mingw-w64 / GCC changelogs?
Erm, as of what version? Or what fix? We need more info here to be able to have any confidence.
there are no evidences that the test case fails for any supported platform.
There’s also not really much evidence anything has been fixed. It seems it’s just assumed that everything is now “fine” because a testcase no-longer appears to fail. However, given the historical issues we’ve seen with GCC & mingw-w64, and in particular, in regards to threading, that doesn’t seem like the right approach.
Please note, that the test case fails when running a Windows binary a.exe on Ubuntu 14.04 LTS. It is reasonable to assume that it uses packages wine-binfmt and wine.
The error messages like “err:ntdll:RtlpWaitForCriticalSection section 0x100a8 “heap.c: main process heap section” wait timed out in thread 0064, blocked by 0055, retrying (60 sec)” are specific to the Wine runtime. For example, https://www.winehq.org/pipermail/wine-devel/2003-January/013655.html.
My point is that the https://github.com/bitcoin/bitcoin/commit/188ca75e5fe4837d16241446558c7566912f67b2 was not justified by the tests that proved a bug in GCC. It rather showed a bug in the Wine runtime.
utACK df879e5a91134a67ada3167ebff4e87f163b81a9
Seems reasonable to assume that this was a wine issue, or another issue that is now fixed.
The previously mentioned erroneous behavior is not an issue anymore. explains nothing.
Now that the research has been done, it should be added to the commit message and PR description, so it’s clear why this change might be safe, or the previous thread_local assumptions misguided. The commit message of
The previously mentioned erroneous behavior is not an issue anymore.explains nothing.
Thanks! Done.
🐙 This pull request conflicts with the target branch and needs rebase.
The assumption in the commit 188ca75e5fe4837d16241446558c7566912f67b2
about the broken `thread_local` implementation in GCC was misguided
because the initial failure was not due to GCC, but a bug in the Wine
runtime, as evidenced, for example, here:
- https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=917307
- https://bugs.freedesktop.org/show_bug.cgi?id=108662
Consequently, it is safe to re-enable `thread_local` support for
MinGW-w64 builds.