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

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added inline comments.


================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362
+  til::Project *P = new (Arena) til::Project(E, D);
+  if (hasCppPointerType(BE))
+    P->setArrow(true);
----------------
aaronpuchert wrote:
> 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.
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.


Repository:
  rC Clang

https://reviews.llvm.org/D52200