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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for your patience on the review! I've taken a cursory look through and I'm still thinking about the patch. I've not seen anything that's really worrying. But this is pretty dense stuff and @delesley has way more experience with TIL, so I'm hoping he might be able to take a peek as well.



================
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);
----------------
aaronpuchert wrote:
> 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.
I don't know that there's a lot of value to documenting it if we have to disable thread safety analysis, so my instinct is to hold off on the documentation for the moment.


================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h:435
 /// Used to implement lazy copy and rewriting strategies.
 class Future : public SExpr {
 public:
----------------
aaronpuchert wrote:
> By the way, I considered using this, but it didn't seem wholly appropriate. Our placeholder can't be lazily evaluated, we simply don't know initially. And later we do know and can just plug in the `VarDecl`.
Hmmm, naively, it seems like `FS_pending` is "we don't know initially" and `FS_done` is "now we do know and can plugin in the `VarDecl`". But I agree it seems a little different from lazy evaluation in that it's not a performance optimization.


================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h:629
+    else
+      SS << "<temporary>";
   }
----------------
aaronpuchert wrote:
> Perhaps no warning will ever print this, but I'm not entirely sure.
Should we assert here instead?


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1538
   FactSet FSet;
+  SmallVector<ConstructedObject, 2> ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
----------------
aaronpuchert wrote:
> 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.
Would it make more sense to use a `SmallDenseMap` here?


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2422
+        case CFGElement::TemporaryDtor: {
+          CFGTemporaryDtor TD = BI.castAs<CFGTemporaryDtor>();
+
----------------



================
Comment at: clang/lib/Analysis/ThreadSafetyCommon.cpp:339-342
+    if (const Expr *SelfArg = dyn_cast<const Expr *>(Ctx->SelfArg))
+      return translate(SelfArg, Ctx->Prev);
+    else
+      return cast<til::SExpr *>(Ctx->SelfArg);
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129755/new/

https://reviews.llvm.org/D129755