[PATCH] D84604: Thread safety analysis: Consider global variables in scope

rupprecht added a comment. In D84604#2364568 , @aaronpuchert wrote: > In D84604#2363768 , @rupprecht wrote: > >> I applied D87194 locally and rebuilt the >> original source, and

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added a comment. In D84604#2363768 , @rupprecht wrote: > I applied D87194 locally and rebuilt the > original source, and not only am I seeing the original issue (also firing on > `DoThings()` when it should

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

rupprecht added a comment. In D84604#2363445 , @aaronpuchert wrote: > Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97 > . For > now we just consider all static members as

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added a comment. Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97 . For now we just consider all static members as inaccessible, so we'll treat them as we did before this change. I have proposed making

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added a comment. In D84604#2363134 , @rupprecht wrote: > I'm seeing failures which I think are due to this patch -- I don't have a > nice godbolt repro yet, but it's something like: Yes, that's very likely. > I'm also seeing the same error

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

rupprecht added a comment. I'm seeing failures which I think are due to this patch -- I don't have a nice godbolt repro yet, but it's something like: foo.h: class Foo { public: static void DoStuff(); // Grabs mu_ private: static std::vector blah_ GUARDED_BY(mu_);

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5250a03a9959: Thread safety analysis: Consider global variables in scope (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

lebedev.ri resigned from this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Thanks. Note that i didn't check that this doesn't cause any new false-positives Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89 + void

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89 + void test4() { +MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}} + } lebedev.ri wrote: >

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89 + void test4() { +MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}} + } aaronpuchert wrote: >

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:1275 +const ValueDecl *VD = LP->clangDecl(); +return VD->isDefinedOutsideFunctionOrMethod(); + } aaron.ballman wrote: > Hmm, I've not seen that function used a whole lot

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. Please can you point me where you've added the negative test for the false-positive issue that caused the revert last time? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM Comment at: clang/lib/Analysis/ThreadSafety.cpp:1275 +const ValueDecl *VD = LP->clangDecl(); +return VD->isDefinedOutsideFunctionOrMethod(); + } Hmm, I've not seen that function

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert updated this revision to Diff 300507. aaronpuchert added a comment. `LiteralPtr`s aren't always globals, local variables are also translated that way. So we ask the stored `ValueDecl` if it `isDefinedOutsideFunctionOrMethod`. That seems like the right thing. Repository: rG LLVM

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert reopened this revision. aaronpuchert added a comment. This revision is now accepted and ready to land. Almost forgot about that. I think I've figured it out. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84604/new/

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

lebedev.ri added a comment. In D84604#2262745 , @lebedev.ri wrote: > I'm not sure this is the problematic patch, but i'm now seeing some weird > warnings: > > :304:15: warning: acquiring mutex 'guard' requires negative > capability '!guard'

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

lebedev.ri added a comment. I'm not sure this is the problematic patch, but i'm now seeing some weird warnings: :304:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative] MutexLocker guard(); ^ :309:15: warning: acquiring

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

This revision was automatically updated to reflect the committed changes. Closed by commit rG9dcc82f34ea9: Thread safety analysis: Consider global variables in scope (authored by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84604/new/ https://reviews.llvm.org/D84604

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert updated this revision to Diff 289833. aaronpuchert marked 2 inline comments as done. aaronpuchert added a comment. Add tests with qualified names, let tests rely on shadowing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84604/new/

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87 +Mutex globalMutex; +void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex); + aaron.ballman wrote: > Can you add a test that uses `!::globalMutex`? I'd like to verify

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaron.ballman added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87 +Mutex globalMutex; +void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex); + Can you add a test that uses `!::globalMutex`? I'd like to verify that lookup rules

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert updated this revision to Diff 289316. aaronpuchert added a comment. Rebase on top of D84603 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84604/new/ https://reviews.llvm.org/D84604 Files:

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added a comment. Test failure is expected because I based this on D84603 locally, but I'm not sure how to tell that to Phabricator. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84604/new/

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert added a subscriber: anarazel. aaronpuchert added a comment. @anarazel, that should fix the issue you reported on IRC. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84604/new/ https://reviews.llvm.org/D84604

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a project: clang. Herald added a subscriber: cfe-commits. Instead of just mutex members we also consider mutex globals. Unsurprisingly they are always in scope. Now the paper says that The