Closed Bug 1330240 Opened 7 years ago Closed 7 years ago

SpiderMonkey fails to build with -Werror=thread-safety on FreeBSD

Categories

(Firefox Build System :: General, defect)

Unspecified
FreeBSD
defect
Not set
normal

Tracking

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

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)
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
7 new since comment 2 in vanilla build (i.e. no .mozconfig).
Component: JavaScript Engine → Build Config
Blocks: 1233297
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 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 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 on attachment 8825885 [details]
Bug 1330240 - Limit -Wthread-safety to WebRTC due to lack of annotations.

WFM.
Attachment #8825885 - Flags: review?(nfroyd) → review+
Thanks. Once Gecko is properly annotated this can be backed out.
Keywords: checkin-needed
Keywords: checkin-needed
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`.
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 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.
Alternative approach would be to add nop annotations like https://reviews.llvm.org/D28520
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
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
https://hg.mozilla.org/mozilla-central/rev/20a81b2adf80
Status: NEW → RESOLVED
Closed: 7 years ago
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 → ---
-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)
And mda1 seems to still be orange after the backout, so this wasn't the cause
Hmm, m-c is open despite mda1 orange. Let's retry via autoland then.
Keywords: checkin-needed
Assignee: nobody → jbeich
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
https://hg.mozilla.org/mozilla-central/rev/fc38f7129d38
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1336329
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 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+
Blocks: buildwarning
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.