Page MenuHomePhabricator

Thread safety analysis: Consider global variables in scope
ClosedPublic

Authored by aaronpuchert on Jul 26 2020, 11:53 AM.

Details

Summary

Instead of just mutex members we also consider mutex globals.
Unsurprisingly they are always in scope. Now the paper [1] says that

The scope of a class member is assumed to be its enclosing class,
while the scope of a global variable is the translation unit in
which it is defined.

But I don't think we should limit this to TUs where a definition is
available - a declaration is enough to acquire the mutex, and if a mutex
is really limited in scope to a translation unit, it should probably be
only declared there.

[1] https://static.googleusercontent.com/media/research.google.com/en/us/pubs/archive/42958.pdf

Fixes PR46354.

Diff Detail

Event Timeline

aaronpuchert created this revision.Jul 26 2020, 11:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2020, 11:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaronpuchert edited the summary of this revision. (Show Details)Jul 26 2020, 11:57 AM

@anarazel, that should fix the issue you reported on IRC.

Test failure is expected because I based this on D84603 locally, but I'm not sure how to tell that to Phabricator.

Rebase on top of D84603.

aaron.ballman added inline comments.Sep 3 2020, 6:17 AM
clang/test/SemaCXX/warn-thread-safety-negative.cpp
97

Can you add a test that uses !::globalMutex? I'd like to verify that lookup rules are honored for naming the mutex, so more complex examples with name hiding would also be useful.

101

Can you also add a test that uses !ns::namespaceMutex?

aaronpuchert added inline comments.Sep 3 2020, 5:05 PM
clang/test/SemaCXX/warn-thread-safety-negative.cpp
97

Just noticed that we're in namespace ScopeTest here, I'll probably need to move that mutex out first. For name hiding I can just name both mutexes the same, I think.

aaronpuchert marked 2 inline comments as done.

Add tests with qualified names, let tests rely on shadowing.

aaron.ballman accepted this revision.Sep 4 2020, 6:21 AM

LGTM, thanks!

This revision is now accepted and ready to land.Sep 4 2020, 6:21 AM

I'm not sure this is the problematic patch, but i'm now seeing some weird warnings:

<source>:304:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:309:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:322:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
3 warnings generated.

https://godbolt.org/z/5zYT55

If this is the patch that causes it, then i think it's an obvious false-positive (wasn't this patch supposed to only handle globals?),
if not, the warning's wording is not great.

I'm not sure this is the problematic patch, but i'm now seeing some weird warnings:

<source>:304:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:309:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:322:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
3 warnings generated.

https://godbolt.org/z/5zYT55

If this is the patch that causes it, then i think it's an obvious false-positive (wasn't this patch supposed to only handle globals?),
if not, the warning's wording is not great.

I've verified that this is indeed the change that causes it,
and therefore gone ahead and temporairly reverted it in rG8427885e27813c457dccb011f65e8ded74444e31.

aaronpuchert reopened this revision.Oct 24 2020, 2:54 PM

Almost forgot about that. I think I've figured it out.

This revision is now accepted and ready to land.Oct 24 2020, 2:54 PM

LiteralPtrs 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.

aaron.ballman accepted this revision.Oct 25 2020, 6:22 AM

LGTM

clang/lib/Analysis/ThreadSafety.cpp
1275

Hmm, I've not seen that function used a whole lot before, but looking at the implementation of it, I think it does what we need it to do here. FWIW, I was expecting something more like this:

if (const DeclContext *DC = VD->getLexicalDeclContext())
  return !DC->getRedeclContext()->isFunctionOrMethod();

But I'm not certain if this would ever give a different answer from your approach.

lebedev.ri requested changes to this revision.Oct 25 2020, 6:27 AM

Please can you point me where you've added the negative test for the false-positive issue that caused the revert last time?

This revision now requires changes to proceed.Oct 25 2020, 6:27 AM
aaronpuchert added inline comments.Oct 25 2020, 10:39 AM
clang/lib/Analysis/ThreadSafety.cpp
1275

Never seen it before as well, but it does a DC = DC->getParent() loop, so there is probably a difference. Think about DC being a local struct or Objective-C block declaration.

clang/test/SemaCXX/warn-thread-safety-negative.cpp
87–89

@lebedev.ri, I think that's pretty much your case. On my original change, this would have also warned about scope, not just mu.

lebedev.ri added inline comments.Oct 25 2020, 10:49 AM
clang/test/SemaCXX/warn-thread-safety-negative.cpp
87–89

I think i'm missing the point here.
I originally reverted this because the diagnostics i was seeing were pretty unambiguously )to me) bogus.
But the only test change since then ensures that diagnostic is emitted in some case,
there are no tests to ensure it is not emitted in some cases.
So either my revert was wrong, or this still is either issuing seemingly bogus diagnostic,
or lacks test coverage that it doesn't issue said diagnostic.

Which one is it?

aaronpuchert added inline comments.Oct 25 2020, 10:58 AM
clang/test/SemaCXX/warn-thread-safety-negative.cpp
87–89

This test fails on the original, reverted change as follows:

error: 'warning' diagnostics seen but not expected: 
  File [...]/clang/test/SemaCXX/warn-thread-safety-negative.cpp Line 88: acquiring mutex 'lock' requires negative capability '!lock'
1 error generated.

Maybe you're not familiar with the -verify mechanism: it doesn't just check that the expected errors/warnings/notes are emitted, it also checks that nothing more is emitted.

lebedev.ri resigned from this revision.Oct 25 2020, 11:12 AM

Thanks.
Note that i didn't check that this doesn't cause any new false-positives

clang/test/SemaCXX/warn-thread-safety-negative.cpp
87–89

This test fails on the original, reverted change as follows:

Perfect, thanks for checking/confirming!

Maybe you're not familiar with the -verify mechanism: it doesn't just check that the expected errors/warnings/notes are emitted, it also checks that nothing more is emitted.

I was aware of that, but did not recall that detail until now. Thank you.

This revision is now accepted and ready to land.Oct 25 2020, 11:12 AM
This revision was landed with ongoing or failed builds.Oct 25 2020, 11:35 AM
This revision was automatically updated to reflect the committed changes.

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<int> blah_ GUARDED_BY(mu_);
  static Mutex mu_;
};

bar.h:
class Bar {
  static void DoThings(); // calls Foo::DoStuff()
};

Because DoStuff() grabs mu_, it needs the lock, and this patch gives an error like requires negative capability 'mu_'. That's fine, and in fact better analysis is welcome.

However, I'm also seeing the same error for DoThings(), which doesn't make sense. When I add it, I then get errors that mu_ is private, and therefore needs to be made public or friended for the thread analysis -- which is not at all a good change (the mutex should be encapsulated in the class). Even though it's "global" in the linkage sense, it's not visible outside of the class.

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 for DoThings(), which doesn't make sense. When I add it, I then get errors that mu_ is private, and therefore needs to be made public or friended for the thread analysis -- which is not at all a good change (the mutex should be encapsulated in the class). Even though it's "global" in the linkage sense, it's not visible outside of the class.

I'll see if I can fix this quickly.

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 the check stronger for non-static members in D87194, perhaps it makes sense to extend this to static members as well so that it fires on DoStuff() again.

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 can confirm this fixes the breakage -- thanks!

I have proposed making the check stronger for non-static members in D87194, perhaps it makes sense to extend this to static members as well so that it fires on DoStuff() again.

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 only be on DoStuff()), I'm also seeing: error: acquiring mutex 'lock' requires negative capability '!lock' [-Werror,-Wthread-safety-negative], where lock is a local variable, defined as MutexLock lock(mutex_).

I'll work on getting a better repro for this.

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 only be on DoStuff()), I'm also seeing: error: acquiring mutex 'lock' requires negative capability '!lock' [-Werror,-Wthread-safety-negative], where lock is a local variable, defined as MutexLock lock(mutex_).

Oh yes, I need to rebase this, sorry if I wasted your time. This is still on top of the bug that @lebedev.ri pointed out in D84604#2262745 and on top of the bug that you pointed out.

I'll work on getting a better repro for this.

Maybe wait a bit with that, I'll add you as reviewer when I've done the rebase and then you can try it again. I hope to have covered both locals and static members now.

Another issue is linkage, but I'll have to read up on that a bit.

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 only be on DoStuff()), I'm also seeing: error: acquiring mutex 'lock' requires negative capability '!lock' [-Werror,-Wthread-safety-negative], where lock is a local variable, defined as MutexLock lock(mutex_).

Oh yes, I need to rebase this, sorry if I wasted your time. This is still on top of the bug that @lebedev.ri pointed out in D84604#2262745 and on top of the bug that you pointed out.

No worries, it was only a minute to patch, and then a few more minutes to take a snack break while it rebuilt :)

I'll work on getting a better repro for this.

Maybe wait a bit with that, I'll add you as reviewer when I've done the rebase and then you can try it again. I hope to have covered both locals and static members now.

Another issue is linkage, but I'll have to read up on that a bit.

Yep, I'm not qualified to actually review the code, but I'd be happy to test any patches.