diff options
24 files changed, 846 insertions, 44 deletions
diff --git a/accessibility/source/extended/accessibleiconchoicectrlentry.cxx b/accessibility/source/extended/accessibleiconchoicectrlentry.cxx index 44ac61e9252a..9e7b75cf1488 100644 --- a/accessibility/source/extended/accessibleiconchoicectrlentry.cxx +++ b/accessibility/source/extended/accessibleiconchoicectrlentry.cxx @@ -261,8 +261,6 @@ namespace accessibility sal_Int64 SAL_CALL AccessibleIconChoiceCtrlEntry::getAccessibleIndexInParent( ) { - ::osl::MutexGuard aGuard( m_aMutex ); - return m_nIndex; } diff --git a/accessibility/source/standard/vclxaccessiblelistitem.cxx b/accessibility/source/standard/vclxaccessiblelistitem.cxx index e1bb9cb18883..5339f93824a9 100644 --- a/accessibility/source/standard/vclxaccessiblelistitem.cxx +++ b/accessibility/source/standard/vclxaccessiblelistitem.cxx @@ -211,7 +211,6 @@ Reference< XAccessible > SAL_CALL VCLXAccessibleListItem::getAccessibleParent( sal_Int64 SAL_CALL VCLXAccessibleListItem::getAccessibleIndexInParent( ) { - ::osl::MutexGuard aGuard( m_aMutex ); return m_nIndexInParent; } diff --git a/accessibility/source/standard/vclxaccessibletoolbox.cxx b/accessibility/source/standard/vclxaccessibletoolbox.cxx index a1a2d5b03d54..aaea955d10f6 100644 --- a/accessibility/source/standard/vclxaccessibletoolbox.cxx +++ b/accessibility/source/standard/vclxaccessibletoolbox.cxx @@ -70,7 +70,6 @@ namespace sal_Int64 SAL_CALL OToolBoxWindowItemContext::getAccessibleIndexInParent( ) { - ::osl::MutexGuard aGuard( m_aMutex ); return m_nIndexInParent; } diff --git a/animations/source/animcore/animcore.cxx b/animations/source/animcore/animcore.cxx index 03c815021c93..7592109c4855 100644 --- a/animations/source/animcore/animcore.cxx +++ b/animations/source/animcore/animcore.cxx @@ -912,7 +912,6 @@ Sequence< OUString > AnimationNode::getSupportedServiceNames() // XAnimationNode sal_Int16 SAL_CALL AnimationNode::getType() { - std::unique_lock aGuard( m_aMutex ); return mnNodeType; } diff --git a/canvas/source/cairo/cairo_canvasfont.cxx b/canvas/source/cairo/cairo_canvasfont.cxx index 82f300e8a205..2445f408853c 100644 --- a/canvas/source/cairo/cairo_canvasfont.cxx +++ b/canvas/source/cairo/cairo_canvasfont.cxx @@ -116,8 +116,6 @@ namespace cairocanvas rendering::FontRequest SAL_CALL CanvasFont::getFontRequest( ) { - SolarMutexGuard aGuard; - return maFontRequest; } diff --git a/canvas/source/cairo/cairo_textlayout.cxx b/canvas/source/cairo/cairo_textlayout.cxx index ff12a6092c13..31ef31fa7a49 100644 --- a/canvas/source/cairo/cairo_textlayout.cxx +++ b/canvas/source/cairo/cairo_textlayout.cxx @@ -244,8 +244,6 @@ namespace cairocanvas sal_Int8 SAL_CALL TextLayout::getMainTextDirection( ) { - std::unique_lock aGuard( m_aMutex ); - return mnTextDirection; } @@ -258,8 +256,6 @@ namespace cairocanvas rendering::StringContext SAL_CALL TextLayout::getText( ) { - std::unique_lock aGuard( m_aMutex ); - return maText; } diff --git a/canvas/source/opengl/ogl_canvasfont.cxx b/canvas/source/opengl/ogl_canvasfont.cxx index 213dbe92b0e5..43a9d860b5ca 100644 --- a/canvas/source/opengl/ogl_canvasfont.cxx +++ b/canvas/source/opengl/ogl_canvasfont.cxx @@ -53,8 +53,6 @@ namespace oglcanvas rendering::FontRequest SAL_CALL CanvasFont::getFontRequest( ) { - std::unique_lock aGuard( m_aMutex ); - return maFontRequest; } diff --git a/canvas/source/opengl/ogl_textlayout.cxx b/canvas/source/opengl/ogl_textlayout.cxx index 4447aef5d828..a87365275436 100644 --- a/canvas/source/opengl/ogl_textlayout.cxx +++ b/canvas/source/opengl/ogl_textlayout.cxx @@ -175,8 +175,6 @@ namespace oglcanvas sal_Int8 SAL_CALL TextLayout::getMainTextDirection( ) { - std::unique_lock aGuard( m_aMutex ); - return mnTextDirection; } @@ -189,8 +187,6 @@ namespace oglcanvas rendering::StringContext SAL_CALL TextLayout::getText( ) { - std::unique_lock aGuard( m_aMutex ); - return maText; } diff --git a/canvas/source/tools/parametricpolypolygon.cxx b/canvas/source/tools/parametricpolypolygon.cxx index acad3b58b7d6..8c660a5e5308 100644 --- a/canvas/source/tools/parametricpolypolygon.cxx +++ b/canvas/source/tools/parametricpolypolygon.cxx @@ -228,8 +228,6 @@ namespace canvas ParametricPolyPolygon::Values ParametricPolyPolygon::getValues() const { - std::unique_lock aGuard( m_aMutex ); - return maValues; } diff --git a/canvas/source/vcl/canvasfont.cxx b/canvas/source/vcl/canvasfont.cxx index 41b7da25016d..336f70d632f0 100644 --- a/canvas/source/vcl/canvasfont.cxx +++ b/canvas/source/vcl/canvasfont.cxx @@ -98,8 +98,6 @@ namespace vclcanvas rendering::FontRequest SAL_CALL CanvasFont::getFontRequest( ) { - SolarMutexGuard aGuard; - return maFontRequest; } @@ -156,8 +154,6 @@ namespace vclcanvas const css::geometry::Matrix2D& CanvasFont::getFontMatrix() const { - SolarMutexGuard aGuard; - return maFontMatrix; } } diff --git a/canvas/source/vcl/textlayout.cxx b/canvas/source/vcl/textlayout.cxx index 12273daf5933..d6474e744c32 100644 --- a/canvas/source/vcl/textlayout.cxx +++ b/canvas/source/vcl/textlayout.cxx @@ -324,8 +324,6 @@ namespace vclcanvas sal_Int8 SAL_CALL TextLayout::getMainTextDirection( ) { - SolarMutexGuard aGuard; - return mnTextDirection; } @@ -338,8 +336,6 @@ namespace vclcanvas rendering::StringContext SAL_CALL TextLayout::getText( ) { - SolarMutexGuard aGuard; - return maText; } diff --git a/comphelper/source/streaming/seqstream.cxx b/comphelper/source/streaming/seqstream.cxx index 49eebc1a99c2..8aca6a6ea687 100644 --- a/comphelper/source/streaming/seqstream.cxx +++ b/comphelper/source/streaming/seqstream.cxx @@ -150,7 +150,6 @@ sal_Int64 SAL_CALL MemoryInputStream::getPosition() sal_Int64 SAL_CALL MemoryInputStream::getLength( ) { - std::scoped_lock aGuard( m_aMutex ); return m_nMemoryDataLength; } diff --git a/compilerplugins/clang/locking2.cxx b/compilerplugins/clang/locking2.cxx new file mode 100644 index 000000000000..d31b29b2023e --- /dev/null +++ b/compilerplugins/clang/locking2.cxx @@ -0,0 +1,716 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#if !defined _WIN32 //TODO, #include <sys/file.h> + +#include <cassert> +#include <string> +#include <iostream> +#include <fstream> +#include <unordered_set> +#include <vector> +#include <algorithm> +#include <optional> +#include <sys/file.h> +#include <unistd.h> + +#include "config_clang.h" + +#include "plugin.hxx" +#include "check.hxx" +#include "compat.hxx" + +#include "clang/AST/ParentMapContext.h" + +/** +Look for fields that are +(a) only assigned to in the constructor using field-init, and can therefore be const. +(b) protected via a mutex guard when accessed + +Which normally means we can remove the mutex guard. + +The process goes something like this: + $ make check + $ make FORCE_COMPILE=all COMPILER_PLUGIN_TOOL='locking2' check + $ ./compilerplugins/clang/locking2.py +*/ + +namespace +{ +struct MyFieldInfo +{ + std::string parentClass; + std::string fieldName; + std::string fieldType; + std::string sourceLocation; +}; +bool operator<(const MyFieldInfo& lhs, const MyFieldInfo& rhs) +{ + return std::tie(lhs.parentClass, lhs.fieldName) < std::tie(rhs.parentClass, rhs.fieldName); +} +struct MyLockedInfo +{ + const MemberExpr* memberExpr; + std::string parentClass; + std::string fieldName; + std::string sourceLocation; +}; +bool operator<(const MyLockedInfo& lhs, const MyLockedInfo& rhs) +{ + return std::tie(lhs.parentClass, lhs.fieldName, lhs.sourceLocation) + < std::tie(rhs.parentClass, rhs.fieldName, rhs.sourceLocation); +} + +// try to limit the voluminous output a little +static std::set<MyFieldInfo> cannotBeConstSet; +static std::set<MyFieldInfo> definitionSet; +static std::set<MyLockedInfo> lockedAtSet; + +/** + * Wrap the different kinds of callable and callee objects in the clang AST so I can define methods that handle everything. + */ +class CallerWrapper +{ + const CallExpr* m_callExpr; + const CXXConstructExpr* m_cxxConstructExpr; + +public: + CallerWrapper(const CallExpr* callExpr) + : m_callExpr(callExpr) + , m_cxxConstructExpr(nullptr) + { + } + CallerWrapper(const CXXConstructExpr* cxxConstructExpr) + : m_callExpr(nullptr) + , m_cxxConstructExpr(cxxConstructExpr) + { + } + unsigned getNumArgs() const + { + return m_callExpr ? m_callExpr->getNumArgs() : m_cxxConstructExpr->getNumArgs(); + } + const Expr* getArg(unsigned i) const + { + return m_callExpr ? m_callExpr->getArg(i) : m_cxxConstructExpr->getArg(i); + } +}; +class CalleeWrapper +{ + const FunctionDecl* m_calleeFunctionDecl = nullptr; + const CXXConstructorDecl* m_cxxConstructorDecl = nullptr; + const FunctionProtoType* m_functionPrototype = nullptr; + +public: + explicit CalleeWrapper(const FunctionDecl* calleeFunctionDecl) + : m_calleeFunctionDecl(calleeFunctionDecl) + { + } + explicit CalleeWrapper(const CXXConstructExpr* cxxConstructExpr) + : m_cxxConstructorDecl(cxxConstructExpr->getConstructor()) + { + } + explicit CalleeWrapper(const FunctionProtoType* functionPrototype) + : m_functionPrototype(functionPrototype) + { + } + unsigned getNumParams() const + { + if (m_calleeFunctionDecl) + return m_calleeFunctionDecl->getNumParams(); + else if (m_cxxConstructorDecl) + return m_cxxConstructorDecl->getNumParams(); + else if (m_functionPrototype->param_type_begin() == m_functionPrototype->param_type_end()) + // FunctionProtoType will assert if we call getParamTypes() and it has no params + return 0; + else + return m_functionPrototype->getParamTypes().size(); + } + const QualType getParamType(unsigned i) const + { + if (m_calleeFunctionDecl) + return m_calleeFunctionDecl->getParamDecl(i)->getType(); + else if (m_cxxConstructorDecl) + return m_cxxConstructorDecl->getParamDecl(i)->getType(); + else + return m_functionPrototype->getParamTypes()[i]; + } + std::string getNameAsString() const + { + if (m_calleeFunctionDecl) + return m_calleeFunctionDecl->getNameAsString(); + else if (m_cxxConstructorDecl) + return m_cxxConstructorDecl->getNameAsString(); + else + return ""; + } + CXXMethodDecl const* getAsCXXMethodDecl() const + { + if (m_calleeFunctionDecl) + return dyn_cast<CXXMethodDecl>(m_calleeFunctionDecl); + return nullptr; + } +}; + +class Locking2 : public RecursiveASTVisitor<Locking2>, public loplugin::Plugin +{ +public: + explicit Locking2(loplugin::InstantiationData const& data) + : Plugin(data) + { + } + + virtual void run() override; + + bool shouldVisitTemplateInstantiations() const { return true; } + bool shouldVisitImplicitCode() const { return true; } + + bool VisitFieldDecl(const FieldDecl*); + bool VisitMemberExpr(const MemberExpr*); + bool TraverseCXXConstructorDecl(CXXConstructorDecl*); + bool TraverseCXXMethodDecl(CXXMethodDecl*); + bool TraverseFunctionDecl(FunctionDecl*); + bool TraverseIfStmt(IfStmt*); + bool VisitCompoundStmt(const CompoundStmt*); + +private: + MyFieldInfo niceName(const FieldDecl*); + MyLockedInfo niceNameLocked(const MemberExpr*); + void check(const FieldDecl* fieldDecl, const Expr* memberExpr); + bool isSomeKindOfZero(const Expr* arg); + bool IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt* child, CallerWrapper callExpr, + CalleeWrapper calleeFunctionDecl); + compat::optional<CalleeWrapper> getCallee(CallExpr const*); + + RecordDecl* insideMoveOrCopyDeclParent = nullptr; + // For reasons I do not understand, parentFunctionDecl() is not reliable, so + // we store the parent function on the way down the AST. + FunctionDecl* insideFunctionDecl = nullptr; + std::vector<FieldDecl const*> insideConditionalCheckOfMemberSet; + + bool isSolarMutexLockGuardStmt(const Stmt*); + const CXXThisExpr* isOtherMutexLockGuardStmt(const Stmt*); +}; + +void Locking2::run() +{ + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + + // dump all our output in one write call - this is to try and limit IO "crosstalk" between multiple processes + // writing to the same logfile + if (!isUnitTestMode()) + { + std::string output; + for (const MyFieldInfo& s : cannotBeConstSet) + output += "write-outside-constructor:\t" + s.parentClass + "\t" + s.fieldName + "\n"; + for (const MyFieldInfo& s : definitionSet) + output += "definition:\t" + s.parentClass + "\t" + s.fieldName + "\t" + s.fieldType + + "\t" + s.sourceLocation + "\n"; + for (const MyLockedInfo& s : lockedAtSet) + output += "locked:\t" + s.parentClass + "\t" + s.fieldName + "\t" + s.sourceLocation + + "\n"; + std::ofstream myfile; + myfile.open(WORKDIR "/loplugin.locking2.log", std::ios::app | std::ios::out); + myfile << output; + myfile.close(); + } + else + { + for (const MyLockedInfo& s : lockedAtSet) + report(DiagnosticsEngine::Warning, "locked %0", s.memberExpr->getBeginLoc()) + << s.fieldName; + } +} + +MyFieldInfo Locking2::niceName(const FieldDecl* fieldDecl) +{ + MyFieldInfo aInfo; + + const RecordDecl* recordDecl = fieldDecl->getParent(); + + if (const CXXRecordDecl* cxxRecordDecl = dyn_cast<CXXRecordDecl>(recordDecl)) + { + if (cxxRecordDecl->getTemplateInstantiationPattern()) + cxxRecordDecl = cxxRecordDecl->getTemplateInstantiationPattern(); + aInfo.parentClass = cxxRecordDecl->getQualifiedNameAsString(); + } + else + { + aInfo.parentClass = recordDecl->getQualifiedNameAsString(); + } + + aInfo.fieldName = fieldDecl->getNameAsString(); + // sometimes the name (if it's an anonymous thing) contains the full path of the build folder, which we don't need + size_t idx = aInfo.fieldName.find(SRCDIR); + if (idx != std::string::npos) + { + aInfo.fieldName = aInfo.fieldName.replace(idx, strlen(SRCDIR), ""); + } + aInfo.fieldType = fieldDecl->getType().getAsString(); + + SourceLocation expansionLoc + = compiler.getSourceManager().getExpansionLoc(fieldDecl->getLocation()); + StringRef name = getFilenameOfLocation(expansionLoc); + aInfo.sourceLocation + = std::string(name.substr(strlen(SRCDIR) + 1)) + ":" + + std::to_string(compiler.getSourceManager().getSpellingLineNumber(expansionLoc)); + loplugin::normalizeDotDotInFilePath(aInfo.sourceLocation); + + return aInfo; +} + +MyLockedInfo Locking2::niceNameLocked(const MemberExpr* memberExpr) +{ + MyLockedInfo aInfo; + + const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl()); + const RecordDecl* recordDecl = fieldDecl->getParent(); + + if (const CXXRecordDecl* cxxRecordDecl = dyn_cast<CXXRecordDecl>(recordDecl)) + { + if (cxxRecordDecl->getTemplateInstantiationPattern()) + cxxRecordDecl = cxxRecordDecl->getTemplateInstantiationPattern(); + aInfo.parentClass = cxxRecordDecl->getQualifiedNameAsString(); + } + else + { + aInfo.parentClass = recordDecl->getQualifiedNameAsString(); + } + aInfo.memberExpr = memberExpr; + + aInfo.fieldName = fieldDecl->getNameAsString(); + // sometimes the name (if it's an anonymous thing) contains the full path of the build folder, which we don't need + size_t idx = aInfo.fieldName.find(SRCDIR); + if (idx != std::string::npos) + { + aInfo.fieldName = aInfo.fieldName.replace(idx, strlen(SRCDIR), ""); + } + + SourceLocation expansionLoc + = compiler.getSourceManager().getExpansionLoc(memberExpr->getBeginLoc()); + StringRef name = getFilenameOfLocation(expansionLoc); + aInfo.sourceLocation + = std::string(name.substr(strlen(SRCDIR) + 1)) + ":" + + std::to_string(compiler.getSourceManager().getSpellingLineNumber(expansionLoc)); + loplugin::normalizeDotDotInFilePath(aInfo.sourceLocation); + + return aInfo; +} + +bool Locking2::VisitFieldDecl(const FieldDecl* fieldDecl) +{ + fieldDecl = fieldDecl->getCanonicalDecl(); + if (ignoreLocation(fieldDecl)) + { + return true; + } + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocation()))) + { + return true; + } + definitionSet.insert(niceName(fieldDecl)); + return true; +} + +bool Locking2::TraverseCXXConstructorDecl(CXXConstructorDecl* cxxConstructorDecl) +{ + auto copy = insideMoveOrCopyDeclParent; + if (!ignoreLocation(cxxConstructorDecl) && cxxConstructorDecl->isThisDeclarationADefinition()) + { + if (cxxConstructorDecl->isCopyOrMoveConstructor()) + insideMoveOrCopyDeclParent = cxxConstructorDecl->getParent(); + } + bool ret = RecursiveASTVisitor::TraverseCXXConstructorDecl(cxxConstructorDecl); + insideMoveOrCopyDeclParent = copy; + return ret; +} + +bool Locking2::TraverseCXXMethodDecl(CXXMethodDecl* cxxMethodDecl) +{ + auto copy1 = insideMoveOrCopyDeclParent; + auto copy2 = insideFunctionDecl; + if (!ignoreLocation(cxxMethodDecl) && cxxMethodDecl->isThisDeclarationADefinition()) + { + if (cxxMethodDecl->isCopyAssignmentOperator() || cxxMethodDecl->isMoveAssignmentOperator()) + insideMoveOrCopyDeclParent = cxxMethodDecl->getParent(); + } + insideFunctionDecl = cxxMethodDecl; + bool ret = RecursiveASTVisitor::TraverseCXXMethodDecl(cxxMethodDecl); + insideMoveOrCopyDeclParent = copy1; + insideFunctionDecl = copy2; + return ret; +} + +bool Locking2::TraverseFunctionDecl(FunctionDecl* functionDecl) +{ + auto copy2 = insideFunctionDecl; + insideFunctionDecl = functionDecl; + bool ret = RecursiveASTVisitor::TraverseFunctionDecl(functionDecl); + insideFunctionDecl = copy2; + return ret; +} + +bool Locking2::TraverseIfStmt(IfStmt* ifStmt) +{ + FieldDecl const* memberFieldDecl = nullptr; + if (Expr const* cond = ifStmt->getCond()) + { + if (auto memberExpr = dyn_cast<MemberExpr>(cond->IgnoreParenImpCasts())) + { + if ((memberFieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl()))) + insideConditionalCheckOfMemberSet.push_back(memberFieldDecl); + } + } + bool ret = RecursiveASTVisitor::TraverseIfStmt(ifStmt); + if (memberFieldDecl) + insideConditionalCheckOfMemberSet.pop_back(); + return ret; +} + +bool Locking2::VisitMemberExpr(const MemberExpr* memberExpr) +{ + const ValueDecl* decl = memberExpr->getMemberDecl(); + const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(decl); + if (!fieldDecl) + { + return true; + } + fieldDecl = fieldDecl->getCanonicalDecl(); + if (ignoreLocation(fieldDecl)) + { + return true; + } + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocation()))) + { + return true; + } + + check(fieldDecl, memberExpr); + + return true; +} + +void Locking2::check(const FieldDecl* fieldDecl, const Expr* memberExpr) +{ + auto parentsRange = compiler.getASTContext().getParents(*memberExpr); + const Stmt* child = memberExpr; + const Stmt* parent + = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>(); + // walk up the tree until we find something interesting + bool bCannotBeConst = false; + bool bDump = false; + auto walkUp = [&]() { + child = parent; + auto parentsRange = compiler.getASTContext().getParents(*parent); + parent = parentsRange.begin() == parentsRange.end() ? nullptr + : parentsRange.begin()->get<Stmt>(); + }; + do + { + if (!parent) + { + // check if we have an expression like + // int& r = m_field; + auto parentsRange = compiler.getASTContext().getParents(*child); + if (parentsRange.begin() != parentsRange.end()) + { + auto varDecl = dyn_cast_or_null<VarDecl>(parentsRange.begin()->get<Decl>()); + // The isImplicit() call is to avoid triggering when we see the vardecl which is part of a for-range statement, + // which is of type 'T&&' and also an l-value-ref ? + if (varDecl && !varDecl->isImplicit() + && loplugin::TypeCheck(varDecl->getType()).LvalueReference().NonConst()) + { + bCannotBeConst = true; + } + } + break; + } + if (isa<CXXReinterpretCastExpr>(parent)) + { + // once we see one of these, there is not much useful we can know + bCannotBeConst = true; + break; + } + else if (isa<CastExpr>(parent) || isa<MemberExpr>(parent) || isa<ParenExpr>(parent) + || isa<ParenListExpr>(parent) || isa<ArrayInitLoopExpr>(parent) + || isa<ExprWithCleanups>(parent)) + { + walkUp(); + } + else if (auto unaryOperator = dyn_cast<UnaryOperator>(parent)) + { + UnaryOperator::Opcode op = unaryOperator->getOpcode(); + if (op == UO_AddrOf || op == UO_PostInc || op == UO_PostDec || op == UO_PreInc + || op == UO_PreDec) + { + bCannotBeConst = true; + } + walkUp(); + } + else if (auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(parent)) + { + auto callee = getCallee(operatorCallExpr); + if (callee) + { + // if calling a non-const operator on the field + auto calleeMethodDecl = callee->getAsCXXMethodDecl(); + if (calleeMethodDecl && operatorCallExpr->getArg(0) == child + && !calleeMethodDecl->isConst()) + { + bCannotBeConst = true; + } + else if (IsPassedByNonConst(fieldDecl, child, operatorCallExpr, *callee)) + { + bCannotBeConst = true; + } + } + else + bCannotBeConst = true; // conservative, could improve + walkUp(); + } + else if (auto cxxMemberCallExpr = dyn_cast<CXXMemberCallExpr>(parent)) + { + const CXXMethodDecl* calleeMethodDecl = cxxMemberCallExpr->getMethodDecl(); + if (calleeMethodDecl) + { + // if calling a non-const method on the field + const Expr* tmp = dyn_cast<Expr>(child); + if (tmp->isBoundMemberFunction(compiler.getASTContext())) + { + tmp = dyn_cast<MemberExpr>(tmp)->getBase(); + } + if (cxxMemberCallExpr->getImplicitObjectArgument() == tmp + && !calleeMethodDecl->isConst()) + { + bCannotBeConst = true; + break; + } + if (IsPassedByNonConst(fieldDecl, child, cxxMemberCallExpr, + CalleeWrapper(calleeMethodDecl))) + bCannotBeConst = true; + } + else + bCannotBeConst = true; // can happen in templates + walkUp(); + } + else if (auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(parent)) + { + if (IsPassedByNonConst(fieldDecl, child, cxxConstructExpr, + CalleeWrapper(cxxConstructExpr))) + bCannotBeConst = true; + walkUp(); + } + else if (auto callExpr = dyn_cast<CallExpr>(parent)) + { + auto callee = getCallee(callExpr); + if (callee) + { + if (IsPassedByNonConst(fieldDecl, child, callExpr, *callee)) + bCannotBeConst = true; + } + else + bCannotBeConst = true; // conservative, could improve + walkUp(); + } + else if (auto binaryOp = dyn_cast<BinaryOperator>(parent)) + { + BinaryOperator::Opcode op = binaryOp->getOpcode(); + const bool assignmentOp = op == BO_Assign || op == BO_MulAssign || op == BO_DivAssign + || op == BO_RemAssign || op == BO_AddAssign + || op == BO_SubAssign || op == BO_ShlAssign + || op == BO_ShrAssign || op == BO_AndAssign + || op == BO_XorAssign || op == BO_OrAssign; + if (assignmentOp) + { + if (binaryOp->getLHS() == child) + bCannotBeConst = true; + else if (loplugin::TypeCheck(binaryOp->getLHS()->getType()) + .LvalueReference() + .NonConst()) + // if the LHS is a non-const reference, we could write to the field later on + bCannotBeConst = true; + } + walkUp(); + } + else if (isa<ReturnStmt>(parent)) + { + if (insideFunctionDecl) + { + auto tc = loplugin::TypeCheck(insideFunctionDecl->getReturnType()); + if (tc.LvalueReference().NonConst()) + bCannotBeConst = true; + } + break; + } + else if (isa<SwitchStmt>(parent) || isa<WhileStmt>(parent) || isa<ForStmt>(parent) + || isa<IfStmt>(parent) || isa<DoStmt>(parent) || isa<CXXForRangeStmt>(parent) + || isa<DefaultStmt>(parent)) + { + break; + } + else + { + walkUp(); + } + } while (true); + + if (bDump) + { + report(DiagnosticsEngine::Warning, "oh dear, what can the matter be? writtenTo=%0", + memberExpr->getBeginLoc()) + << bCannotBeConst << memberExpr->getSourceRange(); + if (parent) + { + report(DiagnosticsEngine::Note, "parent over here", parent->getBeginLoc()) + << parent->getSourceRange(); + parent->dump(); + } + memberExpr->dump(); + fieldDecl->getType()->dump(); + } + + if (bCannotBeConst) + { + cannotBeConstSet.insert(niceName(fieldDecl)); + } +} + +bool Locking2::IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt* child, + CallerWrapper callExpr, CalleeWrapper calleeFunctionDecl) +{ + unsigned len = std::min(callExpr.getNumArgs(), calleeFunctionDecl.getNumParams()); + // if it's an array, passing it by value to a method typically means the + // callee takes a pointer and can modify the array + if (fieldDecl->getType()->isConstantArrayType()) + { + for (unsigned i = 0; i < len; ++i) + if (callExpr.getArg(i) == child) + if (loplugin::TypeCheck(calleeFunctionDecl.getParamType(i)).Pointer().NonConst()) + return true; + } + else + { + for (unsigned i = 0; i < len; ++i) + if (callExpr.getArg(i) == child) + if (loplugin::TypeCheck(calleeFunctionDecl.getParamType(i)) + .LvalueReference() + .NonConst()) + return true; + } + return false; +} + +compat::optional<CalleeWrapper> Locking2::getCallee(CallExpr const* callExpr) +{ + FunctionDecl const* functionDecl = callExpr->getDirectCallee(); + if (functionDecl) + return CalleeWrapper(functionDecl); + + // Extract the functionprototype from a type + clang::Type const* calleeType = callExpr->getCallee()->getType().getTypePtr(); + if (auto pointerType = calleeType->getUnqualifiedDesugaredType()->getAs<clang::PointerType>()) + { + if (auto prototype = pointerType->getPointeeType() + ->getUnqualifiedDesugaredType() + ->getAs<FunctionProtoType>()) + { + return CalleeWrapper(prototype); + } + } + + return compat::optional<CalleeWrapper>(); +} + +bool Locking2::VisitCompoundStmt(const CompoundStmt* compoundStmt) +{ + if (ignoreLocation(compoundStmt)) + return true; + if (compoundStmt->size() < 2) + return true; + + const Stmt* firstStmt = *compoundStmt->body_begin(); + bool solarMutex = isSolarMutexLockGuardStmt(firstStmt); + const CXXThisExpr* ignoreThisStmt = nullptr; + if (!solarMutex) + ignoreThisStmt = isOtherMutexLockGuardStmt(firstStmt); + if (!solarMutex && ignoreThisStmt == nullptr) + return true; + const ReturnStmt* returnStmt = dyn_cast<ReturnStmt>(*(compoundStmt->body_begin() + 1)); + if (!returnStmt || !returnStmt->getRetValue()) + return true; + const Expr* retValue = returnStmt->getRetValue()->IgnoreImplicit(); + if (auto constructExpr = dyn_cast<CXXConstructExpr>(retValue)) + retValue = constructExpr->getArg(0)->IgnoreImplicit(); + const MemberExpr* memberExpr = dyn_cast<MemberExpr>(retValue); + if (!memberExpr) + return true; + + lockedAtSet.insert(niceNameLocked(memberExpr)); + + return true; +} + +bool Locking2::isSolarMutexLockGuardStmt(const Stmt* stmt) +{ + auto declStmt = dyn_cast<DeclStmt>(stmt); + if (!declStmt) + return false; + if (!declStmt->isSingleDecl()) + return false; + auto varDecl = dyn_cast<VarDecl>(declStmt->getSingleDecl()); + if (!varDecl) + return false; + auto tc = loplugin::TypeCheck(varDecl->getType()); + if (!tc.Class("SolarMutexGuard").GlobalNamespace() + && !tc.Class("SolarMutexClearableGuard").GlobalNamespace() + && !tc.Class("SolarMutexResettableGuard").GlobalNamespace() + && !tc.Class("SolarMutexTryAndBuyGuard").GlobalNamespace()) + return false; + return true; +} + +const CXXThisExpr* Locking2::isOtherMutexLockGuardStmt(const Stmt* stmt) +{ + auto declStmt = dyn_cast<DeclStmt>(stmt); + if (!declStmt) + return nullptr; + if (!declStmt->isSingleDecl()) + return nullptr; + auto varDecl = dyn_cast<VarDecl>(declStmt->getSingleDecl()); + if (!varDecl) + return nullptr; + auto tc = loplugin::TypeCheck(varDecl->getType()); + if (!tc.Class("unique_lock").StdNamespace() && !tc.Class("scoped_lock").StdNamespace() + && !tc.Class("Guard") && !tc.Class("ClearableGuard") && !tc.Class("ResettableGuard")) + return nullptr; + auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(varDecl->getInit()); + if (!cxxConstructExpr || cxxConstructExpr->getNumArgs() < 1) + return nullptr; + auto arg0 = cxxConstructExpr->getArg(0); + if (auto memberExpr = dyn_cast<MemberExpr>(arg0)) + { + const CXXThisExpr* thisStmt + = dyn_cast<CXXThisExpr>(memberExpr->getBase()->IgnoreImplicit()); + return thisStmt; + } + else if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(arg0)) + { + return dyn_cast_or_null<CXXThisExpr>( + memberCallExpr->getImplicitObjectArgument()->IgnoreImplicit()); + } + return nullptr; +} + +loplugin::Plugin::Registration<Locking2> X("locking2", false); +} + +#endif + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/locking2.py b/compilerplugins/clang/locking2.py new file mode 100755 index 000000000000..e6c6176052d1 --- /dev/null +++ b/compilerplugins/clang/locking2.py @@ -0,0 +1,79 @@ +#!/usr/bin/python3 + +import re +import io + +definitionToTypeMap = dict() +writeFromOutsideConstructorSet = set() +lockedMap = dict() + +# clang does not always use exactly the same numbers in the type-parameter vars it generates +# so I need to substitute them to ensure we can match correctly. +normalizeTypeParamsRegex = re.compile(r"type-parameter-\d+-\d+") +def normalizeTypeParams( line ): + return normalizeTypeParamsRegex.sub("type-parameter-?-?", line) + +def parseFieldInfo( tokens ): + if len(tokens) == 3: + return (normalizeTypeParams(tokens[1]), tokens[2]) + else: + return (normalizeTypeParams(tokens[1]), "") + +with io.open("workdir/loplugin.locking2.log", "r", buffering=1024*1024) as txt: + for line in txt: + tokens = line.strip().split("\t") + if tokens[0] == "definition:": + fieldInfo = (normalizeTypeParams(tokens[1]), tokens[2]) + srcLoc = tokens[4] + # ignore external source code + if (srcLoc.startswith("external/")): + continue + # ignore build folder + if (srcLoc.startswith("workdir/")): + continue + definitionToTypeMap[fieldInfo] = tokens[3] + elif tokens[0] == "write-outside-constructor:": + writeFromOutsideConstructorSet.add(parseFieldInfo(tokens)) + elif tokens[0] == "locked:": + fieldInfo = (normalizeTypeParams(tokens[1]), tokens[2]) + lockedMap[fieldInfo] = tokens[3] + else: + print( "unknown line: " + line) + +removeLockingSet = set() +for field,srcLoc in lockedMap.items(): + if field in writeFromOutsideConstructorSet: + continue + if field not in definitionToTypeMap: + continue + fieldType = definitionToTypeMap[field] + if "std::unique_ptr" in fieldType: + continue + if "std::shared_ptr" in fieldType: + continue + if "Reference<" in fieldType: + continue + if "VclPtr<" in fieldType: + continue + removeLockingSet.add((field[0] + " " + field[1] + " " + fieldType, srcLoc)) + + +# sort the results using a "natural order" so sequences like [item1,item2,item10] sort nicely +def natural_sort_key(s, _nsre=re.compile('([0-9]+)')): + return [int(text) if text.isdigit() else text.lower() + for text in re.split(_nsre, s)] +# sort by both the source-line and the datatype, so the output file ordering is stable +# when we have multiple items on the same source line +def v_sort_key(v): + return natural_sort_key(v[1]) + [v[0]] + +# sort results by name and line number +tmp6list = sorted(removeLockingSet, key=lambda v: v_sort_key(v)) + +# print out the results +with open("compilerplugins/clang/locking2.results", "wt") as f: + for t in tmp6list: + f.write( t[1] + "\n" ) + f.write( " " + t[0] + "\n" ) + + diff --git a/compilerplugins/clang/test/locking2.cxx b/compilerplugins/clang/test/locking2.cxx new file mode 100644 index 000000000000..3ef1a11228b6 --- /dev/null +++ b/compilerplugins/clang/test/locking2.cxx @@ -0,0 +1,50 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include <mutex> +#include <memory> +#include <osl/mutex.hxx> + +static std::mutex gSolarMutex; + +class SolarMutexGuard +{ + std::unique_lock<std::mutex> lock; + +public: + SolarMutexGuard() + : lock(gSolarMutex) + { + } +}; + +namespace test2 +{ +struct Foo +{ + std::mutex m_aMutex; + int m_foo; + std::shared_ptr<Foo> m_foo2; + + int bar3() + { + std::scoped_lock guard(m_aMutex); + // expected-error@+1 {{locked m_foo [loplugin:locking2]}} + return m_foo; + } + std::shared_ptr<Foo> bar4() + { + std::scoped_lock guard(m_aMutex); + // expected-error@+1 {{locked m_foo2 [loplugin:locking2]}} + return m_foo2; + } +}; +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/framework/source/uielement/statusbaritem.cxx b/framework/source/uielement/statusbaritem.cxx index baf321fd61c8..d9c4b2ccfd21 100644 --- a/framework/source/uielement/statusbaritem.cxx +++ b/framework/source/uielement/statusbaritem.cxx @@ -84,13 +84,11 @@ void StatusbarItem::disposing(std::unique_lock<std::mutex>&) OUString SAL_CALL StatusbarItem::getCommand() { - std::unique_lock aGuard( m_aMutex ); return m_aCommand; } ::sal_uInt16 SAL_CALL StatusbarItem::getItemId() { - std::unique_lock aGuard( m_aMutex ); return m_nId; } diff --git a/sc/source/ui/unoobj/PivotTableDataSequence.cxx b/sc/source/ui/unoobj/PivotTableDataSequence.cxx index 4acaf4dec05c..d1a3c278c6fc 100644 --- a/sc/source/ui/unoobj/PivotTableDataSequence.cxx +++ b/sc/source/ui/unoobj/PivotTableDataSequence.cxx @@ -132,8 +132,6 @@ uno::Sequence<OUString> SAL_CALL PivotTableDataSequence::getTextualData() OUString SAL_CALL PivotTableDataSequence::getSourceRangeRepresentation() { - SolarMutexGuard aGuard; - return m_aID; } diff --git a/sc/source/ui/unoobj/linkuno.cxx b/sc/source/ui/unoobj/linkuno.cxx index 63345c0c9761..8ee0910c3d13 100644 --- a/sc/source/ui/unoobj/linkuno.cxx +++ b/sc/source/ui/unoobj/linkuno.cxx @@ -1050,7 +1050,6 @@ void SAL_CALL ScDDELinkObj::setName( const OUString& /* aName */ ) OUString SAL_CALL ScDDELinkObj::getApplication() { - SolarMutexGuard aGuard; //! Test if the link is still in the document? return aAppl; @@ -1058,7 +1057,6 @@ OUString SAL_CALL ScDDELinkObj::getApplication() OUString SAL_CALL ScDDELinkObj::getTopic() { - SolarMutexGuard aGuard; //! Test if the link is still in the document? return aTopic; @@ -1066,7 +1064,6 @@ OUString SAL_CALL ScDDELinkObj::getTopic() OUString SAL_CALL ScDDELinkObj::getItem() { - SolarMutexGuard aGuard; //! Test if the link is still in the document? return aItem; diff --git a/scripting/source/basprov/baslibnode.cxx b/scripting/source/basprov/baslibnode.cxx index 9d6511dfcac3..566883757b48 100644 --- a/scripting/source/basprov/baslibnode.cxx +++ b/scripting/source/basprov/baslibnode.cxx @@ -68,8 +68,6 @@ namespace basprov OUString BasicLibraryNodeImpl::getName( ) { - SolarMutexGuard aGuard; - return m_sLibName; } diff --git a/sfx2/source/sidebar/UnoDeck.cxx b/sfx2/source/sidebar/UnoDeck.cxx index a7c9ff00d8fd..a2c45b27706a 100644 --- a/sfx2/source/sidebar/UnoDeck.cxx +++ b/sfx2/source/sidebar/UnoDeck.cxx @@ -37,8 +37,6 @@ SidebarController* SfxUnoDeck::getSidebarController() OUString SAL_CALL SfxUnoDeck::getId() { - SolarMutexGuard aGuard; - return mDeckId; } diff --git a/sfx2/source/sidebar/UnoPanels.cxx b/sfx2/source/sidebar/UnoPanels.cxx index de1823d00cc2..85416c046067 100644 --- a/sfx2/source/sidebar/UnoPanels.cxx +++ b/sfx2/source/sidebar/UnoPanels.cxx @@ -38,8 +38,6 @@ SidebarController* SfxUnoPanels::getSidebarController() OUString SAL_CALL SfxUnoPanels::getDeckId() { - SolarMutexGuard aGuard; - return mDeckId; } diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk index db95241b4585..cb48a66f4884 100644 --- a/solenv/CompilerTest_compilerplugins_clang.mk +++ b/solenv/CompilerTest_compilerplugins_clang.mk @@ -42,6 +42,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \ compilerplugins/clang/test/implinheritancehelper \ compilerplugins/clang/test/indentation \ compilerplugins/clang/test/intvsfloat \ + compilerplugins/clang/test/locking2 \ compilerplugins/clang/test/logexceptionnicely \ compilerplugins/clang/test/loopvartoosmall \ compilerplugins/clang/test/mapindex \ diff --git a/svtools/source/control/accessibleruler.cxx b/svtools/source/control/accessibleruler.cxx index 6bb9b63e4b07..cf7436f2d034 100644 --- a/svtools/source/control/accessibleruler.cxx +++ b/svtools/source/control/accessibleruler.cxx @@ -181,7 +181,6 @@ OUString SAL_CALL SvtRulerAccessible::getAccessibleDescription() OUString SAL_CALL SvtRulerAccessible::getAccessibleName() { - ::osl::MutexGuard aGuard( m_aMutex ); return msName; } diff --git a/svx/source/accessibility/svxrectctaccessiblecontext.cxx b/svx/source/accessibility/svxrectctaccessiblecontext.cxx index 6736cf5a7e5e..84d66aa33c0b 100644 --- a/svx/source/accessibility/svxrectctaccessiblecontext.cxx +++ b/svx/source/accessibility/svxrectctaccessiblecontext.cxx @@ -466,13 +466,11 @@ sal_Int16 SAL_CALL SvxRectCtlChildAccessibleContext::getAccessibleRole() OUString SAL_CALL SvxRectCtlChildAccessibleContext::getAccessibleDescription() { - ::osl::MutexGuard aGuard( m_aMutex ); return msDescription; } OUString SAL_CALL SvxRectCtlChildAccessibleContext::getAccessibleName() { - ::osl::MutexGuard aGuard( m_aMutex ); return msName; } |