[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert planned changes to this revision.
aaronpuchert added a comment.

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.



================
Comment at: include/clang/Analysis/Analyses/ThreadSafetyCommon.h:400
   til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx);
+  til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *ME,
+                                       CallingContext *Ctx);
----------------
aaron.ballman wrote:
> `ME` is kind of a poor name; can you switch to `IVRE` like you used in the implementation?
Of course, that's a copy-and-paste error.


================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362
+  til::Project *P = new (Arena) til::Project(E, D);
+  if (hasCppPointerType(BE))
+    P->setArrow(true);
----------------
rjmccall wrote:
> aaron.ballman wrote:
> > I feel like this will always return false. However, I wonder if `IVRE->getBase()->isArrow()` is more appropriate here?
> 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.
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.


Repository:
  rC Clang

https://reviews.llvm.org/D52200