r218087 - Thread Safety Analysis: add new warning flag, -Wthread-safety-reference, which
DeLesley Hutchins
delesley at google.com
Thu Sep 18 16:02:27 PDT 2014
Author: delesley
Date: Thu Sep 18 18:02:26 2014
New Revision: 218087
URL: http://llvm.org/viewvc/llvm-project?rev=218087&view=rev
Log:
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.
Modified:
cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Analysis/ThreadSafety.cpp
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
Modified: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h?rev=218087&r1=218086&r2=218087&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h (original)
+++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h Thu Sep 18 18:02:26 2014
@@ -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
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=218087&r1=218086&r2=218087&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Sep 18 18:02:26 2014
@@ -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">;
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=218087&r1=218086&r2=218087&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Sep 18 18:02:26 2014
@@ -2350,6 +2350,16 @@ def warn_acquire_requires_negative_cap :
"acquiring %0 '%1' requires negative capability '%2'">,
InGroup<ThreadSafetyNegative>, 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<ThreadSafetyReference>, 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<ThreadSafetyReference>, DefaultIgnore;
+
// Imprecise thread safety warnings
def warn_variable_requires_lock : Warning<
"%select{reading|writing}3 variable '%1' requires holding %0 "
Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=218087&r1=218086&r2=218087&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Thu Sep 18 18:02:26 2014
@@ -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
/// 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 Exp
if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(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<ArraySubscriptExpr>(Exp)) {
- checkPtAccess(AE->getLHS(), AK);
+ checkPtAccess(AE->getLHS(), AK, POK);
return;
}
if (const MemberExpr *ME = dyn_cast<MemberExpr>(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 Exp
return;
if (D->hasAttr<GuardedVarAttr>() && 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<GuardedByAttr>())
- 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<ParenExpr>(Exp)) {
Exp = PE->getSubExpr();
@@ -1460,7 +1464,7 @@ void BuildLockset::checkPtAccess(const E
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 E
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<PtGuardedVarAttr>() && 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<PtGuardedByAttr>())
- 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(CastExp
void BuildLockset::VisitCallExpr(CallExpr *Exp) {
+ bool ExamineArgs = true;
+ bool OperatorFun = false;
+
if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(Exp)) {
MemberExpr *ME = dyn_cast<MemberExpr>(CE->getCallee());
// ME can be null when calling a method pointer
@@ -1688,8 +1699,12 @@ void BuildLockset::VisitCallExpr(CallExp
}
}
} else if (CXXOperatorCallExpr *OE = dyn_cast<CXXOperatorCallExpr>(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(CallExp
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<CXXMethodDecl>(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<NamedDecl>(Exp->getCalleeDecl());
if(!D || !D->hasAttrs())
return;
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=218087&r1=218086&r2=218087&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Thu Sep 18 18:02:26 2014
@@ -1475,6 +1475,20 @@ class ThreadSafetyReporter : public clan
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 clan
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 clan
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 clan
}
}
+
virtual void handleNegativeNotHeld(StringRef Kind, Name LockName, Name Neg,
SourceLocation Loc) {
PartialDiagnosticAt Warning(Loc,
Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=218087&r1=218086&r2=218087&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Thu Sep 18 18:02:26 2014
@@ -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<class T>
+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<Foo> 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
More information about the cfe-commits
mailing list