Commit 9b889f82 authored by Aaron Puchert's avatar Aaron Puchert
Browse files

Thread safety analysis: Warn when demoting locks on back edges 

Previously in D104261 we warned about dropping locks from back edges,
this is the corresponding change for exclusive/shared joins. If we're
entering the loop with an exclusive change, which is then relaxed to a
shared lock before we loop back, we have already analyzed the loop body
with the stronger exclusive lock and thus might have false positives.

There is a minor non-observable change: we modify the exit lock set of a
function, but since that isn't used further it doesn't change anything.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D106713
parent 0eb75a41

clang/lib/Analysis/ThreadSafety.cpp

+17 −14
Original line number Diff line number Diff line
@@ -1050,7 +1050,7 @@ public: 
                      const CFGBlock* PredBlock,

                      const CFGBlock *CurrBlock);



  bool join(const FactEntry &a, const FactEntry &b);

  bool join(const FactEntry &a, const FactEntry &b, bool CanModify);



  void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet,

                        SourceLocation JoinLoc, LockErrorKind EntryLEK,
@@ -2188,25 +2188,28 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) { 
  }

}



/// Given two facts merging on a join point, decide whether to warn and which

/// one to keep.

/// Given two facts merging on a join point, possibly warn and decide whether to

/// keep or replace.

///

/// \return  false if we should keep \p A, true if we should keep \p B.

bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B) {

/// \param CanModify Whether we can replace \p A by \p B.

/// \return  false if we should keep \p A, true if we should take \p B.

bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B,

                                bool CanModify) {

  if (A.kind() != B.kind()) {

    // For managed capabilities, the destructor should unlock in the right mode

    // anyway. For asserted capabilities no unlocking is needed.

    if ((A.managed() || A.asserted()) && (B.managed() || B.asserted())) {

      // The shared capability subsumes the exclusive capability.

      return B.kind() == LK_Shared;

    } else {

      // The shared capability subsumes the exclusive capability, if possible.

      bool ShouldTakeB = B.kind() == LK_Shared;

      if (CanModify || !ShouldTakeB)

        return ShouldTakeB;

    }

    Handler.handleExclusiveAndShared("mutex", B.toString(), B.loc(), A.loc());

    // Take the exclusive capability to reduce further warnings.

      return B.kind() == LK_Exclusive;

    }

    return CanModify && B.kind() == LK_Exclusive;

  } else {

    // The non-asserted capability is the one we want to track.

    return A.asserted() && !B.asserted();

    return CanModify && A.asserted() && !B.asserted();

  }

}
@@ -2237,8 +2240,8 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet, 


    FactSet::iterator EntryIt = EntrySet.findLockIter(FactMan, ExitFact);

    if (EntryIt != EntrySet.end()) {

      if (join(FactMan[*EntryIt], ExitFact) &&

          EntryLEK == LEK_LockedSomePredecessors)

      if (join(FactMan[*EntryIt], ExitFact,

               EntryLEK != LEK_LockedSomeLoopIterations))

        *EntryIt = Fact;

    } else if (!ExitFact.managed()) {

      ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

+42 −0
Original line number Diff line number Diff line
@@ -2789,6 +2789,25 @@ void loopRelease() { 
  }

}



void loopPromote() {

  RelockableMutexLock scope(&mu, SharedTraits{});

  for (unsigned i = 1; i < 10; ++i) {

    x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}

    if (i == 5)

      scope.PromoteShared();

  }

}



void loopDemote() {

  RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{the other acquisition of mutex 'mu' is here}}

  // We have to warn on this join point despite the lock being managed ...

  for (unsigned i = 1; i < 10; ++i) {

    x = 1; // ... because we might miss that this doesn't always happen under exclusive lock.

    if (i == 5)

      scope.DemoteExclusive(); // expected-warning {{mutex 'mu' is acquired exclusively and shared in the same scope}}

  }

}



void loopAcquireContinue() {

  RelockableMutexLock scope(&mu, DeferTraits{});

  for (unsigned i = 1; i < 10; ++i) {
@@ -2812,6 +2831,29 @@ void loopReleaseContinue() { 
  }

}



void loopPromoteContinue() {

  RelockableMutexLock scope(&mu, SharedTraits{});

  for (unsigned i = 1; i < 10; ++i) {

    x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}

    if (i == 5) {

      scope.PromoteShared();

      continue;

    }

  }

}



void loopDemoteContinue() {

  RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{the other acquisition of mutex 'mu' is here}}

  // We have to warn on this join point despite the lock being managed ...

  for (unsigned i = 1; i < 10; ++i) {

    x = 1; // ... because we might miss that this doesn't always happen under exclusive lock.

    if (i == 5) {

      scope.DemoteExclusive(); // expected-warning {{mutex 'mu' is acquired exclusively and shared in the same scope}}

      continue;

    }

  }

}



void exclusiveSharedJoin() {

  RelockableMutexLock scope(&mu, DeferTraits{});

  if (b)