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 @@ -31,6 +31,7 @@ #include "clang/Basic/LLVM.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/PointerIntPair.h" +#include "llvm/ADT/PointerUnion.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" #include @@ -354,7 +355,7 @@ const NamedDecl *AttrDecl; // Implicit object argument -- e.g. 'this' - const Expr *SelfArg = nullptr; + llvm::PointerUnion SelfArg = nullptr; // Number of funArgs unsigned NumArgs = 0; @@ -378,10 +379,18 @@ // Translate a clang expression in an attribute to a til::SExpr. // Constructs the context from D, DeclExp, and SelfDecl. CapabilityExpr translateAttrExpr(const Expr *AttrExp, const NamedDecl *D, - const Expr *DeclExp, VarDecl *SelfD=nullptr); + const Expr *DeclExp, + til::SExpr *Self = nullptr); CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx); + // Translate a variable reference. + til::LiteralPtr *createVariable(const VarDecl *VD); + + // Create placeholder for this: we don't know the VarDecl on construction yet. + std::pair + createThisPlaceholder(const Expr *Exp); + // Translate a clang statement or expression to a TIL expression. // Also performs substitution of variables; Ctx provides the context. // Dispatches on the type of S. diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h @@ -634,15 +634,14 @@ /// At compile time, pointer literals are represented by symbolic names. class LiteralPtr : public SExpr { public: - LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) { - assert(D && "ValueDecl must not be null"); - } + LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) {} LiteralPtr(const LiteralPtr &) = default; static bool classof(const SExpr *E) { return E->opcode() == COP_LiteralPtr; } // The clang declaration for the value that this pointer points to. const ValueDecl *clangDecl() const { return Cvdecl; } + void setClangDecl(const ValueDecl *VD) { Cvdecl = VD; } template typename V::R_SExpr traverse(V &Vs, typename V::R_Ctx Ctx) { @@ -651,6 +650,8 @@ template typename C::CType compare(const LiteralPtr* E, C& Cmp) const { + if (!Cvdecl || !E->Cvdecl) + return Cmp.comparePointers(this, E); return Cmp.comparePointers(Cvdecl, E->Cvdecl); } diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h @@ -623,7 +623,10 @@ } void printLiteralPtr(const LiteralPtr *E, StreamType &SS) { - SS << E->clangDecl()->getNameAsString(); + if (const NamedDecl *D = E->clangDecl()) + SS << D->getNameAsString(); + else + SS << ""; } void printVariable(const Variable *V, StreamType &SS, bool IsVarDecl=false) { 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 @@ -1029,7 +1029,7 @@ template void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, - const NamedDecl *D, VarDecl *SelfDecl = nullptr); + const NamedDecl *D, til::SExpr *Self = nullptr); template void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, @@ -1220,7 +1220,7 @@ if (const auto *LP = dyn_cast(SExp)) { const ValueDecl *VD = LP->clangDecl(); // Variables defined in a function are always inaccessible. - if (!VD->isDefinedOutsideFunctionOrMethod()) + if (!VD || !VD->isDefinedOutsideFunctionOrMethod()) return false; // For now we consider static class members to be inaccessible. if (isa(VD->getDeclContext())) @@ -1311,10 +1311,10 @@ template void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, const NamedDecl *D, - VarDecl *SelfDecl) { + til::SExpr *Self) { if (Attr->args_size() == 0) { // The mutex held is the "this" object. - CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, SelfDecl); + CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, Self); if (Cp.isInvalid()) { warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind()); return; @@ -1326,7 +1326,7 @@ } for (const auto *Arg : Attr->args()) { - CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, SelfDecl); + CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, Self); if (Cp.isInvalid()) { warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind()); continue; @@ -1529,6 +1529,8 @@ ThreadSafetyAnalyzer *Analyzer; FactSet FSet; + /// Maps constructed objects to `this` placeholder prior to initialization. + llvm::SmallDenseMap ConstructedObjects; LocalVariableMap::Context LVarCtx; unsigned CtxIndex; @@ -1536,14 +1538,17 @@ void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK, Expr *MutexExp, ProtectedOperationKind POK, SourceLocation Loc); - void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp); + void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp, + SourceLocation Loc); void checkAccess(const Expr *Exp, AccessKind AK, ProtectedOperationKind POK = POK_VarAccess); void checkPtAccess(const Expr *Exp, AccessKind AK, ProtectedOperationKind POK = POK_VarAccess); - void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr); + void handleCall(const Expr *Exp, const NamedDecl *D, + til::LiteralPtr *Self = nullptr, + SourceLocation Loc = SourceLocation()); void examineArguments(const FunctionDecl *FD, CallExpr::const_arg_iterator ArgBegin, CallExpr::const_arg_iterator ArgEnd, @@ -1560,6 +1565,7 @@ void VisitCallExpr(const CallExpr *Exp); void VisitCXXConstructExpr(const CXXConstructExpr *Exp); void VisitDeclStmt(const DeclStmt *S); + void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp); }; } // namespace @@ -1629,7 +1635,7 @@ /// Warn if the LSet contains the given lock. void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, - Expr *MutexExp) { + Expr *MutexExp, SourceLocation Loc) { CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp); if (Cp.isInvalid()) { warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind()); @@ -1641,7 +1647,7 @@ const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, Cp); if (LDat) { Analyzer->Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(), - Cp.toString(), Exp->getExprLoc()); + Cp.toString(), Loc); } } @@ -1761,21 +1767,34 @@ /// and check that the appropriate locks are held. Non-const method calls with /// the same signature as const method calls can be also treated as reads. /// +/// \param Exp The call expression. +/// \param D The callee declaration. +/// \param Self If \p Exp = nullptr, the implicit this argument. +/// \param Loc If \p Exp = nullptr, the location. void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D, - VarDecl *VD) { - SourceLocation Loc = Exp->getExprLoc(); + til::LiteralPtr *Self, SourceLocation Loc) { CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd; CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove; CapExprSet ScopedReqsAndExcludes; // Figure out if we're constructing an object of scoped lockable class - bool isScopedVar = false; - if (VD) { - if (const auto *CD = dyn_cast(D)) { - const CXXRecordDecl* PD = CD->getParent(); - if (PD && PD->hasAttr()) - isScopedVar = true; + CapabilityExpr Scp; + if (Exp) { + assert(!Self); + const auto *TagT = Exp->getType()->getAs(); + if (TagT && Exp->isPRValue()) { + std::pair Placeholder = + Analyzer->SxBuilder.createThisPlaceholder(Exp); + auto inserted = ConstructedObjects.insert({Exp, Placeholder.first}); + assert(inserted.second && "Are we visiting the same expression again?"); + if (isa(Exp)) + Self = Placeholder.first; + if (TagT->getDecl()->hasAttr()) + Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false); } + + assert(Loc.isInvalid()); + Loc = Exp->getExprLoc(); } for(const Attr *At : D->attrs()) { @@ -1786,7 +1805,7 @@ const auto *A = cast(At); Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, - A, Exp, D, VD); + A, Exp, D, Self); break; } @@ -1797,7 +1816,7 @@ const auto *A = cast(At); CapExprSet AssertLocks; - Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD); + Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self); for (const auto &AssertLock : AssertLocks) Analyzer->addLock( FSet, std::make_unique( @@ -1808,7 +1827,7 @@ const auto *A = cast(At); CapExprSet AssertLocks; - Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD); + Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self); for (const auto &AssertLock : AssertLocks) Analyzer->addLock( FSet, std::make_unique( @@ -1819,7 +1838,7 @@ case attr::AssertCapability: { const auto *A = cast(At); CapExprSet AssertLocks; - Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD); + Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self); for (const auto &AssertLock : AssertLocks) Analyzer->addLock(FSet, std::make_unique( AssertLock, @@ -1833,11 +1852,11 @@ case attr::ReleaseCapability: { const auto *A = cast(At); if (A->isGeneric()) - Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, VD); + Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self); else if (A->isShared()) - Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, VD); + Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self); else - Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, VD); + Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self); break; } @@ -1845,10 +1864,10 @@ const auto *A = cast(At); for (auto *Arg : A->args()) { warnIfMutexNotHeld(D, Exp, A->isShared() ? AK_Read : AK_Written, Arg, - POK_FunctionCall, Exp->getExprLoc()); + POK_FunctionCall, Loc); // use for adopting a lock - if (isScopedVar) - Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD); + if (!Scp.shouldIgnore()) + Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self); } break; } @@ -1856,10 +1875,10 @@ case attr::LocksExcluded: { const auto *A = cast(At); for (auto *Arg : A->args()) { - warnIfMutexHeld(D, Exp, Arg); + warnIfMutexHeld(D, Exp, Arg, Loc); // use for deferring a lock - if (isScopedVar) - Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD); + if (!Scp.shouldIgnore()) + Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self); } break; } @@ -1882,7 +1901,7 @@ // Add locks. FactEntry::SourceKind Source = - isScopedVar ? FactEntry::Managed : FactEntry::Acquired; + !Scp.shouldIgnore() ? FactEntry::Managed : FactEntry::Acquired; for (const auto &M : ExclusiveLocksToAdd) Analyzer->addLock(FSet, std::make_unique(M, LK_Exclusive, Loc, Source)); @@ -1890,15 +1909,9 @@ Analyzer->addLock( FSet, std::make_unique(M, LK_Shared, Loc, Source)); - if (isScopedVar) { + if (!Scp.shouldIgnore()) { // Add the managing object as a dummy mutex, mapped to the underlying mutex. - SourceLocation MLoc = VD->getLocation(); - DeclRefExpr DRE(VD->getASTContext(), VD, false, VD->getType(), VK_LValue, - VD->getLocation()); - // FIXME: does this store a pointer to DRE? - CapabilityExpr Scp = Analyzer->SxBuilder.translateAttrExpr(&DRE, nullptr); - - auto ScopedEntry = std::make_unique(Scp, MLoc); + auto ScopedEntry = std::make_unique(Scp, Loc); for (const auto &M : ExclusiveLocksToAdd) ScopedEntry->addLock(M); for (const auto &M : SharedLocksToAdd) @@ -2058,36 +2071,11 @@ } else { examineArguments(D, Exp->arg_begin(), Exp->arg_end()); } + if (D && D->hasAttrs()) + handleCall(Exp, D); } -static CXXConstructorDecl * -findConstructorForByValueReturn(const CXXRecordDecl *RD) { - // Prefer a move constructor over a copy constructor. If there's more than - // one copy constructor or more than one move constructor, we arbitrarily - // pick the first declared such constructor rather than trying to guess which - // one is more appropriate. - CXXConstructorDecl *CopyCtor = nullptr; - for (auto *Ctor : RD->ctors()) { - if (Ctor->isDeleted()) - continue; - if (Ctor->isMoveConstructor()) - return Ctor; - if (!CopyCtor && Ctor->isCopyConstructor()) - CopyCtor = Ctor; - } - return CopyCtor; -} - -static Expr *buildFakeCtorCall(CXXConstructorDecl *CD, ArrayRef Args, - SourceLocation Loc) { - ASTContext &Ctx = CD->getASTContext(); - return CXXConstructExpr::Create(Ctx, Ctx.getRecordType(CD->getParent()), Loc, - CD, true, Args, false, false, false, false, - CXXConstructExpr::CK_Complete, - SourceRange(Loc, Loc)); -} - -static Expr *UnpackConstruction(Expr *E) { +static const Expr *UnpackConstruction(const Expr *E) { if (auto *CE = dyn_cast(E)) if (CE->getCastKind() == CK_NoOp) E = CE->getSubExpr()->IgnoreParens(); @@ -2106,7 +2094,7 @@ for (auto *D : S->getDeclGroup()) { if (auto *VD = dyn_cast_or_null(D)) { - Expr *E = VD->getInit(); + const Expr *E = VD->getInit(); if (!E) continue; E = E->IgnoreParens(); @@ -2116,29 +2104,27 @@ E = EWC->getSubExpr()->IgnoreParens(); E = UnpackConstruction(E); - if (const auto *CE = dyn_cast(E)) { - const auto *CtorD = dyn_cast_or_null(CE->getConstructor()); - if (!CtorD || !CtorD->hasAttrs()) - continue; - handleCall(E, CtorD, VD); - } else if (isa(E) && E->isPRValue()) { - // If the object is initialized by a function call that returns a - // scoped lockable by value, use the attributes on the copy or move - // constructor to figure out what effect that should have on the - // lockset. - // FIXME: Is this really the best way to handle this situation? - auto *RD = E->getType()->getAsCXXRecordDecl(); - if (!RD || !RD->hasAttr()) - continue; - CXXConstructorDecl *CtorD = findConstructorForByValueReturn(RD); - if (!CtorD || !CtorD->hasAttrs()) - continue; - handleCall(buildFakeCtorCall(CtorD, {E}, E->getBeginLoc()), CtorD, VD); + if (auto Object = ConstructedObjects.find(E); + Object != ConstructedObjects.end()) { + Object->second->setClangDecl(VD); + ConstructedObjects.erase(Object); } } } } +void BuildLockset::VisitMaterializeTemporaryExpr( + const MaterializeTemporaryExpr *Exp) { + if (const ValueDecl *ExtD = Exp->getExtendingDecl()) { + if (auto Object = + ConstructedObjects.find(UnpackConstruction(Exp->getSubExpr())); + Object != ConstructedObjects.end()) { + Object->second->setClangDecl(ExtD); + ConstructedObjects.erase(Object); + } + } +} + /// Given two facts merging on a join point, possibly warn and decide whether to /// keep or replace. /// @@ -2411,19 +2397,33 @@ LocksetBuilder.Visit(CS.getStmt()); break; } - // Ignore BaseDtor, MemberDtor, and TemporaryDtor for now. + // Ignore BaseDtor and MemberDtor for now. case CFGElement::AutomaticObjectDtor: { CFGAutomaticObjDtor AD = BI.castAs(); const auto *DD = AD.getDestructorDecl(AC.getASTContext()); if (!DD->hasAttrs()) break; - // Create a dummy expression, - auto *VD = const_cast(AD.getVarDecl()); - DeclRefExpr DRE(VD->getASTContext(), VD, false, - VD->getType().getNonReferenceType(), VK_LValue, - AD.getTriggerStmt()->getEndLoc()); - LocksetBuilder.handleCall(&DRE, DD); + LocksetBuilder.handleCall(nullptr, DD, + SxBuilder.createVariable(AD.getVarDecl()), + AD.getTriggerStmt()->getEndLoc()); + break; + } + case CFGElement::TemporaryDtor: { + auto TD = BI.castAs(); + + // Clean up constructed object even if there are no attributes to + // keep the number of objects in limbo as small as possible. + if (auto Object = LocksetBuilder.ConstructedObjects.find( + TD.getBindTemporaryExpr()->getSubExpr()); + Object != LocksetBuilder.ConstructedObjects.end()) { + const auto *DD = TD.getDestructorDecl(AC.getASTContext()); + if (DD->hasAttrs()) + // TODO: the location here isn't quite correct. + LocksetBuilder.handleCall(nullptr, DD, Object->second, + TD.getBindTemporaryExpr()->getEndLoc()); + LocksetBuilder.ConstructedObjects.erase(Object); + } break; } default: 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 @@ -115,19 +115,22 @@ /// \param D The declaration to which the attribute is attached. /// \param DeclExp An expression involving the Decl to which the attribute /// is attached. E.g. the call to a function. +/// \param Self S-expression to substitute for a \ref CXXThisExpr. CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp, const NamedDecl *D, const Expr *DeclExp, - VarDecl *SelfDecl) { + til::SExpr *Self) { // If we are processing a raw attribute expression, with no substitutions. - if (!DeclExp) + if (!DeclExp && !Self) return translateAttrExpr(AttrExp, nullptr); CallingContext Ctx(nullptr, D); // Examine DeclExp to find SelfArg and FunArgs, which are used to substitute // for formal parameters when we call buildMutexID later. - if (const auto *ME = dyn_cast(DeclExp)) { + if (!DeclExp) + /* We'll use Self. */; + else if (const auto *ME = dyn_cast(DeclExp)) { Ctx.SelfArg = ME->getBase(); Ctx.SelfArrow = ME->isArrow(); } else if (const auto *CE = dyn_cast(DeclExp)) { @@ -142,29 +145,24 @@ Ctx.SelfArg = nullptr; // Will be set below Ctx.NumArgs = CE->getNumArgs(); Ctx.FunArgs = CE->getArgs(); - } else if (D && isa(D)) { - // There's no such thing as a "destructor call" in the AST. - Ctx.SelfArg = DeclExp; } - // Hack to handle constructors, where self cannot be recovered from - // the expression. - if (SelfDecl && !Ctx.SelfArg) { - DeclRefExpr SelfDRE(SelfDecl->getASTContext(), SelfDecl, false, - SelfDecl->getType(), VK_LValue, - SelfDecl->getLocation()); - Ctx.SelfArg = &SelfDRE; + if (Self) { + assert(!Ctx.SelfArg && "Ambiguous self argument"); + Ctx.SelfArg = Self; // If the attribute has no arguments, then assume the argument is "this". if (!AttrExp) - return translateAttrExpr(Ctx.SelfArg, nullptr); + return CapabilityExpr( + Self, ClassifyDiagnostic(cast(D)->getThisObjectType()), + false); else // For most attributes. return translateAttrExpr(AttrExp, &Ctx); } // If the attribute has no arguments, then assume the argument is "this". if (!AttrExp) - return translateAttrExpr(Ctx.SelfArg, nullptr); + return translateAttrExpr(cast(Ctx.SelfArg), nullptr); else // For most attributes. return translateAttrExpr(AttrExp, &Ctx); } @@ -218,6 +216,16 @@ return CapabilityExpr(E, Kind, Neg); } +til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) { + return new (Arena) til::LiteralPtr(VD); +} + +std::pair +SExprBuilder::createThisPlaceholder(const Expr *Exp) { + return {new (Arena) til::LiteralPtr(nullptr), + ClassifyDiagnostic(Exp->getType())}; +} + // Translate a clang statement or expression to a TIL expression. // Also performs substitution of variables; Ctx provides the context. // Dispatches on the type of S. @@ -327,8 +335,12 @@ til::SExpr *SExprBuilder::translateCXXThisExpr(const CXXThisExpr *TE, CallingContext *Ctx) { // Substitute for 'this' - if (Ctx && Ctx->SelfArg) - return translate(Ctx->SelfArg, Ctx->Prev); + if (Ctx && Ctx->SelfArg) { + if (const auto *SelfArg = dyn_cast(Ctx->SelfArg)) + return translate(SelfArg, Ctx->Prev); + else + return cast(Ctx->SelfArg); + } assert(SelfVar && "We have no variable for 'this'!"); return SelfVar; } 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 @@ -1690,6 +1690,15 @@ } #endif + void temporary() { + MutexLock{&mu1}, a = 5; + } + + void lifetime_extension() { + const MutexLock &mulock = MutexLock(&mu1); + a = 5; + } + void foo2() { ReaderMutexLock mulock1(&mu1); if (getBool()) { @@ -1708,6 +1717,12 @@ // expected-warning {{acquiring mutex 'mu1' that is already held}} } + void temporary_double_lock() { + MutexLock mulock_a(&mu1); // expected-note{{mutex acquired here}} + MutexLock{&mu1}; // \ + // expected-warning {{acquiring mutex 'mu1' that is already held}} + } + void foo4() { MutexLock mulock1(&mu1), mulock2(&mu2); a = b+1; @@ -4187,6 +4202,20 @@ void foo() EXCLUSIVE_LOCKS_REQUIRED(this); }; +class SelfLockDeferred { +public: + SelfLockDeferred() LOCKS_EXCLUDED(mu_); + ~SelfLockDeferred() UNLOCK_FUNCTION(mu_); + + Mutex mu_; +}; + +class LOCKABLE SelfLockDeferred2 { +public: + SelfLockDeferred2() LOCKS_EXCLUDED(this); + ~SelfLockDeferred2() UNLOCK_FUNCTION(); +}; + void test() { SelfLock s; @@ -4198,6 +4227,14 @@ s2.foo(); } +void testDeferredTemporary() { + SelfLockDeferred(); // expected-warning {{releasing mutex '.mu_' that was not held}} +} + +void testDeferredTemporary2() { + SelfLockDeferred2(); // expected-warning {{releasing mutex '' that was not held}} +} + } // end namespace SelfConstructorTest @@ -5892,47 +5929,41 @@ void f() { c[A()]->g(); } } // namespace PR34800 -namespace ReturnScopedLockable { - template class SCOPED_LOCKABLE ReadLockedPtr { - public: - ReadLockedPtr(Object *ptr) SHARED_LOCK_FUNCTION((*this)->mutex); - ReadLockedPtr(ReadLockedPtr &&) SHARED_LOCK_FUNCTION((*this)->mutex); - ~ReadLockedPtr() UNLOCK_FUNCTION(); +#ifdef __cpp_guaranteed_copy_elision - Object *operator->() const { return object; } +namespace ReturnScopedLockable { - private: - Object *object; - }; +class Object { +public: + MutexLock lock() EXCLUSIVE_LOCK_FUNCTION(mutex) { + // TODO: False positive because scoped lock isn't destructed. + return MutexLock(&mutex); // expected-note {{mutex acquired here}} + } // expected-warning {{mutex 'mutex' is still held at the end of function}} - struct Object { - int f() SHARED_LOCKS_REQUIRED(mutex); - Mutex mutex; - }; + int x GUARDED_BY(mutex); + void needsLock() EXCLUSIVE_LOCKS_REQUIRED(mutex); - ReadLockedPtr get(); - int use() { - auto ptr = get(); - return ptr->f(); - } - void use_constructor() { - auto ptr = ReadLockedPtr(nullptr); - ptr->f(); - auto ptr2 = ReadLockedPtr{nullptr}; - ptr2->f(); - auto ptr3 = (ReadLockedPtr{nullptr}); - ptr3->f(); - } - struct Convertible { - Convertible(); - operator ReadLockedPtr(); - }; - void use_conversion() { - ReadLockedPtr ptr = Convertible(); - ptr->f(); + void testInside() { + MutexLock scope = lock(); + x = 1; + needsLock(); } + +private: + Mutex mutex; +}; + +void testOutside() { + Object obj; + MutexLock scope = obj.lock(); + obj.x = 1; + obj.needsLock(); } +} // namespace ReturnScopedLockable + +#endif + namespace PR38640 { void f() { // Self-referencing assignment previously caused an infinite loop when thread