[all-commits] [llvm/llvm-project] d8fa40: Thread safety analysis: Handle additional cast in ...

[Aaron Puchert via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: d8fa40dfa7adb062c969ce7ea6ce505e10626838
      https://github.com/llvm/llvm-project/commit/d8fa40dfa7adb062c969ce7ea6ce505e10626838
  Author: Aaron Puchert <aaron.puchert at sap.com>
  Date:   2022-10-06 (Thu, 06 Oct 2022)

  Changed paths:
    M clang/lib/Analysis/ThreadSafety.cpp
    M clang/test/SemaCXX/warn-thread-safety-analysis.cpp

  Log Message:
  -----------
  Thread safety analysis: Handle additional cast in scoped capability construction

We might have a CK_NoOp cast and a further CK_ConstructorConversion.
As an optimization, drop some IgnoreParens calls: inside of the
CK_{Constructor,UserDefined}Conversion should be no more parentheses,
and inside the CXXBindTemporaryExpr should also be none.

Lastly, we factor out the unpacking so that we can reuse it for
MaterializeTemporaryExprs later on.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D129752


  Commit: 0041a69495f828f6732803cfb0f1e3fddd7fbf2a
      https://github.com/llvm/llvm-project/commit/0041a69495f828f6732803cfb0f1e3fddd7fbf2a
  Author: Aaron Puchert <aaron.puchert at sap.com>
  Date:   2022-10-06 (Thu, 06 Oct 2022)

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

  Log Message:
  -----------
  Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

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.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D129755


Compare: https://github.com/llvm/llvm-project/compare/32dc48094b0c...0041a69495f8