[Libreoffice-commits] core.git: compilerplugins/clang dbaccess/source extensions/source
Stephan Bergmann
sbergman at redhat.com
Fri Dec 8 17:03:35 UTC 2017
compilerplugins/clang/salcall.cxx | 249 +++++++++++++++++++----
compilerplugins/clang/test/salcall.cxx | 29 ++
dbaccess/source/core/inc/definitioncontainer.hxx | 2
extensions/source/update/check/updatecheck.hxx | 2
extensions/source/update/check/updatehdl.hxx | 2
5 files changed, 243 insertions(+), 41 deletions(-)
New commits:
commit a31267be1bb42e8a5f80a3b660bbf969eeb5b647
Author: Stephan Bergmann <sbergman at redhat.com>
Date: Tue Dec 5 20:25:16 2017 +0100
Fix isSalCallFunction so it also works on Windows
...where FunctionDecl::getReturnTypeSourceRange returns an invalid range because
it fails to take AttributedTypeLoc (as caused by SAL_CALL -> __cdecl) into
account.
Change-Id: I7835dfca7b890ba1bfdb99adaad78a627b6e0e17
Reviewed-on: https://gerrit.libreoffice.org/45909
Tested-by: Jenkins <ci at libreoffice.org>
Reviewed-by: Stephan Bergmann <sbergman at redhat.com>
diff --git a/compilerplugins/clang/salcall.cxx b/compilerplugins/clang/salcall.cxx
index 36a8635d4f7f..8bdac1256759 100644
--- a/compilerplugins/clang/salcall.cxx
+++ b/compilerplugins/clang/salcall.cxx
@@ -9,15 +9,34 @@
#include "plugin.hxx"
#include "check.hxx"
-#include <cassert>
+
+#include <algorithm>
+#include <set>
#include <string>
-#include <iostream>
-#include <fstream>
+#include <utility>
+#include <vector>
// The SAL_CALL function annotation is only necessary on our outward
// facing C++ ABI, anywhere else it is just cargo-cult.
//
+//TODO: To find inconsistencies like
+//
+// template<typename> struct S { void f(); }; // #1
+// template<typename T> void S<T>::f() {} // #2
+// template void SAL_CALL S<void>::f();
+//
+// VisitFunctionDecl would need to also visit explicit instantiations, by letting
+// shouldVisitTemplateInstantiations return true and returning from VisitFunctionDecl early iff
+// decl->getTemplateSpecializationKind() == TSK_ImplicitInstantiation. However, an instantiatied
+// FunctionDecl is created in TemplateDeclInstantiator::VisitCXXMethodDecl by copying information
+// (including source locations) from the declaration at #1, and later modified in
+// Sema::InstantiateFunctionDefinition with some source location information from the definition at
+// #2. That means that the source scanning in isSalCallFunction below would be thoroughly confused
+// and break. (This happens for both explicit and implicit template instantiations, which is the
+// reason why calls to isSalCallFunction make sure to not call it with any FunctionDecls
+// representing such template instantiations.)
+
namespace
{
//static bool startswith(const std::string& rStr, const char* pSubStr)
@@ -25,6 +44,12 @@ namespace
// return rStr.compare(0, strlen(pSubStr), pSubStr) == 0;
//}
+CXXMethodDecl const* getTemplateInstantiationPattern(CXXMethodDecl const* decl)
+{
+ auto const p = decl->getTemplateInstantiationPattern();
+ return p == nullptr ? decl : cast<CXXMethodDecl>(p);
+}
+
class SalCall final : public RecursiveASTVisitor<SalCall>, public loplugin::RewritePlugin
{
public:
@@ -189,7 +214,8 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
for (auto iter = methodDecl->begin_overridden_methods();
iter != methodDecl->end_overridden_methods(); ++iter)
{
- const CXXMethodDecl* overriddenMethod = (*iter)->getCanonicalDecl();
+ const CXXMethodDecl* overriddenMethod
+ = getTemplateInstantiationPattern(*iter)->getCanonicalDecl();
if (bCanonicalDeclIsSalCall != isSalCallFunction(overriddenMethod))
{
report(DiagnosticsEngine::Warning, "SAL_CALL inconsistency",
@@ -277,7 +303,8 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
for (auto iter = methodDecl->begin_overridden_methods();
iter != methodDecl->end_overridden_methods(); ++iter)
{
- const CXXMethodDecl* overriddenMethod = (*iter)->getCanonicalDecl();
+ const CXXMethodDecl* overriddenMethod
+ = getTemplateInstantiationPattern(*iter)->getCanonicalDecl();
if (isSalCallFunction(overriddenMethod))
return true;
}
@@ -319,49 +346,195 @@ bool SalCall::VisitFunctionDecl(FunctionDecl const* decl)
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.
+ assert(!functionDecl->isTemplateInstantiation());
+
+ //TODO: It appears that FunctionDecls representing explicit template specializations have the
+ // same issue as those representing (implicit or explicit) instantiations, namely that their
+ // data (including relevant source locations) is an incoherent combination of data from the
+ // original template declaration and the later specialization definition. For example, for the
+ // OValueLimitedType<double>::registerProperties specialization at
+ // forms/source/xforms/datatyperepository.cxx:241, the FunctionDecl (which is even considered
+ // canonic) representing the base-class function overridden by ODecimalType::registerProperties
+ // (forms/source/xforms/datatypes.hxx:299) is dumped as
//
- // So I use getReturnTypeSourceRange() for the start of the range
- // instead, and search for the "(" in the function decl.
+ // CXXMethodDecl <forms/source/xforms/datatypes.hxx:217:9, col:54>
+ // forms/source/xforms/datatyperepository.cxx:242:37 registerProperties 'void (void)' virtual
+ //
+ // mixing the source range ("datatypes.hxx:217:9, col:54") from the original declaration with
+ // the name location ("datatyperepository.cxx:242:37") from the explicit specialization. Just
+ // give up for now and assume no "SAL_CALL" is present:
+ if (functionDecl->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+ {
+ return false;
+ }
- 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)
+ // Stop searching for "SAL_CALL" at the start of the function declaration's name (for qualified
+ // names this will point after the qualifiers, but needlessly including those in the search
+ // should be harmless):
+ SourceLocation endLoc = functionDecl->getNameInfo().getLocStart();
+ while (endLoc.isMacroID() && SM.isAtStartOfImmediateMacroExpansion(endLoc))
+ {
+ endLoc = SM.getImmediateMacroCallerLoc(endLoc);
+ }
+ endLoc = SM.getSpellingLoc(endLoc);
+
+ SourceLocation startLoc;
+ bool noReturnType = isa<CXXConstructorDecl>(functionDecl)
+ || isa<CXXDestructorDecl>(functionDecl)
+ || isa<CXXConversionDecl>(functionDecl);
+ bool startAfterReturnType = !noReturnType;
+ if (startAfterReturnType)
+ {
+ // For functions that do have a return type, start searching for "SAL_CALL" after the return
+ // type (which for SAL_CALL functions on Windows will be an AttributedTypeLoc, which the
+ // implementation of FunctionDecl::getReturnTypeSourceRange does not take into account, so
+ // do that here explicitly):
+ auto const TSI = functionDecl->getTypeSourceInfo();
+ if (TSI == nullptr)
+ {
+ return false;
+ }
+ auto TL = TSI->getTypeLoc().IgnoreParens();
+ if (auto ATL = TL.getAs<AttributedTypeLoc>())
+ {
+ TL = ATL.getModifiedLoc();
+ }
+ auto const FTL = TL.getAs<FunctionTypeLoc>();
+ if (!FTL)
+ {
+ // Happens when a function declaration uses a typedef for the function type, as in
+ //
+ // SAL_JNI_EXPORT javaunohelper::detail::Func_bootstrap
+ // Java_com_sun_star_comp_helper_Bootstrap_cppuhelper_1bootstrap;
+ //
+ // in javaunohelper/source/juhx-export-functions.hxx.
+ //TODO: check the typedef for mention of "SAL_CALL" (and also check for usage of such
+ // typedefs in the !startAfterReturnType case below)
+ return false;
+ }
+ startLoc = FTL.getReturnLoc().getEndLoc();
+ auto const slEnd = Lexer::getLocForEndOfToken(startLoc, 0, SM, compiler.getLangOpts());
+ if (slEnd.isValid()) // i.e., startLoc either non-macro, or at end of macro
+ {
+ startLoc = slEnd;
+ }
+ else // otherwise, resolve into macro text
+ {
+ startLoc = Lexer::getLocForEndOfToken(SM.getSpellingLoc(startLoc), 0, SM,
+ compiler.getLangOpts());
+ }
+ while (startLoc.isMacroID() && SM.isAtEndOfImmediateMacroExpansion(startLoc, &startLoc))
+ {
+ }
+ startLoc = SM.getSpellingLoc(startLoc);
+
+ if (startLoc.isValid() && endLoc.isValid() && startLoc != endLoc
+ && !SM.isBeforeInTranslationUnit(startLoc, endLoc))
+ {
+ // Happens for uses of trailing return type (in which case starting instead at the start
+ // of the function declaration should be fine), but also for cases like
+ //
+ // void (*f())();
+ //
+ // where the function name is within the function type (TODO: in which case starting at
+ // the start can erroneously pick up the "SAL_CALL" from the returned pointer-to-
+ // function type in cases like
+ //
+ // void SAL_CALL (*f())();
+ //
+ // that are hopefully rare):
+ startAfterReturnType = false;
+ }
+ }
+ if (!startAfterReturnType)
+ {
+ // Ctors/dtors/conversion functions don't have a return type, start searching for "SAL_CALL"
+ // at the start of the function declaration:
+ startLoc = functionDecl->getSourceRange().getBegin();
+ while (startLoc.isMacroID() && SM.isAtStartOfImmediateMacroExpansion(startLoc))
+ {
+ startLoc = SM.getImmediateMacroCallerLoc(startLoc);
+ }
+ startLoc = SM.getSpellingLoc(startLoc);
+#if !defined _WIN32
+ // When the SAL_CALL macro expands to nothing, it may even precede the function
+ // declaration's source range, so go back one token (unless the declaration is known to
+ // start with a token that must precede a possible "SAL_CALL", like "virtual" or
+ // "explicit"):
+ //TODO: this will produce false positives if the declaration is immediately preceded by a
+ // macro definition whose replacement text ends in "SAL_CALL"
+ if (noReturnType
+ && !(functionDecl->isVirtualAsWritten()
+ || (isa<CXXConstructorDecl>(functionDecl)
+ && cast<CXXConstructorDecl>(functionDecl)->isExplicitSpecified())
+ || (isa<CXXConversionDecl>(functionDecl)
+ && cast<CXXConversionDecl>(functionDecl)->isExplicitSpecified())))
+ {
+ for (;;)
+ {
+ startLoc = Lexer::GetBeginningOfToken(startLoc.getLocWithOffset(-1), SM,
+ compiler.getLangOpts());
+ auto const s
+ = StringRef(SM.getCharacterData(startLoc),
+ Lexer::MeasureTokenLength(startLoc, SM, compiler.getLangOpts()));
+ // When looking backward at least through a function-like macro replacement like
+ //
+ // | foo\ |
+ // | barbaz##X |
+ //
+ // starting at "barbaz" in the secod line, the next token reported will start at "\"
+ // in the first line and include the intervening spaces and (part of? looks like an
+ // error in Clang) "barbaz", so just skip any tokens starting with backslash-newline
+ // when looking backwards here, without even trying to look at their content:
+ if (!(s.empty() || s.startswith("/*") || s.startswith("//")
+ || s.startswith("\\\n")))
+ {
+ break;
+ }
+ }
+ }
+#endif
+ }
+
+ if (startLoc.isInvalid() || endLoc.isInvalid())
+ //TODO: should probably not happen
return false;
- char const* found = std::search(p1, leftBracket, SAL_CALL, SAL_CALL + strlen(SAL_CALL));
+ SourceLocation found;
+
+ while (SM.isBeforeInTranslationUnit(startLoc, endLoc))
+ {
+ unsigned n = Lexer::MeasureTokenLength(startLoc, SM, compiler.getLangOpts());
+ auto s = StringRef(compiler.getSourceManager().getCharacterData(startLoc), n);
+ while (s.startswith("\\\n"))
+ {
+ s = s.drop_front(2);
+ while (!s.empty()
+ && (s.front() == ' ' || s.front() == '\t' || s.front() == '\n'
+ || s.front() == '\v' || s.front() == '\f'))
+ {
+ s = s.drop_front(1);
+ }
+ }
+ if (s == "SAL_CALL")
+ {
+ found = startLoc;
+ break;
+ }
+ if (startLoc == endLoc)
+ {
+ break;
+ }
+ startLoc = startLoc.getLocWithOffset(std::max<unsigned>(n, 1));
+ }
- if (found >= leftBracket)
+ if (found.isInvalid())
return false;
if (pLoc)
- // the -1 is to remove the space before the SAL_CALL
- *pLoc = startLoc.getLocWithOffset(found - p1 - 1);
+ *pLoc = found;
return true;
}
diff --git a/compilerplugins/clang/test/salcall.cxx b/compilerplugins/clang/test/salcall.cxx
index 92172ab9e3df..e3ed95b106ce 100644
--- a/compilerplugins/clang/test/salcall.cxx
+++ b/compilerplugins/clang/test/salcall.cxx
@@ -9,9 +9,21 @@
#include <sal/types.h>
+#define VOID void
+
class Class1
{
+ SAL_CALL Class1() {} // expected-error {{SAL_CALL unnecessary here [loplugin:salcall]}}
+ SAL_CALL ~Class1() {} // expected-error {{SAL_CALL unnecessary here [loplugin:salcall]}}
+ SAL_CALL operator int() // expected-error {{SAL_CALL unnecessary here [loplugin:salcall]}}
+ {
+ return 0;
+ }
+
void SAL_CALL method1(); // expected-error {{SAL_CALL unnecessary here [loplugin:salcall]}}
+ VOID method2() {}
+ // no SAL_CALL for above method2, even though "SAL_CALL" appears between definition of VOID and
+ // the declaration's name, "method2"
};
void SAL_CALL Class1::method1()
{ // expected-error at -1 {{SAL_CALL unnecessary here [loplugin:salcall]}}
@@ -105,4 +117,21 @@ class Class8_3 : public Class8_1, public Class8_2
virtual ~Class8_3();
};
+#if 0 //TODO
+template<typename> struct S {
+ virtual ~S();
+ virtual void f();
+};
+template<typename T> S<T>::~S() {}
+template<typename T> void S<T>::f() {}
+struct S2: S<int> {
+ ~S2();
+ void f() {}
+};
+int main() {
+ S2 s2;
+ s2->f();
+}
+#endif
+
/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/dbaccess/source/core/inc/definitioncontainer.hxx b/dbaccess/source/core/inc/definitioncontainer.hxx
index 9b3bb4cb626d..394d95761ebd 100644
--- a/dbaccess/source/core/inc/definitioncontainer.hxx
+++ b/dbaccess/source/core/inc/definitioncontainer.hxx
@@ -292,7 +292,7 @@ protected:
ListenerType _eType
);
- SAL_CALL operator css::uno::Reference< css::uno::XInterface > () const
+ operator css::uno::Reference< css::uno::XInterface > () const
{
return const_cast< XContainer* >( static_cast< const XContainer* >( this ) );
}
diff --git a/extensions/source/update/check/updatecheck.hxx b/extensions/source/update/check/updatecheck.hxx
index 7a797136b692..e50fa3859b0c 100644
--- a/extensions/source/update/check/updatecheck.hxx
+++ b/extensions/source/update/check/updatecheck.hxx
@@ -61,7 +61,7 @@ class UpdateCheck :
virtual ~UpdateCheck() override;
public:
- SAL_CALL operator rtl::Reference< UpdateCheckConfigListener > ()
+ operator rtl::Reference< UpdateCheckConfigListener > ()
{ return static_cast< UpdateCheckConfigListener * > (this); }
void initialize(const css::uno::Sequence<css::beans::NamedValue>& rValues,
diff --git a/extensions/source/update/check/updatehdl.hxx b/extensions/source/update/check/updatehdl.hxx
index 519669ebfb48..6870c13e3c45 100644
--- a/extensions/source/update/check/updatehdl.hxx
+++ b/extensions/source/update/check/updatehdl.hxx
@@ -178,7 +178,7 @@ public:
bool showOverwriteWarning() const;
// Allows runtime exceptions to be thrown by const methods
- SAL_CALL operator css::uno::Reference< css::uno::XInterface > () const
+ operator css::uno::Reference< css::uno::XInterface > () const
{ return const_cast< cppu::OWeakObject * > (static_cast< cppu::OWeakObject const * > (this)); };
// XActionListener
More information about the Libreoffice-commits
mailing list