aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
Herald added a subscriber: cfe-commits.
When passing by reference, we check if the reference is const-qualified
and if it isn't, we demand an exclusive lock. Unlike checking const
qualifiers on member functions, there are probably not many false
positives here: if a function takes a non-const reference, it will
in almost all cases modify the object that we passed it.
Repository:
rC Clang
https://reviews.llvm.org/D52395
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5026,19 +5026,19 @@
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'}}
+ write1(foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ write2(10, foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
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'}}
+ destroy(mymove(foo)); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
- 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'}}
+ mwrite1(foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ mwrite2(10, foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
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'}}
+ smwrite1(foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ smwrite2(10, foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
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'}}
@@ -5053,12 +5053,13 @@
(*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'}}
+ write1(*foop); // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+ write2(10, *foop); // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
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'}}
+ destroy(mymove(*foop)); // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+ // TODO -- sometimes this should warn about writing.
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'}}
@@ -5074,6 +5075,57 @@
read2(10, *foosp.get());
destroy(mymove(*foosp.get()));
}
+
+ void test2() {
+ mu.ReaderLock();
+
+ copy(foo);
+ write1(foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ write2(10, foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ read1(foo);
+ read2(10, foo);
+ destroy(mymove(foo)); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+
+ mwrite1(foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ mwrite2(10, foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ mread1(foo);
+ mread2(10, foo);
+
+ smwrite1(foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ smwrite2(10, foo); // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+ smread1(foo);
+ smread2(10, foo);
+
+ foo + foo2;
+ foo / foo2;
+ foo * foo2;
+ foo[foo2];
+ (*this) << foo;
+
+ copy(*foop);
+ write1(*foop); // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+ write2(10, *foop); // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+ read1(*foop);
+ read2(10, *foop);
+ destroy(mymove(*foop)); // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+
+ // TODO -- these require better smart pointer handling.
+ copy(*foosp);
+ write1(*foosp);
+ write2(10, *foosp);
+ read1(*foosp);
+ read2(10, *foosp);
+ destroy(mymove(*foosp));
+
+ copy(*foosp.get());
+ write1(*foosp.get());
+ write2(10, *foosp.get());
+ read1(*foosp.get());
+ read2(10, *foosp.get());
+ destroy(mymove(*foosp.get()));
+
+ mu.ReaderUnlock();
+ }
};
Index: lib/Analysis/ThreadSafety.cpp
===================================================================
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -2020,8 +2020,12 @@
const ParmVarDecl *Pvd = FD->getParamDecl(i);
const Expr *Arg = Exp->getArg(i + Skip);
QualType Qt = Pvd->getType();
- if (Qt->isReferenceType())
- checkAccess(Arg, AK_Read, POK_PassByRef);
+ if (const auto* RT = dyn_cast<const ReferenceType>(&*Qt)) {
+ if (RT->getPointeeType().isConstQualified())
+ checkAccess(Arg, AK_Read, POK_PassByRef);
+ else
+ checkAccess(Arg, AK_Written, POK_PassByRef);
+ }
}
}
}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3090,12 +3090,12 @@
// 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">,
+ "passing variable %1 by %select{reference|non-const reference}3 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">,
+ "passing the value that %1 points to by %select{reference|non-const "
+ "reference}3 requires holding %0 %select{'%2'|'%2' exclusively}3">,
InGroup<ThreadSafetyReference>, DefaultIgnore;
// Imprecise thread safety warnings
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
