From: DeLesley Hutchins Date: Thu, 18 Sep 2014 23:02:26 +0000 (+0000) Subject: Thread Safety Analysis: add new warning flag, -Wthread-safety-reference, which X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9308d1067c5b1dd4faf80d4b572d2f79b6da87c2;p=clang Thread Safety Analysis: add new warning flag, -Wthread-safety-reference, which warns when a guarded variable is passed by reference as a function argument. This is released as a separate warning flag, because it could potentially break existing code that uses thread safety analysis. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@218087 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/Analyses/ThreadSafety.h b/include/clang/Analysis/Analyses/ThreadSafety.h index 7cd82fdf8c..458bb576f4 100644 --- a/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/include/clang/Analysis/Analyses/ThreadSafety.h @@ -31,7 +31,9 @@ namespace threadSafety { enum ProtectedOperationKind { POK_VarDereference, ///< Dereferencing a variable (e.g. p in *p = 5;) POK_VarAccess, ///< Reading or writing a variable (e.g. x in x = 5;) - POK_FunctionCall ///< Making a function call (e.g. fool()) + POK_FunctionCall, ///< Making a function call (e.g. fool()) + POK_PassByRef, ///< Passing a guarded variable by reference. + POK_PtPassByRef, ///< Passing a pt-guarded variable by reference. }; /// This enum distinguishes between different kinds of lock actions. For diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index ea733876e8..19c08d36dc 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -593,11 +593,13 @@ def Most : DiagGroup<"most", [ def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">; def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">; +def ThreadSafetyReference : DiagGroup<"thread-safety-reference">; def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">; def ThreadSafety : DiagGroup<"thread-safety", [ThreadSafetyAttributes, ThreadSafetyAnalysis, - ThreadSafetyPrecise]>; + ThreadSafetyPrecise, + ThreadSafetyReference]>; def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">; def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 163bceaaa2..d567cce2d1 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -2350,6 +2350,16 @@ def warn_acquire_requires_negative_cap : Warning< "acquiring %0 '%1' requires negative capability '%2'">, InGroup, DefaultIgnore; +// Thread safety warnings on pass by reference +def warn_guarded_pass_by_reference : Warning< + "passing variable '%1' by reference requires holding %0 " + "%select{'%2'|'%2' exclusively}3">, + InGroup, DefaultIgnore; +def warn_pt_guarded_pass_by_reference : Warning< + "passing the value that '%1' points to by reference requires holding %0 " + "%select{'%2'|'%2' exclusively}3">, + InGroup, DefaultIgnore; + // Imprecise thread safety warnings def warn_variable_requires_lock : Warning< "%select{reading|writing}3 variable '%1' requires holding %0 " diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index 008561d5e7..285b8924a0 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -1285,8 +1285,10 @@ class BuildLockset : public StmtVisitor { void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp, StringRef DiagKind); - void checkAccess(const Expr *Exp, AccessKind AK); - void checkPtAccess(const Expr *Exp, AccessKind AK); + void checkAccess(const Expr *Exp, AccessKind AK, + ProtectedOperationKind POK = POK_VarAccess); + void checkPtAccess(const Expr *Exp, AccessKind AK, + ProtectedOperationKind POK = POK_VarAccess); void handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr); @@ -1395,7 +1397,8 @@ void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, /// marked with guarded_by, we must ensure the appropriate mutexes are held. /// Similarly, we check if the access is to an expression that dereferences /// a pointer marked with pt_guarded_by. -void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK) { +void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK, + ProtectedOperationKind POK) { Exp = Exp->IgnoreParenCasts(); SourceLocation Loc = Exp->getExprLoc(); @@ -1418,20 +1421,20 @@ void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK) { if (const UnaryOperator *UO = dyn_cast(Exp)) { // For dereferences if (UO->getOpcode() == clang::UO_Deref) - checkPtAccess(UO->getSubExpr(), AK); + checkPtAccess(UO->getSubExpr(), AK, POK); return; } if (const ArraySubscriptExpr *AE = dyn_cast(Exp)) { - checkPtAccess(AE->getLHS(), AK); + checkPtAccess(AE->getLHS(), AK, POK); return; } if (const MemberExpr *ME = dyn_cast(Exp)) { if (ME->isArrow()) - checkPtAccess(ME->getBase(), AK); + checkPtAccess(ME->getBase(), AK, POK); else - checkAccess(ME->getBase(), AK); + checkAccess(ME->getBase(), AK, POK); } const ValueDecl *D = getValueDecl(Exp); @@ -1439,18 +1442,19 @@ void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK) { return; if (D->hasAttr() && FSet.isEmpty(Analyzer->FactMan)) { - Analyzer->Handler.handleNoMutexHeld("mutex", D, POK_VarAccess, AK, - Loc); + Analyzer->Handler.handleNoMutexHeld("mutex", D, POK, AK, Loc); } for (const auto *I : D->specific_attrs()) - warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK_VarAccess, + warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, ClassifyDiagnostic(I), Loc); } /// \brief Checks pt_guarded_by and pt_guarded_var attributes. -void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK) { +/// POK is the same operationKind that was passed to checkAccess. +void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK, + ProtectedOperationKind POK) { while (true) { if (const ParenExpr *PE = dyn_cast(Exp)) { Exp = PE->getSubExpr(); @@ -1460,7 +1464,7 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK) { if (CE->getCastKind() == CK_ArrayToPointerDecay) { // If it's an actual array, and not a pointer, then it's elements // are protected by GUARDED_BY, not PT_GUARDED_BY; - checkAccess(CE->getSubExpr(), AK); + checkAccess(CE->getSubExpr(), AK, POK); return; } Exp = CE->getSubExpr(); @@ -1469,16 +1473,20 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK) { break; } + // Pass by reference warnings are under a different flag. + ProtectedOperationKind PtPOK = POK_VarDereference; + if (POK == POK_PassByRef) PtPOK = POK_PtPassByRef; + const ValueDecl *D = getValueDecl(Exp); if (!D || !D->hasAttrs()) return; if (D->hasAttr() && FSet.isEmpty(Analyzer->FactMan)) - Analyzer->Handler.handleNoMutexHeld("mutex", D, POK_VarDereference, AK, + Analyzer->Handler.handleNoMutexHeld("mutex", D, PtPOK, AK, Exp->getExprLoc()); for (auto const *I : D->specific_attrs()) - warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK_VarDereference, + warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, ClassifyDiagnostic(I), Exp->getExprLoc()); } @@ -1668,6 +1676,9 @@ void BuildLockset::VisitCastExpr(CastExpr *CE) { void BuildLockset::VisitCallExpr(CallExpr *Exp) { + bool ExamineArgs = true; + bool OperatorFun = false; + if (CXXMemberCallExpr *CE = dyn_cast(Exp)) { MemberExpr *ME = dyn_cast(CE->getCallee()); // ME can be null when calling a method pointer @@ -1688,8 +1699,12 @@ void BuildLockset::VisitCallExpr(CallExpr *Exp) { } } } else if (CXXOperatorCallExpr *OE = dyn_cast(Exp)) { - switch (OE->getOperator()) { + OperatorFun = true; + + auto OEop = OE->getOperator(); + switch (OEop) { case OO_Equal: { + ExamineArgs = false; const Expr *Target = OE->getArg(0); const Expr *Source = OE->getArg(1); checkAccess(Target, AK_Written); @@ -1701,16 +1716,53 @@ void BuildLockset::VisitCallExpr(CallExpr *Exp) { case OO_Subscript: { const Expr *Obj = OE->getArg(0); checkAccess(Obj, AK_Read); - checkPtAccess(Obj, AK_Read); + if (!(OEop == OO_Star && OE->getNumArgs() > 1)) { + // Grrr. operator* can be multiplication... + checkPtAccess(Obj, AK_Read); + } break; } default: { + // TODO: get rid of this, and rely on pass-by-ref instead. const Expr *Obj = OE->getArg(0); checkAccess(Obj, AK_Read); break; } } } + + + if (ExamineArgs) { + if (FunctionDecl *FD = Exp->getDirectCallee()) { + unsigned Fn = FD->getNumParams(); + unsigned Cn = Exp->getNumArgs(); + unsigned Skip = 0; + + unsigned i = 0; + if (OperatorFun) { + if (isa(FD)) { + // First arg in operator call is implicit self argument, + // and doesn't appear in the FunctionDecl. + Skip = 1; + Cn--; + } else { + // Ignore the first argument of operators; it's been checked above. + i = 1; + } + } + // Ignore default arguments + unsigned n = (Fn < Cn) ? Fn : Cn; + + for (; i < n; ++i) { + ParmVarDecl* Pvd = FD->getParamDecl(i); + Expr* Arg = Exp->getArg(i+Skip); + QualType Qt = Pvd->getType(); + if (Qt->isReferenceType()) + checkAccess(Arg, AK_Read, POK_PassByRef); + } + } + } + NamedDecl *D = dyn_cast_or_null(Exp->getCalleeDecl()); if(!D || !D->hasAttrs()) return; diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index 20b7a0240d..b207df9fc5 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -1475,6 +1475,20 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { return ONS; } + OptionalNotes getNotes(const PartialDiagnosticAt &Note1, + const PartialDiagnosticAt &Note2) const { + OptionalNotes ONS; + ONS.push_back(Note1); + ONS.push_back(Note2); + if (Verbose && CurrentFunction) { + PartialDiagnosticAt FNote(CurrentFunction->getBody()->getLocStart(), + S.PDiag(diag::note_thread_warning_in_fun) + << CurrentFunction->getNameAsString()); + ONS.push_back(FNote); + } + return ONS; + } + // Helper functions void warnLockMismatch(unsigned DiagID, StringRef Kind, Name LockName, SourceLocation Loc) { @@ -1605,13 +1619,25 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { case POK_FunctionCall: DiagID = diag::warn_fun_requires_lock_precise; break; + case POK_PassByRef: + DiagID = diag::warn_guarded_pass_by_reference; + break; + case POK_PtPassByRef: + DiagID = diag::warn_pt_guarded_pass_by_reference; + break; } PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << D->getNameAsString() << LockName << LK); PartialDiagnosticAt Note(Loc, S.PDiag(diag::note_found_mutex_near_match) << *PossibleMatch); - Warnings.push_back(DelayedDiag(Warning, getNotes(Note))); + if (Verbose && POK == POK_VarAccess) { + PartialDiagnosticAt VNote(D->getLocation(), + S.PDiag(diag::note_guarded_by_declared_here) + << D->getNameAsString()); + Warnings.push_back(DelayedDiag(Warning, getNotes(Note, VNote))); + } else + Warnings.push_back(DelayedDiag(Warning, getNotes(Note))); } else { switch (POK) { case POK_VarAccess: @@ -1623,6 +1649,12 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { case POK_FunctionCall: DiagID = diag::warn_fun_requires_lock; break; + case POK_PassByRef: + DiagID = diag::warn_guarded_pass_by_reference; + break; + case POK_PtPassByRef: + DiagID = diag::warn_pt_guarded_pass_by_reference; + break; } PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << D->getNameAsString() @@ -1637,6 +1669,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { } } + virtual void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg, SourceLocation Loc) { PartialDiagnosticAt Warning(Loc, diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index 7f0a2d1ccb..091e47335b 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -4739,4 +4739,112 @@ class Foo { +namespace PassByRefTest { + +class Foo { +public: + Foo() : a(0), b(0) { } + + int a; + int b; + + void operator+(const Foo& f); + + void operator[](const Foo& g); +}; + +template +T&& mymove(T& f); + + +// test top-level functions +void copy(Foo f); +void write1(Foo& f); +void write2(int a, Foo& f); +void read1(const Foo& f); +void read2(int a, const Foo& f); +void destroy(Foo&& f); + +void operator/(const Foo& f, const Foo& g); +void operator*(const Foo& f, const Foo& g); + + + + +class Bar { +public: + Mutex mu; + Foo foo GUARDED_BY(mu); + Foo foo2 GUARDED_BY(mu); + Foo* foop PT_GUARDED_BY(mu); + SmartPtr foosp PT_GUARDED_BY(mu); + + // test methods. + void mwrite1(Foo& f); + void mwrite2(int a, Foo& f); + void mread1(const Foo& f); + void mread2(int a, const Foo& f); + + // static methods + static void smwrite1(Foo& f); + static void smwrite2(int a, Foo& f); + static void smread1(const Foo& f); + static void smread2(int a, const Foo& f); + + void operator<<(const Foo& f); + + void test1() { + copy(foo); // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} + write1(foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + write2(10, foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + read1(foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + read2(10, foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + destroy(mymove(foo)); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + + mwrite1(foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + mwrite2(10, foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + mread1(foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + mread2(10, foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + + smwrite1(foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + smwrite2(10, foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + smread1(foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + smread2(10, foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + + foo + foo2; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \ + // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}} + foo / foo2; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \ + // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}} + foo * foo2; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \ + // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}} + foo[foo2]; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \ + // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}} + (*this) << foo; // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}} + + copy(*foop); // expected-warning {{reading the value pointed to by 'foop' requires holding mutex 'mu'}} + write1(*foop); // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}} + write2(10, *foop); // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}} + read1(*foop); // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}} + read2(10, *foop); // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}} + destroy(mymove(*foop)); // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}} + + copy(*foosp); // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}} + write1(*foosp); // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}} + write2(10, *foosp); // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}} + read1(*foosp); // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}} + read2(10, *foosp); // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}} + destroy(mymove(*foosp)); // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}} + + // TODO -- these requires better smart pointer handling. + copy(*foosp.get()); + write1(*foosp.get()); + write2(10, *foosp.get()); + read1(*foosp.get()); + read2(10, *foosp.get()); + destroy(mymove(*foosp.get())); + } +}; + + +} // end namespace PassByRefTest