From f93b5886031e1cbdc5e9d4738cf2bc45666ab23d Mon Sep 17 00:00:00 2001 From: Maksim Derbasov Date: Sun, 4 Sep 2022 00:29:06 +0300 Subject: [PATCH] New check: use memcpy/memset instead of loop (#4257) --- lib/astutils.cpp | 2 + lib/checkfunctions.cpp | 109 +++++++++++++++++++++++++++++++++ lib/checkfunctions.h | 8 ++- lib/token.h | 3 + test/testfunctions.cpp | 136 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 257 insertions(+), 1 deletion(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 1b4a043e5..b1b3f3973 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1728,6 +1728,8 @@ static bool functionModifiesArguments(const Function* f) bool isConstFunctionCall(const Token* ftok, const Library& library) { + if (isSizeOfEtc(ftok)) + return true; if (!Token::Match(ftok, "%name% (")) return false; if (const Function* f = ftok->function()) { diff --git a/lib/checkfunctions.cpp b/lib/checkfunctions.cpp index 56476a442..aad634181 100644 --- a/lib/checkfunctions.cpp +++ b/lib/checkfunctions.cpp @@ -689,3 +689,112 @@ void CheckFunctions::copyElisionError(const Token *tok) "Using std::move for returning object by-value from function will affect copy elision optimization." " More: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-move-local"); } + +void CheckFunctions::useStandardLibrary() +{ + if (!mSettings->severity.isEnabled(Severity::style)) + return; + + for (const Scope& scope: mTokenizer->getSymbolDatabase()->scopeList) { + if (scope.type != Scope::ScopeType::eFor) + continue; + + const Token *forToken = scope.classDef; + // for ( initToken ; condToken ; stepToken ) + const Token* initToken = getInitTok(forToken); + if (!initToken) + continue; + const Token* condToken = getCondTok(forToken); + if (!condToken) + continue; + const Token* stepToken = getStepTok(forToken); + if (!stepToken) + continue; + + // 1. we expect that idx variable will be initialized with 0 + const Token* idxToken = initToken->astOperand1(); + const Token* initVal = initToken->astOperand2(); + if (!idxToken || !initVal || !initVal->hasKnownIntValue() || initVal->getKnownIntValue() != 0) + continue; + const auto idxVarId = idxToken->varId(); + if (0 == idxVarId) + continue; + + // 2. we expect that idx will be less of some variable + if (!condToken->isComparisonOp()) + continue; + + const auto& secondOp = condToken->str(); + const bool isLess = "<" == secondOp && + isConstExpression(condToken->astOperand2(), mSettings->library, true, mTokenizer->isCPP()) && + condToken->astOperand1()->varId() == idxVarId; + const bool isMore = ">" == secondOp && + isConstExpression(condToken->astOperand1(), mSettings->library, true, mTokenizer->isCPP()) && + condToken->astOperand2()->varId() == idxVarId; + + if (!(isLess || isMore)) + continue; + + // 3. we expect idx incrementing by 1 + const bool inc = stepToken->str() == "++" && stepToken->astOperand1()->varId() == idxVarId; + const bool plusOne = stepToken->isBinaryOp() && stepToken->str() == "+=" && + stepToken->astOperand1()->varId() == idxVarId && + stepToken->astOperand2()->str() == "1"; + if (!inc && !plusOne) + continue; + + // technically using void* here is not correct but some compilers could allow it + + const Token *tok = scope.bodyStart; + const std::string memcpyName = mTokenizer->isCPP() ? "std::memcpy" : "memcpy"; + // (reinterpret_cast(dest))[i] = (reinterpret_cast(src))[i]; + if (Token::Match(tok, "{ (| reinterpret_cast < uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] = " + "(| reinterpret_cast < const| uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] ; }", idxVarId)) { + useStandardLibraryError(tok->next(), memcpyName); + continue; + } + + // ((char*)dst)[i] = ((const char*)src)[i]; + if (Token::Match(tok, "{ ( ( uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] = " + "( ( const| uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] ; }", idxVarId)) { + useStandardLibraryError(tok->next(), memcpyName); + continue; + } + + + const static std::string memsetName = mTokenizer->isCPP() ? "std::memset" : "memset"; + // ((char*)dst)[i] = 0; + if (Token::Match(tok, "{ ( ( uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] = %char%|%num% ; }", idxVarId)) { + useStandardLibraryError(tok->next(), memsetName); + continue; + } + + // ((char*)dst)[i] = (const char*)0; + if (Token::Match(tok, "{ ( ( uint8_t|int8_t|char|void * ) (| %var% ) )| [ %varid% ] = " + "( const| uint8_t|int8_t|char ) (| %char%|%num% )| ; }", idxVarId)) { + useStandardLibraryError(tok->next(), memsetName); + continue; + } + + // (reinterpret_cast(dest))[i] = static_cast(0); + if (Token::Match(tok, "{ (| reinterpret_cast < uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] = " + "(| static_cast < const| uint8_t|int8_t|char > ( %char%|%num% ) )| ; }", idxVarId)) { + useStandardLibraryError(tok->next(), memsetName); + continue; + } + + // (reinterpret_cast(dest))[i] = 0; + if (Token::Match(tok, "{ (| reinterpret_cast < uint8_t|int8_t|char|void * > ( %var% ) )| [ %varid% ] = " + "%char%|%num% ; }", idxVarId)) { + useStandardLibraryError(tok->next(), memsetName); + continue; + } + } +} + +void CheckFunctions::useStandardLibraryError(const Token *tok, const std::string& expected) +{ + reportError(tok, Severity::style, + "useStandardLibrary", + "Consider using " + expected + " instead of loop."); +} diff --git a/lib/checkfunctions.h b/lib/checkfunctions.h index a5e336e06..9f39959aa 100644 --- a/lib/checkfunctions.h +++ b/lib/checkfunctions.h @@ -73,6 +73,7 @@ public: checkFunctions.memsetZeroBytes(); checkFunctions.memsetInvalid2ndParam(); checkFunctions.returnLocalStdMove(); + checkFunctions.useStandardLibrary(); } /** Check for functions that should not be used */ @@ -103,6 +104,8 @@ public: /** @brief %Check for copy elision by RVO|NRVO */ void returnLocalStdMove(); + void useStandardLibrary(); + /** @brief --check-library: warn for unconfigured function calls */ void checkLibraryMatchFunctions(); @@ -122,6 +125,7 @@ private: void memsetValueOutOfRangeError(const Token *tok, const std::string &value); void missingReturnError(const Token *tok); void copyElisionError(const Token *tok); + void useStandardLibraryError(const Token *tok, const std::string& expected); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override { CheckFunctions c(nullptr, settings, errorLogger); @@ -141,6 +145,7 @@ private: c.memsetValueOutOfRangeError(nullptr, "varname"); c.missingReturnError(nullptr); c.copyElisionError(nullptr); + c.useStandardLibraryError(nullptr, "memcpy"); } static std::string myName() { @@ -156,7 +161,8 @@ private: "- memset() third argument is zero\n" "- memset() with a value out of range as the 2nd parameter\n" "- memset() with a float as the 2nd parameter\n" - "- copy elision optimization for returning value affected by std::move\n"; + "- copy elision optimization for returning value affected by std::move\n" + "- use memcpy()/memset() instead of for loop\n"; } }; /// @} diff --git a/lib/token.h b/lib/token.h index ef2b4b2b1..992e65b20 100644 --- a/lib/token.h +++ b/lib/token.h @@ -401,6 +401,9 @@ public: bool isEnumerator() const { return mTokType == eEnumerator; } + bool isVariable() const { + return mTokType == eVariable; + } bool isOp() const { return (isConstOp() || isAssignmentOp() || diff --git a/test/testfunctions.cpp b/test/testfunctions.cpp index a59062712..458198a7b 100644 --- a/test/testfunctions.cpp +++ b/test/testfunctions.cpp @@ -102,6 +102,21 @@ private: TEST_CASE(negativeMemoryAllocationSizeError); // #389 TEST_CASE(checkLibraryMatchFunctions); + + TEST_CASE(checkUseStandardLibrary1); + TEST_CASE(checkUseStandardLibrary2); + TEST_CASE(checkUseStandardLibrary3); + TEST_CASE(checkUseStandardLibrary4); + TEST_CASE(checkUseStandardLibrary5); + TEST_CASE(checkUseStandardLibrary6); + TEST_CASE(checkUseStandardLibrary7); + TEST_CASE(checkUseStandardLibrary8); + TEST_CASE(checkUseStandardLibrary9); + TEST_CASE(checkUseStandardLibrary10); + TEST_CASE(checkUseStandardLibrary11); + TEST_CASE(checkUseStandardLibrary12); + TEST_CASE(checkUseStandardLibrary13); + TEST_CASE(checkUseStandardLibrary14); } #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__) @@ -1853,6 +1868,127 @@ private: settings.severity = severity_old; settings.checkLibrary = false; } + + void checkUseStandardLibrary1() { + check("void f(void* dest, void const* src, const size_t count) {\n" + " size_t i;\n" + " for (i = 0; count > i; ++i)\n" + " (reinterpret_cast(dest))[i] = (reinterpret_cast(src))[i];\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memcpy instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary2() { + check("void f(void* dest, void const* src, const size_t count) {\n" + " for (size_t i = 0; i < count; i++) {\n" + " (reinterpret_cast(dest))[i] = (reinterpret_cast(src))[i];\n" + "}}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memcpy instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary3() { + check("void f(void* dst, const void* src, const size_t count) {\n" + " size_t i;\n" + " for (i = 0; count > i; i++)\n" + " ((char*)dst)[i] = ((const char*)src)[i];\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memcpy instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary4() { + check("void f(void* dst, void* src, const size_t size) {\n" + " for (size_t i = 0; i < size; i += 1) {\n" + " ((int8_t*)dst)[i] = ((int8_t*)src)[i];\n" + "}}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memcpy instead of loop.\n", errout.str()); + } + + // different indexes + void checkUseStandardLibrary5() { + check("void f(void* dst, void* src, const size_t size, const size_t from_idx) {\n" + " for (size_t i = 0; i < size; ++i) {\n" + " ((int8_t*)dst)[i] = ((int8_t*)src)[from_idx];\n" + "}}\n"); + ASSERT_EQUALS("", errout.str()); + } + + // unknown count + void checkUseStandardLibrary6() { + check("void f(void* dst, void* src, const size_t size) {\n" + " for (size_t i = 0; ((int8_t*)src)[i] != 0; ++i) {\n" + " ((int8_t*)dst)[i] = ((int8_t*)src)[i];\n" + "}}\n"); + ASSERT_EQUALS("", errout.str()); + } + + // increment with 2 + void checkUseStandardLibrary7() { + check("void f(void* dst, void* src, const size_t size) {\n" + " for (size_t i = 0; i < size; i += 2) {\n" + " ((int8_t*)dst)[i] = ((int8_t*)src)[i];\n" + "}}\n"); + ASSERT_EQUALS("", errout.str()); + } + + // right argument of assignment could be static_cast, functional cast, c-style and implicit cast + // functional cast case not covered + void checkUseStandardLibrary8() { + check("void f(void* dest, const size_t count) {\n" + " size_t i;\n" + " for (i = 0; i < count; ++i)\n" + " (reinterpret_cast(dest))[i] = static_cast(0);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memset instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary9() { + check("void f(void* dest, const size_t count) {\n" + " for (size_t i = 0; i < count; i++) {\n" + " (reinterpret_cast(dest))[i] = (static_cast(0));\n" + "}}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary10() { + check("void f(void* dst, const size_t size) {\n" + " size_t i;\n" + " for (i = 0; i < size; i++)\n" + " ((char*)dst)[i] = (const char)0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::memset instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary11() { + check("void f(void* dst, const size_t size) {\n" + " for (size_t i = 0; i < size; i += 1) {\n" + " ((int8_t*)dst)[i] = ((int8_t)0);\n" + "}}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary12() { + check("void f(void* dst, const size_t size) {\n" + " for (size_t i = 0; i < size; i += 1) {\n" + " ((int8_t*)dst)[i] = 42;\n" + "}}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary13() { + check("void f(void* dest, const size_t count) {\n" + " for (size_t i = 0; i < count; i++) {\n" + " reinterpret_cast(dest)[i] = '0';\n" + "}}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str()); + } + + void checkUseStandardLibrary14() { + check("void f(void* dest) {\n" + " for (size_t i = 0; i < sizeof(int)*(15 + 42/2 - 7); i++) {\n" + " reinterpret_cast(dest)[i] = '0';\n" + "}}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::memset instead of loop.\n", errout.str()); + } }; REGISTER_TEST(TestFunctions)