[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Sorry that it took so long, I had to debug it and think a bit. This should do it:

  diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  index 9d28325c1ea6..13e37ac2b56b 100644
  --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  @@ -361,7 +361,7 @@ public:
       unsigned NumArgs = 0;
   
       // Function arguments
  -    const Expr *const *FunArgs = nullptr;
  +    llvm::PointerUnion<const Expr *const *, til::SExpr *> FunArgs = nullptr;
   
       // is Self referred to with -> or .?
       bool SelfArrow = false;
  diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
  index b8286cef396c..1eb48c0d5554 100644
  --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
  +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
  @@ -110,7 +110,8 @@ static StringRef ClassifyDiagnostic(QualType VDT) {
   /// \param D       The declaration to which the attribute is attached.
   /// \param DeclExp An expression involving the Decl to which the attribute
   ///                is attached.  E.g. the call to a function.
  -/// \param Self    S-expression to substitute for a \ref CXXThisExpr.
  +/// \param Self    S-expression to substitute for a \ref CXXThisExpr in a call,
  +///                or argument to a cleanup function.
   CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
                                                  const NamedDecl *D,
                                                  const Expr *DeclExp,
  @@ -144,7 +145,11 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
   
     if (Self) {
       assert(!Ctx.SelfArg && "Ambiguous self argument");
  -    Ctx.SelfArg = Self;
  +    assert(isa<FunctionDecl>(D) && "Self argument requires function");
  +    if (isa<CXXMethodDecl>(D))
  +      Ctx.SelfArg = Self;
  +    else
  +      Ctx.FunArgs = Self;
   
       // If the attribute has no arguments, then assume the argument is "this".
       if (!AttrExp)
  @@ -312,8 +317,14 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
                 ? (cast<FunctionDecl>(D)->getCanonicalDecl() == Canonical)
                 : (cast<ObjCMethodDecl>(D)->getCanonicalDecl() == Canonical)) {
           // Substitute call arguments for references to function parameters
  -        assert(I < Ctx->NumArgs);
  -        return translate(Ctx->FunArgs[I], Ctx->Prev);
  +        if (const Expr *const *FunArgs =
  +                Ctx->FunArgs.dyn_cast<const Expr *const *>()) {
  +          assert(I < Ctx->NumArgs);
  +          return translate(FunArgs[I], Ctx->Prev);
  +        } else {
  +          assert(I == 0);
  +          return Ctx->FunArgs.get<til::SExpr *>();
  +        }
         }
       }
       // Map the param back to the param of the original function declaration

The idea (which might be expressed in additional comments) is that until now we only had `Self` for `CXXMethodDecl`, but now we also have it for other `FunctionDecl`, in which case it is to be understood as the sole function argument.



================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1776
 /// \param D     The callee declaration.
 /// \param Self  If \p Exp = nullptr, the implicit this argument.
 /// \param Loc   If \p Exp = nullptr, the location.
----------------
We should relax this like "the implicit this argument or the argument to an implicitly called cleanup function".


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2436
+                                    CF.getVarDecl()->getLocation());
+          break;
+        }
----------------
tbaeder wrote:
> tbaeder wrote:
> > tbaeder wrote:
> > > aaronpuchert wrote:
> > > > aaronpuchert wrote:
> > > > > tbaeder wrote:
> > > > > > This handles the function call, but without the instance parameter. I was wondering how to best do that.
> > > > > Should you not simply pass `SxBuilder.createVariable(CF.getVarDecl())` as third parameter in analogy with the `AutomaticObjectDtor` case? It might also make sense to copy the attribute check.
> > > > Can you write a test case that relies on passing the variable? Here is an idea:
> > > > ```
> > > > void unlock_scope(Mutex **mu) __attribute__((release_capability(*mu))) {
> > > >   mutex_exclusive_unlock(*mu);
> > > > }
> > > > 
> > > > Mutex* const CLEANUP(unlock_scope) scope = &mu1;
> > > > mutex_exclusive_lock(*scope);
> > > > // Unlock should happen automatically.
> > > > ```
> > > > I think this is mildly more interesting than the cleanup function with an unused parameter.
> > > > 
> > > > Unfortunately this is not quite as powerful as a scoped lock in C++, as we don't track the identity `scope == &mu1`. So `guarded_by` won't work with this. But we can at least see warnings on balanced locking/unlocking.
> > > > 
> > > > As for proper scoped locking, we could treat some variable initializations like construction of a C++ scoped lock. But let's discuss this separately.
> > > Yeah, it doesn't find the lock; I assume that's because the first parameter here is not an _actual_ this parameter; I'd have to handle the var decl as a regular parameter.
> > @aaronpuchert Can you explain how that would work? One goal from before was to avoid creating new fake AST nodes, would I'd have to insert the instance parameter as a new `DeclRefExpr` in the CFG, wouldn't I?
> Ping
> I assume that's because the first parameter here is not an _actual_ this parameter; I'd have to handle the var decl as a regular parameter.

Right, in `SExprBuilder::translateAttrExpr` we put `Self` into `Ctx.SelfArg`, but this is only used for substituting a `CXXThisExpr`, not a function parameter.


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

https://reviews.llvm.org/D152504