Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[aaronpuchert]

Summary

This imitates the code for MemberExpr. I hope it is right, for I have
absolutely no understanding of ObjC++.

Fixes 38896.

[aaron.ballman]

Adding a reviewer who knows more about ObjC than I do.

include/clang/Analysis/Analyses/ThreadSafetyCommon.h

400

ME is kind of a poor name; can you switch to IVRE like you used in the implementation?

lib/Analysis/ThreadSafetyCommon.cpp

375

I feel like this will always return false. However, I wonder if IVRE->getBase()->isArrow() is more appropriate here?

[rjmccall]

lib/Analysis/ThreadSafetyCommon.cpp

375

The base of an ObjCIvarRefExpr will never directly be a C pointer type. I don't know whether this data structure looks through casts, but it's certainly *possible* to cast arbitrarily weird C stuff to ObjC pointer type. On the other hand, by and large nobody actually ever does that, so I wouldn't make a special case for it here.

ObjCIvarRefExpr just has an isArrow() method directly if this is just supposed to be capturing the difference between . and ->. But then, so does MemberExpr, and in that case you're doing this odd analysis of the base instead of trusting isArrow(), so I don't know what to tell you to do.

Overall it is appropriate to treat an ObjC ivar reference as a perfectly ordinary projection. The only thing that's special about it is that the actual offset isn't necessarily statically known, but ivars still behave as if they're all independently declared, so I wouldn't worry about it.

[aaronpuchert]

Thanks to both of you for the reviews. I'll see what I can do about the arrows. My gut feeling is that using {Member,ObjCIVarRef}Expr::isArrow is the right way, but it's not yet obvious to me how.

include/clang/Analysis/Analyses/ThreadSafetyCommon.h

400

Of course, that's a copy-and-paste error.

lib/Analysis/ThreadSafetyCommon.cpp

375

I had wondered about this as well, but when I changed hasCppPointerType(BE) to ME->isArrow() in the MemberExpr case, I got some test failures. For example:

struct Foo {
  int a __attribute__((guarded_by(mu)));
  Mutex mu;
};

void f() {
  Foo foo;
  foo.a = 5; // \
    // expected-warning{{writing variable 'a' requires holding mutex 'foo.mu' exclusively}}
}

Instead we warn writing variable 'a' requires holding mutex 'foo->mu' exclusively. That's not right.

The expression (ME) we are processing is mu from the attribute on a, which the compiler sees as this->mu. So we get ME->isArrow() == true and ME->getBase() is a CXXThisExpr.

Basically the translate* functions do a substitution. They try to derive foo.mu from this->mu on the a member and the expression foo.a. The annotation this->mu is the expression we get as parameter, and foo.a is encoded in the CallingContext.

The information whether foo.a is an arrow (it isn't) seems to be in the CallingContext's SelfArrow member.

[aaronpuchert]

I found something that would theoretically work:

P->setArrow((isa<CXXThisExpr>(ME->getBase()) && Ctx && Ctx->SelfArg) ? Ctx->SelfArrow : ME->isArrow());

So if we have this and a context that tells us we have to replace this by something else, then we check Ctx->SelfArrow, otherwise we take ME->isArrow().

But that doesn't work. When translating into the TIL (typed intermediate language), referencing and dereferencing operators are completely ignored.

struct Foo {
  Mutex mu_;
  void foo1(Foo *f_declared) EXCLUSIVE_LOCKS_REQUIRED(f_declared->mu_);
};

void test() {
  Foo myfoo;
  myfoo.foo1(&myfoo);  // \
    // expected-warning {{calling function 'foo1' requires holding mutex 'myfoo.mu_' exclusively}}
}

With the above change we warn that calling function 'foo1' requires holding mutex 'myfoo->mu_' exclusively. It should be (&myfoo)->mu_, but the & is lost. So we can't derive the information that we want from isArrow alone.

Now there is a reason why these operators are ignored — the TIL tries to "canonicalize" expressions, so that it detects that (&myfoo)->mu_ and myfoo.mu_ are the same thing. Changing that is probably possible, but beyond the scope of this change.

Short of that, we must be able to detect pointers. I think we could use Type::isAnyPointerType instead of Type::isPointerType in hasCppPointerType (and probably rename that).

For later I think we should consider a different canonicalization that doesn't just ignore & and *.

[aaronpuchert]

Detect ObjC pointer types as well as ordinary pointers.

[aaronpuchert]

I think it should be possible to get rid of self-> in the warning message if we want to, after all this-> is omitted in C++ as well.

test/SemaObjCXX/warn-thread-safety-analysis.mm

43

@rjmccall Would you rather write self->lock_ or lock_ in the warning message?

[rjmccall]

In D52200#1239007, @aaronpuchert wrote:

I think it should be possible to get rid of self-> in the warning message if we want to, after all this-> is omitted in C++ as well.

Hmm. It would be consistent to apply the same rule to both cases, and I don't have any serious concerns about dropping it. If anything, Objective-C pushes the ivar-decorating convention harder than C++ does, since it's officially blessed in the language.

[delesley]

lib/Analysis/ThreadSafetyCommon.cpp

375

This is a recurring problem. The fundamental issue is the analysis must compare expressions for equality, but many C++ expressions are syntactically different but semantically the same, like (&foo)->mu and foo.mu. It's particularly problematic because (as Aaron notes) we frequently substitute arguments when constructing expressions, which makes it easy to get things like (&foo)->mu instead of foo.mu, when substituting &foo for a pointer argument.

The purpose of the TIL is to translate C++ expressions into a simpler, lower-level IR, where syntactic equality in the IR corresponds to semantic equality between expressions. The TIL doesn't distinguish between pointers and references, doesn't distinguish between -> and ., and ignores * and & as no-ops. But then we have to recover the distinction between -> and . when we generate an error message.

In the presence of substitution, you can't go by whether the source syntax has an ->. You have to look at whether the type of the underlying argument (after stripping away * and &) requires an arrow.

I know nothing about objective C, so I don't know what the right answer is here.

[aaronpuchert]

It seems that self is an ordinary DeclRefExpr unlike this, which is a CXXThisExpr. Which means we'd have to make it dependent on the name whether we drop it, but self in C/C++ is just an ordinary variable. So I think I'll leave the self-> part for now. It certainly doesn't hurt.

include/clang/Analysis/Analyses/ThreadSafetyCommon.h

 private:
   // We implement the CFGVisitor API
   friend class CFGWalker;
 
   til::SExpr *translateDeclRefExpr(const DeclRefExpr *DRE,
                                    CallingContext *Ctx) ;
   til::SExpr *translateCXXThisExpr(const CXXThisExpr *TE, CallingContext *Ctx);
   til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx);
+  til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *IVRE,
+                                       CallingContext *Ctx);
   til::SExpr *translateCallExpr(const CallExpr *CE, CallingContext *Ctx,
                                 const Expr *SelfE = nullptr);
   til::SExpr *translateCXXMemberCallExpr(const CXXMemberCallExpr *ME,
                                          CallingContext *Ctx);
   til::SExpr *translateCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE,
                                            CallingContext *Ctx);
   til::SExpr *translateUnaryOperator(const UnaryOperator *UO,
                                      CallingContext *Ctx);

lib/Analysis/ThreadSafetyCommon.cpp

til::SExpr *SExprBuilder::translate(const Stmt *S, CallingContext *Ctx) {
 
   switch (S->getStmtClass()) {
   case Stmt::DeclRefExprClass:
     return translateDeclRefExpr(cast<DeclRefExpr>(S), Ctx);
   case Stmt::CXXThisExprClass:
     return translateCXXThisExpr(cast<CXXThisExpr>(S), Ctx);
   case Stmt::MemberExprClass:
     return translateMemberExpr(cast<MemberExpr>(S), Ctx);
+  case Stmt::ObjCIvarRefExprClass:
+    return translateObjCIVarRefExpr(cast<ObjCIvarRefExpr>(S), Ctx);
   case Stmt::CallExprClass:
     return translateCallExpr(cast<CallExpr>(S), Ctx);
   case Stmt::CXXMemberCallExprClass:
     return translateCXXMemberCallExpr(cast<CXXMemberCallExpr>(S), Ctx);
   case Stmt::CXXOperatorCallExprClass:
     return translateCXXOperatorCallExpr(cast<CXXOperatorCallExpr>(S), Ctx);
   case Stmt::UnaryOperatorClass:
     return translateUnaryOperator(cast<UnaryOperator>(S), Ctx);
if (const auto *Ph = dyn_cast<til::Phi>(E))
     return Ph->clangDecl();
   if (const auto *P = dyn_cast<til::Project>(E))
     return P->clangDecl();
   if (const auto *L = dyn_cast<til::LiteralPtr>(E))
     return L->clangDecl();
   return nullptr;
 }
 
-static bool hasCppPointerType(const til::SExpr *E) {
+static bool hasAnyPointerType(const til::SExpr *E) {
   auto *VD = getValueDeclFromSExpr(E);
-  if (VD && VD->getType()->isPointerType())
+  if (VD && VD->getType()->isAnyPointerType())
     return true;
   if (const auto *C = dyn_cast<til::Cast>(E))
     return C->castOpcode() == til::CAST_objToPtr;
 
   return false;
 }
 
 // Grab the very first declaration of virtual method D
til::SExpr *SExprBuilder::translateMemberExpr(const MemberExpr *ME,
   til::SExpr *BE = translate(ME->getBase(), Ctx);
   til::SExpr *E  = new (Arena) til::SApply(BE);
 
   const auto *D = cast<ValueDecl>(ME->getMemberDecl()->getCanonicalDecl());
   if (const auto *VD = dyn_cast<CXXMethodDecl>(D))
     D = getFirstVirtualDecl(VD);
 
   til::Project *P = new (Arena) til::Project(E, D);
-  if (hasCppPointerType(BE))
+  if (hasAnyPointerType(BE))
+    P->setArrow(true);
+  return P;
+}
+
+til::SExpr *SExprBuilder::translateObjCIVarRefExpr(const ObjCIvarRefExpr *IVRE,
+                                                   CallingContext *Ctx) {
+  til::SExpr *BE = translate(IVRE->getBase(), Ctx);
+  til::SExpr *E = new (Arena) til::SApply(BE);
+
+  const auto *D = cast<ObjCIvarDecl>(IVRE->getDecl()->getCanonicalDecl());
+
+  til::Project *P = new (Arena) til::Project(E, D);
+  if (hasAnyPointerType(BE))
     P->setArrow(true);
   return P;
 }
 
 til::SExpr *SExprBuilder::translateCallExpr(const CallExpr *CE,
                                             CallingContext *Ctx,
                                             const Expr *SelfE) {
   if (CapabilityExprMode) {
     // Handle LOCK_RETURNED
     if (const FunctionDecl *FD = CE->getDirectCallee()) {
       FD = FD->getMostRecentDecl();
       if (LockReturnedAttr *At = FD->getAttr<LockReturnedAttr>()) {
         CallingContext LRCallCtx(Ctx);
         LRCallCtx.AttrDecl = CE->getDirectCallee();
         LRCallCtx.SelfArg = SelfE;
         LRCallCtx.NumArgs = CE->getNumArgs();
         LRCallCtx.FunArgs = CE->getArgs();
         return const_cast<til::SExpr *>(
             translateAttrExpr(At->getArg(), &LRCallCtx).sexpr());
       }
     }

test/SemaObjCXX/warn-thread-safety-analysis.mm

+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -Wno-objc-root-class %s
+
+class __attribute__((lockable)) Lock {
+public:
+  void Acquire() __attribute__((exclusive_lock_function())) {}
+  void Release() __attribute__((unlock_function())) {}
+};
+
+class __attribute__((scoped_lockable)) AutoLock {
+public:
+  AutoLock(Lock &lock) __attribute__((exclusive_lock_function(lock)))
+  : lock_(lock) {
+    lock.Acquire();
+  }
+  ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+  Lock &lock_;
+};
+
+@interface MyInterface {
+@private
+  Lock lock_;
+  int value_;
+}
+
+- (void)incrementValue;
+- (void)decrementValue;
+
+@end
+
+@implementation MyInterface
+
+- (void)incrementValue {
+  AutoLock lock(lock_);
+  value_ += 1;
+}
+
+- (void)decrementValue {
+  lock_.Acquire(); // expected-note{{mutex acquired here}}
+  value_ -= 1;
+} // expected-warning{{mutex 'self->lock_' is still held at the end of function}}
+
+@end