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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
Herald added a reviewer: NoQ.
Herald added a project: All.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When support for copy elision was initially added in e97654b2f2807, it
was taking attributes from a constructor call, although that constructor
call is actually not involved. It seems more natural to use attributes
on the function returning the scoped capability, which is where it's
actually coming from. This would also support a number of interesting
use cases, like producing different scope kinds without the need for tag
types, or producing scopes from a private mutex.

Changing the behavior was surprisingly difficult: we were not handling
CXXConstructorExpr calls like regular calls but instead handled them
through the DeclStmt they're contained in. This was based on the
assumption that constructors are basically only called in variable
declarations (not true because of temporaries), and that variable
declarations necessitate constructors (not true with C++17 anymore).

Untangling this required separating construction from assigning a
variable name. When a call produces an object, we use a placeholder
til::LiteralPtr for `this`, and we collect the call expression and
placeholder in a map. Later when going through a DeclStmt, we look up
the call expression and set the placeholder to the new VarDecl.

The change has a couple of nice side effects:

- We don't miss constructor calls not contained in DeclStmts anymore, allowing patterns like MutexLock{&mu}, requiresMutex(); The scoped lock temporary will be destructed at the end of the full statement, so it protects the following call without the need for a scope, but with the ability to unlock in case of an exception.
- We support lifetime extension of temporaries. While unusual, one can now write const MutexLock &scope = MutexLock(&mu); and have it behave as expected.
- Destructors used to be handled in a weird way: since there is no expression in the AST for implicit destructor calls, we instead provided a made-up DeclRefExpr to the variable being destructed, and passed that instead of a CallExpr. Then later in translateAttrExpr there was special code that knew that destructor expressions worked a bit different.
- We were producing dummy DeclRefExprs in a number of places, this has been eliminated. We now use til::SExprs instead.

Technically this could break existing code, but the current handling
seems unexpected enough to justify this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129755

Files:
  clang/docs/ThreadSafetyAnalysis.rst
  clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
  clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h
  clang/lib/Analysis/ThreadSafety.cpp
  clang/lib/Analysis/ThreadSafetyCommon.cpp
  clang/test/SemaCXX/warn-thread-safety-analysis.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129755.444597.patch
Type: text/x-patch
Size: 29255 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220714/258e9149/attachment-0001.bin>