[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
This revision was automatically updated to reflect the committed changes.
Closed by commit rC342600: Thread safety analysis: Handle ObjCIvarRefExpr in
SExprBuilder::translate (authored by aaronpuchert, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D52200?vs=166051=166197#toc
Repository:
rC Clang
https://reviews.llvm.org/D52200
Files:
include/clang/Analysis/Analyses/ThreadSafetyCommon.h
lib/Analysis/ThreadSafetyCommon.cpp
test/SemaObjCXX/warn-thread-safety-analysis.mm
Index: include/clang/Analysis/Analyses/ThreadSafetyCommon.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -397,6 +397,8 @@
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,
Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- test/SemaObjCXX/warn-thread-safety-analysis.mm
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// 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 ) __attribute__((exclusive_lock_function(lock)))
+ : lock_(lock) {
+lock.Acquire();
+ }
+ ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+ 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
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -211,6 +211,8 @@
return translateCXXThisExpr(cast(S), Ctx);
case Stmt::MemberExprClass:
return translateMemberExpr(cast(S), Ctx);
+ case Stmt::ObjCIvarRefExprClass:
+return translateObjCIVarRefExpr(cast(S), Ctx);
case Stmt::CallExprClass:
return translateCallExpr(cast(S), Ctx);
case Stmt::CXXMemberCallExprClass:
@@ -311,9 +313,9 @@
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(E))
return C->castOpcode() == til::CAST_objToPtr;
@@ -344,7 +346,20 @@
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(IVRE->getDecl()->getCanonicalDecl());
+
+ til::Project *P = new (Arena) til::Project(E, D);
+ if (hasAnyPointerType(BE))
P->setArrow(true);
return P;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
aaronpuchert added a comment. 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. 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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
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 ()->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
()->mu instead of foo.mu, when substituting 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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
rjmccall added a comment. In https://reviews.llvm.org/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. 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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.
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.
Comment at: test/SemaObjCXX/warn-thread-safety-analysis.mm:42
+ value_ -= 1;
+} // expected-warning{{mutex 'self->lock_' is still held at the end of
function}}
+
@rjmccall Would you rather write `self->lock_` or `lock_` in the warning
message?
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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
aaronpuchert updated this revision to Diff 166051.
aaronpuchert added a comment.
Detect ObjC pointer types as well as ordinary pointers.
Repository:
rC Clang
https://reviews.llvm.org/D52200
Files:
include/clang/Analysis/Analyses/ThreadSafetyCommon.h
lib/Analysis/ThreadSafetyCommon.cpp
test/SemaObjCXX/warn-thread-safety-analysis.mm
Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// 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 ) __attribute__((exclusive_lock_function(lock)))
+ : lock_(lock) {
+lock.Acquire();
+ }
+ ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+ 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
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -211,6 +211,8 @@
return translateCXXThisExpr(cast(S), Ctx);
case Stmt::MemberExprClass:
return translateMemberExpr(cast(S), Ctx);
+ case Stmt::ObjCIvarRefExprClass:
+return translateObjCIVarRefExpr(cast(S), Ctx);
case Stmt::CallExprClass:
return translateCallExpr(cast(S), Ctx);
case Stmt::CXXMemberCallExprClass:
@@ -311,9 +313,9 @@
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(E))
return C->castOpcode() == til::CAST_objToPtr;
@@ -344,7 +346,20 @@
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(IVRE->getDecl()->getCanonicalDecl());
+
+ til::Project *P = new (Arena) til::Project(E, D);
+ if (hasAnyPointerType(BE))
P->setArrow(true);
return P;
}
Index: include/clang/Analysis/Analyses/ThreadSafetyCommon.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -397,6 +397,8 @@
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,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
aaronpuchert added a comment.
I found something that would theoretically work:
P->setArrow((isa(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(); // \
// 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 `()->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 `()->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 `*`.
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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
rjmccall 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); 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. 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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
aaron.ballman added a reviewer: rjmccall. aaron.ballman added a comment. Adding a reviewer who knows more about ObjC than I do. 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); `ME` is kind of a poor name; can you switch to `IVRE` like you used in the implementation? Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362 + til::Project *P = new (Arena) til::Project(E, D); + if (hasCppPointerType(BE)) +P->setArrow(true); I feel like this will always return false. However, I wonder if `IVRE->getBase()->isArrow()` is more appropriate 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
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley, lukasza.
Herald added a subscriber: cfe-commits.
This imitates the code for MemberExpr. I hope it is right, for I have
absolutely no understanding of ObjC++.
Fixes 38896.
Repository:
rC Clang
https://reviews.llvm.org/D52200
Files:
include/clang/Analysis/Analyses/ThreadSafetyCommon.h
lib/Analysis/ThreadSafetyCommon.cpp
test/SemaObjCXX/warn-thread-safety-analysis.mm
Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// 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 ) __attribute__((exclusive_lock_function(lock)))
+ : lock_(lock) {
+lock.Acquire();
+ }
+ ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+ 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
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -211,6 +211,8 @@
return translateCXXThisExpr(cast(S), Ctx);
case Stmt::MemberExprClass:
return translateMemberExpr(cast(S), Ctx);
+ case Stmt::ObjCIvarRefExprClass:
+return translateObjCIVarRefExpr(cast(S), Ctx);
case Stmt::CallExprClass:
return translateCallExpr(cast(S), Ctx);
case Stmt::CXXMemberCallExprClass:
@@ -349,6 +351,19 @@
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(IVRE->getDecl()->getCanonicalDecl());
+
+ til::Project *P = new (Arena) til::Project(E, D);
+ if (hasCppPointerType(BE))
+P->setArrow(true);
+ return P;
+}
+
til::SExpr *SExprBuilder::translateCallExpr(const CallExpr *CE,
CallingContext *Ctx,
const Expr *SelfE) {
Index: include/clang/Analysis/Analyses/ThreadSafetyCommon.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -397,6 +397,8 @@
CallingContext *Ctx) ;
til::SExpr *translateCXXThisExpr(const CXXThisExpr *TE, CallingContext *Ctx);
til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx);
+ til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *ME,
+ CallingContext *Ctx);
til::SExpr *translateCallExpr(const CallExpr *CE, CallingContext *Ctx,
const Expr *SelfE = nullptr);
til::SExpr *translateCXXMemberCallExpr(const CXXMemberCallExpr *ME,
Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// 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 ) __attribute__((exclusive_lock_function(lock)))
+ : lock_(lock) {
+lock.Acquire();
+ }
+ ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+ 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
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
---
