build: Enable thread_local for MinGW-w64 builds
#30099
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee 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. |
Erm, as of what version? Or what fix? We need more info here to be able to have any confidence. |
... which means, starting from gcc 9.3 (Ubuntu 20.04) and onwards.
It is hard to say because 188ca75 refers to the mentioned test case only. There was no links to any upstream issues.
At this moment, there are no evidences that the test case fails for any supported platform. |
What did you find in the mingw-w64 / GCC changelogs?
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 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 188ca75 was not justified by the tests that proved a bug in GCC. It rather showed a bug in the Wine runtime. |
|
I found that the test case error messages are quite similar to ones reported in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=917307. That bug has been fixed as well. |
|
utACK df879e5 Seems reasonable to assume that this was a wine issue, or another issue that is now fixed. |
|
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 |
Thanks! Done. |
|
utACK 5822a82 |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
The assumption in the commit 188ca75 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.
|
Rebased on top of the merged #30095. |
|
Closing in favour of #30137. |
17fe948 build: remove --enable-threadlocal (fanquake) 1e7c20b doc: remove comment about using thread_local (fanquake) 5bba433 build: Enable `thread_local` for MinGW-w64 builds (Hennadii Stepanov) Pull request description: Includes a commit from #30099. Closes #29952. ACKs for top commit: laanwj: Code review ACK 17fe948 maflcko: utACK 17fe948 hebasto: ACK 17fe948. theuni: utACK 17fe948 Tree-SHA512: 2aad6d19e79c4d6d7aefd0f41b215ac8d9320f5908808221d78e6ee1c77503832a02759bee2ad397e235b6739e22aca8dcf5c5ef8854deb8c697b18ac56a06da
The assumption in the commit 188ca75 about the broken
thread_localimplementation in GCC was misguided because the initial failure was not due to GCC, but a bug in the Wineruntime, as evidenced, for example, here:
Consequently, it is safe to re-enable
thread_localsupport for MinGW-w64 builds.Fixes one item from #29952.