This is an archive of the discontinued LLVM Phabricator instance.

Thread safety analysis: Allow relockable scopes
ClosedPublic

Authored by aaronpuchert on Jul 26 2018, 4:05 PM.

Details

Summary

It's already allowed to prematurely release a scoped lock, now we also
allow relocking it again, possibly even in another mode.

Arguably the solution is not very elegant, but maybe that can only be
solved by comprehensive refactoring.

Diff Detail

Event Timeline

aaronpuchert created this revision.Jul 26 2018, 4:05 PM

Imagine having a producer loop, where we check a loop condition while holding a mutex, but release it in the loop body to let other producers/consumers do their work. In that scenario it makes sense to allow "relocking" a scope.

RelockableScope Scope(mu);
while (WorkToDo()) {
    Scope.Unlock();
    // Produce something...
    Scope.Lock();
    PublishWork();
}
lib/Analysis/ThreadSafety.cpp
986–987

Looks a bit weird, but clang-format told me to do that.

1323–1326

Not sure about this part, but it's probably not worth worrying about. A user would have to call a member function on a scoped object after manually ending its lifetime by calling the destructor, see test RelockableScopedLock::destructLock.

1329–1330

I hope this reasoning is sufficient to justify the following static_cast, otherwise I need to introduce LLVM-style RTTI into FactEntry.

aaron.ballman accepted this revision.Jul 28 2018, 10:51 AM

The changes look reasonable to me (after fixing a few nits), but please give @delesley a chance to weigh in before committing.

lib/Analysis/ThreadSafety.cpp
981

Can elide the braces.

986–987

This line looks correct to me, but the else statement above doesn't match our usual formatting rules, so I'm worried you're running clang-format in a way that's not picking up the LLVM coding style options.

1331

auto * to remind folks that it's a pointer.

This revision is now accepted and ready to land.Jul 28 2018, 10:51 AM

Formatting changes suggested by Aaron Ballman.

aaronpuchert marked 3 inline comments as done.Jul 28 2018, 12:43 PM
aaronpuchert added inline comments.
lib/Analysis/ThreadSafety.cpp
986–987

You're right about the else, not sure what happened there.

aaronpuchert marked an inline comment as done.Aug 7 2018, 3:13 PM

@delesley Did you have a chance to look at this yet?

Committed in r339456; if @delesley has concerns with the commit, they can be addressed post-commit. Thank you for the patch!

hokein added a subscriber: hokein.Aug 13 2018, 5:53 AM

Hello, this patch seems introduce a new crash, and I have reverted it in r339558.

Here is the minimal test case:

class SCOPED_LOCKABLE FileLock {
 public:
  explicit FileLock()
      EXCLUSIVE_LOCK_FUNCTION(file_);
  ~FileLock() UNLOCK_FUNCTION(file_);
  //void Release() UNLOCK_FUNCTION(file_);
  void Lock() EXCLUSIVE_LOCK_FUNCTION(file_);
  Mutex file_;
};

void relockShared2() {
  FileLock file_lock;
  file_lock.Lock();
}
aaronpuchert reopened this revision.Aug 13 2018, 6:42 PM

This didn't cross my mind, because an ACQUIRES attribute with arguments on a function other than the constructor does not add the argument locks to the set of managed mutexes. So while I'm not sure why someone would do that, it's completely possible without warning. Indeed it would be possible to have ACQUIRES both with and without arguments on the same function.

This revision is now accepted and ready to land.Aug 13 2018, 6:42 PM
aaronpuchert updated this revision to Diff 160505.EditedAug 13 2018, 6:59 PM

Fix crash. The problem was that ACQUIRES with arguments did no longer have (only) this as argument, hence our assumption that we would have only ScopedLockableFactEntry's was false. So now we look a bit closer which kind of ACQUIRES we actually have.

The case distinction in case attr::AcquireCapability is not very beautiful, but it's due to the fact that scoped capabilities are not "real" capabilities and so we need to distinguish them.

What this still doesn't allow for is attributes on other classes than the scoped capability that reacquire it from the outside. So maybe this isn't the right solution, and we need to approach it in a different way. Maybe instead of modifying the BuildLockset::handleCall, we should change ThreadSafetyAnalyzer::addLock. I'll think about that, but not today.

test/SemaCXX/warn-thread-safety-analysis.cpp
2769–2781

@hokein This is your test case, I just renamed some things.

Found a much simpler solution. After introducing a new virtual function HandleLock() in FactEntry, I just needed to change two lines in ThreadSafetyAnalyzer::addLock. Changes in BuildLockset::handleCall are no longer necessary.

aaronpuchert marked 4 inline comments as done.Aug 14 2018, 4:56 PM

@aaron.ballman Maybe you can have a look again — this is much more elegant. I'm not sure why I didn't see this in the first place.

test/SemaCXX/warn-thread-safety-analysis.cpp
2765–2768

I have a local patch that addresses this, but I think it should be discussed separately as it introduces additional changes.

Proper capitalization of member function, reduction of test code.

Formatting changes.

Reformat tests. I promise, this is the last one.

delesley accepted this revision.Aug 20 2018, 4:13 PM

This looks good to me. Thanks for the patch.

lib/Analysis/ThreadSafety.cpp
932

Minor nit. Use curly braces on the if, to match the else.

This revision was automatically updated to reflect the committed changes.
aaronpuchert added inline comments.Aug 22 2018, 3:35 PM
lib/Analysis/ThreadSafety.cpp
932

Removing the braces was suggested by @aaron.ballman in an earlier patch set, but I can just add them in again. I slightly favor having them, but I don't feel strongly either way.