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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
