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