[all-commits] [llvm/llvm-project] 54bfd0: Thread safety analysis: Support copy-elided produc...

Aaron Puchert via All-commits all-commits at lists.llvm.org
Thu Oct 13 10:37:27 PDT 2022

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 54bfd04846156dbd5e0a6b88f539c3d4569a455f
  Author: Aaron Puchert <aaron.puchert at sap.com>
  Date:   2022-10-13 (Thu, 13 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

More information about the All-commits mailing list