From 7fb75ecbd4226ca3fccdb7e64ce19850059c8c13 Mon Sep 17 00:00:00 2001 From: "andresp@webrtc.org" Date: Fri, 20 Dec 2013 20:20:50 +0000 Subject: [PATCH] Add thread_annotations for clang targets. TESTED: As expected clang bots catched a few issues which are fixed with this CL, other bots ignore the annotations and compile fine. R=niklas.enbom@webrtc.org, phoglund@webrtc.org Review URL: https://webrtc-codereview.appspot.com/6209004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5328 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/build/common.gypi | 5 + .../linux/audio_device_alsa_linux.h | 4 +- .../linux/audio_device_pulse_linux.cc | 2 +- .../linux/audio_device_pulse_linux.h | 8 +- webrtc/modules/pacing/paced_sender.cc | 3 +- .../rtp_rtcp/source/fec_receiver_impl.cc | 1 + .../modules/rtp_rtcp/source/rtcp_receiver.cc | 14 +-- .../interface/critical_section_wrapper.h | 23 ++--- .../interface/rw_lock_wrapper.h | 30 +++--- .../interface/thread_annotations.h | 99 +++++++++++++++++++ .../source/critical_section_unittest.cc | 4 +- .../source/system_wrappers.gyp | 1 + .../testbed/tb_external_transport.cc | 2 - webrtc/video_engine/vie_manager_base.h | 19 ++-- 14 files changed, 157 insertions(+), 58 deletions(-) create mode 100644 webrtc/system_wrappers/interface/thread_annotations.h diff --git a/webrtc/build/common.gypi b/webrtc/build/common.gypi index aab65ea7a5..b4d6aa15ce 100644 --- a/webrtc/build/common.gypi +++ b/webrtc/build/common.gypi @@ -186,6 +186,11 @@ '-Woverloaded-virtual', ], }], + ['clang==1', { + 'cflags': [ + '-Wthread-safety', + ], + }], ], }], ['target_arch=="arm" or target_arch=="armv7"', { diff --git a/webrtc/modules/audio_device/linux/audio_device_alsa_linux.h b/webrtc/modules/audio_device/linux/audio_device_alsa_linux.h index 35abc152fc..4591c293d2 100644 --- a/webrtc/modules/audio_device/linux/audio_device_alsa_linux.h +++ b/webrtc/modules/audio_device/linux/audio_device_alsa_linux.h @@ -174,8 +174,8 @@ private: bool KeyPressed() const; private: - void Lock() { _critSect.Enter(); }; - void UnLock() { _critSect.Leave(); }; + void Lock() EXCLUSIVE_LOCK_FUNCTION(_critSect) { _critSect.Enter(); }; + void UnLock() UNLOCK_FUNCTION(_critSect) { _critSect.Leave(); }; private: inline int32_t InputSanityCheckAfterUnlockedPeriod() const; inline int32_t OutputSanityCheckAfterUnlockedPeriod() const; diff --git a/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc b/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc index e095eed0d5..15c6866ef8 100644 --- a/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc +++ b/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc @@ -2613,7 +2613,7 @@ int32_t AudioDeviceLinuxPulse::ReadRecordedData(const void* bufferData, int32_t AudioDeviceLinuxPulse::ProcessRecordedData( int8_t *bufferData, uint32_t bufferSizeInSamples, - uint32_t recDelay) + uint32_t recDelay) EXCLUSIVE_LOCKS_REQUIRED(_critSect) { uint32_t currentMicLevel(0); uint32_t newMicLevel(0); diff --git a/webrtc/modules/audio_device/linux/audio_device_pulse_linux.h b/webrtc/modules/audio_device/linux/audio_device_pulse_linux.h index 43228a1bab..daf247f4cf 100644 --- a/webrtc/modules/audio_device/linux/audio_device_pulse_linux.h +++ b/webrtc/modules/audio_device/linux/audio_device_pulse_linux.h @@ -224,16 +224,12 @@ public: virtual void AttachAudioBuffer(AudioDeviceBuffer* audioBuffer) OVERRIDE; private: - void Lock() - { + void Lock() EXCLUSIVE_LOCK_FUNCTION(_critSect) { _critSect.Enter(); } - ; - void UnLock() - { + void UnLock() UNLOCK_FUNCTION(_critSect) { _critSect.Leave(); } - ; void WaitForOperationCompletion(pa_operation* paOperation) const; void WaitForSuccess(pa_operation* paOperation) const; diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc index c46bd04ed7..19c108c2bf 100644 --- a/webrtc/modules/pacing/paced_sender.cc +++ b/webrtc/modules/pacing/paced_sender.cc @@ -290,7 +290,8 @@ int32_t PacedSender::Process() { } // MUST have critsect_ when calling. -bool PacedSender::SendPacketFromList(paced_sender::PacketList* packet_list) { +bool PacedSender::SendPacketFromList(paced_sender::PacketList* packet_list) + EXCLUSIVE_LOCKS_REQUIRED(critsect_) { paced_sender::Packet packet = GetNextPacketFromList(packet_list); critsect_->Leave(); diff --git a/webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc b/webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc index 0dc142f867..20be2d5dd3 100644 --- a/webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc @@ -223,6 +223,7 @@ int32_t FecReceiverImpl::ProcessReceivedFec() { crit_sect_->Enter(); } if (fec_->DecodeFEC(&received_packet_list_, &recovered_packet_list_) != 0) { + crit_sect_->Leave(); return -1; } assert(received_packet_list_.empty()); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc index a95fddede2..c9fb49289d 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -480,11 +480,12 @@ RTCPReceiver::HandleSenderReceiverReport(RTCPUtility::RTCPParserV2& rtcpParser, } // no need for critsect we have _criticalSectionRTCPReceiver -void -RTCPReceiver::HandleReportBlock(const RTCPUtility::RTCPPacket& rtcpPacket, - RTCPPacketInformation& rtcpPacketInformation, - const uint32_t remoteSSRC, - const uint8_t numberOfReportBlocks) { +void RTCPReceiver::HandleReportBlock( + const RTCPUtility::RTCPPacket& rtcpPacket, + RTCPPacketInformation& rtcpPacketInformation, + const uint32_t remoteSSRC, + const uint8_t numberOfReportBlocks) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPReceiver) { // This will be called once per report block in the RTCP packet. // We filter out all report blocks that are not for us. // Each packet has max 31 RR blocks. @@ -940,7 +941,8 @@ void RTCPReceiver::HandleXrDlrrReportBlock( void RTCPReceiver::HandleXrDlrrReportBlockItem( const RTCPUtility::RTCPPacket& packet, - RTCPPacketInformation& rtcpPacketInformation) { + RTCPPacketInformation& rtcpPacketInformation) + EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPReceiver) { if (registered_ssrcs_.find(packet.XRDLRRReportBlockItem.SSRC) == registered_ssrcs_.end()) { // Not to us. diff --git a/webrtc/system_wrappers/interface/critical_section_wrapper.h b/webrtc/system_wrappers/interface/critical_section_wrapper.h index 0253a282d1..4979b5c7dd 100644 --- a/webrtc/system_wrappers/interface/critical_section_wrapper.h +++ b/webrtc/system_wrappers/interface/critical_section_wrapper.h @@ -15,9 +15,10 @@ // read/write locks instead. #include "webrtc/common_types.h" +#include "webrtc/system_wrappers/interface/thread_annotations.h" namespace webrtc { -class CriticalSectionWrapper { +class LOCKABLE CriticalSectionWrapper { public: // Factory method, constructor disabled static CriticalSectionWrapper* CreateCriticalSection(); @@ -26,33 +27,25 @@ class CriticalSectionWrapper { // Tries to grab lock, beginning of a critical section. Will wait for the // lock to become available if the grab failed. - virtual void Enter() = 0; + virtual void Enter() EXCLUSIVE_LOCK_FUNCTION() = 0; // Returns a grabbed lock, end of critical section. - virtual void Leave() = 0; + virtual void Leave() UNLOCK_FUNCTION() = 0; }; // RAII extension of the critical section. Prevents Enter/Leave mismatches and // provides more compact critical section syntax. -class CriticalSectionScoped { +class SCOPED_LOCKABLE CriticalSectionScoped { public: explicit CriticalSectionScoped(CriticalSectionWrapper* critsec) - : ptr_crit_sec_(critsec) { + EXCLUSIVE_LOCK_FUNCTION(critsec) + : ptr_crit_sec_(critsec) { ptr_crit_sec_->Enter(); } - ~CriticalSectionScoped() { - if (ptr_crit_sec_) { - Leave(); - } - } + ~CriticalSectionScoped() UNLOCK_FUNCTION() { ptr_crit_sec_->Leave(); } private: - void Leave() { - ptr_crit_sec_->Leave(); - ptr_crit_sec_ = 0; - } - CriticalSectionWrapper* ptr_crit_sec_; }; diff --git a/webrtc/system_wrappers/interface/rw_lock_wrapper.h b/webrtc/system_wrappers/interface/rw_lock_wrapper.h index 80eb5da8a9..91126e5d78 100644 --- a/webrtc/system_wrappers/interface/rw_lock_wrapper.h +++ b/webrtc/system_wrappers/interface/rw_lock_wrapper.h @@ -11,35 +11,36 @@ #ifndef WEBRTC_SYSTEM_WRAPPERS_INTERFACE_RW_LOCK_WRAPPER_H_ #define WEBRTC_SYSTEM_WRAPPERS_INTERFACE_RW_LOCK_WRAPPER_H_ +#include "webrtc/system_wrappers/interface/thread_annotations.h" + // Note, Windows pre-Vista version of RW locks are not supported natively. For // these OSs regular critical sections have been used to approximate RW lock // functionality and will therefore have worse performance. namespace webrtc { -class RWLockWrapper { +class LOCKABLE RWLockWrapper { public: static RWLockWrapper* CreateRWLock(); virtual ~RWLockWrapper() {} - virtual void AcquireLockExclusive() = 0; - virtual void ReleaseLockExclusive() = 0; + virtual void AcquireLockExclusive() EXCLUSIVE_LOCK_FUNCTION() = 0; + virtual void ReleaseLockExclusive() UNLOCK_FUNCTION() = 0; - virtual void AcquireLockShared() = 0; - virtual void ReleaseLockShared() = 0; + virtual void AcquireLockShared() SHARED_LOCK_FUNCTION() = 0; + virtual void ReleaseLockShared() UNLOCK_FUNCTION() = 0; }; // RAII extensions of the RW lock. Prevents Acquire/Release missmatches and // provides more compact locking syntax. -class ReadLockScoped { +class SCOPED_LOCKABLE ReadLockScoped { public: - ReadLockScoped(RWLockWrapper& rw_lock) - : - rw_lock_(rw_lock) { + ReadLockScoped(RWLockWrapper& rw_lock) SHARED_LOCK_FUNCTION(rw_lock) + : rw_lock_(rw_lock) { rw_lock_.AcquireLockShared(); } - ~ReadLockScoped() { + ~ReadLockScoped() UNLOCK_FUNCTION() { rw_lock_.ReleaseLockShared(); } @@ -47,15 +48,14 @@ class ReadLockScoped { RWLockWrapper& rw_lock_; }; -class WriteLockScoped { +class SCOPED_LOCKABLE WriteLockScoped { public: - WriteLockScoped(RWLockWrapper& rw_lock) - : - rw_lock_(rw_lock) { + WriteLockScoped(RWLockWrapper& rw_lock) EXCLUSIVE_LOCK_FUNCTION(rw_lock) + : rw_lock_(rw_lock) { rw_lock_.AcquireLockExclusive(); } - ~WriteLockScoped() { + ~WriteLockScoped() UNLOCK_FUNCTION() { rw_lock_.ReleaseLockExclusive(); } diff --git a/webrtc/system_wrappers/interface/thread_annotations.h b/webrtc/system_wrappers/interface/thread_annotations.h new file mode 100644 index 0000000000..612242d611 --- /dev/null +++ b/webrtc/system_wrappers/interface/thread_annotations.h @@ -0,0 +1,99 @@ +// +// Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. +// +// Use of this source code is governed by a BSD-style license +// that can be found in the LICENSE file in the root of the source +// tree. An additional intellectual property rights grant can be found +// in the file PATENTS. All contributing project authors may +// be found in the AUTHORS file in the root of the source tree. +// +// Borrowed from +// https://code.google.com/p/gperftools/source/browse/src/base/thread_annotations.h +// but adapted for clang attributes instead of the gcc. +// +// This header file contains the macro definitions for thread safety +// annotations that allow the developers to document the locking policies +// of their multi-threaded code. The annotations can also help program +// analysis tools to identify potential thread safety issues. + +#ifndef BASE_THREAD_ANNOTATIONS_H_ +#define BASE_THREAD_ANNOTATIONS_H_ + +#if defined(__clang__) && (!defined(SWIG)) +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x)) +#else +#define THREAD_ANNOTATION_ATTRIBUTE__(x) // no-op +#endif + +// Document if a shared variable/field needs to be protected by a lock. +// GUARDED_BY allows the user to specify a particular lock that should be +// held when accessing the annotated variable, while GUARDED_VAR only +// indicates a shared variable should be guarded (by any lock). GUARDED_VAR +// is primarily used when the client cannot express the name of the lock. +#define GUARDED_BY(x) THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x)) +#define GUARDED_VAR THREAD_ANNOTATION_ATTRIBUTE__(guarded) + +// Document if the memory location pointed to by a pointer should be guarded +// by a lock when dereferencing the pointer. Similar to GUARDED_VAR, +// PT_GUARDED_VAR is primarily used when the client cannot express the name +// of the lock. Note that a pointer variable to a shared memory location +// could itself be a shared variable. For example, if a shared global pointer +// q, which is guarded by mu1, points to a shared memory location that is +// guarded by mu2, q should be annotated as follows: +// int *q GUARDED_BY(mu1) PT_GUARDED_BY(mu2); +#define PT_GUARDED_BY(x) THREAD_ANNOTATION_ATTRIBUTE__(point_to_guarded_by(x)) +#define PT_GUARDED_VAR THREAD_ANNOTATION_ATTRIBUTE__(point_to_guarded) + +// Document the acquisition order between locks that can be held +// simultaneously by a thread. For any two locks that need to be annotated +// to establish an acquisition order, only one of them needs the annotation. +// (i.e. You don't have to annotate both locks with both ACQUIRED_AFTER +// and ACQUIRED_BEFORE.) +#define ACQUIRED_AFTER(x) THREAD_ANNOTATION_ATTRIBUTE__(acquired_after(x)) +#define ACQUIRED_BEFORE(x) THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(x)) + +// The following three annotations document the lock requirements for +// functions/methods. + +// Document if a function expects certain locks to be held before it is called +#define EXCLUSIVE_LOCKS_REQUIRED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_locks_required(__VA_ARGS__)) + +#define SHARED_LOCKS_REQUIRED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(shared_locks_required(__VA_ARGS__)) + +// Document the locks acquired in the body of the function. These locks +// cannot be held when calling this function (as google3's Mutex locks are +// non-reentrant). +#define LOCKS_EXCLUDED(x) THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(x)) + +// Document the lock the annotated function returns without acquiring it. +#define LOCK_RETURNED(x) THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x)) + +// Document if a class/type is a lockable type (such as the Mutex class). +#define LOCKABLE THREAD_ANNOTATION_ATTRIBUTE__(lockable) + +// Document if a class is a scoped lockable type (such as the MutexLock class). +#define SCOPED_LOCKABLE THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable) + +// The following annotations specify lock and unlock primitives. +#define EXCLUSIVE_LOCK_FUNCTION(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__)) + +#define SHARED_LOCK_FUNCTION(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(shared_lock_function(__VA_ARGS__)) + +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(exclusive_trylock_function(__VA_ARGS__)) + +#define SHARED_TRYLOCK_FUNCTION(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(shared_trylock_function(__VA_ARGS__)) + +#define UNLOCK_FUNCTION(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__)) + +// An escape hatch for thread safety analysis to ignore the annotated function. +#define NO_THREAD_SAFETY_ANALYSIS \ + THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis) + +#endif // BASE_THREAD_ANNOTATIONS_H_ diff --git a/webrtc/system_wrappers/source/critical_section_unittest.cc b/webrtc/system_wrappers/source/critical_section_unittest.cc index 40df570e9a..5c416b2de3 100644 --- a/webrtc/system_wrappers/source/critical_section_unittest.cc +++ b/webrtc/system_wrappers/source/critical_section_unittest.cc @@ -81,7 +81,7 @@ bool LockUnlockThenStopRunFunction(void* obj) { return false; } -TEST_F(CritSectTest, ThreadWakesOnce) { +TEST_F(CritSectTest, ThreadWakesOnce) NO_THREAD_SAFETY_ANALYSIS { CriticalSectionWrapper* crit_sect = CriticalSectionWrapper::CreateCriticalSection(); ProtectedCount count(crit_sect); @@ -110,7 +110,7 @@ bool LockUnlockRunFunction(void* obj) { return true; } -TEST_F(CritSectTest, ThreadWakesTwice) { +TEST_F(CritSectTest, ThreadWakesTwice) NO_THREAD_SAFETY_ANALYSIS { CriticalSectionWrapper* crit_sect = CriticalSectionWrapper::CreateCriticalSection(); ProtectedCount count(crit_sect); diff --git a/webrtc/system_wrappers/source/system_wrappers.gyp b/webrtc/system_wrappers/source/system_wrappers.gyp index 41a736e14b..07806d1ec2 100644 --- a/webrtc/system_wrappers/source/system_wrappers.gyp +++ b/webrtc/system_wrappers/source/system_wrappers.gyp @@ -48,6 +48,7 @@ '../interface/sort.h', '../interface/static_instance.h', '../interface/stringize_macros.h', + '../interface/thread_annotations.h', '../interface/thread_wrapper.h', '../interface/tick_util.h', '../interface/trace.h', diff --git a/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc b/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc index 566a01a063..7ccedc2f10 100644 --- a/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc +++ b/webrtc/video_engine/test/libvietest/testbed/tb_external_transport.cc @@ -404,7 +404,6 @@ bool TbExternalTransport::ViEExternalTransportProcess() { waitTime = (unsigned int) timeToReceive; } - _crit.Leave(); break; } _rtpPackets.pop_front(); @@ -488,7 +487,6 @@ bool TbExternalTransport::ViEExternalTransportProcess() { waitTime = (unsigned int) timeToReceive; } - _crit.Leave(); break; } _rtcpPackets.pop_front(); diff --git a/webrtc/video_engine/vie_manager_base.h b/webrtc/video_engine/vie_manager_base.h index 088a2b8a31..f9f008f321 100644 --- a/webrtc/video_engine/vie_manager_base.h +++ b/webrtc/video_engine/vie_manager_base.h @@ -11,11 +11,13 @@ #ifndef WEBRTC_VIDEO_ENGINE_VIE_MANAGER_BASE_H_ #define WEBRTC_VIDEO_ENGINE_VIE_MANAGER_BASE_H_ +#include "webrtc/system_wrappers/interface/thread_annotations.h" + namespace webrtc { class RWLockWrapper; -class ViEManagerBase { +class LOCKABLE ViEManagerBase { friend class ViEManagedItemScopedBase; friend class ViEManagerScopedBase; friend class ViEManagerWriteScoped; @@ -25,24 +27,25 @@ class ViEManagerBase { private: // Exclusive lock, used by ViEManagerWriteScoped. - void WriteLockManager(); + void WriteLockManager() EXCLUSIVE_LOCK_FUNCTION(); // Releases exclusive lock, used by ViEManagerWriteScoped. - void ReleaseWriteLockManager(); + void ReleaseWriteLockManager() UNLOCK_FUNCTION(); // Increases lock count, used by ViEManagerScopedBase. - void ReadLockManager() const; + void ReadLockManager() const SHARED_LOCK_FUNCTION(); // Releases the lock count, used by ViEManagerScopedBase. - void ReleaseLockManager() const; + void ReleaseLockManager() const UNLOCK_FUNCTION(); RWLockWrapper& instance_rwlock_; }; -class ViEManagerWriteScoped { +class SCOPED_LOCKABLE ViEManagerWriteScoped { public: - explicit ViEManagerWriteScoped(ViEManagerBase* vie_manager); - ~ViEManagerWriteScoped(); + explicit ViEManagerWriteScoped(ViEManagerBase* vie_manager) + EXCLUSIVE_LOCK_FUNCTION(vie_manager); + ~ViEManagerWriteScoped() UNLOCK_FUNCTION(); private: ViEManagerBase* vie_manager_;