Index: clang/lib/Analysis/ThreadSafety.cpp =================================================================== --- clang/lib/Analysis/ThreadSafety.cpp +++ clang/lib/Analysis/ThreadSafety.cpp @@ -141,7 +141,8 @@ StringRef DiagKind) const = 0; virtual void handleUnlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, SourceLocation UnlockLoc, - bool FullyRemove, ThreadSafetyHandler &Handler, + bool FullyRemove, LockKind ReceivedKind, + ThreadSafetyHandler &Handler, StringRef DiagKind) const = 0; // Return true if LKind >= LK, where exclusive > shared @@ -878,12 +879,13 @@ void handleUnlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, SourceLocation UnlockLoc, - bool FullyRemove, ThreadSafetyHandler &Handler, + bool FullyRemove, LockKind ReceivedKind, + ThreadSafetyHandler &Handler, StringRef DiagKind) const override { FSet.removeLock(FactMan, Cp); if (!Cp.negative()) { FSet.addLock(FactMan, llvm::make_unique( - !Cp, LK_Exclusive, UnlockLoc)); + !Cp, ReceivedKind, UnlockLoc)); } } }; @@ -949,13 +951,15 @@ lock(FSet, FactMan, UnderCp, entry.kind(), entry.loc(), &Handler, DiagKind); else - unlock(FSet, FactMan, UnderCp, entry.loc(), &Handler, DiagKind); + unlock(FSet, FactMan, UnderCp, entry.loc(), entry.kind(), &Handler, + DiagKind); } } void handleUnlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, SourceLocation UnlockLoc, - bool FullyRemove, ThreadSafetyHandler &Handler, + bool FullyRemove, LockKind ReceivedKind, + ThreadSafetyHandler &Handler, StringRef DiagKind) const override { assert(!Cp.negative() && "Managing object cannot be negative."); for (const auto &UnderlyingMutex : UnderlyingMutexes) { @@ -965,7 +969,8 @@ // on double unlocking/locking if we're not destroying the scoped object. ThreadSafetyHandler *TSHandler = FullyRemove ? nullptr : &Handler; if (UnderlyingMutex.getInt() == UCK_Acquired) { - unlock(FSet, FactMan, UnderCp, UnlockLoc, TSHandler, DiagKind); + unlock(FSet, FactMan, UnderCp, UnlockLoc, ReceivedKind, TSHandler, + DiagKind); } else { LockKind kind = UnderlyingMutex.getInt() == UCK_ReleasedShared ? LK_Shared @@ -992,12 +997,12 @@ } void unlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, - SourceLocation loc, ThreadSafetyHandler *Handler, - StringRef DiagKind) const { + SourceLocation loc, LockKind kind, + ThreadSafetyHandler *Handler, StringRef DiagKind) const { if (FSet.findLock(FactMan, Cp)) { FSet.removeLock(FactMan, Cp); FSet.addLock(FactMan, llvm::make_unique( - !Cp, LK_Exclusive, loc)); + !Cp, kind, loc)); } else if (Handler) { Handler->handleUnmatchedUnlock(DiagKind, Cp.toString(), loc); } @@ -1339,8 +1344,8 @@ ReceivedKind, LDat->loc(), UnlockLoc); } - LDat->handleUnlock(FSet, FactMan, Cp, UnlockLoc, FullyRemove, Handler, - DiagKind); + LDat->handleUnlock(FSet, FactMan, Cp, UnlockLoc, FullyRemove, ReceivedKind, + Handler, DiagKind); } /// Extract the list of mutexIDs from the attribute on an expression, @@ -2177,11 +2182,20 @@ /// are the same. In the event of a difference, we use the intersection of these /// two locksets at the start of D. /// +/// At a merge point, the same lock in the two sets can be different kinds, +/// exclusive, shared or generic. generic type is incured only by +/// UNLOCK_FUNCTION and RELEASE_GENERIC_CAPABILITY attributes. +/// shared + exclusive = exclusive +/// generic + exclusive = exclusive +/// generic + shared = shared +/// A warning will also be issued for shared + exclusive merge. +/// /// \param FSet1 The first lockset. /// \param FSet2 The second lockset. /// \param JoinLoc The location of the join point for error reporting /// \param LEK1 The error message to report if a mutex is missing from LSet1 /// \param LEK2 The error message to report if a mutex is missing from Lset2 +/// \param Modify whether to modify FSet1 or not. void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &FSet1, const FactSet &FSet2, SourceLocation JoinLoc, @@ -2199,14 +2213,23 @@ if (LDat1) { if (LDat1->kind() != LDat2->kind()) { - Handler.handleExclusiveAndShared("mutex", LDat2->toString(), - LDat2->loc(), LDat1->loc()); - if (Modify && LDat1->kind() != LK_Exclusive) { - // Take the exclusive lock, which is the one in FSet2. - *Iter1 = Fact; + // If one lock is generic, take the type of the other lock, which + // is the lock in FSet2. Otherwise, both locks are not generic. + // We are merging exclusive with shared + if (LDat1->kind() == LK_Generic || LDat2->kind() == LK_Generic) { + // No warning is issued in this case. + if (Modify && LDat1->kind() == LK_Generic) { + *Iter1 = Fact; + } + } else { + Handler.handleExclusiveAndShared("mutex", LDat2->toString(), + LDat2->loc(), LDat1->loc()); + if (Modify && LDat1->kind() != LK_Exclusive) { + // Take the exclusive lock, which is the one in FSet2. + *Iter1 = Fact; + } } - } - else if (Modify && LDat1->asserted() && !LDat2->asserted()) { + } else if (Modify && LDat1->asserted() && !LDat2->asserted()) { // The non-asserted lock in FSet2 is the one we want to track. *Iter1 = Fact; } Index: clang/test/SemaCXX/thread-safety-annotations.h =================================================================== --- clang/test/SemaCXX/thread-safety-annotations.h +++ clang/test/SemaCXX/thread-safety-annotations.h @@ -40,3 +40,55 @@ #define LOCK_RETURNED(x) __attribute__((lock_returned(x))) #define LOCKS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__))) #define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis)) + + +// New terminologies: +// Enable thread safety attributes only with clang. +// The attributes can be safely erased when compiling with other compilers. +#if defined(__clang__) && (!defined(SWIG)) +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x)) +#else +#define THREAD_ANNOTATION_ATTRIBUTE__(x) // no-op +#endif + +#define CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(capability(x)) + +#define SCOPED_CAPABILITY \ + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable) + +#define REQUIRES(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__)) + +#define REQUIRES_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__)) + +#define ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__)) + +#define ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__)) + +#define RELEASE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__)) + +#define RELEASE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__)) + +#define EXCLUDES(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__)) + +#define ASSERT_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x)) + +#define ASSERT_SHARED_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x)) + +#define RETURN_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x)) Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp =================================================================== --- clang/test/SemaCXX/warn-thread-safety-negative.cpp +++ clang/test/SemaCXX/warn-thread-safety-negative.cpp @@ -97,3 +97,56 @@ void test() { TemplateClass TC; } } // end namespace DoubleAttribute + + +// Test new terminology and accurate unlock. +class CAPABILITY("mutex") NewMutex { +public: + void Lock() ACQUIRE(); + void Unlock() RELEASE(); + + void ReaderLock() ACQUIRE_SHARED(); + void ReaderUnlock() RELEASE_SHARED(); + + void GenericUnlock() UNLOCK_FUNCTION(); + + // for negative capabilities + const NewMutex& operator!() const { return *this; } +}; + + +namespace TestMerge { + +NewMutex mu; +bool condition; + +void assertNotHeld() ASSERT_SHARED_CAPABILITY(!mu); + +void test1() { + if (condition) { + assertNotHeld(); + } else { + mu.ReaderLock(); + mu.ReaderUnlock(); + } // merge point no problem. +} + +void test2() { + if (condition) { + assertNotHeld(); // expected-warning {{mutex '!mu' is acquired exclusively and shared in the same scope}} + } else { + mu.Lock(); + mu.Unlock(); // expected-warning {{the other acquisition of mutex '!mu' is here}} + } +} + +void test3() { + if (condition) { + assertNotHeld(); + } else { + mu.Lock(); + mu.GenericUnlock(); + } // merge point no problem. +} + +} // end namespace NewTerminology