| Summary: | No thread safety when built with mingw-w64 | ||
|---|---|---|---|
| Product: | pixman | Reporter: | Benjamin Gilbert <bgilbert> |
| Component: | pixman | Assignee: | Søren Sandmann Pedersen <soren.sandmann> |
| Status: | RESOLVED FIXED | QA Contact: | Søren Sandmann Pedersen <soren.sandmann> |
| Severity: | normal | ||
| Priority: | medium | CC: | jon.turney, siarhei.siamashka |
| Version: | 0.28.x | ||
| Hardware: | Other | ||
| OS: | Windows (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | configure.ac patch | ||
That's interesting, so mingw is broken too, not just clang:
http://lists.freedesktop.org/archives/pixman/2012-October/002320.html
Seems to be worth fixing.
Does changing the order of __declspec(thread) and __thread fix the problem? It fixes the current problem, yes. But it still leaves a timebomb: pixman would almost-silently disable thread safety on any compiler that rejects __thread and ignores __declspec(thread). What is the __declspec(thread) case trying to accomplish? Does pixman really support using Autotools to configure MSVC builds? (In reply to comment #3) > It fixes the current problem, yes. But it still leaves a timebomb: pixman > would almost-silently disable thread safety on any compiler that rejects > __thread and ignores __declspec(thread). Well, in my opinion the compilers should not normally do that (and it's better to report it as a bug to the compiler developers). How are we supposed to know that some feature is not supported if the compiler does not reject it with an error? Adding -Werror to CFLAGS may be a good idea here, but seems like there were some problems with it in some environments: http://cgit.freedesktop.org/pixman/commit/?id=af803be17b4ea5f53db9af57b6c6ef06db99ebbd Running compiled executables for runtime checks is not an option because this breaks crosscompilation. Regarding your timebomb remark, at least we are safe in linux because "make check" detects the problem (at least on multicore processors) in the case if TLS is broken. Does mingw-w64 support OpenMP? > What is the __declspec(thread) case trying to accomplish? Does pixman > really support using Autotools to configure MSVC builds? I also would like to know. Presumably it it is somehow useful for iOS, but the author of the problematic commit did not reply to my question "Regarding your original patch, how does iOS behave when somebody tries to use __thread keyword? Does it fail compilation?" at http://lists.freedesktop.org/archives/pixman/2012-October/002322.html The commit messages for both pixman and xserver do not explain whether they are fixing any real problem and what was the issue with the __thread keyword in the first place: http://cgit.freedesktop.org/pixman/commit/?id=a069da6c66da http://cgit.freedesktop.org/xorg/xserver/commit/?id=bb4d145bd25e2aee (In reply to comment #4) > Well, in my opinion the compilers should not normally do that (and it's > better to report it as a bug to the compiler developers). Clearly at least two compilers *do* do that, so configure ought to consider that case. (And from the compilers' perspective, the behavior is probably reasonable.) > Regarding your timebomb remark, at least we are safe in linux because "make > check" detects the problem (at least on multicore processors) in the case if > TLS is broken. Does mingw-w64 support OpenMP? "make check" doesn't help for cross-compiles, which was the case that bit me. (In reply to comment #5) > (In reply to comment #4) > > Well, in my opinion the compilers should not normally do that (and it's > > better to report it as a bug to the compiler developers). > > Clearly at least two compilers *do* do that, so configure ought to consider > that case. Yes, two compilers have questionable behaviour, and clang 3.1 even silently ignores __declspec(thread) which is really bad and makes -Werror workaround unusable. Because of various bugs in different systems, the configure.ac keeps accumulating some ugly cruft. > (And from the compilers' perspective, the behavior is probably reasonable.) Well, it's better to report it and let them decide: http://llvm.org/bugs/show_bug.cgi?id=6269#c17 https://sourceforge.net/tracker/?func=detail&aid=3590718&group_id=202880&atid=983354 > > Regarding your timebomb remark, at least we are safe in linux because "make > > check" detects the problem (at least on multicore processors) in the case if > > TLS is broken. Does mingw-w64 support OpenMP? > > "make check" doesn't help for cross-compiles, which was the case that bit me. That's not totally accurate. One can still use "make check" even when crosscompiling with a little help from QEMU and WINE. For the use of QEMU, you can find some instructions here: https://github.com/ssvb/QEMU/wiki/MIPS In the case of OpenMP capable mingw-w64 crosscompiler and WINE, I get the following results for unpatched pixman: PASS: a1-trap-test.exe PASS: pdf-op-test.exe PASS: region-test.exe PASS: region-translate-test.exe PASS: combiner-test.exe PASS: fetch-test.exe rotate test passed (checksum=03A24D51) PASS: rotate-test.exe PASS: oob-test.exe PASS: infinite-loop.exe PASS: trap-crasher.exe PASS: alpha-loop.exe PASS: scaling-crash-test.exe PASS: scaling-helpers-test.exe PASS: gradient-crash-test.exe region_contains test passed (checksum=D2BF8C73) PASS: region-contains-test.exe PASS: alphamap.exe PASS: stress-test.exe composite traps test passed (checksum=33BFAA55) PASS: composite-traps-test.exe blitters test passed (checksum=46136E0A) PASS: blitters-test.exe glyph test passed (checksum=79E74996) PASS: glyph-test.exe scaling test passed (checksum=03A23E0C) PASS: scaling-test.exe affine test passed (checksum=74050F50) PASS: affine-test.exe ---- Test 673729 failed ---- Operator: DISJOINT_DST CA Source: a2b10g10r10, 10x10 Mask: a8, 1x1 Destination: x1r5g5b5, 1x1 R G B A Rounded Source color: 1.000 0.000 0.000 1.000 1.000 0.000 0.000 1.000 Mask color: 0.500 0.000 0.000 0.500 0.502 0.502 0.502 0.502 Dest. color: 0.500 0.000 0.000 0.500 0.516 0.000 0.000 1.000 Expected: 0.516 0.000 0.000 1.000 Got: 0 0 0 0 [pixel: 0x00000000] Min accepted: 16 0 0 0 Max accepted: 16 0 0 0 Test 0x000A47C1 failed. FAIL: composite.exe ============================================= 1 of 23 tests failed Please report to pixman@lists.freedesktop.org ============================================= Out of about 5 runs, all of them failed. The "composite" test is the most reliable for checking this issue. The other tests also occasionally segfault, resulting in about 2-3 failed tests on average. Changing the order of __declspec(thread) and __thread in configure.ac fixes the problem. So that's just another confirmation that pixman test suite is reliable enough to detect when TLS is fubared. It is also quite interesting that tinderbox.x.org is running pixman tests on a cygwin system: http://tinderbox.x.org/builds/2012-10-26-0011/logs/pixman/ The TLS is also detected as __declspec(thread) there, but they explicitly pass --disable-openmp to pixman configure. That's especially strange, considering that the compilation is run with -j4 and they apparently do have more that one CPU core readily available. Sent a patch to the pixman mailing list: http://lists.freedesktop.org/archives/pixman/2012-November/002379.html Yeah, we have a problem with "any compiler that rejects __thread and ignores __declspec(thread)" as noted by Benjamin Gilbert. So any suggestions for a better fix are surely welcome. But this at least seems to improve the situation with the currently known compilers. Rather than reversing the list of directives to test, it would be more conventional (and slightly faster) to break out of the loop after the first successful test. (In reply to comment #8) > Rather than reversing the list of directives to test, it would be more > conventional (and slightly faster) to break out of the loop after the first > successful test. Yes, that's a good point. Feel free to submit a patch with the fixes you think are appropriate. Either replacing my patch or applying on top of it and doing these cleanups. Created attachment 70902 [details] [review] configure.ac patch Since there's no consistent way to detect when a compiler is ignoring __declspec(thread), and since the self-tests should catch thread-safety problems, this patch just corrects the priorities of __thread and __declspec(thread). (In reply to comment #10) > Created attachment 70902 [details] [review] [review] > configure.ac patch > > Since there's no consistent way to detect when a compiler is ignoring > __declspec(thread), and since the self-tests should catch thread-safety > problems, this patch just corrects the priorities of __thread and > __declspec(thread). Looks good to me. Please send it to pixman@lists.freedesktop.org to give the other people a chance to see it, review and test on their systems. Fixed in master and 0.28.2. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
On mingw-w64 (4.7.2), pixman 0.28.0 builds with this warning: warning: 'thread' attribute directive ignored [-Wattributes] and the resulting binary behaves as if PIXMAN_NO_TLS was defined. This is due to two configure.ac bugs introduced by a069da6c: 1. gcc ignores __declspec(thread) but does not throw an error, so configure concludes that it works. 2. configure tests __thread and __declspec(thread) and accepts the *last* working one it finds. On mingw both appear to work, so we get __declspec(thread). Result: configure:13782: checking for thread local storage (TLS) support configure:13810: i686-w64-mingw32-gcc -c -g -O2 -Wall -fno-strict-aliasing conftest.c >&5 configure:13810: $? = 0 configure:13810: i686-w64-mingw32-gcc -c -g -O2 -Wall -fno-strict-aliasing conftest.c >&5 conftest.c:39:1: warning: 'thread' attribute directive ignored [-Wattributes] configure:13810: $? = 0 configure:13818: result: __declspec(thread)