From bca43c26fb5250256245b194afcfd99035917803 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Wed, 10 Apr 2024 17:55:20 +0200 Subject: [PATCH] Use ValueFlow::getSizeOf() in CheckOther (#6257) --- lib/checkother.cpp | 57 ++++------------------------------------------ lib/valueflow.cpp | 40 +++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 59 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 70e07ccf55e..145693b504c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1206,55 +1206,6 @@ void CheckOther::commaSeparatedReturnError(const Token *tok) "macro is then used in a return statement, it is less likely such code is misunderstood.", CWE398, Certainty::normal); } -//--------------------------------------------------------------------------- -// Check for function parameters that should be passed by const reference -//--------------------------------------------------------------------------- -static int estimateSize(const Type* type, const Settings* settings, const SymbolDatabase* symbolDatabase, int recursionDepth = 0) -{ - if (recursionDepth > 20) - return 0; - - int cumulatedSize = 0; - const bool isUnion = type->classScope->type == Scope::ScopeType::eUnion; - const auto accumulateSize = [](int& cumulatedSize, int size, bool isUnion) -> void { - if (isUnion) - cumulatedSize = std::max(cumulatedSize, size); - else - cumulatedSize += size; - }; - std::set anonScopes; - for (const Variable& var : type->classScope->varlist) { - int size = 0; - if (var.isStatic()) - continue; - if (var.isPointer() || var.isReference()) - size = settings->platform.sizeof_pointer; - else if (var.type() && var.type()->classScope) - size = estimateSize(var.type(), settings, symbolDatabase, recursionDepth + 1); - else if (var.valueType() && var.valueType()->type == ValueType::Type::CONTAINER) - size = 3 * settings->platform.sizeof_pointer; // Just guess - else if (var.nameToken()->scope() != type->classScope && var.nameToken()->scope()->definedType) { // anonymous union - const auto ret = anonScopes.insert(var.nameToken()->scope()); - if (ret.second) - size = estimateSize(var.nameToken()->scope()->definedType, settings, symbolDatabase, recursionDepth + 1); - } - else - size = symbolDatabase->sizeOfType(var.typeStartToken()); - - if (var.isArray()) - size *= std::accumulate(var.dimensions().cbegin(), var.dimensions().cend(), 1, [](int v, const Dimension& d) { - return v *= d.num; - }); - - accumulateSize(cumulatedSize, size, isUnion); - } - return std::accumulate(type->derivedFrom.cbegin(), type->derivedFrom.cend(), cumulatedSize, [&](int v, const Type::BaseInfo& baseInfo) { - if (baseInfo.type && baseInfo.type->classScope) - v += estimateSize(baseInfo.type, settings, symbolDatabase, recursionDepth + 1); - return v; - }); -} - void CheckOther::checkPassByReference() { if (!mSettings->severity.isEnabled(Severity::performance) || mTokenizer->isC()) @@ -1288,7 +1239,7 @@ void CheckOther::checkPassByReference() // Ensure that it is a large object. if (!var->type()->classScope) inconclusive = true; - else if (estimateSize(var->type(), mSettings, symbolDatabase) <= 2 * mSettings->platform.sizeof_pointer) + else if (!var->valueType() || ValueFlow::getSizeOf(*var->valueType(), *mSettings) <= 2 * mSettings->platform.sizeof_pointer) continue; } else @@ -2928,7 +2879,7 @@ void CheckOther::checkRedundantCopy() const Token* varTok = fScope->bodyEnd->tokAt(-2); if (varTok->variable() && !varTok->variable()->isGlobal() && (!varTok->variable()->type() || !varTok->variable()->type()->classScope || - estimateSize(varTok->variable()->type(), mSettings, symbolDatabase) > 2 * mSettings->platform.sizeof_pointer)) + (varTok->variable()->valueType() && ValueFlow::getSizeOf(*varTok->variable()->valueType(), *mSettings) > 2 * mSettings->platform.sizeof_pointer))) redundantCopyError(startTok, startTok->str()); } } @@ -3041,8 +2992,8 @@ void CheckOther::checkIncompleteArrayFill() int size = mTokenizer->sizeOfType(var->typeStartToken()); if (size == 0 && var->valueType()->pointer) size = mSettings->platform.sizeof_pointer; - else if (size == 0 && var->type()) - size = estimateSize(var->type(), mSettings, symbolDatabase); + else if (size == 0 && var->valueType()) + size = ValueFlow::getSizeOf(*var->valueType(), *mSettings); const Token* tok3 = tok->next()->astOperand2()->astOperand1()->astOperand1(); if ((size != 1 && size != 100 && size != 0) || var->isPointer()) { if (printWarning) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 61cfc06acaa..ffdb189d012 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1110,6 +1110,7 @@ template static size_t accumulateStructMembers(const Scope* scope, F f) { size_t total = 0; + std::set anonScopes; for (const Variable& var : scope->varlist) { if (var.isStatic()) continue; @@ -1119,7 +1120,13 @@ static size_t accumulateStructMembers(const Scope* scope, F f) const MathLib::bigint dim = std::accumulate(var.dimensions().cbegin(), var.dimensions().cend(), 1LL, [](MathLib::bigint i1, const Dimension& dim) { return i1 * dim.num; }); - total = f(total, *vt, dim); + if (var.nameToken()->scope() != scope && var.nameToken()->scope()->definedType) { // anonymous union + const auto ret = anonScopes.insert(var.nameToken()->scope()); + if (ret.second) + total = f(total, *vt, dim); + } + else + total = f(total, *vt, dim); } if (total == 0) return 0; @@ -1148,11 +1155,22 @@ static size_t getAlignOf(const ValueType& vt, const Settings& settings) return align == 0 ? 0 : bitCeil(align); } if (vt.type == ValueType::Type::RECORD && vt.typeScope) { - return accumulateStructMembers(vt.typeScope, [&](size_t max, const ValueType& vt2, size_t /*dim*/) { + auto accHelper = [&](size_t max, const ValueType& vt2, size_t /*dim*/) { size_t a = getAlignOf(vt2, settings); return std::max(max, a); - }); + }; + size_t total = 0; + if (const Type* dt = vt.typeScope->definedType) { + total = std::accumulate(dt->derivedFrom.begin(), dt->derivedFrom.end(), total, [&](size_t v, const Type::BaseInfo& bi) { + if (bi.type && bi.type->classScope) + v += accumulateStructMembers(bi.type->classScope, accHelper); + return v; + }); + } + return total + accumulateStructMembers(vt.typeScope, accHelper); } + if (vt.type == ValueType::Type::CONTAINER) + return settings.platform.sizeof_pointer; // Just guess return 0; } @@ -1185,16 +1203,26 @@ size_t ValueFlow::getSizeOf(const ValueType &vt, const Settings &settings) return settings.platform.sizeof_double; if (vt.type == ValueType::Type::LONGDOUBLE) return settings.platform.sizeof_long_double; + if (vt.type == ValueType::Type::CONTAINER) + return 3 * settings.platform.sizeof_pointer; // Just guess if (vt.type == ValueType::Type::RECORD && vt.typeScope) { - size_t total = accumulateStructMembers(vt.typeScope, [&](size_t total, const ValueType& vt2, size_t dim) -> size_t { + auto accHelper = [&](size_t total, const ValueType& vt2, size_t dim) -> size_t { size_t n = ValueFlow::getSizeOf(vt2, settings); size_t a = getAlignOf(vt2, settings); if (n == 0 || a == 0) return 0; n *= dim; size_t padding = (a - (total % a)) % a; - return total + padding + n; - }); + return vt.typeScope->type == Scope::eUnion ? std::max(total, n) : total + padding + n; + }; + size_t total = accumulateStructMembers(vt.typeScope, accHelper); + if (const Type* dt = vt.typeScope->definedType) { + total = std::accumulate(dt->derivedFrom.begin(), dt->derivedFrom.end(), total, [&](size_t v, const Type::BaseInfo& bi) { + if (bi.type && bi.type->classScope) + v += accumulateStructMembers(bi.type->classScope, accHelper); + return v; + }); + } if (total == 0) return 0; size_t align = getAlignOf(vt, settings);