[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/include/clang/Analysis/Analyses/ThreadSafetyTIL.h:435
 /// Used to implement lazy copy and rewriting strategies.
 class Future : public SExpr {
 public:
----------------
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`.


================
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});
----------------
aaronpuchert wrote:
> 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).
Sorry, records **without** `ScopedLockableAttr` of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755