[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Aaron Puchert via Phabricator via cfe-commits]

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