Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions clang/lib/Analysis/ThreadSafety.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2443,6 +2443,22 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
if (join(FactMan[*EntryIt], ExitFact, JoinLoc, EntryLEK))
*EntryIt = Fact;
} else if (!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction) {
if (EntryLEK == LEK_LockedAtEndOfFunction) {
const til::SExpr *Sexp = ExitFact.sexpr();
const VarDecl *MutexVar = nullptr;

if (const auto *Proj = dyn_cast<til::Project>(Sexp)) {
if (const auto *Base = dyn_cast<til::LiteralPtr>(Proj->record()))
MutexVar = dyn_cast_or_null<VarDecl>(Base->clangDecl());
} else if (const auto *LP = dyn_cast<til::LiteralPtr>(Sexp)) {
MutexVar = dyn_cast_or_null<VarDecl>(LP->clangDecl());
}
Comment on lines +2447 to +2455
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scoped lockables should be unlocked by the automatic destructor. I don't think we should add this code.


if (MutexVar && MutexVar->getStorageDuration() == SD_Automatic &&
MutexVar->getDeclContext() == CurrentFunction) {
continue;
}
}
ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,
EntryLEK, Handler);
}
Expand Down
45 changes: 45 additions & 0 deletions clang/test/Analysis/thread-safety-lock-scope.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// RUN: %clang_cc1 -include %s -verify -fsyntax-only -Wthread-safety %s

#ifndef HEADER
#define HEADER

#define LOCKABLE __attribute__ ((lockable))
#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__ ((exclusive_lock_function(__VA_ARGS__)))

class LOCKABLE Mutex{};

template<typename T>
struct lock_guard {
lock_guard<T>(T) {}
~lock_guard<T>() {}
};
template<typename T>
struct unique_lock {
unique_lock<T>(T) {}
~unique_lock<T>() {}
};

template <class T, class... Ts>
void LockMutexes(T &m, Ts &...ms) EXCLUSIVE_LOCK_FUNCTION(m, ms...);

#else

Mutex m0, m1;
void non_local_mutex_held() {
LockMutexes(m0, m1); // expected-note {{mutex acquired here}} \
// expected-note {{mutex acquired here}}
} // expected-warning{{mutex 'm0' is still held at the end of function}} \
// expected-warning{{mutex 'm1' is still held at the end of function}}

void no_local_mutex_held_warning() {
Mutex local_m0;
Mutex local_m1;
LockMutexes(local_m0, local_m1);
} // No warnings expected at end of function scope as the mutexes are function local.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure I agree here. I think mutexes should be properly released even if they go out of scope.


void no_local_unique_locks_held_warning() {
unique_lock<Mutex> ul0(m0);
unique_lock<Mutex> ul1(m1);
LockMutexes(ul0, ul1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several issues here:

  • unique_lock isn't annotated as a mutex (lockable) or scoped lockable class. I guess we don't warn about it because it's a template, but I'd have to check.
  • Once you have a scoped lockable class with properly annotated destructor, this should already be handled by the destructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we're still not able to emit attribute warnings on template instantiation. The reason is that we're doing Sema on ParsedAttr, but template instantiation has a regular attribute. I don't have a good idea how to extend the warning there.

} // No warnings expected at end of function scope as the unique_locks held are function local.
#endif