diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h @@ -273,14 +273,23 @@ /// The capability expression and whether it's negated. llvm::PointerIntPair CapExpr; + /// The kind of capability as specified by @ref CapabilityAttr::getName. + StringRef CapKind; + public: - CapabilityExpr(const til::SExpr *E, bool Neg) : CapExpr(E, Neg) {} + CapabilityExpr() : CapExpr(nullptr, false) {} + CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg) + : CapExpr(E, Neg), CapKind(Kind) {} + + // Don't allow implicitly-constructed StringRefs since we'll capture them. + template CapabilityExpr(const til::SExpr *, T, bool) = delete; const til::SExpr *sexpr() const { return CapExpr.getPointer(); } + StringRef getKind() const { return CapKind; } bool negative() const { return CapExpr.getInt(); } CapabilityExpr operator!() const { - return CapabilityExpr(CapExpr.getPointer(), !CapExpr.getInt()); + return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt()); } bool equals(const CapabilityExpr &other) const { diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -139,12 +139,12 @@ SourceLocation JoinLoc, LockErrorKind LEK, ThreadSafetyHandler &Handler) const = 0; virtual void handleLock(FactSet &FSet, FactManager &FactMan, - const FactEntry &entry, ThreadSafetyHandler &Handler, - StringRef DiagKind) const = 0; + const FactEntry &entry, + ThreadSafetyHandler &Handler) const = 0; virtual void handleUnlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, SourceLocation UnlockLoc, - bool FullyRemove, ThreadSafetyHandler &Handler, - StringRef DiagKind) const = 0; + bool FullyRemove, + ThreadSafetyHandler &Handler) const = 0; // Return true if LKind >= LK, where exclusive > shared bool isAtLeast(LockKind LK) const { @@ -865,21 +865,21 @@ SourceLocation JoinLoc, LockErrorKind LEK, ThreadSafetyHandler &Handler) const override { if (!asserted() && !negative() && !isUniversal()) { - Handler.handleMutexHeldEndOfScope("mutex", toString(), loc(), JoinLoc, + Handler.handleMutexHeldEndOfScope(getKind(), toString(), loc(), JoinLoc, LEK); } } void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry, - ThreadSafetyHandler &Handler, - StringRef DiagKind) const override { - Handler.handleDoubleLock(DiagKind, entry.toString(), loc(), entry.loc()); + ThreadSafetyHandler &Handler) const override { + Handler.handleDoubleLock(entry.getKind(), entry.toString(), loc(), + entry.loc()); } void handleUnlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, SourceLocation UnlockLoc, - bool FullyRemove, ThreadSafetyHandler &Handler, - StringRef DiagKind) const override { + bool FullyRemove, + ThreadSafetyHandler &Handler) const override { FSet.removeLock(FactMan, Cp); if (!Cp.negative()) { FSet.addLock(FactMan, std::make_unique( @@ -929,43 +929,40 @@ (UnderlyingMutex.Kind != UCK_Acquired && !Entry)) { // If this scoped lock manages another mutex, and if the underlying // mutex is still/not held, then warn about the underlying mutex. - Handler.handleMutexHeldEndOfScope( - "mutex", UnderlyingMutex.Cap.toString(), loc(), JoinLoc, LEK); + Handler.handleMutexHeldEndOfScope(UnderlyingMutex.Cap.getKind(), + UnderlyingMutex.Cap.toString(), loc(), + JoinLoc, LEK); } } } void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry, - ThreadSafetyHandler &Handler, - StringRef DiagKind) const override { + ThreadSafetyHandler &Handler) const override { for (const auto &UnderlyingMutex : UnderlyingMutexes) { if (UnderlyingMutex.Kind == UCK_Acquired) lock(FSet, FactMan, UnderlyingMutex.Cap, entry.kind(), entry.loc(), - &Handler, DiagKind); + &Handler); else - unlock(FSet, FactMan, UnderlyingMutex.Cap, entry.loc(), &Handler, - DiagKind); + unlock(FSet, FactMan, UnderlyingMutex.Cap, entry.loc(), &Handler); } } void handleUnlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, SourceLocation UnlockLoc, - bool FullyRemove, ThreadSafetyHandler &Handler, - StringRef DiagKind) const override { + bool FullyRemove, + ThreadSafetyHandler &Handler) const override { assert(!Cp.negative() && "Managing object cannot be negative."); for (const auto &UnderlyingMutex : UnderlyingMutexes) { // Remove/lock the underlying mutex if it exists/is still unlocked; warn // on double unlocking/locking if we're not destroying the scoped object. ThreadSafetyHandler *TSHandler = FullyRemove ? nullptr : &Handler; if (UnderlyingMutex.Kind == UCK_Acquired) { - unlock(FSet, FactMan, UnderlyingMutex.Cap, UnlockLoc, TSHandler, - DiagKind); + unlock(FSet, FactMan, UnderlyingMutex.Cap, UnlockLoc, TSHandler); } else { LockKind kind = UnderlyingMutex.Kind == UCK_ReleasedShared ? LK_Shared : LK_Exclusive; - lock(FSet, FactMan, UnderlyingMutex.Cap, kind, UnlockLoc, TSHandler, - DiagKind); + lock(FSet, FactMan, UnderlyingMutex.Cap, kind, UnlockLoc, TSHandler); } } if (FullyRemove) @@ -974,11 +971,12 @@ private: void lock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, - LockKind kind, SourceLocation loc, ThreadSafetyHandler *Handler, - StringRef DiagKind) const { + LockKind kind, SourceLocation loc, + ThreadSafetyHandler *Handler) const { if (const FactEntry *Fact = FSet.findLock(FactMan, Cp)) { if (Handler) - Handler->handleDoubleLock(DiagKind, Cp.toString(), Fact->loc(), loc); + Handler->handleDoubleLock(Cp.getKind(), Cp.toString(), Fact->loc(), + loc); } else { FSet.removeLock(FactMan, !Cp); FSet.addLock(FactMan, @@ -987,8 +985,7 @@ } void unlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp, - SourceLocation loc, ThreadSafetyHandler *Handler, - StringRef DiagKind) const { + SourceLocation loc, ThreadSafetyHandler *Handler) const { if (FSet.findLock(FactMan, Cp)) { FSet.removeLock(FactMan, Cp); FSet.addLock(FactMan, std::make_unique( @@ -997,7 +994,7 @@ SourceLocation PrevLoc; if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp)) PrevLoc = Neg->loc(); - Handler->handleUnmatchedUnlock(DiagKind, Cp.toString(), loc, PrevLoc); + Handler->handleUnmatchedUnlock(Cp.getKind(), Cp.toString(), loc, PrevLoc); } } }; @@ -1026,10 +1023,9 @@ bool inCurrentScope(const CapabilityExpr &CapE); void addLock(FactSet &FSet, std::unique_ptr Entry, - StringRef DiagKind, bool ReqAttr = false); + bool ReqAttr = false); void removeLock(FactSet &FSet, const CapabilityExpr &CapE, - SourceLocation UnlockLoc, bool FullyRemove, LockKind Kind, - StringRef DiagKind); + SourceLocation UnlockLoc, bool FullyRemove, LockKind Kind); template void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, @@ -1217,53 +1213,6 @@ } // namespace -static StringRef ClassifyDiagnostic(const CapabilityAttr *A) { - return A->getName(); -} - -static StringRef ClassifyDiagnostic(QualType VDT) { - // We need to look at the declaration of the type of the value to determine - // which it is. The type should either be a record or a typedef, or a pointer - // or reference thereof. - if (const auto *RT = VDT->getAs()) { - if (const auto *RD = RT->getDecl()) - if (const auto *CA = RD->getAttr()) - return ClassifyDiagnostic(CA); - } else if (const auto *TT = VDT->getAs()) { - if (const auto *TD = TT->getDecl()) - if (const auto *CA = TD->getAttr()) - return ClassifyDiagnostic(CA); - } else if (VDT->isPointerType() || VDT->isReferenceType()) - return ClassifyDiagnostic(VDT->getPointeeType()); - - return "mutex"; -} - -static StringRef ClassifyDiagnostic(const ValueDecl *VD) { - assert(VD && "No ValueDecl passed"); - - // The ValueDecl is the declaration of a mutex or role (hopefully). - return ClassifyDiagnostic(VD->getType()); -} - -template -static std::enable_if_t::value, StringRef> -ClassifyDiagnostic(const AttrTy *A) { - if (const ValueDecl *VD = getValueDecl(A->getArg())) - return ClassifyDiagnostic(VD); - return "mutex"; -} - -template -static std::enable_if_t::value, StringRef> -ClassifyDiagnostic(const AttrTy *A) { - for (const auto *Arg : A->args()) { - if (const ValueDecl *VD = getValueDecl(Arg)) - return ClassifyDiagnostic(VD); - } - return "mutex"; -} - bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) { const threadSafety::til::SExpr *SExp = CapE.sexpr(); assert(SExp && "Null expressions should be ignored"); @@ -1295,7 +1244,7 @@ /// \param ReqAttr -- true if this is part of an initial Requires attribute. void ThreadSafetyAnalyzer::addLock(FactSet &FSet, std::unique_ptr Entry, - StringRef DiagKind, bool ReqAttr) { + bool ReqAttr) { if (Entry->shouldIgnore()) return; @@ -1308,7 +1257,7 @@ } else { if (inCurrentScope(*Entry) && !Entry->asserted()) - Handler.handleNegativeNotHeld(DiagKind, Entry->toString(), + Handler.handleNegativeNotHeld(Entry->getKind(), Entry->toString(), NegC.toString(), Entry->loc()); } } @@ -1317,13 +1266,13 @@ if (Handler.issueBetaWarnings() && !Entry->asserted() && !Entry->declared()) { GlobalBeforeSet->checkBeforeAfter(Entry->valueDecl(), FSet, *this, - Entry->loc(), DiagKind); + Entry->loc(), Entry->getKind()); } // FIXME: Don't always warn when we have support for reentrant locks. if (const FactEntry *Cp = FSet.findLock(FactMan, *Entry)) { if (!Entry->asserted()) - Cp->handleLock(FSet, FactMan, *Entry, Handler, DiagKind); + Cp->handleLock(FSet, FactMan, *Entry, Handler); } else { FSet.addLock(FactMan, std::move(Entry)); } @@ -1333,8 +1282,7 @@ /// \param UnlockLoc The source location of the unlock (only used in error msg) void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp, SourceLocation UnlockLoc, - bool FullyRemove, LockKind ReceivedKind, - StringRef DiagKind) { + bool FullyRemove, LockKind ReceivedKind) { if (Cp.shouldIgnore()) return; @@ -1343,19 +1291,19 @@ SourceLocation PrevLoc; if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp)) PrevLoc = Neg->loc(); - Handler.handleUnmatchedUnlock(DiagKind, Cp.toString(), UnlockLoc, PrevLoc); + Handler.handleUnmatchedUnlock(Cp.getKind(), Cp.toString(), UnlockLoc, + PrevLoc); return; } // Generic lock removal doesn't care about lock kind mismatches, but // otherwise diagnose when the lock kinds are mismatched. if (ReceivedKind != LK_Generic && LDat->kind() != ReceivedKind) { - Handler.handleIncorrectUnlockKind(DiagKind, Cp.toString(), LDat->kind(), + Handler.handleIncorrectUnlockKind(Cp.getKind(), Cp.toString(), LDat->kind(), ReceivedKind, LDat->loc(), UnlockLoc); } - LDat->handleUnlock(FSet, FactMan, Cp, UnlockLoc, FullyRemove, Handler, - DiagKind); + LDat->handleUnlock(FSet, FactMan, Cp, UnlockLoc, FullyRemove, Handler); } /// Extract the list of mutexIDs from the attribute on an expression, @@ -1368,8 +1316,8 @@ // The mutex held is the "this" object. CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, SelfDecl); if (Cp.isInvalid()) { - warnInvalidLock(Handler, nullptr, D, Exp, ClassifyDiagnostic(Attr)); - return; + warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind()); + return; } //else if (!Cp.shouldIgnore()) @@ -1380,8 +1328,8 @@ for (const auto *Arg : Attr->args()) { CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, SelfDecl); if (Cp.isInvalid()) { - warnInvalidLock(Handler, nullptr, D, Exp, ClassifyDiagnostic(Attr)); - continue; + warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind()); + continue; } //else if (!Cp.shouldIgnore()) @@ -1520,7 +1468,6 @@ bool Negate = false; const CFGBlockInfo *PredBlockInfo = &BlockInfo[PredBlock->getBlockID()]; const LocalVarContext &LVarCtx = PredBlockInfo->ExitContext; - StringRef CapDiagKind = "mutex"; const auto *Exp = getTrylockCallExpr(Cond, LVarCtx, Negate); if (!Exp) @@ -1541,21 +1488,18 @@ getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, A->getSuccessValue(), Negate); - CapDiagKind = ClassifyDiagnostic(A); break; }; case attr::ExclusiveTrylockFunction: { const auto *A = cast(Attr); - getMutexIDs(ExclusiveLocksToAdd, A, Exp, FunDecl, - PredBlock, CurrBlock, A->getSuccessValue(), Negate); - CapDiagKind = ClassifyDiagnostic(A); + getMutexIDs(ExclusiveLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, + A->getSuccessValue(), Negate); break; } case attr::SharedTrylockFunction: { const auto *A = cast(Attr); - getMutexIDs(SharedLocksToAdd, A, Exp, FunDecl, - PredBlock, CurrBlock, A->getSuccessValue(), Negate); - CapDiagKind = ClassifyDiagnostic(A); + getMutexIDs(SharedLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, + A->getSuccessValue(), Negate); break; } default: @@ -1567,12 +1511,10 @@ SourceLocation Loc = Exp->getExprLoc(); for (const auto &ExclusiveLockToAdd : ExclusiveLocksToAdd) addLock(Result, std::make_unique(ExclusiveLockToAdd, - LK_Exclusive, Loc), - CapDiagKind); + LK_Exclusive, Loc)); for (const auto &SharedLockToAdd : SharedLocksToAdd) addLock(Result, std::make_unique(SharedLockToAdd, - LK_Shared, Loc), - CapDiagKind); + LK_Shared, Loc)); } namespace { @@ -1593,9 +1535,8 @@ // helper functions void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK, Expr *MutexExp, ProtectedOperationKind POK, - StringRef DiagKind, SourceLocation Loc); - void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp, - StringRef DiagKind); + SourceLocation Loc); + void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp); void checkAccess(const Expr *Exp, AccessKind AK, ProtectedOperationKind POK = POK_VarAccess); @@ -1628,12 +1569,12 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK, Expr *MutexExp, ProtectedOperationKind POK, - StringRef DiagKind, SourceLocation Loc) { + SourceLocation Loc) { LockKind LK = getLockKindFromAccessKind(AK); CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp); if (Cp.isInvalid()) { - warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, DiagKind); + warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind()); return; } else if (Cp.shouldIgnore()) { return; @@ -1644,7 +1585,7 @@ const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, !Cp); if (LDat) { Analyzer->Handler.handleFunExcludesLock( - DiagKind, D->getNameAsString(), (!Cp).toString(), Loc); + Cp.getKind(), D->getNameAsString(), (!Cp).toString(), Loc); return; } @@ -1670,28 +1611,28 @@ // Warn that there's no precise match. std::string PartMatchStr = LDat->toString(); StringRef PartMatchName(PartMatchStr); - Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK, Cp.toString(), + Analyzer->Handler.handleMutexNotHeld(Cp.getKind(), D, POK, Cp.toString(), LK, Loc, &PartMatchName); } else { // Warn that there's no match at all. - Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK, Cp.toString(), + Analyzer->Handler.handleMutexNotHeld(Cp.getKind(), D, POK, Cp.toString(), LK, Loc); } NoError = false; } // Make sure the mutex we found is the right kind. if (NoError && LDat && !LDat->isAtLeast(LK)) { - Analyzer->Handler.handleMutexNotHeld(DiagKind, D, POK, Cp.toString(), + Analyzer->Handler.handleMutexNotHeld(Cp.getKind(), D, POK, Cp.toString(), LK, Loc); } } /// Warn if the LSet contains the given lock. void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, - Expr *MutexExp, StringRef DiagKind) { + Expr *MutexExp) { CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp); if (Cp.isInvalid()) { - warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, DiagKind); + warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind()); return; } else if (Cp.shouldIgnore()) { return; @@ -1699,8 +1640,8 @@ const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, Cp); if (LDat) { - Analyzer->Handler.handleFunExcludesLock( - DiagKind, D->getNameAsString(), Cp.toString(), Exp->getExprLoc()); + Analyzer->Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(), + Cp.toString(), Exp->getExprLoc()); } } @@ -1759,8 +1700,7 @@ } for (const auto *I : D->specific_attrs()) - warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, - ClassifyDiagnostic(I), Loc); + warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, Loc); } /// Checks pt_guarded_by and pt_guarded_var attributes. @@ -1798,8 +1738,7 @@ Exp->getExprLoc()); for (auto const *I : D->specific_attrs()) - warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, - ClassifyDiagnostic(I), Exp->getExprLoc()); + warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, Exp->getExprLoc()); } /// Process a function call, method call, constructor call, @@ -1818,7 +1757,6 @@ CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd; CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove; CapExprSet ScopedReqsAndExcludes; - StringRef CapDiagKind = "mutex"; // Figure out if we're constructing an object of scoped lockable class bool isScopedVar = false; @@ -1839,8 +1777,6 @@ Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A, Exp, D, VD); - - CapDiagKind = ClassifyDiagnostic(A); break; } @@ -1854,10 +1790,8 @@ Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD); for (const auto &AssertLock : AssertLocks) Analyzer->addLock( - FSet, - std::make_unique(AssertLock, LK_Exclusive, Loc, - FactEntry::Asserted), - ClassifyDiagnostic(A)); + FSet, std::make_unique( + AssertLock, LK_Exclusive, Loc, FactEntry::Asserted)); break; } case attr::AssertSharedLock: { @@ -1867,10 +1801,8 @@ Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD); for (const auto &AssertLock : AssertLocks) Analyzer->addLock( - FSet, - std::make_unique(AssertLock, LK_Shared, Loc, - FactEntry::Asserted), - ClassifyDiagnostic(A)); + FSet, std::make_unique( + AssertLock, LK_Shared, Loc, FactEntry::Asserted)); break; } @@ -1879,12 +1811,10 @@ CapExprSet AssertLocks; Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD); for (const auto &AssertLock : AssertLocks) - Analyzer->addLock(FSet, - std::make_unique( - AssertLock, - A->isShared() ? LK_Shared : LK_Exclusive, Loc, - FactEntry::Asserted), - ClassifyDiagnostic(A)); + Analyzer->addLock(FSet, std::make_unique( + AssertLock, + A->isShared() ? LK_Shared : LK_Exclusive, + Loc, FactEntry::Asserted)); break; } @@ -1898,8 +1828,6 @@ Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, VD); else Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, VD); - - CapDiagKind = ClassifyDiagnostic(A); break; } @@ -1907,8 +1835,7 @@ const auto *A = cast(At); for (auto *Arg : A->args()) { warnIfMutexNotHeld(D, Exp, A->isShared() ? AK_Read : AK_Written, Arg, - POK_FunctionCall, ClassifyDiagnostic(A), - Exp->getExprLoc()); + POK_FunctionCall, Exp->getExprLoc()); // use for adopting a lock if (isScopedVar) Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD); @@ -1919,7 +1846,7 @@ case attr::LocksExcluded: { const auto *A = cast(At); for (auto *Arg : A->args()) { - warnIfMutexHeld(D, Exp, Arg, ClassifyDiagnostic(A)); + warnIfMutexHeld(D, Exp, Arg); // use for deferring a lock if (isScopedVar) Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD); @@ -1937,23 +1864,21 @@ // FIXME -- should only fully remove if the attribute refers to 'this'. bool Dtor = isa(D); for (const auto &M : ExclusiveLocksToRemove) - Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Exclusive, CapDiagKind); + Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Exclusive); for (const auto &M : SharedLocksToRemove) - Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Shared, CapDiagKind); + Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Shared); for (const auto &M : GenericLocksToRemove) - Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic, CapDiagKind); + Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic); // Add locks. FactEntry::SourceKind Source = isScopedVar ? FactEntry::Managed : FactEntry::Acquired; for (const auto &M : ExclusiveLocksToAdd) - Analyzer->addLock( - FSet, std::make_unique(M, LK_Exclusive, Loc, Source), - CapDiagKind); + Analyzer->addLock(FSet, std::make_unique(M, LK_Exclusive, + Loc, Source)); for (const auto &M : SharedLocksToAdd) Analyzer->addLock( - FSet, std::make_unique(M, LK_Shared, Loc, Source), - CapDiagKind); + FSet, std::make_unique(M, LK_Shared, Loc, Source)); if (isScopedVar) { // Add the managing object as a dummy mutex, mapped to the underlying mutex. @@ -1974,7 +1899,7 @@ ScopedEntry->addExclusiveUnlock(M); for (const auto &M : SharedLocksToRemove) ScopedEntry->addSharedUnlock(M); - Analyzer->addLock(FSet, std::move(ScopedEntry), CapDiagKind); + Analyzer->addLock(FSet, std::move(ScopedEntry)); } } @@ -2202,7 +2127,8 @@ if (CanModify || !ShouldTakeB) return ShouldTakeB; } - Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc()); + Handler.handleExclusiveAndShared(B.getKind(), B.toString(), B.loc(), + A.loc()); // Take the exclusive capability to reduce further warnings. return CanModify && B.kind() == LK_Exclusive; } else { @@ -2342,7 +2268,6 @@ CapExprSet ExclusiveLocksToAdd; CapExprSet SharedLocksToAdd; - StringRef CapDiagKind = "mutex"; SourceLocation Loc = D->getLocation(); for (const auto *Attr : D->attrs()) { @@ -2350,7 +2275,6 @@ if (const auto *A = dyn_cast(Attr)) { getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A, nullptr, D); - CapDiagKind = ClassifyDiagnostic(A); } else if (const auto *A = dyn_cast(Attr)) { // UNLOCK_FUNCTION() is used to hide the underlying lock implementation. // We must ignore such methods. @@ -2359,14 +2283,12 @@ getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A, nullptr, D); getMutexIDs(LocksReleased, A, nullptr, D); - CapDiagKind = ClassifyDiagnostic(A); } else if (const auto *A = dyn_cast(Attr)) { if (A->args_size() == 0) return; getMutexIDs(A->isShared() ? SharedLocksAcquired : ExclusiveLocksAcquired, A, nullptr, D); - CapDiagKind = ClassifyDiagnostic(A); } else if (isa(Attr)) { // Don't try to check trylock functions for now. return; @@ -2383,12 +2305,12 @@ for (const auto &Mu : ExclusiveLocksToAdd) { auto Entry = std::make_unique(Mu, LK_Exclusive, Loc, FactEntry::Declared); - addLock(InitialLockset, std::move(Entry), CapDiagKind, true); + addLock(InitialLockset, std::move(Entry), true); } for (const auto &Mu : SharedLocksToAdd) { auto Entry = std::make_unique(Mu, LK_Shared, Loc, FactEntry::Declared); - addLock(InitialLockset, std::move(Entry), CapDiagKind, true); + addLock(InitialLockset, std::move(Entry), true); } } diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -86,6 +86,28 @@ return ME ? ME->isArrow() : false; } +static StringRef ClassifyDiagnostic(const CapabilityAttr *A) { + return A->getName(); +} + +static StringRef ClassifyDiagnostic(QualType VDT) { + // We need to look at the declaration of the type of the value to determine + // which it is. The type should either be a record or a typedef, or a pointer + // or reference thereof. + if (const auto *RT = VDT->getAs()) { + if (const auto *RD = RT->getDecl()) + if (const auto *CA = RD->getAttr()) + return ClassifyDiagnostic(CA); + } else if (const auto *TT = VDT->getAs()) { + if (const auto *TD = TT->getDecl()) + if (const auto *CA = TD->getAttr()) + return ClassifyDiagnostic(CA); + } else if (VDT->isPointerType() || VDT->isReferenceType()) + return ClassifyDiagnostic(VDT->getPointeeType()); + + return "mutex"; +} + /// Translate a clang expression in an attribute to a til::SExpr. /// Constructs the context from D, DeclExp, and SelfDecl. /// @@ -152,16 +174,17 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx) { if (!AttrExp) - return CapabilityExpr(nullptr, false); + return CapabilityExpr(); if (const auto* SLit = dyn_cast(AttrExp)) { if (SLit->getString() == StringRef("*")) // The "*" expr is a universal lock, which essentially turns off // checks until it is removed from the lockset. - return CapabilityExpr(new (Arena) til::Wildcard(), false); + return CapabilityExpr(new (Arena) til::Wildcard(), StringRef("wildcard"), + false); else // Ignore other string literals for now. - return CapabilityExpr(nullptr, false); + return CapabilityExpr(); } bool Neg = false; @@ -183,14 +206,16 @@ // Trap mutex expressions like nullptr, or 0. // Any literal value is nonsense. if (!E || isa(E)) - return CapabilityExpr(nullptr, false); + return CapabilityExpr(); + + StringRef Kind = ClassifyDiagnostic(AttrExp->getType()); // Hack to deal with smart pointers -- strip off top-level pointer casts. if (const auto *CE = dyn_cast(E)) { if (CE->castOpcode() == til::CAST_objToPtr) - return CapabilityExpr(CE->expr(), Neg); + return CapabilityExpr(CE->expr(), Kind, Neg); } - return CapabilityExpr(E, Neg); + return CapabilityExpr(E, Kind, Neg); } // Translate a clang statement or expression to a TIL expression. diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -3862,8 +3862,8 @@ } a = 0; // \ // expected-warning {{writing variable 'a' requires holding mutex 'mu_' exclusively}} - endNoWarnOnWrites(); // \ - // expected-warning {{releasing mutex '*' that was not held}} + endNoWarnOnWrites(); // \ + // expected-warning {{releasing wildcard '*' that was not held}} }