[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356940: Thread Safety: also look at ObjC methods (authored 
by jfb, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523

Files:
  cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp
  cfe/trunk/test/SemaObjCXX/no-crash-thread-safety-analysis.mm
  cfe/trunk/test/SemaObjCXX/thread-safety-analysis.h
  cfe/trunk/test/SemaObjCXX/warn-thread-safety-analysis.mm

Index: cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp
===
--- cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const DeclContext *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const Decl *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
 }
 // Map the param back to the param of the original function declaration
 // for consistent comparisons.
-VD = FD->getParamDecl(I);
+VD = isa(D)
+ ? cast(D)->getCanonicalDecl()->getParamDecl(I)
+ : cast(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.
Index: cfe/trunk/test/SemaObjCXX/thread-safety-analysis.h
===
--- cfe/trunk/test/SemaObjCXX/thread-safety-analysis.h
+++ cfe/trunk/test/SemaObjCXX/thread-safety-analysis.h
@@ -0,0 +1,17 @@
+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 _;
+};
Index: cfe/trunk/test/SemaObjCXX/no-crash-thread-safety-analysis.mm
===
--- cfe/trunk/test/SemaObjCXX/no-crash-thread-safety-analysis.mm
+++ cfe/trunk/test/SemaObjCXX/no-crash-thread-safety-analysis.mm
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -Wthread-safety -Wno-objc-root-class %s
+
+// Thread safety analysis used to crash when used with ObjC methods.
+
+#include "thread-safety-analysis.h"
+
+@interface MyInterface
+- (void)doIt:(class Lock *)myLock;
+@end
+
+@implementation MyInterface
+- (void)doIt:(class Lock *)myLock {
+  AutoLock lock(*myLock);
+}
+@end
Index: cfe/trunk/test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- cfe/trunk/test/SemaObjCXX/warn-thread-safety-analysis.mm
+++ cfe/trunk/test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -1,22 +1,6 @@
 // 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 _;
-};
+#include "thread-safety-analysis.h"
 
 @interface MyInterface {
 @private
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D59523#1440263 , @aaron.ballman 
wrote:

> In D59523#1440238 , @jfb wrote:
>
> > This is very concrete: this specific code used to cause a crash. This test 
> > has exactly this purpose, nothing more. What I'm doing is extremely normal 
> > for LLVM.
>
>
> Agreed, this is a normal practice for tests verifying that a crash no longer 
> happens.


It certainly makes sense in many components, and I'm not questioning their 
judgement. But in the seven years prior to my involvement, no one thought it 
necessary to create a separate test for crashes in the thread safety analysis, 
and it's not because there haven't been any.

Warnings are in a different situations than the parser, optimizer passes or the 
backend, as they only operate read-only on the AST (or CFG). Looking at the 
commit log for `test/SemaCXX`, the majority of recent commits that fix crashes 
(and say so in the commit message) have been extending existing test cases 
instead of creating new ones. (Take for example rC337766 
, rC338089 
, rC342192 
, rC352047 
.) The thread safety analysis only looks at 
a single function's source-level CFG at a time. So we can reasonably consider 
functions as sufficiently isolated test cases and declarations as their setup.

That this doesn't work for a crash in the backend or parser is completely 
obvious to me. When working there, I would probably do the same and create a 
new file to reproduce my case in isolation. But what makes sense and is 
established practice in one component might not always make sense in another.

I'll leave the judgement to you, I'm just not convinced that this needs to be 
done.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert accepted this revision.
aaronpuchert added a comment.

Alright, go ahead. I don't want this to be held up by such a minor detail.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 191968.
jfb added a comment.

- No verify, no expected.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523

Files:
  lib/Analysis/ThreadSafetyCommon.cpp
  test/SemaObjCXX/no-crash-thread-safety-analysis.mm
  test/SemaObjCXX/thread-safety-analysis.h
  test/SemaObjCXX/warn-thread-safety-analysis.mm

Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- test/SemaObjCXX/warn-thread-safety-analysis.mm
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -1,22 +1,6 @@
 // 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 _;
-};
+#include "thread-safety-analysis.h"
 
 @interface MyInterface {
 @private
Index: test/SemaObjCXX/thread-safety-analysis.h
===
--- /dev/null
+++ test/SemaObjCXX/thread-safety-analysis.h
@@ -0,0 +1,17 @@
+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 _;
+};
Index: test/SemaObjCXX/no-crash-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/no-crash-thread-safety-analysis.mm
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -Wthread-safety -Wno-objc-root-class %s
+
+// Thread safety analysis used to crash when used with ObjC methods.
+
+#include "thread-safety-analysis.h"
+
+@interface MyInterface
+- (void)doIt:(class Lock *)myLock;
+@end
+
+@implementation MyInterface
+- (void)doIt:(class Lock *)myLock {
+  AutoLock lock(*myLock);
+}
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const DeclContext *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const Decl *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
 }
 // Map the param back to the param of the original function declaration
 // for consistent comparisons.
-VD = FD->getParamDecl(I);
+VD = isa(D)
+ ? cast(D)->getCanonicalDecl()->getParamDecl(I)
+ : cast(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D59523#1440263 , @aaron.ballman 
wrote:

> In D59523#1440238 , @jfb wrote:
>
> > In D59523#1440232 , @aaronpuchert 
> > wrote:
> >
> > > > It's less about the regressions and more about the kind of regressions 
> > > > we're testing against.
> > >
> > > But the test also verifies that no diagnostics are omitted (`// 
> > > expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. 
> > > Which is why I think it's a nice seed to build more systematic tests 
> > > around it. I'd generally like to test this more systematically, but that 
> > > isn't made easier if we're sprinkling the test cases over the code base.
> >
> >
> > No, I wrote the test purely to check that the crash is gone. These tests 
> > *require* a diagnostic check, or `// expected-no-diagnostics`, so I put the 
> > later.
>
>
> They only require `// expected-no-diagnostics` because you pass in `-verify` 
> on the RUN line. That isn't needed to test the crashing regression, which may 
> be part of what's causing confusion.


Thanks for pointing this out! I've removed both.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59523#1440238 , @jfb wrote:

> In D59523#1440232 , @aaronpuchert 
> wrote:
>
> > > It's less about the regressions and more about the kind of regressions 
> > > we're testing against.
> >
> > But the test also verifies that no diagnostics are omitted (`// 
> > expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. 
> > Which is why I think it's a nice seed to build more systematic tests around 
> > it. I'd generally like to test this more systematically, but that isn't 
> > made easier if we're sprinkling the test cases over the code base.
>
>
> No, I wrote the test purely to check that the crash is gone. These tests 
> *require* a diagnostic check, or `// expected-no-diagnostics`, so I put the 
> later.


They only require `// expected-no-diagnostics` because you pass in `-verify` on 
the RUN line. That isn't needed to test the crashing regression, which may be 
part of what's causing confusion.

> Agreed that ObjC needs to be tested more systematically, but this patch isn't 
> doing this. It's fixing a crash, and adding a test to make sure the crash is 
> gone.
> 
>>> Basically, this file is here to prevent regressions.
>> 
>> Isn't every test about exactly that? That there was a similar bug in the 
>> past is at best informative, but not necessarily relevant. And if a 
>> functional test crashes, isn't that just as bad? I don't understand why the 
>> historical reason for a test needs to have an influence on where the test is 
>> placed. It makes much more sense to me to group tests by the code they test.
>> 
>> If there is a unit test for a class and you find a bug in it that makes it 
>> crash, would you write a complete new unit test? Or would you add a test 
>> case to the existing test? These files are our “poor man's unit test” for 
>> warnings.
> 
> This is very concrete: this specific code used to cause a crash. This test 
> has exactly this purpose, nothing more. What I'm doing is extremely normal 
> for LLVM.

Agreed, this is a normal practice for tests verifying that a crash no longer 
happens.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Delesley Hutchins via Phabricator via cfe-commits]

delesley accepted this revision.
delesley added a comment.
This revision is now accepted and ready to land.

The if logic does not enhance readability, but I suppose it can't be helped.  
Looks good to me.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D59523#1440232 , @aaronpuchert 
wrote:

> > It's less about the regressions and more about the kind of regressions 
> > we're testing against.
>
> But the test also verifies that no diagnostics are omitted (`// 
> expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. 
> Which is why I think it's a nice seed to build more systematic tests around 
> it. I'd generally like to test this more systematically, but that isn't made 
> easier if we're sprinkling the test cases over the code base.


No, I wrote the test purely to check that the crash is gone. These tests 
*require* a diagnostic check, or `// expected-no-diagnostics`, so I put the 
later.

Agreed that ObjC needs to be tested more systematically, but this patch isn't 
doing this. It's fixing a crash, and adding a test to make sure the crash is 
gone.

>> Basically, this file is here to prevent regressions.
> 
> Isn't every test about exactly that? That there was a similar bug in the past 
> is at best informative, but not necessarily relevant. And if a functional 
> test crashes, isn't that just as bad? I don't understand why the historical 
> reason for a test needs to have an influence on where the test is placed. It 
> makes much more sense to me to group tests by the code they test.
> 
> If there is a unit test for a class and you find a bug in it that makes it 
> crash, would you write a complete new unit test? Or would you add a test case 
> to the existing test? These files are our “poor man's unit test” for warnings.

This is very concrete: this specific code used to cause a crash. This test has 
exactly this purpose, nothing more. What I'm doing is extremely normal for LLVM.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> It's less about the regressions and more about the kind of regressions we're 
> testing against.

But the test also verifies that no diagnostics are omitted (`// 
expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. Which 
is why I think it's a nice seed to build more systematic tests around it. I'd 
generally like to test this more systematically, but that isn't made easier if 
we're sprinkling the test cases over the code base.

> Basically, this file is here to prevent regressions.

Isn't every test about exactly that? That there was a similar bug in the past 
is at best informative, but not necessarily relevant. And if a functional test 
crashes, isn't that just as bad? I don't understand why the historical reason 
for a test needs to have an influence on where the test is placed. It makes 
much more sense to me to group tests by the code they test.

If there is a unit test for a class and you find a bug in it that makes it 
crash, would you write a complete new unit test? Or would you add a test case 
to the existing test? These files are our “poor man's unit test” for warnings.




Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)

jfb wrote:
> aaron.ballman wrote:
> > Same.
> It's a `Decl` (that's canonical), but same.
Depends on the type of `Ctx->AttrDecl`, some derived types of `Decl` have a 
stricter return type for `getCanonicalDecl()`. So I guess it boils down to what 
one thinks is 
[obvious](https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable).
 We're erring on the “be explicit” end of the scale here.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:288
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }

jfb wrote:
> aaronpuchert wrote:
> > Does your test run into this with an `ObjCMethodDecl`? I see how we run 
> > into the assignment to `VD` down below, but I don't understand how we could 
> > get here without annotating a method.
> The problem is in the cast above:
> ```
> (lldb) 
> frame #5: clang::threadSafety::SExprBuilder::translateDeclRefExpr(this, DRE, 
> Ctx) at ThreadSafetyCommon.cpp:280:9
>277  // Function parameters require substitution and/or renaming.
>278  if (const auto *PV = dyn_cast_or_null(VD)) {
>279const auto *FD =
> -> 280
> cast(PV->getDeclContext())->getCanonicalDecl();
>281unsigned I = PV->getFunctionScopeIndex();
>282
>283if (Ctx && Ctx->FunArgs && FD == 
> Ctx->AttrDecl->getCanonicalDecl()) {
> ```
Ok. I just thought it would be interesting to see if can we run into the `if`, 
but I can try that myself.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:280
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const auto *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {

aaron.ballman wrote:
> Please don't use `auto` when the type is not explicitly spelled out in the 
> initialization.
It's a `DeclContext` as spelled in `getDeclContext`, but sure I can drop the 
`auto` nonetheless.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)

aaron.ballman wrote:
> Same.
It's a `Decl` (that's canonical), but same.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:288
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }

aaronpuchert wrote:
> Does your test run into this with an `ObjCMethodDecl`? I see how we run into 
> the assignment to `VD` down below, but I don't understand how we could get 
> here without annotating a method.
The problem is in the cast above:
```
(lldb) 
frame #5: clang::threadSafety::SExprBuilder::translateDeclRefExpr(this, DRE, 
Ctx) at ThreadSafetyCommon.cpp:280:9
   277// Function parameters require substitution and/or renaming.
   278if (const auto *PV = dyn_cast_or_null(VD)) {
   279  const auto *FD =
-> 280  cast(PV->getDeclContext())->getCanonicalDecl();
   281  unsigned I = PV->getFunctionScopeIndex();
   282  
   283  if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) 
{
```


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 191939.
jfb marked 13 inline comments as done.
jfb added a comment.

- Almost Never Auto.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523

Files:
  lib/Analysis/ThreadSafetyCommon.cpp
  test/SemaObjCXX/no-crash-thread-safety-analysis.mm
  test/SemaObjCXX/thread-safety-analysis.h
  test/SemaObjCXX/warn-thread-safety-analysis.mm

Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- test/SemaObjCXX/warn-thread-safety-analysis.mm
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -1,22 +1,6 @@
 // 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 _;
-};
+#include "thread-safety-analysis.h"
 
 @interface MyInterface {
 @private
Index: test/SemaObjCXX/thread-safety-analysis.h
===
--- /dev/null
+++ test/SemaObjCXX/thread-safety-analysis.h
@@ -0,0 +1,17 @@
+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 _;
+};
Index: test/SemaObjCXX/no-crash-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/no-crash-thread-safety-analysis.mm
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s
+
+// expected-no-diagnostics
+
+// Thread safety analysis used to crash when used with ObjC methods.
+
+#include "thread-safety-analysis.h"
+
+@interface MyInterface
+- (void)doIt:(class Lock *)myLock;
+@end
+
+@implementation MyInterface
+- (void)doIt:(class Lock *)myLock {
+  AutoLock lock(*myLock);
+}
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const DeclContext *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const Decl *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
 }
 // Map the param back to the param of the original function declaration
 // for consistent comparisons.
-VD = FD->getParamDecl(I);
+VD = isa(D)
+ ? cast(D)->getCanonicalDecl()->getParamDecl(I)
+ : cast(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Creating a new test makes sense to me if it tests things across components. We 
have such tests for modules, PCH, and templates. There are also separate tests 
for the attribute parsing, which doesn't work terribly well in ObjC either. I 
would agree to making a new test for that when someone fixes PR38896.




Comment at: lib/Analysis/ThreadSafetyCommon.cpp:288
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }

Does your test run into this with an `ObjCMethodDecl`? I see how we run into 
the assignment to `VD` down below, but I don't understand how we could get here 
without annotating a method.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

aaron.ballman wrote:
> jfb wrote:
> > aaron.ballman wrote:
> > > jfb wrote:
> > > > aaron.ballman wrote:
> > > > > aaronpuchert wrote:
> > > > > > jfb wrote:
> > > > > > > aaronpuchert wrote:
> > > > > > > > Test is fine for me, but I would like if you could integrate it 
> > > > > > > > with the existing 
> > > > > > > > test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread 
> > > > > > > > safety analysis requires a bit of setup, that's why we tend to 
> > > > > > > > keep the tests in one file. I'll admit that the C++ tests have 
> > > > > > > > grown quite large, but for ObjC++ it's still very manageable. 
> > > > > > > Sure thing! I created a header that's shared and simplified this 
> > > > > > > repro a bit. I validated that the shared code was crashing before 
> > > > > > > and this fixes the crash.
> > > > > > I would still like all ObjCXX tests for -Wthread-safety in one 
> > > > > > file, because that's what we have for the other languages as well. 
> > > > > > (more or less)
> > > > > > 
> > > > > > It's somewhat easier to see which aspects are tested then and which 
> > > > > > are not. Right now we only have tests for reported crashes, but 
> > > > > > maybe someday someone finds the time to test a bit more 
> > > > > > exhaustively.
> > > > > > 
> > > > > > But perhaps @aaron.ballman sees this another way, in that case 
> > > > > > follow his opinion.
> > > > > On one hand, the all-in-one test files get to be unwieldy size, but 
> > > > > on the other hand, it's nice having everything integrated into a 
> > > > > single file for testing purposes. I'd say go with the all-in-one 
> > > > > approach because it's consistent with how we otherwise test thread 
> > > > > safety and there are some (albeit, minor) benefits.
> > > > I'm not a fan of huge tests, and it's not consistent with other parts 
> > > > of LLVM. Specifically, this is testing for a regression, and should 
> > > > standalone to future edits don't refactor the test away. The existing 
> > > > test checks for functionality, and refactoring it is fine.
> > > I'm not going to insist, but the two people who do the most active work 
> > > in this area in recent history just told you what they prefer. ;-) We add 
> > > regression tests to the existing thread safety test files and it's worked 
> > > out just fine.
> > 4% of files under clang/test (and llvm/test) start with "PR".  I appreciate 
> > your being gentle... and I'll gently nudge folks working on thread safety 
> > to follow the established process clang and LLVM follow ;-)
> > Maybe I should file a bug to follow said process myself?
> > 
> > Basically, this file is here to prevent regressions. It really shouldn't 
> > change, ever, and I'd like to make this explicit.
> Thanks to an IRC discussion, I now understand the point @jfb is making about 
> this being a crashing regression test. You're right, we don't usually 
> incorporate those into larger functionality tests. It's less about the 
> regressions and more about the kind of regressions we're testing against.
> 
> I'm fine with the approach in this patch now.
The other test is not really a functional test, I added it in rC342600 as part 
of a fix for another bug that was reported as 
[PR38896](https://bugs.llvm.org/show_bug.cgi?id=38896). But the underlying 
cause for both bugs is not an oversight, it's that ObjC[++] is just not 
supported, and someone was too lazy to completely turn the analysis off or 
disallow it. Now people try it out and unsurprisingly it doesn't work. Your 
test is not some weird special case, so it could become part of a more 
systematic functional test.

In Clang it's quite usual to have one test per warning flag, and the thread 
safety analysis is just one flag. I guess the reason is that it makes 
refactoring easier: you see all the special cases in one file. That's quite 
nice! Have a look at some of these tests, they basically allow you to write the 
warning yourself. If all these special cases were spread around the code base, 
that would certainly make it harder to understand what a warning does.

The test for C++ also contains tests for at least two bugs: PR34800, PR38640. 
(I just grepped this, there is probably more. You can try `grep PR 
test/SemaCXX/warn-*`.) If you fear someone is going to change the test, rename 
your class from `MyInterface` to `PRX` or something else which indicates a 
crash.

Also indicate the issue it detects: that ObjC methods weren't handled in 
`SExprBuilder::translateDeclRefExpr`. Or that we test that they are handled 
now. 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

jfb wrote:
> aaron.ballman wrote:
> > jfb wrote:
> > > aaron.ballman wrote:
> > > > aaronpuchert wrote:
> > > > > jfb wrote:
> > > > > > aaronpuchert wrote:
> > > > > > > Test is fine for me, but I would like if you could integrate it 
> > > > > > > with the existing test/SemaObjCXX/warn-thread-safety-analysis.mm. 
> > > > > > > The thread safety analysis requires a bit of setup, that's why we 
> > > > > > > tend to keep the tests in one file. I'll admit that the C++ tests 
> > > > > > > have grown quite large, but for ObjC++ it's still very 
> > > > > > > manageable. 
> > > > > > Sure thing! I created a header that's shared and simplified this 
> > > > > > repro a bit. I validated that the shared code was crashing before 
> > > > > > and this fixes the crash.
> > > > > I would still like all ObjCXX tests for -Wthread-safety in one file, 
> > > > > because that's what we have for the other languages as well. (more or 
> > > > > less)
> > > > > 
> > > > > It's somewhat easier to see which aspects are tested then and which 
> > > > > are not. Right now we only have tests for reported crashes, but maybe 
> > > > > someday someone finds the time to test a bit more exhaustively.
> > > > > 
> > > > > But perhaps @aaron.ballman sees this another way, in that case follow 
> > > > > his opinion.
> > > > On one hand, the all-in-one test files get to be unwieldy size, but on 
> > > > the other hand, it's nice having everything integrated into a single 
> > > > file for testing purposes. I'd say go with the all-in-one approach 
> > > > because it's consistent with how we otherwise test thread safety and 
> > > > there are some (albeit, minor) benefits.
> > > I'm not a fan of huge tests, and it's not consistent with other parts of 
> > > LLVM. Specifically, this is testing for a regression, and should 
> > > standalone to future edits don't refactor the test away. The existing 
> > > test checks for functionality, and refactoring it is fine.
> > I'm not going to insist, but the two people who do the most active work in 
> > this area in recent history just told you what they prefer. ;-) We add 
> > regression tests to the existing thread safety test files and it's worked 
> > out just fine.
> 4% of files under clang/test (and llvm/test) start with "PR".  I appreciate 
> your being gentle... and I'll gently nudge folks working on thread safety to 
> follow the established process clang and LLVM follow ;-)
> Maybe I should file a bug to follow said process myself?
> 
> Basically, this file is here to prevent regressions. It really shouldn't 
> change, ever, and I'd like to make this explicit.
Thanks to an IRC discussion, I now understand the point @jfb is making about 
this being a crashing regression test. You're right, we don't usually 
incorporate those into larger functionality tests. It's less about the 
regressions and more about the kind of regressions we're testing against.

I'm fine with the approach in this patch now.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb marked an inline comment as done.
jfb added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

aaron.ballman wrote:
> jfb wrote:
> > aaron.ballman wrote:
> > > aaronpuchert wrote:
> > > > jfb wrote:
> > > > > aaronpuchert wrote:
> > > > > > Test is fine for me, but I would like if you could integrate it 
> > > > > > with the existing test/SemaObjCXX/warn-thread-safety-analysis.mm. 
> > > > > > The thread safety analysis requires a bit of setup, that's why we 
> > > > > > tend to keep the tests in one file. I'll admit that the C++ tests 
> > > > > > have grown quite large, but for ObjC++ it's still very manageable. 
> > > > > Sure thing! I created a header that's shared and simplified this 
> > > > > repro a bit. I validated that the shared code was crashing before and 
> > > > > this fixes the crash.
> > > > I would still like all ObjCXX tests for -Wthread-safety in one file, 
> > > > because that's what we have for the other languages as well. (more or 
> > > > less)
> > > > 
> > > > It's somewhat easier to see which aspects are tested then and which are 
> > > > not. Right now we only have tests for reported crashes, but maybe 
> > > > someday someone finds the time to test a bit more exhaustively.
> > > > 
> > > > But perhaps @aaron.ballman sees this another way, in that case follow 
> > > > his opinion.
> > > On one hand, the all-in-one test files get to be unwieldy size, but on 
> > > the other hand, it's nice having everything integrated into a single file 
> > > for testing purposes. I'd say go with the all-in-one approach because 
> > > it's consistent with how we otherwise test thread safety and there are 
> > > some (albeit, minor) benefits.
> > I'm not a fan of huge tests, and it's not consistent with other parts of 
> > LLVM. Specifically, this is testing for a regression, and should standalone 
> > to future edits don't refactor the test away. The existing test checks for 
> > functionality, and refactoring it is fine.
> I'm not going to insist, but the two people who do the most active work in 
> this area in recent history just told you what they prefer. ;-) We add 
> regression tests to the existing thread safety test files and it's worked out 
> just fine.
4% of files under clang/test (and llvm/test) start with "PR".  I appreciate 
your being gentle... and I'll gently nudge folks working on thread safety to 
follow the established process clang and LLVM follow ;-)
Maybe I should file a bug to follow said process myself?

Basically, this file is here to prevent regressions. It really shouldn't 
change, ever, and I'd like to make this explicit.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

jfb wrote:
> aaron.ballman wrote:
> > aaronpuchert wrote:
> > > jfb wrote:
> > > > aaronpuchert wrote:
> > > > > Test is fine for me, but I would like if you could integrate it with 
> > > > > the existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The 
> > > > > thread safety analysis requires a bit of setup, that's why we tend to 
> > > > > keep the tests in one file. I'll admit that the C++ tests have grown 
> > > > > quite large, but for ObjC++ it's still very manageable. 
> > > > Sure thing! I created a header that's shared and simplified this repro 
> > > > a bit. I validated that the shared code was crashing before and this 
> > > > fixes the crash.
> > > I would still like all ObjCXX tests for -Wthread-safety in one file, 
> > > because that's what we have for the other languages as well. (more or 
> > > less)
> > > 
> > > It's somewhat easier to see which aspects are tested then and which are 
> > > not. Right now we only have tests for reported crashes, but maybe someday 
> > > someone finds the time to test a bit more exhaustively.
> > > 
> > > But perhaps @aaron.ballman sees this another way, in that case follow his 
> > > opinion.
> > On one hand, the all-in-one test files get to be unwieldy size, but on the 
> > other hand, it's nice having everything integrated into a single file for 
> > testing purposes. I'd say go with the all-in-one approach because it's 
> > consistent with how we otherwise test thread safety and there are some 
> > (albeit, minor) benefits.
> I'm not a fan of huge tests, and it's not consistent with other parts of 
> LLVM. Specifically, this is testing for a regression, and should standalone 
> to future edits don't refactor the test away. The existing test checks for 
> functionality, and refactoring it is fine.
I'm not going to insist, but the two people who do the most active work in this 
area in recent history just told you what they prefer. ;-) We add regression 
tests to the existing thread safety test files and it's worked out just fine.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb marked an inline comment as done.
jfb added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

aaron.ballman wrote:
> aaronpuchert wrote:
> > jfb wrote:
> > > aaronpuchert wrote:
> > > > Test is fine for me, but I would like if you could integrate it with 
> > > > the existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread 
> > > > safety analysis requires a bit of setup, that's why we tend to keep the 
> > > > tests in one file. I'll admit that the C++ tests have grown quite 
> > > > large, but for ObjC++ it's still very manageable. 
> > > Sure thing! I created a header that's shared and simplified this repro a 
> > > bit. I validated that the shared code was crashing before and this fixes 
> > > the crash.
> > I would still like all ObjCXX tests for -Wthread-safety in one file, 
> > because that's what we have for the other languages as well. (more or less)
> > 
> > It's somewhat easier to see which aspects are tested then and which are 
> > not. Right now we only have tests for reported crashes, but maybe someday 
> > someone finds the time to test a bit more exhaustively.
> > 
> > But perhaps @aaron.ballman sees this another way, in that case follow his 
> > opinion.
> On one hand, the all-in-one test files get to be unwieldy size, but on the 
> other hand, it's nice having everything integrated into a single file for 
> testing purposes. I'd say go with the all-in-one approach because it's 
> consistent with how we otherwise test thread safety and there are some 
> (albeit, minor) benefits.
I'm not a fan of huge tests, and it's not consistent with other parts of LLVM. 
Specifically, this is testing for a regression, and should standalone to future 
edits don't refactor the test away. The existing test checks for functionality, 
and refactoring it is fine.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

aaronpuchert wrote:
> jfb wrote:
> > aaronpuchert wrote:
> > > Test is fine for me, but I would like if you could integrate it with the 
> > > existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread 
> > > safety analysis requires a bit of setup, that's why we tend to keep the 
> > > tests in one file. I'll admit that the C++ tests have grown quite large, 
> > > but for ObjC++ it's still very manageable. 
> > Sure thing! I created a header that's shared and simplified this repro a 
> > bit. I validated that the shared code was crashing before and this fixes 
> > the crash.
> I would still like all ObjCXX tests for -Wthread-safety in one file, because 
> that's what we have for the other languages as well. (more or less)
> 
> It's somewhat easier to see which aspects are tested then and which are not. 
> Right now we only have tests for reported crashes, but maybe someday someone 
> finds the time to test a bit more exhaustively.
> 
> But perhaps @aaron.ballman sees this another way, in that case follow his 
> opinion.
On one hand, the all-in-one test files get to be unwieldy size, but on the 
other hand, it's nice having everything integrated into a single file for 
testing purposes. I'd say go with the all-in-one approach because it's 
consistent with how we otherwise test thread safety and there are some (albeit, 
minor) benefits.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:283-285
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {

aaron.ballman wrote:
> Also somewhat orthogonal to your changes, but... ugh, the lack of any common 
> base between `FunctionDecl` and `ObjcMethodDecl` strikes again. I sort of 
> wish we would introduce a CallableDecl base class that these two (and 
> BlockDecl) could all inherit from to take care of this sort of ugly hack.
Seems I was wrong, because `getCanonicalDecl()` is actually virtual. I was 
confused by the different return type, apparently an overriding function 
doesn't need to have the same return type. So no problem here.

This doesn't make our case easier: both `FunctionDecl` and `ObjCMethodDecl` 
inherit from both `Decl` and `DeclContext`, but independently. So we can't 
directly cast from `DeclContext` to `Decl`, we need to know which leaf we are.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

jfb wrote:
> aaronpuchert wrote:
> > Test is fine for me, but I would like if you could integrate it with the 
> > existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread safety 
> > analysis requires a bit of setup, that's why we tend to keep the tests in 
> > one file. I'll admit that the C++ tests have grown quite large, but for 
> > ObjC++ it's still very manageable. 
> Sure thing! I created a header that's shared and simplified this repro a bit. 
> I validated that the shared code was crashing before and this fixes the crash.
I would still like all ObjCXX tests for -Wthread-safety in one file, because 
that's what we have for the other languages as well. (more or less)

It's somewhat easier to see which aspects are tested then and which are not. 
Right now we only have tests for reported crashes, but maybe someday someone 
finds the time to test a bit more exhaustively.

But perhaps @aaron.ballman sees this another way, in that case follow his 
opinion.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: delesley.
aaron.ballman added a comment.

Adding Delesley in case he has input on design.




Comment at: lib/Analysis/ThreadSafetyCommon.cpp:280
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const auto *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {

Please don't use `auto` when the type is not explicitly spelled out in the 
initialization.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)

Same.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:283-285
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {

Also somewhat orthogonal to your changes, but... ugh, the lack of any common 
base between `FunctionDecl` and `ObjcMethodDecl` strikes again. I sort of wish 
we would introduce a CallableDecl base class that these two (and BlockDecl) 
could all inherit from to take care of this sort of ugly hack.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb marked an inline comment as done.
jfb added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

aaronpuchert wrote:
> Test is fine for me, but I would like if you could integrate it with the 
> existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread safety 
> analysis requires a bit of setup, that's why we tend to keep the tests in one 
> file. I'll admit that the C++ tests have grown quite large, but for ObjC++ 
> it's still very manageable. 
Sure thing! I created a header that's shared and simplified this repro a bit. I 
validated that the shared code was crashing before and this fixes the crash.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 191580.
jfb marked 3 inline comments as done.
jfb added a comment.

- Simlify tests, share code


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523

Files:
  lib/Analysis/ThreadSafetyCommon.cpp
  test/SemaObjCXX/no-crash-thread-safety-analysis.mm
  test/SemaObjCXX/thread-safety-analysis.h
  test/SemaObjCXX/warn-thread-safety-analysis.mm

Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- test/SemaObjCXX/warn-thread-safety-analysis.mm
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -1,22 +1,6 @@
 // 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 _;
-};
+#include "thread-safety-analysis.h"
 
 @interface MyInterface {
 @private
Index: test/SemaObjCXX/thread-safety-analysis.h
===
--- /dev/null
+++ test/SemaObjCXX/thread-safety-analysis.h
@@ -0,0 +1,17 @@
+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 _;
+};
Index: test/SemaObjCXX/no-crash-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/no-crash-thread-safety-analysis.mm
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s
+
+// expected-no-diagnostics
+
+// Thread safety analysis used to crash when used with ObjC methods.
+
+#include "thread-safety-analysis.h"
+
+@interface MyInterface
+- (void)doIt:(class Lock *)myLock;
+@end
+
+@implementation MyInterface
+- (void)doIt:(class Lock *)myLock {
+  AutoLock lock(*myLock);
+}
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const auto *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
 }
 // Map the param back to the param of the original function declaration
 // for consistent comparisons.
-VD = FD->getParamDecl(I);
+VD = isa(D)
+ ? cast(D)->getCanonicalDecl()->getParamDecl(I)
+ : cast(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a reviewer: aaronpuchert.
aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282-285
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {

Unrelated to your change, but I'm wondering if this is right. `Ctx->AttrDecl` 
is a `NamedDecl`, on which `getCanonicalDecl()` just returns `*this` (so we 
could in fact omit it here without functional change), while on `FunctionDecl` 
and `ObjCMethodDecl` it does something non-trivial instead.

I guess I need to look into this a bit, don't let this bother you. But I think 
we might actually have to do the cast on both sides of the equation to get the 
same kind of canonical declaration. Or we make sure that `Ctx->AttrDecl` is 
already canonical.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

Test is fine for me, but I would like if you could integrate it with the 
existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread safety 
analysis requires a bit of setup, that's why we tend to keep the tests in one 
file. I'll admit that the C++ tests have grown quite large, but for ObjC++ it's 
still very manageable. 



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:10
+
+template  struct __attribute__((scoped_lockable)) Locker {
+  T &_l;

Can we hard-code `T` = `MyLock`? We can still change it if we need it more 
general later.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 191417.
jfb added a comment.

- Add test


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523

Files:
  lib/Analysis/ThreadSafetyCommon.cpp
  test/SemaObjCXX/no-crash-thread-safety-analysis.mm


Index: test/SemaObjCXX/no-crash-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/no-crash-thread-safety-analysis.mm
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+
+// expected-no-diagnostics
+
+struct __attribute__((capability("mutex"))) MyLock {
+  void lock() __attribute__((acquire_capability())) {}
+  void unlock() __attribute__((release_capability())) {}
+};
+
+template  struct __attribute__((scoped_lockable)) Locker {
+  T &_l;
+  Locker(T ) __attribute__((acquire_capability(l))) : _l(l) { _l.lock(); }
+  ~Locker() __attribute__((release_capability())) { _l.unlock(); }
+};
+
+struct MyLockable {
+  MyLock lock;
+};
+
+@protocol MyProtocolBase;
+@protocol MyProtocol 
+@end
+
+@interface MyProtocol
+@end
+
+@interface MyProtocol ()
+- (void)doIt:(struct MyLockable *)myLockable;
+@end
+
+@implementation MyProtocol
+- (void)doIt:(struct MyLockable *)myLockable {
+  Locker lock(myLockable->lock);
+}
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const auto *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
 }
 // Map the param back to the param of the original function declaration
 // for consistent comparisons.
-VD = FD->getParamDecl(I);
+VD = isa(D)
+ ? cast(D)->getCanonicalDecl()->getParamDecl(I)
+ : cast(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.


Index: test/SemaObjCXX/no-crash-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/no-crash-thread-safety-analysis.mm
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s
+
+// expected-no-diagnostics
+
+struct __attribute__((capability("mutex"))) MyLock {
+  void lock() __attribute__((acquire_capability())) {}
+  void unlock() __attribute__((release_capability())) {}
+};
+
+template  struct __attribute__((scoped_lockable)) Locker {
+  T &_l;
+  Locker(T ) __attribute__((acquire_capability(l))) : _l(l) { _l.lock(); }
+  ~Locker() __attribute__((release_capability())) { _l.unlock(); }
+};
+
+struct MyLockable {
+  MyLock lock;
+};
+
+@protocol MyProtocolBase;
+@protocol MyProtocol 
+@end
+
+@interface MyProtocol
+@end
+
+@interface MyProtocol ()
+- (void)doIt:(struct MyLockable *)myLockable;
+@end
+
+@implementation MyProtocol
+- (void)doIt:(struct MyLockable *)myLockable {
+  Locker lock(myLockable->lock);
+}
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const auto *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < 

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

Actually I'm wrong, this repros properly, will send an update with test.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I reduced the code I had to this:

  struct __attribute__((capability("mutex"))) MyLock {
void lock() __attribute__((acquire_capability())) {}
void unlock() __attribute__((release_capability())) {}
  };
  
  template  struct __attribute__((scoped_lockable)) Locker {
T &_l;
Locker(T ) __attribute__((acquire_capability(l))) : _l(l) { _l.lock(); }
~Locker() __attribute__((release_capability())) { _l.unlock(); }
  };
  
  struct MyLockable {
MyLock lock;
  };
  
  @protocol MyProtocolBase;
  @protocol MyProtocol 
  @end
  
  @interface MyProtocol
  @end
  
  @interface MyProtocol ()
  - (void)doIt:(struct MyLockable *)myLockable;
  @end
  
  @implementation MyProtocol
  - (void)doIt:(struct MyLockable *)myLockable {
Locker lock(myLockable->lock);
  }
  @end

But now it doesn't repro... I probably missed something important :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 191416.
jfb added a comment.

- Use suggested format.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523

Files:
  lib/Analysis/ThreadSafetyCommon.cpp


Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const auto *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
 }
 // Map the param back to the param of the original function declaration
 // for consistent comparisons.
-VD = FD->getParamDecl(I);
+VD = isa(D)
+ ? cast(D)->getCanonicalDecl()->getParamDecl(I)
+ : cast(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.


Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,23 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
 unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+const auto *D = PV->getDeclContext();
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
 }
 // Map the param back to the param of the original function declaration
 // for consistent comparisons.
-VD = FD->getParamDecl(I);
+VD = isa(D)
+ ? cast(D)->getCanonicalDecl()->getParamDecl(I)
+ : cast(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> It should consider both because the attributes can be used on Objective-C as 
> well.

Well, it's not really supported that well though. There are known bugs like 
https://bugs.llvm.org/show_bug.cgi?id=38892, and I don't really have the time 
to fix that. (You're free to jump in of course.)

> It seems weird how both do pretty similar things and e.g. duplicate 
> getParamDecl.

I have no idea about ObjC at all, but I believe that ObjC methods are 
substantially different from ordinary functions. (Unlike C++ methods, which 
just have an additional implicit parameter.)

You could do something like `SExprBuilder::enterCFG`:

  auto Parms = isa(D) ? cast(D)->parameters()
  : cast(D)->parameters();



> The repro I have is 20M, creduce chokes on it, and manually reducing failed 
> to repro... ☹️

You could try declaring an ObjC method with a thread safety attribute that 
refers to a parameter, then somewhere else call that function. Again, I don't 
know ObjC, but for a C function you would do:

  struct Data { Mutex mu; }
  void f(struct Data *data) __attribute__ 
((exclusive_locks_required(data->mu)));
  
  void g() {
Data d;
Lock();
f();  // Should only work with your change.
Unlock();
  }

Then the SExprBuilder should have to replace the reference to the parameter 
(here `data`) by the value it is called with (here ``). I'll have to admit I 
don't find it easy to wrap my head around this logic either, so maybe I'm 
missing something. The tests for ObjC[++] are very brief, but you can look at 
the C++ tests for hints. (See 
`test/Sema{ObjC,ObjC++,C++}/warn-thread-safety-analysis.{m,mm,cpp}`.)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D59523#1434121 , @erik.pilkington 
wrote:

> I don't understand this code at all, but what about `BlockDecl`?


Agreed that it has `getParamDecl` as well, and the context would fit. It just 
seems weird how stuff is repeated, Iw as hoping someone would have some insight 
I failed at gaining through inspection of the code!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

I don't understand this code at all, but what about `BlockDecl`?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb created this revision.
jfb added reviewers: dexonsmith, erik.pilkington.
Herald added subscribers: cfe-commits, jdoerfert, jkorous.
Herald added a project: clang.
jfb added a comment.

It seems weird how both do pretty similar things and e.g. duplicate 
`getParamDecl`.

The repro I have is 20M, creduce chokes on it, and manually reducing failed to 
repro... ☹️


SExprBuilder::translateDeclRefExpr was only looking at FunctionDecl and not 
also looking at ObjCMethodDecl. It should consider both because the attributes 
can be used on Objective-C as well.

rdar://problem/48941331


Repository:
  rC Clang

https://reviews.llvm.org/D59523

Files:
  lib/Analysis/ThreadSafetyCommon.cpp


Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,31 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
-unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+if (isa(PV->getDeclContext())) {
+  unsigned I = PV->getFunctionScopeIndex();
+  const auto *FD =
+  cast(PV->getDeclContext())->getCanonicalDecl();
+  if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
+  // Map the param back to the param of the original function declaration
+  // for consistent comparisons.
+  VD = FD->getParamDecl(I);
+} else {
+  unsigned I = PV->getFunctionScopeIndex();
+  const auto *FD =
+  cast(PV->getDeclContext())->getCanonicalDecl();
+  if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
+  // Map the param back to the param of the original function declaration
+  // for consistent comparisons.
+  VD = FD->getParamDecl(I);
 }
-// Map the param back to the param of the original function declaration
-// for consistent comparisons.
-VD = FD->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.


Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -276,18 +276,31 @@
 
   // Function parameters require substitution and/or renaming.
   if (const auto *PV = dyn_cast_or_null(VD)) {
-const auto *FD =
-cast(PV->getDeclContext())->getCanonicalDecl();
-unsigned I = PV->getFunctionScopeIndex();
-
-if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
-  // Substitute call arguments for references to function parameters
-  assert(I < Ctx->NumArgs);
-  return translate(Ctx->FunArgs[I], Ctx->Prev);
+if (isa(PV->getDeclContext())) {
+  unsigned I = PV->getFunctionScopeIndex();
+  const auto *FD =
+  cast(PV->getDeclContext())->getCanonicalDecl();
+  if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
+  // Map the param back to the param of the original function declaration
+  // for consistent comparisons.
+  VD = FD->getParamDecl(I);
+} else {
+  unsigned I = PV->getFunctionScopeIndex();
+  const auto *FD =
+  cast(PV->getDeclContext())->getCanonicalDecl();
+  if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) {
+// Substitute call arguments for references to function parameters
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }
+  // Map the param back to the param of the original function declaration
+  // for consistent comparisons.
+  VD = FD->getParamDecl(I);
 }
-// Map the param back to the param of the original function declaration
-// for consistent comparisons.
-VD = FD->getParamDecl(I);
   }
 
   // For non-local variables, treat it as a reference to a named object.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59523: Thread Safety: also look at ObjC methods

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

It seems weird how both do pretty similar things and e.g. duplicate 
`getParamDecl`.

The repro I have is 20M, creduce chokes on it, and manually reducing failed to 
repro... ☹️


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59523/new/

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits