Optimization: Improved performance of CheckBufferOverrun::checkScope() when dealing with a large number of arrays (#5975)

-> checking time decreases from 1010s to 50s on the code snippet in #5975
-> Dropped a garbage code unit test

lib/checkbufferoverrun.cpp

@@ -860,27 +860,55 @@ void CheckBufferOverrun::valueFlowCheckArrayIndex(const Token * const tok, const
 
 void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo)
 {
-    assert(tok->previous() != nullptr);
-    const MathLib::biguint total_size = arrayInfo.num(0) * arrayInfo.element_size();
-
-    const unsigned int declarationId = arrayInfo.declarationId();
-
-    const bool printPortability = _settings->isEnabled("portability");
-    const bool printWarning = _settings->isEnabled("warning");
-    const bool printInconclusive = _settings->inconclusive;
-
     bool reassigned = false;
 
     for (const Token* const end = tok->scope()->classEnd; tok != end; tok = tok->next()) {
         if (reassigned && tok->str() == ";")
             break;
 
-        if (tok->varId() == declarationId) {
+        if (tok->varId() != arrayInfo.declarationId())
+            continue;
+
         if (tok->strAt(1) == "=") {
             reassigned = true;
         }
 
-            else if (tok->strAt(1) == "[") {
+        checkScope_inner(tok, arrayInfo);
+    }
+}
+
+void CheckBufferOverrun::checkScope(const Token *tok, std::map<unsigned int, ArrayInfo> arrayInfos)
+{
+    unsigned int reassigned = 0;
+
+    for (const Token* const end = tok->scope()->classEnd; tok != end; tok = tok->next()) {
+        if (reassigned && tok->str() == ";") {
+            arrayInfos.erase(reassigned);
+            reassigned = 0;
+        }
+
+        if (!tok->variable() || tok->variable()->nameToken() == tok)
+            continue;
+
+        std::map<unsigned int, ArrayInfo>::const_iterator arrayInfo = arrayInfos.find(tok->varId());
+        if (arrayInfo == arrayInfos.cend())
+            continue;
+
+        if (tok->strAt(1) == "=") {
+            reassigned = tok->varId();
+        }
+
+        checkScope_inner(tok, arrayInfo->second);
+    }
+}
+
+void CheckBufferOverrun::checkScope_inner(const Token *tok, const ArrayInfo &arrayInfo)
+{
+    const bool printPortability = _settings->isEnabled("portability");
+    const bool printWarning = _settings->isEnabled("warning");
+    const bool printInconclusive = _settings->inconclusive;
+
+    if (tok->strAt(1) == "[") {
         valueFlowCheckArrayIndex(tok->next(), arrayInfo);
     }
 
@@ -901,7 +929,7 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
     }
 
     else if (printPortability && tok->astParent() && tok->astParent()->str() == "-") {
-                const Variable *var = _tokenizer->getSymbolDatabase()->getVariableFromVarId(declarationId);
+        const Variable *var = _tokenizer->getSymbolDatabase()->getVariableFromVarId(arrayInfo.declarationId());
         if (var && var->isArray()) {
             const Token *index = tok->astParent()->astOperand2();
             const ValueFlow::Value *value = index ? index->getValueGE(1,_settings) : nullptr;
@@ -911,39 +939,53 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
                 pointerOutOfBoundsError(tok->astParent(), index, value->intvalue);
         }
     }
-        }
 
     if (!tok->scope()->isExecutable()) // No executable code outside of executable scope - continue to increase performance
-            continue;
+        return;
+
+    const Token* tok2 = tok->astParent();
+    if (tok2) {
+        while (tok2->astParent() && !Token::Match(tok2->astParent(), "[,(]"))
+            tok2 = tok2->astParent();
+        while (tok2->astParent() && tok2->astParent()->str() == ",")
+            tok2 = tok2->astParent();
+        if (tok2->astParent() && tok2->astParent()->str() == "(")
+            tok2 = tok2->astParent();
+
+        if (tok2->str() != "(")
+            return;
+
+        tok2 = tok2->previous();
 
-        else if (tok->next() && tok->next()->link() && Token::Match(tok, "%name% (")) {
         // Check function call..
-            checkFunctionCall(tok, arrayInfo, std::list<const Token*>());
+        checkFunctionCall(tok2, arrayInfo, std::list<const Token*>());
 
-            if (printWarning && printInconclusive && Token::Match(tok, "strncpy|memcpy|memmove ( %varid% , %str% , %num% )", declarationId)) {
+        const MathLib::biguint total_size = arrayInfo.num(0) * arrayInfo.element_size();
-                if (Token::getStrLength(tok->tokAt(4)) >= total_size) {
+
-                    const MathLib::biguint num = MathLib::toULongNumber(tok->strAt(6));
+        if (printWarning && printInconclusive && Token::Match(tok2, "strncpy|memcpy|memmove ( %varid% , %str% , %num% )", arrayInfo.declarationId())) {
+            if (Token::getStrLength(tok2->tokAt(4)) >= total_size) {
+                const MathLib::biguint num = MathLib::toULongNumber(tok2->strAt(6));
                 if (total_size == num)
-                        bufferNotZeroTerminatedError(tok, tok->strAt(2), tok->str());
+                    bufferNotZeroTerminatedError(tok2, tok2->strAt(2), tok2->str());
             }
         }
 
-            if (printWarning && Token::Match(tok, "strncpy|strncat ( %varid% ,", declarationId) && Token::Match(tok->linkAt(1)->tokAt(-2), ", %num% )")) {
+        if (printWarning && Token::Match(tok2, "strncpy|strncat ( %varid% ,", arrayInfo.declarationId()) && Token::Match(tok2->linkAt(1)->tokAt(-2), ", %num% )")) {
-                const Token* param3 = tok->linkAt(1)->previous();
+            const Token* param3 = tok2->linkAt(1)->previous();
 
             // check for strncpy which is not terminated
-                if (tok->str() == "strncpy") {
+            if (tok2->str() == "strncpy") {
                 // strncpy takes entire variable length as input size
                 const MathLib::biguint num = MathLib::toULongNumber(param3->str());
 
                 // this is currently 'inconclusive'. See TestBufferOverrun::terminateStrncpy3
                 if (printInconclusive && num >= total_size) {
-                        const Token *tok2 = tok->next()->link()->next();
+                    const Token *tok4 = tok2->next()->link()->next();
-                        for (; tok2; tok2 = tok2->next()) {
+                    for (; tok4; tok4 = tok4->next()) {
-                            const Token* tok3 = tok->tokAt(2);
+                        const Token* tok3 = tok2->tokAt(2);
-                            if (tok2->varId() == tok3->varId()) {
+                        if (tok4->varId() == tok3->varId()) {
-                                if (!Token::Match(tok2, "%varid% [ %any% ] = 0 ;", tok3->varId())) {
+                            if (!Token::Match(tok4, "%varid% [ %any% ] = 0 ;", tok3->varId())) {
-                                    terminateStrncpyError(tok, tok3->str());
+                                terminateStrncpyError(tok2, tok3->str());
                             }
 
                             break;
@@ -953,14 +995,14 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
             }
 
             // Dangerous usage of strncat..
-                else if (tok->str() == "strncat") {
+            else if (tok2->str() == "strncat") {
                 const MathLib::biguint n = MathLib::toULongNumber(param3->str());
                 if (n >= total_size)
-                        strncatUsageError(tok);
+                    strncatUsageError(tok2);
             }
 
             // Dangerous usage of strncpy + strncat..
-                if (Token::Match(param3->tokAt(2), "; strncat ( %varid% ,", declarationId) && Token::Match(param3->linkAt(4)->tokAt(-2), ", %num% )")) {
+            if (Token::Match(param3->tokAt(2), "; strncat ( %varid% ,", arrayInfo.declarationId()) && Token::Match(param3->linkAt(4)->tokAt(-2), ", %num% )")) {
                 const MathLib::biguint n = MathLib::toULongNumber(param3->str()) + MathLib::toULongNumber(param3->linkAt(4)->strAt(-1));
                 if (n > total_size)
                     strncatUsageError(param3->tokAt(3));
@@ -969,26 +1011,25 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
 
         // Writing data into array..
         if (total_size > 0) {
-                if (Token::Match(tok, "strcpy ( %varid% , %str% )", declarationId)) {
+            if (Token::Match(tok2, "strcpy ( %varid% , %str% )", arrayInfo.declarationId())) {
-                    const std::size_t len = Token::getStrLength(tok->tokAt(4));
+                const std::size_t len = Token::getStrLength(tok2->tokAt(4));
                 if (len >= total_size) {
-                        bufferOverrunError(tok, arrayInfo.varname());
+                    bufferOverrunError(tok2, arrayInfo.varname());
-                        continue;
+                    return;
                 }
             }
 
             // Detect few strcat() calls
             MathLib::biguint charactersAppend = 0;
-                const Token *tok2 = tok;
+            const Token *tok3 = tok2;
 
-                while (Token::Match(tok2, "strcat ( %varid% , %str% ) ;", declarationId)) {
+            while (Token::Match(tok3, "strcat ( %varid% , %str% ) ;", arrayInfo.declarationId())) {
-                    charactersAppend += Token::getStrLength(tok2->tokAt(4));
+                charactersAppend += Token::getStrLength(tok3->tokAt(4));
                 if (charactersAppend >= total_size) {
-                        bufferOverrunError(tok2, arrayInfo.varname());
+                    bufferOverrunError(tok3, arrayInfo.varname());
                     break;
                 }
-                    tok2 = tok2->tokAt(7);
+                tok3 = tok3->tokAt(7);
-                }
             }
         }
     }
@@ -1127,9 +1168,10 @@ void CheckBufferOverrun::checkGlobalAndLocalVariable()
 
     // check all known fixed size arrays first by just looking them up
     const SymbolDatabase* symbolDatabase = _tokenizer->getSymbolDatabase();
-    for (unsigned int i = 1; i <= _tokenizer->varIdCount(); i++) {
+    for (std::list<Scope>::const_iterator scope = symbolDatabase->scopeList.cbegin(); scope != symbolDatabase->scopeList.cend(); ++scope) {
-        const Variable * const var = symbolDatabase->getVariableFromVarId(i);
+        std::map<unsigned int, ArrayInfo> arrayInfos;
-        if (var && var->isArray() && var->dimension(0) > 0) {
+        for (std::list<Variable>::const_iterator var = scope->varlist.cbegin(); var != scope->varlist.cend(); ++var) {
+            if (var->isArray() && var->dimension(0) > 0) {
                 _errorLogger->reportProgress(_tokenizer->list.getSourceFilePath(),
                                              "Check (BufferOverrun::checkGlobalAndLocalVariable 1)",
                                              var->nameToken()->progressValue());
@@ -1151,10 +1193,12 @@ void CheckBufferOverrun::checkGlobalAndLocalVariable()
                     break;
                 if (tok->str() == "{")
                     tok = tok->next();
-            const ArrayInfo arrayInfo(var, _tokenizer, &_settings->library, i);
+                arrayInfos[var->declarationId()] = ArrayInfo(&*var, _tokenizer, &_settings->library, var->declarationId());
-            checkScope(tok, arrayInfo);
             }
         }
+        if (!arrayInfos.empty())
+            checkScope(scope->classStart ? scope->classStart : _tokenizer->tokens(), arrayInfos);
+    }
 
     // find all dynamically allocated arrays next
     const std::size_t functions = symbolDatabase->functionScopes.size();

lib/checkbufferoverrun.h

@@ -172,6 +172,8 @@ public:
 
     /** Check for buffer overruns (based on ArrayInfo) */
     void checkScope(const Token *tok, const ArrayInfo &arrayInfo);
+    void checkScope(const Token *tok, std::map<unsigned int, ArrayInfo> arrayInfo);
+    void checkScope_inner(const Token *tok, const ArrayInfo &arrayInfo);
 
     /** Check for buffer overruns */
     void checkScope(const Token *tok, const std::vector<std::string> &varname, const ArrayInfo &arrayInfo);

test/testbufferoverrun.cpp

@@ -227,8 +227,6 @@ private:
 
         TEST_CASE(getErrorMessages);
 
-        TEST_CASE(unknownMacroNoDecl);    // #2638 - not variable declaration: 'AAA a[0] = 0;'
-
         // Access array and then check if the used index is within bounds
         TEST_CASE(arrayIndexThenCheck);
 
@@ -3749,15 +3747,6 @@ private:
         c.getErrorMessages(this, 0);
     }
 
-    void unknownMacroNoDecl() {
-        check("void f() {\n"
-              "    int a[10];\n"
-              "    AAA a[0] = 0;\n"   // <- not a valid array declaration
-              "    a[1] = 1;\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-    }
-
     void arrayIndexThenCheck() {
         check("void f(const char s[]) {\n"
               "    if (s[i] == 'x' && i < y) {\n"