Fix #15010: ErrorLogger: do not open and read from files unless necessary

cli/cppcheckexecutor.cpp

@@ -661,8 +661,13 @@ void StdLogger::reportErr(const ErrorMessage &msg)
     msgCopy.classification = getClassification(msgCopy.guideline, mSettings.reportType);
 
     // TODO: there should be no need for verbose and default messages here
+    // Don't perform redundant reads for these formats, the code is not needed
+    // for deduplication
+    const bool noCode = mSettings.outputFormat == Settings::OutputFormat::xml ||
+                        mSettings.outputFormat == Settings::OutputFormat::sarif;
     const std::string msgStr =
-        msgCopy.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation);
+        msgCopy.toString(mSettings.verbose, mSettings.templateFormat,
+                         mSettings.templateLocation, noCode);
 
     // Alert only about unique errors
     if (!mSettings.emitDuplicates && !mShownErrors.insert(msgStr).second)

lib/cppcheck.cpp

@@ -210,7 +210,9 @@ class CppCheck::CppCheckLogger : public ErrorLogger
         }
 
         // TODO: there should be no need for the verbose and default messages here
-        std::string errmsg = msg.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation);
+        // Code is not needed for deduplication
+        const bool noCode = true;
+        std::string errmsg = msg.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation, noCode);
         if (errmsg.empty())
             return;
 

lib/errorlogger.cpp

@@ -696,7 +696,7 @@ static void replaceColors(std::string& source, bool erase) {
         replace(source, substitutionMapErase);
 }
 
-std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation) const
+std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation, bool noCode) const
 {
     assert(!templateFormat.empty());
 
@@ -737,7 +737,8 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
                 endl = "\r\n";
             else
                 endl = "\r";
-            findAndReplace(result, "{code}", readCode(callStack.back().getOrigFile(), callStack.back().line, callStack.back().column, endl));
+            const std::string code = noCode ? "" : readCode(callStack.back().getOrigFile(), callStack.back().line, callStack.back().column, endl);
+            findAndReplace(result, "{code}", code);
         }
     } else {
         static const std::unordered_map<std::string, std::string> callStackSubstitutionMap =
@@ -768,7 +769,8 @@ std::string ErrorMessage::toString(bool verbose, const std::string &templateForm
                     endl = "\r\n";
                 else
                     endl = "\r";
-                findAndReplace(text, "{code}", readCode(fileLocation.getOrigFile(), fileLocation.line, fileLocation.column, endl));
+                const std::string code = noCode ? "" : readCode(fileLocation.getOrigFile(), fileLocation.line, fileLocation.column, endl);
+                findAndReplace(text, "{code}", code);
             }
             result += '\n' + text;
         }

lib/errorlogger.h

@@ -152,11 +152,13 @@ class CPPCHECKLIB ErrorMessage {
      * or template to be used. E.g. "{file}:{line},{severity},{id},{message}"
      * @param templateLocation Format Empty string to use default output format
      * or template to be used. E.g. "{file}:{line},{info}"
+     * @param noCode Always replace {code} with an empty string
      * @return formatted string
      */
     std::string toString(bool verbose,
                          const std::string &templateFormat,
-                         const std::string &templateLocation) const;
+                         const std::string &templateLocation,
+                         bool noCode = false) const;
 
     std::string serialize() const;
     /**

test/cli/other_test.py

@@ -4861,4 +4861,62 @@ def test_ipc_inline_suppressions(tmp_path):
     stdout_lines = stdout.splitlines()
     stdout_lines.sort()
     assert stdout_lines == stdout_exp
-    assert stderr.splitlines() == []
+    assert stderr.splitlines() == []
+
+def __count_openat_calls(tmpdir, flags, expected):
+    source_pathname = os.path.join(tmpdir, 'test.c')
+    strace_pathname = os.path.join(tmpdir, 'strace.txt')
+    content = """
+void f(int x) {
+    int y = x / 0;
+    int z = x / 0;
+}
+"""
+    cppcheck_path = __lookup_cppcheck_exe()
+
+    with open(source_pathname, 'wt') as f:
+        f.write(content)
+
+    args = [
+        'strace',
+         '--summary-only',
+         '--summary-columns=count',
+         '--trace=openat',
+         '--follow-forks',
+         f'--output={strace_pathname}',
+         f'--trace-path={source_pathname}',
+         cppcheck_path,
+         '-q',
+         source_pathname,
+    ]
+
+    args += flags
+    proc = subprocess.run(args, check=False)
+
+    assert proc.returncode == 0
+
+    with open(strace_pathname, 'r') as f:
+        strace_content = f.read()
+
+    assert strace_content.splitlines()[-1].strip() == f'{expected} total'
+
+__strace_decorator = pytest.mark.skipif(
+    sys.platform != 'linux' or 'ASAN_OPTIONS' in os.environ,
+    reason="uses strace"
+)
+
+@__strace_decorator
+def test_redundant_file_reads(tmpdir):
+    __count_openat_calls(tmpdir, [], 3)
+
+@__strace_decorator
+def test_redundant_file_reads_suppress(tmpdir):
+    __count_openat_calls(tmpdir, [ '--suppress=zerodiv' ], 1)
+
+@__strace_decorator
+def test_redundant_file_reads_template_cppcheck1(tmpdir):
+    __count_openat_calls(tmpdir, [ '--template=cppcheck1' ], 1)
+
+@__strace_decorator
+def test_redundant_file_reads_xml(tmpdir):
+    __count_openat_calls(tmpdir, [ '--xml' ], 1)

test/cli/performance_test.py

@@ -441,7 +441,7 @@ def test_slow_many_headers(tmpdir):
 
 
 @pytest.mark.skipif(sys.platform == 'darwin', reason='GitHub macOS runners are too slow')
-@pytest.mark.timeout(10)
+@pytest.mark.timeout(5)
 def test_large_number_of_violations_and_suppressions(tmpdir):
     filename_main = os.path.join(tmpdir, 'main.c')
     # This name causes the PathMatch::match() to iterate ~70 times, which is not unrealistic for a header file placed in subdirs.

test/testerrorlogger.cpp

@@ -55,6 +55,7 @@ class TestErrorLogger : public TestFixture {
         TEST_CASE(ErrorMessageVerboseNewline);
         TEST_CASE(ErrorMessageFromInternalError);
         TEST_CASE(ErrorMessageCode);
+        TEST_CASE(ErrorMessageNoCode);
         TEST_CASE(CustomFormat);
         TEST_CASE(CustomFormat2);
         TEST_CASE(CustomFormatLocations);
@@ -386,6 +387,22 @@ class TestErrorLogger : public TestFixture {
                       msg.toString(false, "{file}:{line}:{column}: {severity}:{inconclusive:inconclusive:} {message} [{id}]\n{code}", ""));
     }
 
+    void ErrorMessageNoCode() const {
+        ScopedFile file("code.cpp",
+                        "int i;\n"
+                        "int i2;\n"
+                        "int i3;\n"
+                        );
+
+        ErrorMessage::FileLocation code{"code.cpp", 3, 5};
+        std::list<ErrorMessage::FileLocation> locs = { code };
+        ErrorMessage msg(std::move(locs), "", Severity::error, "Programming error.\nVerbose error", "errorId", Certainty::normal);
+        ASSERT_EQUALS(1, msg.callStack.size());
+        const bool noCode = true;
+        ASSERT_EQUALS("code.cpp:3:5: error: Programming error. [errorId]\n",
+                      msg.toString(false, "{file}:{line}:{column}: {severity}:{inconclusive:inconclusive:} {message} [{id}]\n{code}", "", noCode));
+    }
+
     void CustomFormat() const {
         std::list<ErrorMessage::FileLocation> locs(1, fooCpp5);
         ErrorMessage msg(std::move(locs), "", Severity::error, "Programming error.\nVerbose error", "errorId", Certainty::normal);