Closed
Bug 1136004
Opened 9 years ago
Closed 9 years ago
clang -Wthread-safety-analysis warnings in webrtc: writing variable 'red_enabled_' requires locking 'acm_crit_sect_' exclusively
Categories
(Core :: WebRTC: Audio/Video, defect, P4)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: cpeterson, Assigned: jesup)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files)
1.91 KB,
patch
|
cpeterson
:
review+
|
Details | Diff | Splinter Review |
2.21 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
These warnings are from webrtc code that writes to an enum member variable outside a critical section: > media/webrtc/trunk/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:1443:3 [-Wthread-safety-analysis] writing variable 'red_enabled_' requires locking 'acm_crit_sect_' exclusively > media/webrtc/trunk/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:1446:16 [-Wthread-safety-analysis] reading variable 'red_enabled_' requires locking 'acm_crit_sect_' > media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc:698:5 [-Wthread-safety-analysis] writing variable 'loadstate_' requires locking 'crit_sect_' exclusively These warnings might fairly be treated as false positives because they point to Lock() and Unlock() member functions that wrap _critSect. However, the Lock() and Unlock() member functions are never actually called, so they can be removed. > media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:186:5 [-Wthread-safety-analysis] mutex '_critSect' is still locked at the end of function > media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:190:9 [-Wthread-safety-analysis] unlocking '_critSect' that was not locked
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8568379 -
Flags: review?(cpeterson)
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8568379 [details] [diff] [review] fix TSAN in webrtc warning when RED isn't enabled Review of attachment 8568379 [details] [diff] [review]: ----------------------------------------------------------------- LGTM for the warnings in audio_coding_module_impl.cc.
Attachment #8568379 -
Flags: review?(cpeterson) → review+
Assignee | ||
Comment 3•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/60ac868f9918
Reporter | ||
Comment 4•9 years ago
|
||
btw, I thought all code under media/webrtc/trunk/ was a snapshot of the upstream webrtc code that shouldn't be changed in mozilla-central. Is that still true? Are you making the same fix upstream?
Flags: needinfo?(rjesup)
Keywords: leave-open
Assignee | ||
Comment 5•9 years ago
|
||
It is. pkerr was intended to help manage getting things upstreamed, but (partly due to loop/hello, which started the same time he did) that didn't happen. We haven't been great at linking things to bug 864513; however part of what we'll do eventually is view the diffs against upstream (which is easy because of how I land updates), then walk back through the history to find where our change landed, and then use that to craft an upstream issue/patch. Or in some cases, just roll up some set of changes into a new bug.
Updated•9 years ago
|
Flags: firefox-backlog+
Priority: -- → P4
https://hg.mozilla.org/mozilla-central/rev/60ac868f9918
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Reporter | ||
Comment 7•9 years ago
|
||
This patch fixes the remaining -Wthread-safety-analysis warnings reported in comment 0. These are the last three -Wthread-safety-analysis compiler warnings (not TSan run-time warnings) in mozilla-central. AudioDeviceMac's private member functions Lock(), Unlock(), and Id() seem to be unused, so I removed them. > media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc:698:5 [-Wthread-safety-analysis] writing variable 'loadstate_' requires locking 'crit_sect_' exclusively > media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:186:5 [-Wthread-safety-analysis] mutex '_critSect' is still locked at the end of function > media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:190:9 [-Wthread-safety-analysis] unlocking '_critSect' that was not locked
Attachment #8570843 -
Flags: review?(rjesup)
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8570843 [details] [diff] [review] more-Wthread-safety-analysis-warnings.patch Review of attachment 8570843 [details] [diff] [review]: ----------------------------------------------------------------- r+ for media_optimization change r- of the audio_device_mac.h change unless it causes a problem - we avoid making changes like that just because we don't use it. If it's really annoying, then put it in an "#ifndef WEBRTC_MOZILLA_BUILD". If it's truly kruft that upstream should have removed *and* is annoying, then ok, remove it.
Reporter | ||
Comment 9•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #8) > r- of the audio_device_mac.h change unless it causes a problem - we avoid > making changes like that just because we don't use it. If it's really > annoying, then put it in an "#ifndef WEBRTC_MOZILLA_BUILD". If it's truly > kruft that upstream should have removed *and* is annoying, then ok, remove > it. OK. I will submit a patch upstream to remove these functions instead of making more work for you when you next merge upstream. :) These are the only clang -Wthread-safety-analysis warnings in all C++ code in mozilla-central, so it would be nice to fix or suppress them so we have zero -Wthread-safety-analysis warnings. Upstream recently went out of their way to suppress these clang warnings with clang-specific annotations, but they didn't see that these member functions are never actually used: https://code.google.com/p/webrtc/source/detail?r=7961&path=/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h#
Reporter | ||
Comment 10•9 years ago
|
||
I filed an upstream bug about audio_device_mac.h's -Wthread-safety-analysis warnings: https://code.google.com/p/webrtc/issues/detail?id=4362
Reporter | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c964a0be6c59
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8570843 [details] [diff] [review] more-Wthread-safety-analysis-warnings.patch Review of attachment 8570843 [details] [diff] [review]: ----------------------------------------------------------------- r+ on media_optimization per previous review
Attachment #8570843 -
Flags: review?(rjesup) → review+
Updated•6 years ago
|
No longer blocks: webrtc_upstream_bugs
You need to log in
before you can comment on or make changes to this bug.
Description
•