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

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

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

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

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

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

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

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

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

jfb added a comment. In D59523#1440263 , @aaron.ballman wrote: > In D59523#1440238 , @jfb wrote: > > > In D59523#1440232 , @aaronpuchert > > wrote: > > > > > > It's less

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

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

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

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/

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

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 (`// >

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

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

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

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 -

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

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

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

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

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

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:

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

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:

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

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: > >

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

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:

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

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: > >

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

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

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

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

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

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 ==

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

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

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

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:

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

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() ==

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

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

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

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

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

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 {

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

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:

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

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

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

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

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

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

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

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

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

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/