Closed
Bug 1330240
Opened 8 years ago
Closed 8 years ago
SpiderMonkey fails to build with -Werror=thread-safety on FreeBSD
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 fixed, firefox54 fixed)
RESOLVED
FIXED
mozilla54
People
(Reporter: jbeich, Assigned: jbeich)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Bug 1233297 added -Wthread-safety which turns into an error via MOZ_AUTOMATION or if enabled explicitly. The platform in question is currently NPOTB aka Tier3, so -Werror is disabled downstream.
$ echo ac_add_options --enable-warnings-as-errors >>.mozconfig
$ ./mach build
[...]
js/src/threading/posix/ConditionVariable.cpp:118:11: error: calling function 'pthread_cond_wait' requires holding mutex 'ptMutex' exclusively [-Werror,-Wthread-safety-analysis]
int r = pthread_cond_wait(ptCond, ptMutex);
^
js/src/threading/posix/ConditionVariable.cpp:160:7: error: calling function 'pthread_cond_timedwait' requires holding mutex 'ptMutex' exclusively [-Werror,-Wthread-safety-analysis]
r = pthread_cond_timedwait(ptCond, ptMutex, &abs_ts);
^
2 errors generated.
js/src/threading/posix/MutexImpl.cpp:72:1: error: mutex 'platformData().ptMutex'
is still held at the end of function [-Werror,-Wthread-safety-analysis]
}
^
js/src/threading/posix/MutexImpl.cpp:70:21: note: mutex acquired here
TRY_CALL_PTHREADS(pthread_mutex_lock(&platformData()->ptMutex),
^
js/src/threading/posix/MutexImpl.cpp:77:21: error: releasing mutex
'platformData().ptMutex' that was not held [-Werror,-Wthread-safety-analysis]
TRY_CALL_PTHREADS(pthread_mutex_unlock(&platformData()->ptMutex),
^
2 errors generated.
example full log: http://beefy10.nyi.freebsd.org/data/110i386-default/431044/logs/firefox-50.1.0_5,1.log (grep for ConditionVariable.cpp or Mutex.cpp)
Comment 1•8 years ago
|
||
It looks like -Wthread-safety really wants things that use mutexes and whatnot to be annotated. We should either add all the annotations or turn off the warning.
To give some context, FreeBSD annotated <pthread.h> in 11.0[1] which is the reason for comment 0. The test case can be simplified to the example below. According to "./mach warnings-summary" there're 585 -Wthread-safety-analysis warnings (-Wsign-compare has only 178). And a Tier1 platform may have more in SPS profiler, Crash Reporter, mozjemalloc, etc. But I've only found bug 1136004 fixing such warnings.
$ cat >a.cc
#include <pthread.h>
pthread_mutex_t ptMutex;
void foo(void)
{
pthread_mutex_lock(&ptMutex);
}
# Debian Sid
$ clang-3.8 -Werror=thread-safety -c a.c
# FreeBSD 11.0+
$ clang38 -Werror=thread-safety -c a.c
a.c:8:1: error: mutex 'ptMutex' is still held at the end of function
[-Werror,-Wthread-safety-analysis]
}
^
a.c:7:3: note: mutex acquired here
pthread_mutex_lock(&ptMutex);
^
1 error generated.
[1] https://github.com/freebsd/freebsd/commit/72b4dec586db
https://github.com/freebsd/freebsd/commit/e774df25e598
https://github.com/freebsd/freebsd/commit/b51450dbbdf2
https://github.com/freebsd/freebsd/commit/2a766477ec81
Comment hidden (mozreview-request) |
7 new since comment 2 in vanilla build (i.e. no .mozconfig).
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://reviewboard.mozilla.org/r/103958/#review104666
LGTM
Attachment #8825885 -
Flags: review?(cpeterson) → review+
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://reviewboard.mozilla.org/r/103958/#review104750
WFM. WebRTC uses these extensively, but I trust the upstream developers to compile with the appropriate warnings, and Mozilla WebRTC folks can compile with the appropriate flags if need be.
Attachment #8825885 -
Flags: review?(nfroyd) → review+
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://reviewboard.mozilla.org/r/103958/#review104750
WebRTC not only has [existing annotations](https://chromium.googlesource.com/external/webrtc/+/7fb75ecbd422%5E!/) but Gecko has extensive changes against it. Losing `-Wthread-safety` wouldn't be nice but given media/webrtc/ has `ALLOW_COMPILER_WARNINGS` it can be retained via `-Wno-error=thread-safety` with minimal impact on the current behavior.
Comment hidden (mozreview-request) |
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
I don't know how to turn r+ back into r? via mozreview.
Attachment #8825885 -
Flags: review+ → review?(nfroyd)
Comment 10•8 years ago
|
||
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
WFM.
Attachment #8825885 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Thanks. Once Gecko is properly annotated this can be backed out.
Keywords: checkin-needed
Keywords: checkin-needed
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://reviewboard.mozilla.org/r/103958/#review104922
::: build/moz.configure/warnings.configure:83
(Diff revision 2)
> # catches string literals used in boolean expressions
> check_and_add_gcc_warning('-Wstring-conversion')
>
> # catches inconsistent use of mutexes
> -check_and_add_gcc_warning('-Wthread-safety')
> +# XXX many false positives with annotated <pthread.h>
> +check_and_add_gcc_warning('-Wno-error=thread-safety')
Ugh, `-Wno-error=foo` doesn't enable `-Wfoo`.
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://reviewboard.mozilla.org/r/103958/#review104922
> Ugh, `-Wno-error=foo` doesn't enable `-Wfoo`.
This generates too much noise while hiding WebRTC issues. I'll go back to the previous version and re-enable the warning via GYP.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://reviewboard.mozilla.org/r/103958/#review104922
This version limits false-positives on FreeBSD to `media/webrtc/trunk/webrtc/system_wrappers/` but requires bug 1336329 fix for `'clang==1'` handling. Tested by temporarily turning the warning into an error then removing an existing locking.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
Alternative approach would be to add nop annotations like https://reviews.llvm.org/D28520
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://reviewboard.mozilla.org/r/103958/#review110554
Attachment #8825885 -
Flags: review?(rjesup) → review+
Keywords: checkin-needed
Comment 19•8 years ago
|
||
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20a81b2adf80
Limit -Wthread-safety to WebRTC due to lack of annotations. r=cpeterson,froydnj,jesup
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I had to back this out in https://hg.mozilla.org/mozilla-central/rev/7d50c8e3086d4cd1f05895b3dceac091f8825f1a under suspicion of turning android mda1 tests (nearly) permafail:
https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=d26eb1db5b1c4b9cec29a75c3d2e893dbd3b3e78&filter-searchStr=android%20(mda1
Status: RESOLVED → REOPENED
Flags: needinfo?(jbeich)
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
Assignee | ||
Comment 22•8 years ago
|
||
-Wthread-safety is supported only by Clang platforms (OS X, Linux ASAN) and should have no effect on the produced binaries. The patch just moves the flag from being global to media/webrtc/trunk.
Should be nop on Android as it uses GCC.
Flags: needinfo?(jbeich)
Assignee | ||
Comment 23•8 years ago
|
||
Android mda* tests are green with the patch applied to a m-c from a few days ago.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8dd44d566ec97feaa03205ac02f697de1ec1ca2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd7d310ffdd3056f333be3c44afd608f644137ea
Comment 24•8 years ago
|
||
And mda1 seems to still be orange after the backout, so this wasn't the cause
Assignee | ||
Comment 25•8 years ago
|
||
Hmm, m-c is open despite mda1 orange. Let's retry via autoland then.
Keywords: checkin-needed
Updated•8 years ago
|
Assignee: nobody → jbeich
Comment 26•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc38f7129d38
Limit -Wthread-safety to WebRTC due to lack of annotations. r=cpeterson,froydnj,jesup
Keywords: checkin-needed
Comment 27•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•8 years ago
|
Updated•8 years ago
|
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f109f346f539fa3aff43a1b6a224dca7c1573353
Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: --enable-warnings-as-errors fails to build on FreeBSD 11 or later
[Is this code covered by automated tests?]: Yes, MOZ_AUTOMATION implies --enable-warnings-as-errors. -Wthread-safety is only build-tested by Linux ASan, OS X jobs.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: bug 1336329 for clang check in GYP files
[Is the change risky?]: No
[Why is the change risky/not risky?]: Can only break build due to unrecognized -W* flags.
[String changes made/needed]: None
Attachment #8825885 -
Flags: approval-mozilla-beta?
Comment 29•8 years ago
|
||
Comment on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.
Fix a build issue on FreeBSD 11 or later. Beta53+.
Attachment #8825885 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 30•8 years ago
|
||
bugherder uplift |
Blocks: buildwarning
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•