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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
