[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls
Aaron Puchert via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 14 05:12:26 PDT 2022
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
More information about the cfe-commits
mailing list