From 16e3b84d2e04cc3e976b59ca4cecd59ca1577cbc Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Wed, 13 Nov 2024 15:29:35 +0100 Subject: [PATCH] Improve loplugin:dyncastvisibility to check for non-inline key functions This would have caught the issue discussed in 709b1f3ddb87303a2dec6155dbe0106369c151ed "Make sure VCLXPopupMenu has unique RTTI". (The commit message talks about RTTI there, while what Clang actually compared for an optimized implementation of a dynamic_cast to a final class is vtable pointers, but the overall picture remains the same. Both RTTI and vtables are emitted along the key function, and if that is missing or inline, they are emitted for each dynamic library individually, and as internal symbols on macOS.) This commit also addresses all the issues found by the improved loplugin:dyncastvisibility on Linux. See the newly added TODO in compilerplugins/clang/dyncastvisibility.cxx and 86b86ac87ea0cc90249f156494c98c3c93e4f3fc "Give DocumentEventHolder (aka EventHolder) a key function" for an issue with key functions for class template instantiations. Change-Id: Ia19155efb1d23692c92b9c97ff17f18ae7a1f3ee Reviewed-on: https://gerrit.libreoffice.org/c/core/+/176576 Reviewed-by: Stephan Bergmann Tested-by: Jenkins --- basic/source/sbx/sbxvar.cxx | 2 + compilerplugins/clang/dyncastvisibility.cxx | 84 ++++++++++++++++----- emfio/inc/mtftools.hxx | 2 + emfio/source/reader/mtftools.cxx | 2 + include/basic/sbx.hxx | 1 + include/sfx2/app.hxx | 6 ++ include/svx/ClassificationField.hxx | 2 + include/svx/sxelditm.hxx | 4 + sfx2/source/appl/app.cxx | 2 + svx/Library_svx.mk | 1 + svx/Library_svxcore.mk | 1 + svx/source/dialog/ClassificationField.cxx | 16 ++++ svx/source/svdraw/sxelditm.cxx | 16 ++++ sw/inc/ftninfo.hxx | 2 + sw/source/core/doc/docftn.cxx | 2 + 15 files changed, 125 insertions(+), 18 deletions(-) create mode 100644 svx/source/dialog/ClassificationField.cxx create mode 100644 svx/source/svdraw/sxelditm.cxx diff --git a/basic/source/sbx/sbxvar.cxx b/basic/source/sbx/sbxvar.cxx index 5ae69288af13..97f7c0347ad9 100644 --- a/basic/source/sbx/sbxvar.cxx +++ b/basic/source/sbx/sbxvar.cxx @@ -569,6 +569,8 @@ SbxInfo::SbxInfo( OUString a, sal_uInt32 n ) : aHelpFile(std::move( a )), nHelpId( n ) {} +SbxHint::~SbxHint() = default; + void SbxVariable::Dump( SvStream& rStrm, bool bFill ) { OString aBNameStr(OUStringToOString(GetName( SbxNameType::ShortTypes ), RTL_TEXTENCODING_ASCII_US)); diff --git a/compilerplugins/clang/dyncastvisibility.cxx b/compilerplugins/clang/dyncastvisibility.cxx index 7d6b19aa6e24..236e57b92671 100644 --- a/compilerplugins/clang/dyncastvisibility.cxx +++ b/compilerplugins/clang/dyncastvisibility.cxx @@ -66,7 +66,7 @@ bool isDerivedFrom( } derived = true; } - } + } return derived; } @@ -157,7 +157,14 @@ public: assert(rds != nullptr); Bases bs; bool hidden = false; - if (!(isDerivedFrom(rdd, rds, &bs, &hidden) && hidden)) { + if (!isDerivedFrom(rdd, rds, &bs, &hidden)) { + return true; + } + Decl const * missing = nullptr; + if (rdd->isEffectivelyFinal()) { + missing = missingKeyFunction(rdd); + } + if (!hidden && missing == nullptr) { return true; } report( @@ -167,26 +174,44 @@ public: expr->getExprLoc()) << ts << vis(getTypeVisibility(rds)) << td << vis(getTypeVisibility(rdd)) << expr->getSourceRange(); - report( - DiagnosticsEngine::Note, - "base class %0 with %1 type visibility defined here", - rds->getLocation()) - << ts << vis(getTypeVisibility(rds)) << rds->getSourceRange(); - for (auto const i: bs) { - if (getTypeVisibility(i) != DefaultVisibility) { + if (hidden) { + report( + DiagnosticsEngine::Note, + "base class %0 with %1 type visibility defined here", + rds->getLocation()) + << ts << vis(getTypeVisibility(rds)) << rds->getSourceRange(); + for (auto const i: bs) { + if (getTypeVisibility(i) != DefaultVisibility) { + report( + DiagnosticsEngine::Note, + ("intermediary class %0 with %1 type visibility defined" + " here"), + i->getLocation()) + << i << vis(getTypeVisibility(i)) << i->getSourceRange(); + } + } + report( + DiagnosticsEngine::Note, + "derived class %0 with %1 type visibility defined here", + rdd->getLocation()) + << td << vis(getTypeVisibility(rdd)) << rdd->getSourceRange(); + } + if (missing != nullptr) { + if (isa(missing)) { report( DiagnosticsEngine::Note, - ("intermediary class %0 with %1 type visibility defined" - " here"), - i->getLocation()) - << i << vis(getTypeVisibility(i)) << i->getSourceRange(); + "derived class %0 does not have a key function (at least on some platforms)", + missing->getLocation()) + << td << missing->getSourceRange(); + } else { + report( + DiagnosticsEngine::Note, + "derived class %0 has a key function (at least on some platforms) that is" + " inline", + missing->getLocation()) + << td << missing->getSourceRange(); } } - report( - DiagnosticsEngine::Note, - "derived class %0 with %1 type visibility defined here", - rdd->getLocation()) - << td << vis(getTypeVisibility(rdd)) << rdd->getSourceRange(); return true; } @@ -199,6 +224,29 @@ private: TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); } } + + Decl const * missingKeyFunction(CXXRecordDecl const * decl) { + auto const md = compiler.getASTContext().getCurrentKeyFunction(decl); + if (md != nullptr && !md->isInlined()) { + return nullptr; + } + // Ignore classes defined in the main file: + auto const def = decl->getDefinition(); + assert(def != nullptr); + if (compiler.getSourceManager().isInMainFile(def->getLocation())) { + return nullptr; + } + //TODO: Ignore template instantiations, for which any key function would necessarily be + // inline, unless there is an explicit extern template instantiation (as there should + // arguably be for such cases, cf. comphelper::DocumentEventHolder in + // include/comphelper/asyncnotification.hxx, but which might be complicated to check here): + auto const tsk = decl->getTemplateSpecializationKind(); + if (tsk == TSK_ImplicitInstantiation || tsk == TSK_ExplicitInstantiationDeclaration) { + return nullptr; + } + return md == nullptr ? static_cast(decl) : static_cast(md); + } + }; static loplugin::Plugin::Registration dyncastvisibility( diff --git a/emfio/inc/mtftools.hxx b/emfio/inc/mtftools.hxx index f5638af59ecb..f99abab2f1d1 100644 --- a/emfio/inc/mtftools.hxx +++ b/emfio/inc/mtftools.hxx @@ -392,6 +392,8 @@ namespace emfio vcl::Font aFont; explicit WinMtfFontStyle(LOGFONTW const & rLogFont); + + ~WinMtfFontStyle() override; }; enum class WinMtfFillStyleType diff --git a/emfio/source/reader/mtftools.cxx b/emfio/source/reader/mtftools.cxx index 7ce4cd0fd559..6d920a2ce5a4 100644 --- a/emfio/source/reader/mtftools.cxx +++ b/emfio/source/reader/mtftools.cxx @@ -308,6 +308,8 @@ namespace emfio #endif }; + WinMtfFontStyle::~WinMtfFontStyle() = default; + // tdf#127471 ScaledFontDetectCorrectHelper::ScaledFontDetectCorrectHelper() { diff --git a/include/basic/sbx.hxx b/include/basic/sbx.hxx index dc4fdb7934e3..26cd2654b345 100644 --- a/include/basic/sbx.hxx +++ b/include/basic/sbx.hxx @@ -82,6 +82,7 @@ class BASIC_DLLPUBLIC SbxHint final : public SfxHint SbxVariable* pVar; public: SbxHint( SfxHintId n, SbxVariable* v ) : SfxHint( n ), pVar( v ) {} + ~SbxHint() override; SbxVariable* GetVar() const { return pVar; } }; diff --git a/include/sfx2/app.hxx b/include/sfx2/app.hxx index 312a2c9dbc34..d8c1ca989496 100644 --- a/include/sfx2/app.hxx +++ b/include/sfx2/app.hxx @@ -78,11 +78,17 @@ enum class SfxToolsModule class SFX2_DLLPUBLIC SfxLinkItem final : public SfxPoolItem { Link aLink; + + SfxLinkItem(SfxLinkItem const &) = default; + void operator =(SfxLinkItem const &) = delete; + public: SfxLinkItem( sal_uInt16 nWhichId, const Link& rValue ) : SfxPoolItem( nWhichId, SfxItemType::SfxLinkItemType ) { aLink = rValue; } + virtual ~SfxLinkItem() override; + virtual SfxLinkItem* Clone( SfxItemPool* = nullptr ) const override { return new SfxLinkItem( *this ); } virtual bool operator==( const SfxPoolItem& rL) const override diff --git a/include/svx/ClassificationField.hxx b/include/svx/ClassificationField.hxx index 4e2633d0392b..a7402bf82883 100644 --- a/include/svx/ClassificationField.hxx +++ b/include/svx/ClassificationField.hxx @@ -74,6 +74,8 @@ public: , msIdentifier(std::move(sIdentifier)) {} + ~ClassificationField() override; + std::unique_ptr Clone() const override { return std::make_unique(meType, msDescription, msFullClassName, msIdentifier); diff --git a/include/svx/sxelditm.hxx b/include/svx/sxelditm.hxx index 1b3a8b52f59c..bd8d4fca2353 100644 --- a/include/svx/sxelditm.hxx +++ b/include/svx/sxelditm.hxx @@ -24,12 +24,16 @@ class SAL_DLLPUBLIC_RTTI SdrEdgeLineDeltaCountItem final : public SfxUInt16Item { + SdrEdgeLineDeltaCountItem(SdrEdgeLineDeltaCountItem const&) = default; + void operator=(SdrEdgeLineDeltaCountItem const&) = delete; + public: SdrEdgeLineDeltaCountItem(sal_uInt16 nVal = 0) : SfxUInt16Item(SDRATTR_EDGELINEDELTACOUNT, nVal, SfxItemType::SdrEdgeLineDeltaCountItemType) { } + virtual ~SdrEdgeLineDeltaCountItem() override; virtual SdrEdgeLineDeltaCountItem* Clone(SfxItemPool*) const override { return new SdrEdgeLineDeltaCountItem(*this); diff --git a/sfx2/source/appl/app.cxx b/sfx2/source/appl/app.cxx index 3000c06e4dd0..b2a932c16e67 100644 --- a/sfx2/source/appl/app.cxx +++ b/sfx2/source/appl/app.cxx @@ -60,6 +60,8 @@ using namespace ::com::sun::star; +SfxLinkItem::~SfxLinkItem() = default; + static SfxApplication* g_pSfxApplication = nullptr; #if HAVE_FEATURE_XMLHELP diff --git a/svx/Library_svx.mk b/svx/Library_svx.mk index 0160bc2a4a6d..cddde28842eb 100644 --- a/svx/Library_svx.mk +++ b/svx/Library_svx.mk @@ -140,6 +140,7 @@ $(eval $(call gb_Library_add_exception_objects,svx,\ svx/source/dialog/ClassificationCommon \ svx/source/dialog/ClassificationDialog \ svx/source/dialog/ClassificationEditView \ + svx/source/dialog/ClassificationField \ svx/source/dialog/databaseregistrationui \ svx/source/dialog/dialcontrol \ svx/source/dialog/dlgctl3d \ diff --git a/svx/Library_svxcore.mk b/svx/Library_svxcore.mk index 7fe222959676..496b6ccd17d7 100644 --- a/svx/Library_svxcore.mk +++ b/svx/Library_svxcore.mk @@ -406,6 +406,7 @@ $(eval $(call gb_Library_add_exception_objects,svxcore,\ svx/source/svdraw/svdview \ svx/source/svdraw/svdviter \ svx/source/svdraw/svdxcgv \ + svx/source/svdraw/sxelditm \ svx/source/svdraw/textchain \ svx/source/svdraw/textchainflow \ svx/source/svdraw/textchaincursor \ diff --git a/svx/source/dialog/ClassificationField.cxx b/svx/source/dialog/ClassificationField.cxx new file mode 100644 index 000000000000..f96bf9c55de8 --- /dev/null +++ b/svx/source/dialog/ClassificationField.cxx @@ -0,0 +1,16 @@ +/* -*- 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 + +#include + +svx::ClassificationField::~ClassificationField() = default; + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/svx/source/svdraw/sxelditm.cxx b/svx/source/svdraw/sxelditm.cxx new file mode 100644 index 000000000000..f6521c70f5a7 --- /dev/null +++ b/svx/source/svdraw/sxelditm.cxx @@ -0,0 +1,16 @@ +/* -*- 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 + +#include + +SdrEdgeLineDeltaCountItem::~SdrEdgeLineDeltaCountItem() = default; + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/sw/inc/ftninfo.hxx b/sw/inc/ftninfo.hxx index c5490ed885d0..b7fd5c62998f 100644 --- a/sw/inc/ftninfo.hxx +++ b/sw/inc/ftninfo.hxx @@ -108,6 +108,8 @@ public: SwFootnoteInfo(); SwFootnoteInfo(const SwFootnoteInfo&); + + ~SwFootnoteInfo() override; }; #endif diff --git a/sw/source/core/doc/docftn.cxx b/sw/source/core/doc/docftn.cxx index 28ef3c47bcab..fc820daa6248 100644 --- a/sw/source/core/doc/docftn.cxx +++ b/sw/source/core/doc/docftn.cxx @@ -311,6 +311,8 @@ SwFootnoteInfo::SwFootnoteInfo() : m_bEndNote = false; } +SwFootnoteInfo::~SwFootnoteInfo() = default; + void SwDoc::SetFootnoteInfo(const SwFootnoteInfo& rInfo) { SwRootFrame* pTmpRoot = getIDocumentLayoutAccess().GetCurrentLayout();