[Libreoffice-commits] core.git: compilerplugins/clang solenv/CompilerTest_compilerplugins_clang.mk svtools/source svx/source sw/source
Stephan Bergmann
sbergman at redhat.com
Sat Mar 25 16:22:03 UTC 2017
compilerplugins/clang/loopvartoosmall.cxx | 239 ++++++++++++++++++-------
compilerplugins/clang/test/loopvartoosmall.cxx | 24 ++
solenv/CompilerTest_compilerplugins_clang.mk | 1
svtools/source/brwbox/brwbox2.cxx | 4
svx/source/fmcomp/gridctrl.cxx | 2
sw/source/core/frmedt/fetab.cxx | 2
sw/source/core/text/itrpaint.cxx | 2
sw/source/filter/ww8/ww8par6.cxx | 2
sw/source/filter/ww8/ww8scan.cxx | 2
9 files changed, 207 insertions(+), 71 deletions(-)
New commits:
commit 2258f33a5fd95a5e25f5bb232994ab147d09bfb9
Author: Stephan Bergmann <sbergman at redhat.com>
Date: Sat Mar 25 10:55:44 2017 +0100
Make loplugin:loopvartoosmall find more suspicious cases
...where the "controlling expression" of any sort of loop contains a sub-
expression of the form
var < val
where the type of var is smaller than that of val. Theoretically, this could
turn up lots of false positives, but practically it didn't run into any. Most
findings have been cleaned up over the last weeks. There's just a handful
remaining places that are hard to clean up, so I flagged them here with
(deliberately awkward) sal::static_int_cast for later clean-up.
Change-Id: I0f735d46dda15b9b336150095df65cf247e9d6d3
Reviewed-on: https://gerrit.libreoffice.org/35682
Reviewed-by: Stephan Bergmann <sbergman at redhat.com>
Tested-by: Stephan Bergmann <sbergman at redhat.com>
diff --git a/compilerplugins/clang/loopvartoosmall.cxx b/compilerplugins/clang/loopvartoosmall.cxx
index aaa664827298..eb4cb96d592d 100644
--- a/compilerplugins/clang/loopvartoosmall.cxx
+++ b/compilerplugins/clang/loopvartoosmall.cxx
@@ -7,11 +7,13 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/
-#include <string>
-#include <iostream>
+#include <algorithm>
+#include <cassert>
+#include <list>
+#include <map>
#include "plugin.hxx"
-#include "clang/AST/CXXInheritance.h"
+//#include "clang/AST/CXXInheritance.h"
// Idea from bubli. Check that the index variable in a for loop is able to cover the range
// revealed by the terminating condition.
@@ -30,86 +32,195 @@ public:
TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
}
- bool VisitForStmt( const ForStmt* stmt );
+ bool VisitForStmt( const ForStmt* stmt ) {
+ checkExpr(stmt->getCond());
+ return true;
+ }
+
+ bool VisitWhileStmt(WhileStmt const * stmt) {
+ checkExpr(stmt->getCond());
+ return true;
+ }
+
+ bool VisitDoStmt(DoStmt const * stmt) {
+ checkExpr(stmt->getCond());
+ return true;
+ }
private:
- StringRef getFilename(SourceLocation loc);
+ unsigned getIntValueWidth(QualType type) const;
+
+ void checkSubExpr(Expr const * expr, bool positive);
+
+ void checkExpr(Expr const * expr);
+
+ struct Comparison {
+ BinaryOperator const * op;
+ unsigned rhsWidth;
+ };
+ struct Comparisons {
+ std::list<Comparison> comparisons;
+ unsigned lhsWidth;
+ };
+
+ std::map<Decl const *, Comparisons> comparisons_;
};
-StringRef LoopVarTooSmall::getFilename(SourceLocation loc)
-{
- SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(loc);
- StringRef name { compiler.getSourceManager().getFilename(spellingLocation) };
- return name;
+unsigned LoopVarTooSmall::getIntValueWidth(QualType type) const {
+ if (auto const et = type->getAs<EnumType>()) {
+ auto const ed = et->getDecl();
+ if (!ed->isFixed()) {
+ unsigned pos = ed->getNumPositiveBits();
+ unsigned neg = ed->getNumNegativeBits();
+ return neg == 0 ? std::max(pos, 1U) : std::max(pos + 1, neg);
+ }
+ }
+ return compiler.getASTContext().getIntWidth(type);
}
-bool LoopVarTooSmall::VisitForStmt( const ForStmt* stmt )
-{
- if (ignoreLocation( stmt ))
- return true;
- // ignore sal/ module for now
- StringRef aFileName = getFilename(stmt->getLocStart());
- if (aFileName.startswith(SRCDIR "/sal/")) {
- return true;
+void LoopVarTooSmall::checkSubExpr(Expr const * expr, bool positive) {
+ auto const e = expr->IgnoreParenImpCasts();
+ if (auto const uo = dyn_cast<UnaryOperator>(e)) {
+ if (uo->getOpcode() == UO_LNot) {
+ checkSubExpr(uo->getSubExpr(), !positive);
+ }
+ return;
}
-
- const Stmt* initStmt = stmt->getInit();
- if (!initStmt || !isa<DeclStmt>(initStmt))
- return true;
- const DeclStmt* declStmt = dyn_cast<DeclStmt>(initStmt);
- if (!declStmt->getDeclGroup().isSingleDecl())
- return true;
- const Decl* decl = declStmt->getSingleDecl();
- if (!decl || !isa<VarDecl>(decl))
- return true;
- const VarDecl* varDecl = dyn_cast<VarDecl>(decl);
- QualType qt = varDecl->getType();
- if (!qt->isIntegralType(compiler.getASTContext()))
- return true;
- uint64_t qt1BitWidth = compiler.getASTContext().getTypeSize(qt);
-
- if (!stmt->getCond() || !isa<BinaryOperator>(stmt->getCond()))
- return true;
- const BinaryOperator* binOp = dyn_cast<BinaryOperator>(stmt->getCond());
- if (binOp->getOpcode() != BO_LT && binOp->getOpcode() != BO_LE)
- return true;
- const Expr* binOpRHS = binOp->getRHS();
- // ignore complex expressions for now, promotion rules on conditions like "i < (size()+1)"
- // make it hard to guess at a correct type.
- if (isa<BinaryOperator>(binOpRHS) || isa<ParenExpr>(binOpRHS))
- return true;
- if (isa<ImplicitCastExpr>(binOpRHS)) {
- const ImplicitCastExpr* castExpr = dyn_cast<ImplicitCastExpr>(binOpRHS);
- binOpRHS = castExpr->getSubExpr();
+ const BinaryOperator* binOp = dyn_cast<BinaryOperator>(e);
+ if (!binOp)
+ return;
+ bool less;
+ if (positive) {
+ switch (binOp->getOpcode()) {
+ case BO_LAnd:
+ checkSubExpr(binOp->getLHS(), true);
+ checkSubExpr(binOp->getRHS(), true);
+ return;
+ case BO_LT:
+ case BO_NE:
+ less = true;
+ break;
+ case BO_LE:
+ less = false;
+ break;
+ default:
+ return;
+ }
+ } else {
+ switch (binOp->getOpcode()) {
+ case BO_LOr:
+ checkSubExpr(binOp->getLHS(), false);
+ checkSubExpr(binOp->getRHS(), false);
+ return;
+ case BO_GE:
+ case BO_EQ:
+ less = true;
+ break;
+ case BO_GT:
+ less = false;
+ break;
+ default:
+ return;
+ }
+ }
+ auto lhs = dyn_cast<DeclRefExpr>(binOp->getLHS()->IgnoreParenImpCasts());
+ if (!lhs)
+ return;
+ QualType qt = lhs->getType();
+ if (!qt->isIntegralOrEnumerationType())
+ return;
+ unsigned qt1BitWidth = getIntValueWidth(qt);
+ auto lhsDecl = lhs->getDecl();
+ if (!isa<VarDecl>(lhsDecl)) {
+ if (auto fd = dyn_cast<FieldDecl>(lhsDecl)) {
+ if (fd->isBitField()) {
+ qt1BitWidth = std::max(
+ qt1BitWidth,
+ fd->getBitWidthValue(compiler.getASTContext()));
+ }
+ } else {
+ return;
+ }
}
+ const Expr* binOpRHS = binOp->getRHS()->IgnoreParenImpCasts();
QualType qt2 = binOpRHS->getType();
if (!qt2->isIntegralType(compiler.getASTContext()))
- return true;
- // check for comparison with constants where the compiler just tends to give me the type as "int"
+ return;
+ unsigned qt2BitWidth;
llvm::APSInt aIntResult;
- uint64_t qt2BitWidth = compiler.getASTContext().getTypeSize(qt2);
if (binOpRHS->EvaluateAsInt(aIntResult, compiler.getASTContext())) {
- if (aIntResult.getSExtValue() > 0 && aIntResult.getSExtValue() < 1<<7) {
- qt2BitWidth = 8;
- } else if (aIntResult.getSExtValue() > 0 && aIntResult.getSExtValue() < 1<<15) {
- qt2BitWidth = 16;
- } else if (aIntResult.getSExtValue() > 0 && aIntResult.getSExtValue() < 1L<<31) {
- qt2BitWidth = 32;
+ if (less && aIntResult.isStrictlyPositive()) {
+ --aIntResult;
+ }
+ qt2BitWidth = aIntResult.isUnsigned() || !aIntResult.isNegative()
+ ? std::max(aIntResult.getActiveBits(), 1U)
+ : aIntResult.getBitWidth() - aIntResult.countLeadingOnes() + 1;
+ } else {
+ // Ignore complex expressions for now, promotion rules on conditions
+ // like "i < (size()+1)" make it hard to guess at a correct type:
+ if (isa<BinaryOperator>(binOpRHS) || isa<ConditionalOperator>(binOpRHS))
+ {
+ return;
+ }
+ qt2BitWidth = getIntValueWidth(qt2);
+ if (auto dre = dyn_cast<DeclRefExpr>(binOpRHS)) {
+ if (auto fd = dyn_cast<FieldDecl>(dre->getDecl())) {
+ if (fd->isBitField()) {
+ qt2BitWidth = std::max(
+ qt2BitWidth,
+ fd->getBitWidthValue(compiler.getASTContext()));
+ }
+ }
}
}
- if (qt1BitWidth < qt2BitWidth) {
- report(
- DiagnosticsEngine::Warning,
- "loop index type %0 is narrower than length type %1",
- stmt->getInit()->getLocStart())
- << qt << qt2 << stmt->getInit()->getSourceRange();
- //stmt->getCond()->dump();
+ auto i = comparisons_.find(lhsDecl);
+ if (i == comparisons_.end()) {
+ i = (comparisons_.insert(
+ decltype(comparisons_)::value_type(lhsDecl, {{}, qt1BitWidth}))
+ .first);
+ } else {
+ assert(i->second.lhsWidth == qt1BitWidth);
+ }
+ bool ins = true;
+ for (auto j = i->second.comparisons.begin();
+ j != i->second.comparisons.end();)
+ {
+ if (qt2BitWidth > j->rhsWidth) {
+ ins = false;
+ break;
+ } else if (qt2BitWidth < j->rhsWidth) {
+ j = i->second.comparisons.erase(j);
+ } else {
+ ++j;
+ }
+ }
+ if (ins) {
+ i->second.comparisons.push_back({binOp, qt2BitWidth});
}
- return true;
}
+void LoopVarTooSmall::checkExpr(Expr const * expr) {
+ if (expr != nullptr && !ignoreLocation(expr)) {
+ assert(comparisons_.empty());
+ checkSubExpr(expr, true);
+ for (auto const & i: comparisons_) {
+ for (auto const & j: i.second.comparisons) {
+ if (i.second.lhsWidth < j.rhsWidth) {
+ report(
+ DiagnosticsEngine::Warning,
+ "loop index type %0 is narrower than length type %1",
+ j.op->getExprLoc())
+ << j.op->getLHS()->IgnoreImpCasts()->getType()
+ << j.op->getRHS()->IgnoreImpCasts()->getType()
+ << j.op->getSourceRange();
+ }
+ }
+ }
+ comparisons_.clear();
+ }
+}
loplugin::Plugin::Registration< LoopVarTooSmall > X("loopvartoosmall");
diff --git a/compilerplugins/clang/test/loopvartoosmall.cxx b/compilerplugins/clang/test/loopvartoosmall.cxx
new file mode 100644
index 000000000000..5cb5a23c289c
--- /dev/null
+++ b/compilerplugins/clang/test/loopvartoosmall.cxx
@@ -0,0 +1,24 @@
+/* -*- 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 <cstdint>
+
+std::int32_t size() { return 1; }
+
+int main() {
+ for (std::int16_t i = 0; i < size(); ++i) {} // expected-error {{[loplugin:loopvartoosmall]}}
+ for (std::int16_t i = 0; i <= size(); ++i) {} // expected-error {{[loplugin:loopvartoosmall]}}
+ for (std::int16_t i = 0; i != size(); ++i) {} // expected-error {{[loplugin:loopvartoosmall]}}
+ std::int16_t j;
+ for (j = 0; j < size(); ++j) {} // expected-error {{[loplugin:loopvartoosmall]}}
+ for (j = 0; j <= size(); ++j) {} // expected-error {{[loplugin:loopvartoosmall]}}
+ for (j = 0; j != size(); ++j) {} // expected-error {{[loplugin:loopvartoosmall]}}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
index 80508e8ce6a5..724a9ac07046 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -13,6 +13,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
compilerplugins/clang/test/datamembershadow \
compilerplugins/clang/test/externvar \
compilerplugins/clang/test/finalprotected \
+ compilerplugins/clang/test/loopvartoosmall \
compilerplugins/clang/test/passstuffbyref \
compilerplugins/clang/test/oslendian-1 \
compilerplugins/clang/test/oslendian-2 \
diff --git a/svtools/source/brwbox/brwbox2.cxx b/svtools/source/brwbox/brwbox2.cxx
index 76201c97fa37..5902077176c4 100644
--- a/svtools/source/brwbox/brwbox2.cxx
+++ b/svtools/source/brwbox/brwbox2.cxx
@@ -1271,12 +1271,12 @@ void BrowseBox::ColumnInserted( sal_uInt16 nPos )
sal_uInt16 BrowseBox::FrozenColCount() const
{
- sal_uInt16 nCol;
+ BrowserColumns::size_type nCol;
for ( nCol = 0;
nCol < pCols.size() && pCols[ nCol ]->IsFrozen();
++nCol )
/* empty loop */;
- return nCol;
+ return nCol; //TODO: BrowserColumns::size_type -> sal_uInt16!
}
diff --git a/svx/source/fmcomp/gridctrl.cxx b/svx/source/fmcomp/gridctrl.cxx
index db2ef8918926..e04a5499c314 100644
--- a/svx/source/fmcomp/gridctrl.cxx
+++ b/svx/source/fmcomp/gridctrl.cxx
@@ -1684,7 +1684,7 @@ sal_uInt16 DbGridControl::AppendColumn(const OUString& rName, sal_uInt16 nWidth,
}
// calculate the new id
- for (nId=1; (GetModelColumnPos(nId) != GRID_COLUMN_NOT_FOUND) && (nId <= m_aColumns.size()); ++nId)
+ for (nId=1; (GetModelColumnPos(nId) != GRID_COLUMN_NOT_FOUND) && sal::static_int_cast<DbGridColumns::size_type>(nId) <= m_aColumns.size(); ++nId)
;
DBG_ASSERT(GetViewColumnPos(nId) == GRID_COLUMN_NOT_FOUND, "DbGridControl::AppendColumn : inconsistent internal state !");
// my column's models say "there is no column with id nId", but the view (the base class) says "there is a column ..."
diff --git a/sw/source/core/frmedt/fetab.cxx b/sw/source/core/frmedt/fetab.cxx
index e9d927b9e84d..0cb6f864076a 100644
--- a/sw/source/core/frmedt/fetab.cxx
+++ b/sw/source/core/frmedt/fetab.cxx
@@ -1044,7 +1044,7 @@ static sal_uInt16 lcl_GetRowNumber( const SwPosition& rPos )
const SwTableLine* pTabLine = static_cast<const SwRowFrame*>(pRow)->GetTabLine();
sal_uInt16 nRet = USHRT_MAX;
sal_uInt16 nI = 0;
- while ( nI < pTabFrame->GetTable()->GetTabLines().size() )
+ while ( sal::static_int_cast<SwTableLines::size_type>(nI) < pTabFrame->GetTable()->GetTabLines().size() )
{
if ( pTabFrame->GetTable()->GetTabLines()[ nI ] == pTabLine )
{
diff --git a/sw/source/core/text/itrpaint.cxx b/sw/source/core/text/itrpaint.cxx
index 462ee043c8c1..117317b8047c 100644
--- a/sw/source/core/text/itrpaint.cxx
+++ b/sw/source/core/text/itrpaint.cxx
@@ -579,7 +579,7 @@ void SwTextPainter::CheckSpecialUnderline( const SwLinePortion* pPor,
sal_uInt16 nMaxBaseLineOfst = 0;
int nNumberOfPortions = 0;
- while( nTmpIdx <= nUnderEnd && pPor )
+ while( sal::static_int_cast<long>(nTmpIdx) <= nUnderEnd && pPor )
{
if ( pPor->IsFlyPortion() || pPor->IsFlyCntPortion() ||
pPor->IsBreakPortion() || pPor->IsMarginPortion() ||
diff --git a/sw/source/filter/ww8/ww8par6.cxx b/sw/source/filter/ww8/ww8par6.cxx
index 209ca6771078..f12707b2142b 100644
--- a/sw/source/filter/ww8/ww8par6.cxx
+++ b/sw/source/filter/ww8/ww8par6.cxx
@@ -1655,7 +1655,7 @@ void WW8FlyPara::ReadFull(sal_uInt8 nOrigSp29, SwWW8ImplReader* pIo)
WW8FlyPara *pNowStyleApo=nullptr;
sal_uInt16 nColl = pPap->GetIstd();
ww::sti eSti = eVer < ww::eWW6 ? ww::GetCanonicalStiFromStc( static_cast< sal_uInt8 >(nColl) ) : static_cast<ww::sti>(nColl);
- while (eSti != ww::stiNil && nColl < pIo->m_vColl.size() && nullptr == (pNowStyleApo = pIo->m_vColl[nColl].m_xWWFly.get()))
+ while (eSti != ww::stiNil && sal::static_int_cast<size_t>(nColl) < pIo->m_vColl.size() && nullptr == (pNowStyleApo = pIo->m_vColl[nColl].m_xWWFly.get()))
{
nColl = pIo->m_vColl[nColl].m_nBase;
eSti = eVer < ww::eWW6 ? ww::GetCanonicalStiFromStc( static_cast< sal_uInt8 >(nColl) ) : static_cast<ww::sti>(nColl);
diff --git a/sw/source/filter/ww8/ww8scan.cxx b/sw/source/filter/ww8/ww8scan.cxx
index ae3e44223ab8..3364acdf3f58 100644
--- a/sw/source/filter/ww8/ww8scan.cxx
+++ b/sw/source/filter/ww8/ww8scan.cxx
@@ -4123,7 +4123,7 @@ OUString WW8PLCFx_Book::GetBookmark(long nStart,long nEnd, sal_uInt16 &nIndex)
if (pBook[0] && pBook[1])
{
WW8_CP nStartAkt, nEndAkt;
- while (i < aBookNames.size())
+ while (sal::static_int_cast<decltype(aBookNames)::size_type>(i) < aBookNames.size())
{
void* p;
sal_uInt16 nEndIdx;
More information about the Libreoffice-commits
mailing list