diff options
author | Noel Grandin <noel.grandin@collabora.co.uk> | 2017-12-01 15:24:30 +0200 |
---|---|---|
committer | Noel Grandin <noel.grandin@collabora.co.uk> | 2017-12-04 07:24:52 +0100 |
commit | 68f86457525c60f580954280d1a759aa174e8e96 (patch) | |
tree | d4f0d44d52cf3e744f1555074217e0f42007ac71 /compilerplugins | |
parent | 9634a818e8a9432db52bc8fcd534e7437e6bacee (diff) |
new loplugin salcall: remove unnecessary SAL_CALL
In this first commit, I use the plugin to verify the consistency of our
SAL_CALL annotations.
The point being to make the next commit more mechanical in nature,
purely using the rewriter.
There are various chunks of unix-only code that have never had to be
compiled by MSVC, hence the inconsistencies.
In bridges, I had to inline some typedefs to make the verification code
happy, since it cannot see into typedefs.
Change-Id: Iec6e274bed857febf7295cfcf5e9f21fe4a34da0
Reviewed-on: https://gerrit.libreoffice.org/45502
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Diffstat (limited to 'compilerplugins')
-rw-r--r-- | compilerplugins/clang/salcall.cxx | 370 | ||||
-rw-r--r-- | compilerplugins/clang/test/salcall.cxx | 104 |
2 files changed, 474 insertions, 0 deletions
diff --git a/compilerplugins/clang/salcall.cxx b/compilerplugins/clang/salcall.cxx new file mode 100644 index 000000000000..f982d7f41b11 --- /dev/null +++ b/compilerplugins/clang/salcall.cxx @@ -0,0 +1,370 @@ +/* -*- 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/. + */ + +#include "plugin.hxx" +#include "check.hxx" +#include <cassert> +#include <string> +#include <iostream> +#include <fstream> + +// The SAL_CALL function annotation is only necessary on our outward +// facing C++ ABI, anywhere else it is just cargo-cult. +// + +namespace +{ +//static bool startswith(const std::string& rStr, const char* pSubStr) +//{ +// return rStr.compare(0, strlen(pSubStr), pSubStr) == 0; +//} + +class SalCall final : public RecursiveASTVisitor<SalCall>, public loplugin::RewritePlugin +{ +public: + explicit SalCall(loplugin::InstantiationData const& data) + : RewritePlugin(data) + { + } + + virtual void run() override + { + std::string fn(compiler.getSourceManager() + .getFileEntryForID(compiler.getSourceManager().getMainFileID()) + ->getName()); + loplugin::normalizeDotDotInFilePath(fn); + // ignore this one. I can't get accurate source code from getCharacterData() for it. + if (fn == SRCDIR "/sal/rtl/string.cxx") + return; + m_phase = PluginPhase::FindAddressOf; + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + m_phase = PluginPhase::Warning; + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + } + + bool VisitFunctionDecl(FunctionDecl const*); + bool VisitUnaryAddrOf(UnaryOperator const*); + bool VisitInitListExpr(InitListExpr const*); + bool VisitCallExpr(CallExpr const*); + bool VisitBinAssign(BinaryOperator const*); + bool VisitCXXConstructExpr(CXXConstructExpr const*); + +private: + void checkForFunctionDecl(Expr const*, bool bCheckOnly = false); + bool rewrite(SourceLocation); + bool checkOverlap(SourceRange); + bool isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation* pLoc = nullptr); + + std::set<FunctionDecl const*> m_addressOfSet; + enum class PluginPhase + { + FindAddressOf, + Warning + }; + PluginPhase m_phase; + std::vector<std::pair<char const*, char const*>> mvModifiedRanges; +}; + +bool SalCall::VisitUnaryAddrOf(UnaryOperator const* op) +{ + if (m_phase != PluginPhase::FindAddressOf) + return true; + checkForFunctionDecl(op->getSubExpr()); + return true; +} + +bool SalCall::VisitBinAssign(BinaryOperator const* binaryOperator) +{ + if (m_phase != PluginPhase::FindAddressOf) + return true; + checkForFunctionDecl(binaryOperator->getRHS()); + return true; +} + +bool SalCall::VisitCallExpr(CallExpr const* callExpr) +{ + if (m_phase != PluginPhase::FindAddressOf) + return true; + for (auto arg : callExpr->arguments()) + checkForFunctionDecl(arg); + return true; +} + +bool SalCall::VisitCXXConstructExpr(CXXConstructExpr const* constructExpr) +{ + if (m_phase != PluginPhase::FindAddressOf) + return true; + for (auto arg : constructExpr->arguments()) + checkForFunctionDecl(arg); + return true; +} + +bool SalCall::VisitInitListExpr(InitListExpr const* initListExpr) +{ + if (m_phase != PluginPhase::FindAddressOf) + return true; + for (auto subStmt : *initListExpr) + checkForFunctionDecl(dyn_cast<Expr>(subStmt)); + return true; +} + +void SalCall::checkForFunctionDecl(Expr const* expr, bool bCheckOnly) +{ + auto e1 = expr->IgnoreParenCasts(); + auto declRef = dyn_cast<DeclRefExpr>(e1); + if (!declRef) + return; + auto functionDecl = dyn_cast<FunctionDecl>(declRef->getDecl()); + if (!functionDecl) + return; + if (bCheckOnly) + getParentStmt(expr)->dump(); + else + m_addressOfSet.insert(functionDecl->getCanonicalDecl()); +} + +bool SalCall::VisitFunctionDecl(FunctionDecl const* decl) +{ + if (m_phase != PluginPhase::Warning) + return true; + if (ignoreLocation(decl)) + return true; + + // ignore template stuff + if (decl->getTemplatedKind() != clang::FunctionDecl::TK_NonTemplate) + return true; + auto recordDecl = dyn_cast<CXXRecordDecl>(decl->getDeclContext()); + if (recordDecl + && (recordDecl->getTemplateSpecializationKind() != TSK_Undeclared + || recordDecl->isDependentContext())) + { + return true; + } + + auto canonicalDecl = decl->getCanonicalDecl(); + + // ignore UNO implementations + if (isInUnoIncludeFile( + compiler.getSourceManager().getSpellingLoc(canonicalDecl->getLocation()))) + return true; + + // macros make getCharacterData() extremely unreliable + if (compiler.getSourceManager().isMacroArgExpansion(decl->getLocation()) + || compiler.getSourceManager().isMacroBodyExpansion(decl->getLocation())) + return true; + + SourceLocation rewriteLoc; + SourceLocation rewriteCanonicalLoc; + bool bDeclIsSalCall = isSalCallFunction(decl, &rewriteLoc); + bool bCanonicalDeclIsSalCall = isSalCallFunction(canonicalDecl, &rewriteCanonicalLoc); + + // first, check for consistency, so we don't trip ourselves up on Linux, where we normally run the plugin + if (canonicalDecl != decl) + { + if (bCanonicalDeclIsSalCall) + ; // this is fine, the actual definition have or not have SAL_CALL, and MSVC is fine with it + else if (bDeclIsSalCall) + { + // not fine + report(DiagnosticsEngine::Warning, "SAL_CALL inconsistency", + canonicalDecl->getLocation()) + << canonicalDecl->getSourceRange(); + report(DiagnosticsEngine::Note, "SAL_CALL inconsistency", decl->getLocation()) + << decl->getSourceRange(); + return true; + } + } + auto methodDecl = dyn_cast<CXXMethodDecl>(canonicalDecl); + if (methodDecl) + { + for (auto iter = methodDecl->begin_overridden_methods(); + iter != methodDecl->end_overridden_methods(); ++iter) + { + const CXXMethodDecl* overriddenMethod = (*iter)->getCanonicalDecl(); + if (bCanonicalDeclIsSalCall != isSalCallFunction(overriddenMethod)) + { + report(DiagnosticsEngine::Warning, "SAL_CALL inconsistency", + methodDecl->getLocation()) + << methodDecl->getSourceRange(); + report(DiagnosticsEngine::Note, "SAL_CALL inconsistency", + overriddenMethod->getLocation()) + << overriddenMethod->getSourceRange(); + return true; + } + } + } + + if (!bDeclIsSalCall) + return true; + + // @TODO For now, I am ignore free functions, since those are most likely to have their address taken. + // I'll do these later. They are harder to verify since MSVC does not verify when assigning to function pointers + // that the calling convention of the function matches the calling convention of the function pointer! + if (!methodDecl || methodDecl->isStatic()) + return true; + + // can only check when we have a definition since this is the most likely time + // when the address of the method will be taken + if (!(methodDecl && methodDecl->isPure()) && !decl->isThisDeclarationADefinition()) + return true; + if (m_addressOfSet.find(decl->getCanonicalDecl()) != m_addressOfSet.end()) + return true; + + // ignore extern "C" UNO factory constructor functions + if (decl->isExternC()) + { + if (loplugin::TypeCheck(decl->getReturnType()) + .Pointer() + .Class("XInterface") + .Namespace("uno") + .Namespace("star") + .Namespace("sun") + .Namespace("com") + .GlobalNamespace()) + return true; + if (loplugin::TypeCheck(decl->getReturnType()).Pointer().Void()) + return true; + } + + // some base classes are overridden by sub-classes which override both the base-class and an UNO class + if (recordDecl) + { + if (loplugin::DeclCheck(recordDecl) + .Class("OProxyAggregation") + .Namespace("comphelper") + .GlobalNamespace() + || loplugin::DeclCheck(recordDecl) + .Class("OComponentProxyAggregationHelper") + .Namespace("comphelper") + .GlobalNamespace() + || loplugin::DeclCheck(recordDecl).Class("SvxShapeMaster").GlobalNamespace()) + return true; + } + + if (methodDecl) + { + for (auto iter = methodDecl->begin_overridden_methods(); + iter != methodDecl->end_overridden_methods(); ++iter) + { + const CXXMethodDecl* overriddenMethod = (*iter)->getCanonicalDecl(); + if (isSalCallFunction(overriddenMethod)) + return true; + } + } + + /* + bool bOK = rewrite(rewriteLoc); + if (bOK && canonicalDecl != decl) + { + bOK = rewrite(rewriteCanonicalLoc); + } + if (bOK) + return true; + + //std::cout << "xxx:" << std::string(p1, leftBracket - p1) << std::endl; + report(DiagnosticsEngine::Warning, "SAL_CALL unnecessary here", rewriteLoc) + << decl->getSourceRange(); + if (canonicalDecl != decl) + report(DiagnosticsEngine::Warning, "SAL_CALL unnecessary here", rewriteCanonicalLoc) + << canonicalDecl->getSourceRange(); +*/ + return true; +} + +bool SalCall::isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation* pLoc) +{ + // In certain situations, in header files, clang will return bogus range data + // from decl->getSourceRange(). + // Specifically, for the + // LNG_DLLPUBLIC CapType SAL_CALL capitalType(const OUString&, CharClass const *); + // declaration in + // include/linguistic/misc.hxx + // it looks like it is returning data from definition of the LNG_DLLPUBLIC macro. + // I suspect something inside clang is calling getSpellingLoc() once too often. + // + // So I use getReturnTypeSourceRange() for the start of the range + // instead, and search for the "(" in the function decl. + + SourceRange range = functionDecl->getSourceRange(); + SourceManager& SM = compiler.getSourceManager(); + SourceLocation startLoc = functionDecl->getReturnTypeSourceRange().getBegin(); + SourceLocation endLoc = range.getEnd(); + if (!startLoc.isValid() || !endLoc.isValid()) + return false; + char const* p1 = SM.getCharacterData(startLoc); + char const* p2 = SM.getCharacterData(endLoc); + + // if (functionDecl->getIdentifier() && functionDecl->getName() == "capitalType") + // { + // std::cout << "xxxx " << (long)p1 << " " << (long)p2 << " " << (int)(p2-p1) << std::endl; + // std::cout << " " << std::string(p1, 80) << std::endl; + // report(DiagnosticsEngine::Warning, "jhjkahdashdkash", functionDecl->getLocation()) + // << functionDecl->getSourceRange(); + // } + // + static const char* SAL_CALL = "SAL_CALL"; + + char const* leftBracket = static_cast<char const*>(memchr(p1, '(', p2 - p1)); + if (!leftBracket) + return false; + + char const* found = std::search(p1, leftBracket, SAL_CALL, SAL_CALL + strlen(SAL_CALL)); + + if (found >= leftBracket) + return false; + + if (pLoc) + // the -1 is to remove the space before the SAL_CALL + *pLoc = startLoc.getLocWithOffset(found - p1 - 1); + + return true; +} + +bool SalCall::rewrite(SourceLocation locBegin) +{ + if (!rewriter) + return false; + + auto locEnd = locBegin.getLocWithOffset(8); + + SourceRange range(locBegin, locEnd); + + // If we overlap with a previous area we modified, we cannot perform this change + // without corrupting the source + if (!checkOverlap(range)) + return false; + + if (!replaceText(locBegin, 9, "")) + return false; + + return true; +} + +// If we overlap with a previous area we modified, we cannot perform this change +// without corrupting the source +bool SalCall::checkOverlap(SourceRange range) +{ + SourceManager& SM = compiler.getSourceManager(); + char const* p1 = SM.getCharacterData(range.getBegin()); + char const* p2 = SM.getCharacterData(range.getEnd()); + for (std::pair<char const*, char const*> const& rPair : mvModifiedRanges) + { + if (rPair.first <= p1 && p1 <= rPair.second) + return false; + if (p1 <= rPair.second && rPair.first <= p2) + return false; + } + mvModifiedRanges.emplace_back(p1, p2); + return true; +} + +static loplugin::Plugin::Registration<SalCall> reg("salcall", true); +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/compilerplugins/clang/test/salcall.cxx b/compilerplugins/clang/test/salcall.cxx new file mode 100644 index 000000000000..3b05530e8097 --- /dev/null +++ b/compilerplugins/clang/test/salcall.cxx @@ -0,0 +1,104 @@ +/* -*- 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 <sal/types.h> + +class Class1 +{ + void SAL_CALL method1(); // xxexpected-error {{SAL_CALL unnecessary here [loplugin:salcall]}} +}; +void SAL_CALL Class1::method1() { +} // xxexpected-error {{SAL_CALL unnecessary here [loplugin:salcall]}} + +class Class2 +{ + void method1(); // expected-error {{SAL_CALL inconsistency [loplugin:salcall]}} +}; +void SAL_CALL Class2::method1() {} // expected-note {{SAL_CALL inconsistency [loplugin:salcall]}} + +// no warning, this appears to be legal +class Class3 +{ + void SAL_CALL method1(); +}; +void Class3::method1() {} + +// no warning, normal case for reference +class Class4 +{ + void method1(); +}; +void Class4::method1() {} + +class Class5_1 +{ + virtual void method1(); // expected-note {{SAL_CALL inconsistency [loplugin:salcall]}} + virtual ~Class5_1(); +}; +class Class5_2 +{ + virtual void SAL_CALL method1(); + virtual ~Class5_2(); +}; +class Class5_3 : public Class5_1, public Class5_2 +{ + virtual void SAL_CALL + method1() override; // expected-error {{SAL_CALL inconsistency [loplugin:salcall]}} + virtual ~Class5_3(); +}; + +class Class6_1 +{ + virtual void SAL_CALL method1(); + virtual ~Class6_1(); +}; +class Class6_2 +{ + virtual void SAL_CALL method1(); + virtual ~Class6_2(); +}; +class Class6_3 : public Class6_1, public Class6_2 +{ + virtual void SAL_CALL method1() override; + virtual ~Class6_3(); +}; + +class Class7_1 +{ + virtual void method1(); + virtual ~Class7_1(); +}; +class Class7_2 +{ + virtual void method1(); + virtual ~Class7_2(); +}; +class Class7_3 : public Class7_1, public Class7_2 +{ + virtual void method1() override; + virtual ~Class7_3(); +}; + +class Class8_1 +{ + virtual void method2(); + virtual ~Class8_1(); +}; +class Class8_2 +{ + virtual void SAL_CALL method2(); // expected-note {{SAL_CALL inconsistency [loplugin:salcall]}} + virtual ~Class8_2(); +}; +class Class8_3 : public Class8_1, public Class8_2 +{ + virtual void method2() override; // expected-error {{SAL_CALL inconsistency [loplugin:salcall]}} + virtual ~Class8_3(); +}; + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ |