mozilla
/
gecko-dev
зеркало из https://github.com/mozilla/gecko-dev.git
1
0
Форкнуть 0
Код Задачи Пакеты Проекты Релизы Вики Активность

Bug 1794074 - Revert clang trunk change wrt thread safety analysis. a=land-fix CLOSED TREE 

Differential Revision: https://phabricator.services.mozilla.com/D158824
This commit is contained in:
Mike Hommey 2022-10-13 22:31:17 +00:00
Родитель 50d8456970
Коммит c456ff9412
2 изменённых файлов: 886 добавлений и 0 удалений
Показать статистику Скачать Patch файл Скачать Diff файл Показать все файлы Свернуть все файлы

1
build/build-clang/clang-trunk.json
Просмотреть файл

@ -7,6 +7,7 @@
"bug47258-extract-symbols-mbcs.patch",
"fuzzing_ccov_build_clang_12.patch",
"win64-no-symlink_clang_16.patch",
"revert-llvmorg-16-init-7598-g54bfd0484615.patch",
"revert-llvmorg-15-init-13446-g7524fe962e47.patch",
"revert-llvmorg-14-init-14141-gd6d3000a2f6d.patch",
"revert-llvmorg-14-init-11890-gf86deb18cab6_clang_16.patch",

885
build/build-clang/revert-llvmorg-16-init-7598-g54bfd0484615.patch Normal file
Просмотреть файл

@ -0,0 +1,885 @@
The patch changed the way the no_thread_safety_analysis works in a way
that I think actually makes sense, but we have exceptions in the code
that aren't enough to accomodate that change.
Until our code is adjusted, revert this change.
---
clang/docs/ReleaseNotes.rst | 7 -
clang/docs/ThreadSafetyAnalysis.rst | 10 +-
.../Analysis/Analyses/ThreadSafetyCommon.h | 13 +-
.../clang/Analysis/Analyses/ThreadSafetyTIL.h | 7 +-
.../Analysis/Analyses/ThreadSafetyTraverse.h | 5 +-
clang/lib/Analysis/ThreadSafety.cpp | 202 +++++++++---------
clang/lib/Analysis/ThreadSafetyCommon.cpp | 46 ++--
.../SemaCXX/warn-thread-safety-analysis.cpp | 155 +++-----------
8 files changed, 155 insertions(+), 290 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2a81877f27b7..02a83340588f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -307,13 +307,6 @@ Improvements to Clang's diagnostics
function definition without a prototype which is preceded by a static
declaration of the function with a prototype. Fixes
`Issue 58181 <https://github.com/llvm/llvm-project/issues/58181>`_.
-- Copy-elided initialization of lock scopes is now handled differently in
- ``-Wthread-safety-analysis``: annotations on the move constructor are no
- longer taken into account, in favor of annotations on the function returning
- the lock scope by value. This could result in new warnings if code depended
- on the previous undocumented behavior. As a side effect of this change,
- constructor calls outside of initializer expressions are no longer ignored,
- which can result in new warnings (or make existing warnings disappear).
Non-comprehensive list of changes in this release
-------------------------------------------------
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
index dcde0c706c70..23f460b248e1 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -408,8 +408,7 @@ and destructor refer to the capability via different names; see the
Scoped capabilities are treated as capabilities that are implicitly acquired
on construction and released on destruction. They are associated with
the set of (regular) capabilities named in thread safety attributes on the
-constructor or function returning them by value (using C++17 guaranteed copy
-elision). Acquire-type attributes on other member functions are treated as
+constructor. Acquire-type attributes on other member functions are treated as
applying to that set of associated capabilities, while ``RELEASE`` implies that
a function releases all associated capabilities in whatever mode they're held.
@@ -931,13 +930,6 @@ implementation.
// Assume mu is not held, implicitly acquire *this and associate it with mu.
MutexLocker(Mutex *mu, defer_lock_t) EXCLUDES(mu) : mut(mu), locked(false) {}
- // Same as constructors, but without tag types. (Requires C++17 copy elision.)
- static MutexLocker Lock(Mutex *mu) ACQUIRE(mu);
- static MutexLocker Adopt(Mutex *mu) REQUIRES(mu);
- static MutexLocker ReaderLock(Mutex *mu) ACQUIRE_SHARED(mu);
- static MutexLocker AdoptReaderLock(Mutex *mu) REQUIRES_SHARED(mu);
- static MutexLocker DeferLock(Mutex *mu) EXCLUDES(mu);
-
// Release *this and all associated mutexes, if they are still held.
// There is no warning if the scope was already unlocked before.
~MutexLocker() RELEASE() {
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index 9c73d65db266..da69348ea938 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -31,7 +31,6 @@
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/PointerIntPair.h"
-#include "llvm/ADT/PointerUnion.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
#include <sstream>
@@ -355,7 +354,7 @@ public:
const NamedDecl *AttrDecl;
// Implicit object argument -- e.g. 'this'
- llvm::PointerUnion<const Expr *, til::SExpr *> SelfArg = nullptr;
+ const Expr *SelfArg = nullptr;
// Number of funArgs
unsigned NumArgs = 0;
@@ -379,18 +378,10 @@ public:
// Translate a clang expression in an attribute to a til::SExpr.
// Constructs the context from D, DeclExp, and SelfDecl.
CapabilityExpr translateAttrExpr(const Expr *AttrExp, const NamedDecl *D,
- const Expr *DeclExp,
- til::SExpr *Self = nullptr);
+ const Expr *DeclExp, VarDecl *SelfD=nullptr);
CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx);
- // Translate a variable reference.
- til::LiteralPtr *createVariable(const VarDecl *VD);
-
- // Create placeholder for this: we don't know the VarDecl on construction yet.
- std::pair<til::LiteralPtr *, StringRef>
- createThisPlaceholder(const Expr *Exp);
-
// Translate a clang statement or expression to a TIL expression.
// Also performs substitution of variables; Ctx provides the context.
// Dispatches on the type of S.
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
index 48593516d853..65556c8d584c 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -634,14 +634,15 @@ typename V::R_SExpr Literal::traverse(V &Vs, typename V::R_Ctx Ctx) {
/// At compile time, pointer literals are represented by symbolic names.
class LiteralPtr : public SExpr {
public:
- LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) {}
+ LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) {
+ assert(D && "ValueDecl must not be null");
+ }
LiteralPtr(const LiteralPtr &) = default;
static bool classof(const SExpr *E) { return E->opcode() == COP_LiteralPtr; }
// The clang declaration for the value that this pointer points to.
const ValueDecl *clangDecl() const { return Cvdecl; }
- void setClangDecl(const ValueDecl *VD) { Cvdecl = VD; }
template <class V>
typename V::R_SExpr traverse(V &Vs, typename V::R_Ctx Ctx) {
@@ -650,8 +651,6 @@ public:
template <class C>
typename C::CType compare(const LiteralPtr* E, C& Cmp) const {
- if (!Cvdecl || !E->Cvdecl)
- return Cmp.comparePointers(this, E);
return Cmp.comparePointers(Cvdecl, E->Cvdecl);
}
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h
index 6fc55130655a..e81c00d3dddb 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h
@@ -623,10 +623,7 @@ protected:
}
void printLiteralPtr(const LiteralPtr *E, StreamType &SS) {
- if (const NamedDecl *D = E->clangDecl())
- SS << D->getNameAsString();
- else
- SS << "<temporary>";
+ SS << E->clangDecl()->getNameAsString();
}
void printVariable(const Variable *V, StreamType &SS, bool IsVarDecl=false) {
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 76747561a9a3..a29134c6a5e7 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1029,7 +1029,7 @@ public:
template <typename AttrType>
void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
- const NamedDecl *D, til::SExpr *Self = nullptr);
+ const NamedDecl *D, VarDecl *SelfDecl = nullptr);
template <class AttrType>
void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
@@ -1220,7 +1220,7 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
if (const auto *LP = dyn_cast<til::LiteralPtr>(SExp)) {
const ValueDecl *VD = LP->clangDecl();
// Variables defined in a function are always inaccessible.
- if (!VD || !VD->isDefinedOutsideFunctionOrMethod())
+ if (!VD->isDefinedOutsideFunctionOrMethod())
return false;
// For now we consider static class members to be inaccessible.
if (isa<CXXRecordDecl>(VD->getDeclContext()))
@@ -1311,10 +1311,10 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp,
template <typename AttrType>
void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
const Expr *Exp, const NamedDecl *D,
- til::SExpr *Self) {
+ VarDecl *SelfDecl) {
if (Attr->args_size() == 0) {
// The mutex held is the "this" object.
- CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, Self);
+ CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, SelfDecl);
if (Cp.isInvalid()) {
warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind());
return;
@@ -1326,7 +1326,7 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
}
for (const auto *Arg : Attr->args()) {
- CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, Self);
+ CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, SelfDecl);
if (Cp.isInvalid()) {
warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind());
continue;
@@ -1529,26 +1529,21 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
ThreadSafetyAnalyzer *Analyzer;
FactSet FSet;
- /// Maps constructed objects to `this` placeholder prior to initialization.
- llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
LocalVariableMap::Context LVarCtx;
unsigned CtxIndex;
// helper functions
void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK,
Expr *MutexExp, ProtectedOperationKind POK,
- til::LiteralPtr *Self, SourceLocation Loc);
- void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
- til::LiteralPtr *Self, SourceLocation Loc);
+ SourceLocation Loc);
+ void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp);
void checkAccess(const Expr *Exp, AccessKind AK,
ProtectedOperationKind POK = POK_VarAccess);
void checkPtAccess(const Expr *Exp, AccessKind AK,
ProtectedOperationKind POK = POK_VarAccess);
- void handleCall(const Expr *Exp, const NamedDecl *D,
- til::LiteralPtr *Self = nullptr,
- SourceLocation Loc = SourceLocation());
+ void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr);
void examineArguments(const FunctionDecl *FD,
CallExpr::const_arg_iterator ArgBegin,
CallExpr::const_arg_iterator ArgEnd,
@@ -1565,7 +1560,6 @@ public:
void VisitCallExpr(const CallExpr *Exp);
void VisitCXXConstructExpr(const CXXConstructExpr *Exp);
void VisitDeclStmt(const DeclStmt *S);
- void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp);
};
} // namespace
@@ -1575,12 +1569,10 @@ public:
void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
AccessKind AK, Expr *MutexExp,
ProtectedOperationKind POK,
- til::LiteralPtr *Self,
SourceLocation Loc) {
LockKind LK = getLockKindFromAccessKind(AK);
- CapabilityExpr Cp =
- Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
+ CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
if (Cp.isInvalid()) {
warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
return;
@@ -1637,10 +1629,8 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
/// Warn if the LSet contains the given lock.
void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp,
- Expr *MutexExp, til::LiteralPtr *Self,
- SourceLocation Loc) {
- CapabilityExpr Cp =
- Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
+ Expr *MutexExp) {
+ CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
if (Cp.isInvalid()) {
warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
return;
@@ -1651,7 +1641,7 @@ void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp,
const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, Cp);
if (LDat) {
Analyzer->Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
- Cp.toString(), Loc);
+ Cp.toString(), Exp->getExprLoc());
}
}
@@ -1721,7 +1711,7 @@ void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK,
}
for (const auto *I : D->specific_attrs<GuardedByAttr>())
- warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, nullptr, Loc);
+ warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, Loc);
}
/// Checks pt_guarded_by and pt_guarded_var attributes.
@@ -1758,8 +1748,7 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK,
Analyzer->Handler.handleNoMutexHeld(D, PtPOK, AK, Exp->getExprLoc());
for (auto const *I : D->specific_attrs<PtGuardedByAttr>())
- warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, nullptr,
- Exp->getExprLoc());
+ warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, Exp->getExprLoc());
}
/// Process a function call, method call, constructor call,
@@ -1772,35 +1761,21 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK,
/// and check that the appropriate locks are held. Non-const method calls with
/// the same signature as const method calls can be also treated as reads.
///
-/// \param Exp The call expression.
-/// \param D The callee declaration.
-/// \param Self If \p Exp = nullptr, the implicit this argument.
-/// \param Loc If \p Exp = nullptr, the location.
void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
- til::LiteralPtr *Self, SourceLocation Loc) {
+ VarDecl *VD) {
+ SourceLocation Loc = Exp->getExprLoc();
CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd;
CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove;
CapExprSet ScopedReqsAndExcludes;
// Figure out if we're constructing an object of scoped lockable class
- CapabilityExpr Scp;
- if (Exp) {
- assert(!Self);
- const auto *TagT = Exp->getType()->getAs<TagType>();
- if (TagT && Exp->isPRValue()) {
- std::pair<til::LiteralPtr *, StringRef> Placeholder =
- Analyzer->SxBuilder.createThisPlaceholder(Exp);
- [[maybe_unused]] auto inserted =
- ConstructedObjects.insert({Exp, Placeholder.first});
- assert(inserted.second && "Are we visiting the same expression again?");
- if (isa<CXXConstructExpr>(Exp))
- Self = Placeholder.first;
- if (TagT->getDecl()->hasAttr<ScopedLockableAttr>())
- Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false);
+ bool isScopedVar = false;
+ if (VD) {
+ if (const auto *CD = dyn_cast<const CXXConstructorDecl>(D)) {
+ const CXXRecordDecl* PD = CD->getParent();
+ if (PD && PD->hasAttr<ScopedLockableAttr>())
+ isScopedVar = true;
}
-
- assert(Loc.isInvalid());
- Loc = Exp->getExprLoc();
}
for(const Attr *At : D->attrs()) {
@@ -1811,7 +1786,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
const auto *A = cast<AcquireCapabilityAttr>(At);
Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
: ExclusiveLocksToAdd,
- A, Exp, D, Self);
+ A, Exp, D, VD);
break;
}
@@ -1822,7 +1797,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
const auto *A = cast<AssertExclusiveLockAttr>(At);
CapExprSet AssertLocks;
- Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
+ Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
for (const auto &AssertLock : AssertLocks)
Analyzer->addLock(
FSet, std::make_unique<LockableFactEntry>(
@@ -1833,7 +1808,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
const auto *A = cast<AssertSharedLockAttr>(At);
CapExprSet AssertLocks;
- Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
+ Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
for (const auto &AssertLock : AssertLocks)
Analyzer->addLock(
FSet, std::make_unique<LockableFactEntry>(
@@ -1844,7 +1819,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
case attr::AssertCapability: {
const auto *A = cast<AssertCapabilityAttr>(At);
CapExprSet AssertLocks;
- Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
+ Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
for (const auto &AssertLock : AssertLocks)
Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(
AssertLock,
@@ -1858,11 +1833,11 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
case attr::ReleaseCapability: {
const auto *A = cast<ReleaseCapabilityAttr>(At);
if (A->isGeneric())
- Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self);
+ Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, VD);
else if (A->isShared())
- Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
+ Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, VD);
else
- Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
+ Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, VD);
break;
}
@@ -1870,10 +1845,10 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
const auto *A = cast<RequiresCapabilityAttr>(At);
for (auto *Arg : A->args()) {
warnIfMutexNotHeld(D, Exp, A->isShared() ? AK_Read : AK_Written, Arg,
- POK_FunctionCall, Self, Loc);
+ POK_FunctionCall, Exp->getExprLoc());
// use for adopting a lock
- if (!Scp.shouldIgnore())
- Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self);
+ if (isScopedVar)
+ Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD);
}
break;
}
@@ -1881,10 +1856,10 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
case attr::LocksExcluded: {
const auto *A = cast<LocksExcludedAttr>(At);
for (auto *Arg : A->args()) {
- warnIfMutexHeld(D, Exp, Arg, Self, Loc);
+ warnIfMutexHeld(D, Exp, Arg);
// use for deferring a lock
- if (!Scp.shouldIgnore())
- Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self);
+ if (isScopedVar)
+ Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD);
}
break;
}
@@ -1907,7 +1882,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
// Add locks.
FactEntry::SourceKind Source =
- !Scp.shouldIgnore() ? FactEntry::Managed : FactEntry::Acquired;
+ isScopedVar ? FactEntry::Managed : FactEntry::Acquired;
for (const auto &M : ExclusiveLocksToAdd)
Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(M, LK_Exclusive,
Loc, Source));
@@ -1915,9 +1890,15 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
Analyzer->addLock(
FSet, std::make_unique<LockableFactEntry>(M, LK_Shared, Loc, Source));
- if (!Scp.shouldIgnore()) {
+ if (isScopedVar) {
// Add the managing object as a dummy mutex, mapped to the underlying mutex.
- auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, Loc);
+ SourceLocation MLoc = VD->getLocation();
+ DeclRefExpr DRE(VD->getASTContext(), VD, false, VD->getType(), VK_LValue,
+ VD->getLocation());
+ // FIXME: does this store a pointer to DRE?
+ CapabilityExpr Scp = Analyzer->SxBuilder.translateAttrExpr(&DRE, nullptr);
+
+ auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, MLoc);
for (const auto &M : ExclusiveLocksToAdd)
ScopedEntry->addLock(M);
for (const auto &M : SharedLocksToAdd)
@@ -2077,11 +2058,36 @@ void BuildLockset::VisitCXXConstructExpr(const CXXConstructExpr *Exp) {
} else {
examineArguments(D, Exp->arg_begin(), Exp->arg_end());
}
- if (D && D->hasAttrs())
- handleCall(Exp, D);
}
-static const Expr *UnpackConstruction(const Expr *E) {
+static CXXConstructorDecl *
+findConstructorForByValueReturn(const CXXRecordDecl *RD) {
+ // Prefer a move constructor over a copy constructor. If there's more than
+ // one copy constructor or more than one move constructor, we arbitrarily
+ // pick the first declared such constructor rather than trying to guess which
+ // one is more appropriate.
+ CXXConstructorDecl *CopyCtor = nullptr;
+ for (auto *Ctor : RD->ctors()) {
+ if (Ctor->isDeleted())
+ continue;
+ if (Ctor->isMoveConstructor())
+ return Ctor;
+ if (!CopyCtor && Ctor->isCopyConstructor())
+ CopyCtor = Ctor;
+ }
+ return CopyCtor;
+}
+
+static Expr *buildFakeCtorCall(CXXConstructorDecl *CD, ArrayRef<Expr *> Args,
+ SourceLocation Loc) {
+ ASTContext &Ctx = CD->getASTContext();
+ return CXXConstructExpr::Create(Ctx, Ctx.getRecordType(CD->getParent()), Loc,
+ CD, true, Args, false, false, false, false,
+ CXXConstructExpr::CK_Complete,
+ SourceRange(Loc, Loc));
+}
+
+static Expr *UnpackConstruction(Expr *E) {
if (auto *CE = dyn_cast<CastExpr>(E))
if (CE->getCastKind() == CK_NoOp)
E = CE->getSubExpr()->IgnoreParens();
@@ -2100,7 +2106,7 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
for (auto *D : S->getDeclGroup()) {
if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
- const Expr *E = VD->getInit();
+ Expr *E = VD->getInit();
if (!E)
continue;
E = E->IgnoreParens();
@@ -2110,27 +2116,29 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
E = EWC->getSubExpr()->IgnoreParens();
E = UnpackConstruction(E);
- if (auto Object = ConstructedObjects.find(E);
- Object != ConstructedObjects.end()) {
- Object->second->setClangDecl(VD);
- ConstructedObjects.erase(Object);
+ if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
+ const auto *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
+ if (!CtorD || !CtorD->hasAttrs())
+ continue;
+ handleCall(E, CtorD, VD);
+ } else if (isa<CallExpr>(E) && E->isPRValue()) {
+ // If the object is initialized by a function call that returns a
+ // scoped lockable by value, use the attributes on the copy or move
+ // constructor to figure out what effect that should have on the
+ // lockset.
+ // FIXME: Is this really the best way to handle this situation?
+ auto *RD = E->getType()->getAsCXXRecordDecl();
+ if (!RD || !RD->hasAttr<ScopedLockableAttr>())
+ continue;
+ CXXConstructorDecl *CtorD = findConstructorForByValueReturn(RD);
+ if (!CtorD || !CtorD->hasAttrs())
+ continue;
+ handleCall(buildFakeCtorCall(CtorD, {E}, E->getBeginLoc()), CtorD, VD);
}
}
}
}
-void BuildLockset::VisitMaterializeTemporaryExpr(
- const MaterializeTemporaryExpr *Exp) {
- if (const ValueDecl *ExtD = Exp->getExtendingDecl()) {
- if (auto Object =
- ConstructedObjects.find(UnpackConstruction(Exp->getSubExpr()));
- Object != ConstructedObjects.end()) {
- Object->second->setClangDecl(ExtD);
- ConstructedObjects.erase(Object);
- }
- }
-}
-
/// Given two facts merging on a join point, possibly warn and decide whether to
/// keep or replace.
///
@@ -2403,33 +2411,19 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
LocksetBuilder.Visit(CS.getStmt());
break;
}
- // Ignore BaseDtor and MemberDtor for now.
+ // Ignore BaseDtor, MemberDtor, and TemporaryDtor for now.
case CFGElement::AutomaticObjectDtor: {
CFGAutomaticObjDtor AD = BI.castAs<CFGAutomaticObjDtor>();
const auto *DD = AD.getDestructorDecl(AC.getASTContext());
if (!DD->hasAttrs())
break;
- LocksetBuilder.handleCall(nullptr, DD,
- SxBuilder.createVariable(AD.getVarDecl()),
- AD.getTriggerStmt()->getEndLoc());
- break;
- }
- case CFGElement::TemporaryDtor: {
- auto TD = BI.castAs<CFGTemporaryDtor>();
-
- // Clean up constructed object even if there are no attributes to
- // keep the number of objects in limbo as small as possible.
- if (auto Object = LocksetBuilder.ConstructedObjects.find(
- TD.getBindTemporaryExpr()->getSubExpr());
- Object != LocksetBuilder.ConstructedObjects.end()) {
- const auto *DD = TD.getDestructorDecl(AC.getASTContext());
- if (DD->hasAttrs())
- // TODO: the location here isn't quite correct.
- LocksetBuilder.handleCall(nullptr, DD, Object->second,
- TD.getBindTemporaryExpr()->getEndLoc());
- LocksetBuilder.ConstructedObjects.erase(Object);
- }
+ // Create a dummy expression,
+ auto *VD = const_cast<VarDecl *>(AD.getVarDecl());
+ DeclRefExpr DRE(VD->getASTContext(), VD, false,
+ VD->getType().getNonReferenceType(), VK_LValue,
+ AD.getTriggerStmt()->getEndLoc());
+ LocksetBuilder.handleCall(&DRE, DD);
break;
}
default:
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index a771149f1591..06b61b4de92f 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -115,22 +115,19 @@ static StringRef ClassifyDiagnostic(QualType VDT) {
/// \param D The declaration to which the attribute is attached.
/// \param DeclExp An expression involving the Decl to which the attribute
/// is attached. E.g. the call to a function.
-/// \param Self S-expression to substitute for a \ref CXXThisExpr.
CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
const NamedDecl *D,
const Expr *DeclExp,
- til::SExpr *Self) {
+ VarDecl *SelfDecl) {
// If we are processing a raw attribute expression, with no substitutions.
- if (!DeclExp && !Self)
+ if (!DeclExp)
return translateAttrExpr(AttrExp, nullptr);
CallingContext Ctx(nullptr, D);
// Examine DeclExp to find SelfArg and FunArgs, which are used to substitute
// for formal parameters when we call buildMutexID later.
- if (!DeclExp)
- /* We'll use Self. */;
- else if (const auto *ME = dyn_cast<MemberExpr>(DeclExp)) {
+ if (const auto *ME = dyn_cast<MemberExpr>(DeclExp)) {
Ctx.SelfArg = ME->getBase();
Ctx.SelfArrow = ME->isArrow();
} else if (const auto *CE = dyn_cast<CXXMemberCallExpr>(DeclExp)) {
@@ -145,24 +142,29 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
Ctx.SelfArg = nullptr; // Will be set below
Ctx.NumArgs = CE->getNumArgs();
Ctx.FunArgs = CE->getArgs();
+ } else if (D && isa<CXXDestructorDecl>(D)) {
+ // There's no such thing as a "destructor call" in the AST.
+ Ctx.SelfArg = DeclExp;
}
- if (Self) {
- assert(!Ctx.SelfArg && "Ambiguous self argument");
- Ctx.SelfArg = Self;
+ // Hack to handle constructors, where self cannot be recovered from
+ // the expression.
+ if (SelfDecl && !Ctx.SelfArg) {
+ DeclRefExpr SelfDRE(SelfDecl->getASTContext(), SelfDecl, false,
+ SelfDecl->getType(), VK_LValue,
+ SelfDecl->getLocation());
+ Ctx.SelfArg = &SelfDRE;
// If the attribute has no arguments, then assume the argument is "this".
if (!AttrExp)
- return CapabilityExpr(
- Self, ClassifyDiagnostic(cast<CXXMethodDecl>(D)->getThisObjectType()),
- false);
+ return translateAttrExpr(Ctx.SelfArg, nullptr);
else // For most attributes.
return translateAttrExpr(AttrExp, &Ctx);
}
// If the attribute has no arguments, then assume the argument is "this".
if (!AttrExp)
- return translateAttrExpr(cast<const Expr *>(Ctx.SelfArg), nullptr);
+ return translateAttrExpr(Ctx.SelfArg, nullptr);
else // For most attributes.
return translateAttrExpr(AttrExp, &Ctx);
}
@@ -216,16 +218,6 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
return CapabilityExpr(E, Kind, Neg);
}
-til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) {
- return new (Arena) til::LiteralPtr(VD);
-}
-
-std::pair<til::LiteralPtr *, StringRef>
-SExprBuilder::createThisPlaceholder(const Expr *Exp) {
- return {new (Arena) til::LiteralPtr(nullptr),
- ClassifyDiagnostic(Exp->getType())};
-}
-
// Translate a clang statement or expression to a TIL expression.
// Also performs substitution of variables; Ctx provides the context.
// Dispatches on the type of S.
@@ -335,12 +327,8 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
til::SExpr *SExprBuilder::translateCXXThisExpr(const CXXThisExpr *TE,
CallingContext *Ctx) {
// Substitute for 'this'
- if (Ctx && Ctx->SelfArg) {
- if (const auto *SelfArg = dyn_cast<const Expr *>(Ctx->SelfArg))
- return translate(SelfArg, Ctx->Prev);
- else
- return cast<til::SExpr *>(Ctx->SelfArg);
- }
+ if (Ctx && Ctx->SelfArg)
+ return translate(Ctx->SelfArg, Ctx->Prev);
assert(SelfVar && "We have no variable for 'this'!");
return SelfVar;
}
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 8e312e589d81..e1cfa1f3fd17 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1606,30 +1606,6 @@ namespace substitution_test {
dlr.unlockData(d1);
}
};
-
- // Automatic object destructor calls don't appear as expressions in the CFG,
- // so we have to handle them separately whenever substitutions are required.
- struct DestructorRequires {
- Mutex mu;
- ~DestructorRequires() EXCLUSIVE_LOCKS_REQUIRED(mu);
- };
-
- void destructorRequires() {
- DestructorRequires rd;
- rd.mu.AssertHeld();
- }
-
- struct DestructorExcludes {
- Mutex mu;
- ~DestructorExcludes() LOCKS_EXCLUDED(mu);
- };
-
- void destructorExcludes() {
- DestructorExcludes ed;
- ed.mu.Lock(); // expected-note {{mutex acquired here}}
- } // expected-warning {{cannot call function '~DestructorExcludes' while mutex 'ed.mu' is held}}
- // expected-warning@-1 {{mutex 'ed.mu' is still held at the end of function}}
-
} // end namespace substituation_test
@@ -1714,15 +1690,6 @@ struct TestScopedLockable {
}
#endif
- void temporary() {
- MutexLock{&mu1}, a = 5;
- }
-
- void lifetime_extension() {
- const MutexLock &mulock = MutexLock(&mu1);
- a = 5;
- }
-
void foo2() {
ReaderMutexLock mulock1(&mu1);
if (getBool()) {
@@ -1741,12 +1708,6 @@ struct TestScopedLockable {
// expected-warning {{acquiring mutex 'mu1' that is already held}}
}
- void temporary_double_lock() {
- MutexLock mulock_a(&mu1); // expected-note{{mutex acquired here}}
- MutexLock{&mu1}; // \
- // expected-warning {{acquiring mutex 'mu1' that is already held}}
- }
-
void foo4() {
MutexLock mulock1(&mu1), mulock2(&mu2);
a = b+1;
@@ -4226,20 +4187,6 @@ public:
void foo() EXCLUSIVE_LOCKS_REQUIRED(this);
};
-class SelfLockDeferred {
-public:
- SelfLockDeferred() LOCKS_EXCLUDED(mu_);
- ~SelfLockDeferred() UNLOCK_FUNCTION(mu_);
-
- Mutex mu_;
-};
-
-class LOCKABLE SelfLockDeferred2 {
-public:
- SelfLockDeferred2() LOCKS_EXCLUDED(this);
- ~SelfLockDeferred2() UNLOCK_FUNCTION();
-};
-
void test() {
SelfLock s;
@@ -4251,14 +4198,6 @@ void test2() {
s2.foo();
}
-void testDeferredTemporary() {
- SelfLockDeferred(); // expected-warning {{releasing mutex '<temporary>.mu_' that was not held}}
-}
-
-void testDeferredTemporary2() {
- SelfLockDeferred2(); // expected-warning {{releasing mutex '<temporary>' that was not held}}
-}
-
} // end namespace SelfConstructorTest
@@ -5953,75 +5892,47 @@ C c;
void f() { c[A()]->g(); }
} // namespace PR34800
-#ifdef __cpp_guaranteed_copy_elision
-
namespace ReturnScopedLockable {
+ template<typename Object> class SCOPED_LOCKABLE ReadLockedPtr {
+ public:
+ ReadLockedPtr(Object *ptr) SHARED_LOCK_FUNCTION((*this)->mutex);
+ ReadLockedPtr(ReadLockedPtr &&) SHARED_LOCK_FUNCTION((*this)->mutex);
+ ~ReadLockedPtr() UNLOCK_FUNCTION();
-class Object {
-public:
- MutexLock lock() EXCLUSIVE_LOCK_FUNCTION(mutex) {
- // TODO: False positive because scoped lock isn't destructed.
- return MutexLock(&mutex); // expected-note {{mutex acquired here}}
- } // expected-warning {{mutex 'mutex' is still held at the end of function}}
-
- ReaderMutexLock lockShared() SHARED_LOCK_FUNCTION(mutex) {
- // TODO: False positive because scoped lock isn't destructed.
- return ReaderMutexLock(&mutex); // expected-note {{mutex acquired here}}
- } // expected-warning {{mutex 'mutex' is still held at the end of function}}
-
- MutexLock adopt() EXCLUSIVE_LOCKS_REQUIRED(mutex) {
- // TODO: False positive because scoped lock isn't destructed.
- return MutexLock(&mutex, true); // expected-note {{mutex acquired here}}
- } // expected-warning {{mutex 'mutex' is still held at the end of function}}
+ Object *operator->() const { return object; }
- ReaderMutexLock adoptShared() SHARED_LOCKS_REQUIRED(mutex) {
- // TODO: False positive because scoped lock isn't destructed.
- return ReaderMutexLock(&mutex, true); // expected-note {{mutex acquired here}}
- } // expected-warning {{mutex 'mutex' is still held at the end of function}}
+ private:
+ Object *object;
+ };
- int x GUARDED_BY(mutex);
- void needsLock() EXCLUSIVE_LOCKS_REQUIRED(mutex);
+ struct Object {
+ int f() SHARED_LOCKS_REQUIRED(mutex);
+ Mutex mutex;
+ };
- void testInside() {
- MutexLock scope = lock();
- x = 1;
- needsLock();
+ ReadLockedPtr<Object> get();
+ int use() {
+ auto ptr = get();
+ return ptr->f();
+ }
+ void use_constructor() {
+ auto ptr = ReadLockedPtr<Object>(nullptr);
+ ptr->f();
+ auto ptr2 = ReadLockedPtr<Object>{nullptr};
+ ptr2->f();
+ auto ptr3 = (ReadLockedPtr<Object>{nullptr});
+ ptr3->f();
+ }
+ struct Convertible {
+ Convertible();
+ operator ReadLockedPtr<Object>();
+ };
+ void use_conversion() {
+ ReadLockedPtr<Object> ptr = Convertible();
+ ptr->f();
}
-
- Mutex mutex;
-};
-
-Object obj;
-
-void testLock() {
- MutexLock scope = obj.lock();
- obj.x = 1;
- obj.needsLock();
}
-int testSharedLock() {
- ReaderMutexLock scope = obj.lockShared();
- obj.x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'obj.mutex' exclusively}}
- return obj.x;
-}
-
-void testAdopt() {
- obj.mutex.Lock();
- MutexLock scope = obj.adopt();
- obj.x = 1;
-}
-
-int testAdoptShared() {
- obj.mutex.Lock();
- ReaderMutexLock scope = obj.adoptShared();
- obj.x = 1;
- return obj.x;
-}
-
-} // namespace ReturnScopedLockable
-
-#endif
-
namespace PR38640 {
void f() {
// Self-referencing assignment previously caused an infinite loop when thread
--
2.37.1.1.g659da70093