rsmith created this revision.
rsmith added reviewers: aaron.ballman, delesley, dblaikie.
Herald added a subscriber: sanjoy.
In C++17, guaranteed copy elision means that there isn't necessarily a
constructor call when a local variable is initialized by a function call that
returns a `scoped_lockable` by value. That causes code such as:
// see test for definition of ReadLockedPtr
ReadLockedPtr<X> get();
void f() {
auto my_ptr = get();
my_ptr->do_something_requiring_lock();
}
... to fail for two reasons: the lock `my_ptr->mutex` is not held in the
`do_something_requiring_lock()` call, and the lock `my_ptr` is not held at the
point of the implicit destructor call at the end of the function.
There isn't really a good way to work around this: we don't have a list of
"these are the locks you should expect to be held when an object is returned by
value". But we can approximate that by pretending that the local `my_ptr`
variable was actually initialized by running the move constructor of
`ReadLockedPtr`, which is what this patch does.
[Note that this doesn't address the more problematic case where the returned
type has no copy or move constructor. If that comes up, it can be hacked around
with something like:
struct SCOPED_LOCKABLE MyImmovableButReturnableScopedLock {
MyImmovableButReturnableScopedLock(MyImmovableButReturnableScopedLock &&) =
delete;
MyImmovableButReturnableScopedLock(volatile
MyImmovableButReturnableScopedLock&&) ATTRIBUTES_HERE; // not defined
};
... on the basis that no sane program will ever actually call the other move
constructor.]
Ideas for alternative approaches welcome. Ideally I'd like something that
allows existing C++<=14 code to continue to work in C++17 mode unchanged, which
I think this approach mostly provides.
Repository:
rC Clang
https://reviews.llvm.org/D41933
Files:
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
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=0 %s
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=0 %s
// FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
// FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
@@ -5248,3 +5249,28 @@
C c;
void f() { c[A()]->g(); }
} // namespace PR34800
+
+namespace ReturnScopedLockable {
+ template<typename Object> class SCOPED_LOCKABLE ReadLockedPtr {
+ public:
+ ReadLockedPtr(Object *ptr) SHARED_LOCK_FUNCTION((*this)->mutex);
+ ReadLockedPtr(ReadLockedPtr &&) SHARED_LOCK_FUNCTION((*this)->mutex);
+ ~ReadLockedPtr() UNLOCK_FUNCTION();
+
+ Object *operator->() const { return object; }
+
+ private:
+ Object *object;
+ };
+
+ struct Object {
+ int f() SHARED_LOCKS_REQUIRED(mutex);
+ Mutex mutex;
+ };
+
+ ReadLockedPtr<Object> get();
+ int use() {
+ auto ptr = get();
+ return ptr->f();
+ }
+}
Index: lib/Analysis/ThreadSafety.cpp
===================================================================
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1689,7 +1689,7 @@
CapExprSet ScopedExclusiveReqs, ScopedSharedReqs;
StringRef CapDiagKind = "mutex";
- // Figure out if we're calling the constructor of scoped lockable class
+ // Figure out if we're constructing an object of scoped lockable class
bool isScopedVar = false;
if (VD) {
if (const CXXConstructorDecl *CD = dyn_cast<const CXXConstructorDecl>(D)) {
@@ -1991,22 +1991,67 @@
// FIXME -- only handles constructors in DeclStmt below.
}
+static CXXConstructorDecl *findConstructorForByValueReturn(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 (CXXConstructorDecl *Ctor : RD->ctors()) {
+ if (Ctor->isDeleted() || Ctor->getAccess() != AS_public)
+ continue;
+ if (Ctor->isMoveConstructor())
+ return Ctor;
+ if (!CopyCtor && Ctor->isCopyConstructor())
+ CopyCtor = Ctor;
+ }
+ return CopyCtor;
+}
+
+static Expr *buildFakeCtorCall(CXXConstructorDecl *CD, ArrayRef<Expr *> 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));
+}
+
void BuildLockset::VisitDeclStmt(DeclStmt *S) {
// adjust the context
LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
for (auto *D : S->getDeclGroup()) {
if (VarDecl *VD = dyn_cast_or_null<VarDecl>(D)) {
Expr *E = VD->getInit();
+ if (!E)
+ continue;
+ E = E->IgnoreParens();
+
// handle constructors that involve temporaries
- if (ExprWithCleanups *EWC = dyn_cast_or_null<ExprWithCleanups>(E))
+ if (auto *EWC = dyn_cast<ExprWithCleanups>(E))
E = EWC->getSubExpr();
+ if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
+ E = BTE->getSubExpr();
- if (CXXConstructExpr *CE = dyn_cast_or_null<CXXConstructExpr>(E)) {
+ if (CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(E)) {
NamedDecl *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
if (!CtorD || !CtorD->hasAttrs())
- return;
- handleCall(CE, CtorD, VD);
+ continue;
+ handleCall(E, CtorD, VD);
+ } else if (isa<CallExpr>(E) && E->isRValue()) {
+ // 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<ScopedLockableAttr>())
+ continue;
+ CXXConstructorDecl *CtorD = findConstructorForByValueReturn(RD);
+ if (!CtorD || !CtorD->hasAttrs())
+ continue;
+ handleCall(buildFakeCtorCall(CtorD, {E}, E->getLocStart()), CtorD, VD);
}
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
