aaronpuchert added inline comments.
================ Comment at: clang/docs/ThreadSafetyAnalysis.rst:935 + // Same as constructors, but without tag types. (Requires C++17 copy elision.) + static MutexLocker Lock(Mutex *mu) ACQUIRE(mu) NO_THREAD_SAFETY_ANALYSIS { + return MutexLocker(mu); ---------------- The `NO_THREAD_SAFETY_ANALYSIS` here is due to a false positive warning because the scope is not destructed in the function body. Maybe we should postpone documentation until this is resolved. ================ Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h:653-654 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); ---------------- For placeholders we're comparing identity. ================ Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h:629 + else + SS << "<temporary>"; } ---------------- Perhaps no warning will ever print this, but I'm not entirely sure. ================ Comment at: clang/lib/Analysis/ThreadSafety.cpp:1223 // Variables defined in a function are always inaccessible. - if (!VD->isDefinedOutsideFunctionOrMethod()) + if (!VD || !VD->isDefinedOutsideFunctionOrMethod()) return false; ---------------- Placeholders are always local variables. ================ Comment at: clang/lib/Analysis/ThreadSafety.cpp:1538 FactSet FSet; + SmallVector<ConstructedObject, 2> ConstructedObjects; LocalVariableMap::Context LVarCtx; ---------------- This is essentially an `std::map<const Expr *, til::LiteralPtr *>`, but since it only keeps objects between construction and assignment to a variable or temporary destruction, I thought that a small vector would be more appropriate. However, there is no guarantee that this doesn't grow in size significantly: temporaries with trivial destructors (which don't show up in the CFG) would never be removed from this list. That's unlikely for properly annotated scopes, but not impossible. ================ Comment at: clang/lib/Analysis/ThreadSafety.cpp:1791-1793 + std::pair<til::LiteralPtr *, StringRef> Placeholder = + Analyzer->SxBuilder.createThisPlaceholder(Exp); + ConstructedObjects.push_back(ConstructedObject{Exp, Placeholder.first}); ---------------- In case you're wondering why we need this for records with `ScopedLockableAttr`: otherwise we get failures in `SelfConstructorTest`: ``` class SelfLock { public: SelfLock() EXCLUSIVE_LOCK_FUNCTION(mu_); ~SelfLock() UNLOCK_FUNCTION(mu_); void foo() EXCLUSIVE_LOCKS_REQUIRED(mu_); Mutex mu_; }; void test() { SelfLock s; s.foo(); } ``` For `s.foo()` we need `s.mu`, and to see that we've acquired this we need to plugin `s` for the placeholder that we used for construction, although `SelfLock` is not a scoped lockable. Though we could restrict this to functions that have actual thread safety attributes (instead of other attributes). ================ Comment at: clang/lib/Analysis/ThreadSafety.cpp:2123-2135 +void BuildLockset::VisitMaterializeTemporaryExpr( + const MaterializeTemporaryExpr *Exp) { + if (const ValueDecl *ExtD = Exp->getExtendingDecl()) { + auto Object = llvm::find_if( + ConstructedObjects, + [E = UnpackConstruction(Exp->getSubExpr())]( + const ConstructedObject &CS) { return CS.ConstructExpr == E; }); ---------------- This is not strictly required, but it enables the `lifetime_extension` test below. It could also be a separate change, but having it here demonstrates that the approach can handle this case as well. ================ Comment at: clang/lib/Analysis/ThreadSafety.cpp:2421-2439 + case CFGElement::TemporaryDtor: { + CFGTemporaryDtor TD = BI.castAs<CFGTemporaryDtor>(); + + // Clean up constructed object even if there are no attributes to + // keep the number of objects in limbo as small as possible. + auto Object = llvm::find_if( + LocksetBuilder.ConstructedObjects, ---------------- Scopes as temporaries are probably unusual, but as the `temporary` test demonstrates they could be useful. In any event, if we recognize acquisition via `CXXConstructExpr`s not contained in a `DeclStmt`, we should probably also remove locks when they disappear again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129755/new/ https://reviews.llvm.org/D129755 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits