From f6605523e8de916a481186d030f0a97308d2bf4d Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Sun, 14 Apr 2024 20:37:12 +1000 Subject: [PATCH] cppheck: improvements to checking script Various changes that simplify running cppcheck, comparing results from the previous execution. - Create a summary of the log that groups errors by type, useful since some kinds of warnings tend to lead to errors in the code more than others. - Keep a copy of the previous runs logs - useful for comparisons. - Log the output in the order of the files selected to check. - Fix non thread-safe output sometimes mixing warnings from different processes. --- GNUmakefile | 4 +- .../cmake/cmake_static_check_cppcheck.py | 244 +++++++++++++++--- build_files/cmake/project_source_info.py | 54 +++- 3 files changed, 258 insertions(+), 44 deletions(-) diff --git a/GNUmakefile b/GNUmakefile index 2f569df144a..92a603c5657 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -481,9 +481,7 @@ check_cppcheck: .FORCE @$(CMAKE_CONFIG) @cd "$(BUILD_DIR)" ; \ $(PYTHON) \ - "$(BLENDER_DIR)/build_files/cmake/cmake_static_check_cppcheck.py" 2> \ - "$(BLENDER_DIR)/check_cppcheck.txt" - @echo "written: check_cppcheck.txt" + "$(BLENDER_DIR)/build_files/cmake/cmake_static_check_cppcheck.py" check_struct_comments: .FORCE @$(CMAKE_CONFIG) diff --git a/build_files/cmake/cmake_static_check_cppcheck.py b/build_files/cmake/cmake_static_check_cppcheck.py index c21f1e8f7d2..af6cf1f4720 100644 --- a/build_files/cmake/cmake_static_check_cppcheck.py +++ b/build_files/cmake/cmake_static_check_cppcheck.py @@ -7,10 +7,14 @@ import project_source_info import subprocess import sys import os +import re import tempfile +import time from typing import ( Any, + Dict, + IO, List, Tuple, ) @@ -25,6 +29,12 @@ CHECKER_IGNORE_PREFIX = [ "extern", ] +# Optionally use a separate build dir for each source code directory. +# According to CPPCHECK docs using one directory is a way to take advantage of "whole program" checks, +# although it looks as if there might be name-space issues - overwriting files with similar names across +# different parts of the source. +CHECKER_ISOLATE_BUILD_DIR = False + # To add files use a relative path. CHECKER_EXCLUDE_SOURCE_FILES = set(os.path.join(*f.split("/")) for f in ( "source/blender/draw/engines/eevee_next/eevee_lut.cc", @@ -37,7 +47,10 @@ CHECKER_ARGS = ( "--max-configs=1", # Enable this when includes are missing. - # "--check-config", + # `"--check-config",` + + # May be interesting to check on increasing this for better results: + # `"--max-ctu-depth=2",` # This is slower, for a comprehensive output it is needed. "--check-level=exhaustive", @@ -45,6 +58,8 @@ CHECKER_ARGS = ( # Shows many pedantic issues, some are quite useful. "--enable=all", + # Tends to give many false positives, could investigate if there are any ways to resolve, for now it's noisy. + "--disable=unusedFunction", # Also shows useful messages, even if some are false-positives. "--inconclusive", @@ -52,19 +67,6 @@ CHECKER_ARGS = ( # Generates many warnings, CPPCHECK known about system includes without resolving them. "--suppress=missingIncludeSystem", - # Not considered an error. - "--suppress=allocaCalled", - # Overly noisy, we could consider resolving all of these at some point. - "--suppress=cstyleCast", - # There are various classes which don't have copy or equal constructors (GHOST windows for e.g.) - "--suppress=noCopyConstructor", - # Similar for `noCopyConstructor`. - "--suppress=nonoOperatorEq", - # There seems to be many false positives here. - "--suppress=unusedFunction", - # May be interesting to handle but very noisy currently. - "--suppress=variableScope", - # Quiet output, otherwise all defines/includes are printed (overly verbose). # Only enable this for troubleshooting (if defines are not set as expected for example). *(() if USE_VERBOSE else ("--quiet",)) @@ -80,6 +82,30 @@ CHECKER_ARGS_CXX = ( "--std=c++17", ) +# NOTE: it seems we can't exclude these from CPPCHECK directly (from what I can see) +# so exclude them from the summary. +CHECKER_EXCLUDE_FROM_SUMMARY = { + # Not considered an error. + "allocaCalled", + # Typically these can't be made `const`. + "constParameterCallback", + # Overly noisy, we could consider resolving all of these at some point. + "cstyleCast", + # Calling `memset` of float may technically be a bug but works in practice. + "memsetClassFloat", + # There are various classes which don't have copy or equal constructors (GHOST windows for e.g.) + "noCopyConstructor", + # Similar for `noCopyConstructor`. + "nonoOperatorEq", + # There seems to be many false positives here. + "unusedFunction", + # TODO: consider enabling this, more of a preference, + # not using STL algorithm's doesn't often hint at actual errors. + "useStlAlgorithm", + # May be interesting to handle but very noisy currently. + "variableScope", +} + def source_info_filter( source_info: List[Tuple[str, List[str], List[str]]], @@ -95,20 +121,30 @@ def source_info_filter( if c_relative in CHECKER_EXCLUDE_SOURCE_FILES: CHECKER_EXCLUDE_SOURCE_FILES.remove(c_relative) continue + # Exclude generated RNA, harmless but also not very useful and are quite slow. + if c.endswith("_gen.cc") and os.path.basename(c).startswith("rna_"): + continue + # TODO: support filtering on filepath. + # if "/editors/mask" not in c: + # continue source_info_result.append(item) if CHECKER_EXCLUDE_SOURCE_FILES: - sys.stderr.write("Error: exclude file(s) are missing: %r\n" % list(sorted(CHECKER_EXCLUDE_SOURCE_FILES))) + sys.stderr.write( + "Error: exclude file(s) are missing: {!r}\n".format(list(sorted(CHECKER_EXCLUDE_SOURCE_FILES))) + ) sys.exit(1) return source_info_result -def cppcheck(temp_dir: str) -> None: - temp_build_dir = os.path.join(temp_dir, "build") +def cppcheck(cppcheck_dir: str, temp_dir: str, log_fh: IO[bytes]) -> None: temp_source_dir = os.path.join(temp_dir, "source") + os.mkdir(temp_source_dir) del temp_dir - os.mkdir(temp_build_dir) - os.mkdir(temp_source_dir) + source_dir = os.path.normpath(os.path.abspath(project_source_info.SOURCE_DIR)) + + cppcheck_build_dir = os.path.join(cppcheck_dir, "build") + os.makedirs(cppcheck_build_dir, exist_ok=True) source_info = project_source_info.build_info(ignore_prefix_list=CHECKER_IGNORE_PREFIX) cppcheck_compiler_h = os.path.join(temp_source_dir, "cppcheck_compiler.h") @@ -132,20 +168,32 @@ def cppcheck(temp_dir: str) -> None: else: checker_args_extra = CHECKER_ARGS_CXX + if CHECKER_ISOLATE_BUILD_DIR: + build_dir_for_source = os.path.relpath(os.path.dirname(os.path.normpath(os.path.abspath(c))), source_dir) + build_dir_for_source = os.sep + build_dir_for_source + os.sep + build_dir_for_source = build_dir_for_source.replace( + os.sep + ".." + os.sep, + os.sep + "__" + os.sep, + ).strip(os.sep) + + build_dir_for_source = os.path.join(cppcheck_build_dir, build_dir_for_source) + + os.makedirs(build_dir_for_source, exist_ok=True) + else: + build_dir_for_source = cppcheck_build_dir + cmd = ( CHECKER_BIN, *CHECKER_ARGS, *checker_args_extra, - "--cppcheck-build-dir=" + temp_build_dir, + "--cppcheck-build-dir=" + build_dir_for_source, "--include=" + cppcheck_compiler_h, # NOTE: for some reason failing to include this crease a large number of syntax errors # from `intern/guardedalloc/MEM_guardedalloc.h`. Include directly to resolve. - "--include=" + os.path.join( - project_source_info.SOURCE_DIR, "source", "blender", "blenlib", "BLI_compiler_attrs.h", - ), + "--include={:s}".format(os.path.join(source_dir, "source", "blender", "blenlib", "BLI_compiler_attrs.h")), c, - *[("-I%s" % i) for i in inc_dirs], - *[("-D%s" % d) for d in defs], + *[("-I{:s}".format(i)) for i in inc_dirs], + *[("-D{:s}".format(d)) for d in defs], ) check_commands.append((c, cmd)) @@ -153,29 +201,153 @@ def cppcheck(temp_dir: str) -> None: process_functions = [] def my_process(i: int, c: str, cmd: List[str]) -> subprocess.Popen[Any]: - if USE_VERBOSE_PROGRESS: - percent = 100.0 * (i / len(check_commands)) - percent_str = "[" + ("%.2f]" % percent).rjust(7) + " %:" + proc = subprocess.Popen( + cmd, + stderr=subprocess.PIPE, + stdout=subprocess.PIPE, + ) - sys.stdout.flush() - sys.stdout.write("%s %s\n" % ( - percent_str, - os.path.relpath(c, project_source_info.SOURCE_DIR) - )) + # A bit dirty, but simplifies logic to read these back later. + proc.my_index = i # type: ignore + proc.my_time = time.time() # type: ignore - return subprocess.Popen(cmd) + return proc for i, (c, cmd) in enumerate(check_commands): process_functions.append((my_process, (i, c, cmd))) - project_source_info.queue_processes(process_functions) + index_current = 0 + index_count = 0 + proc_results_by_index: Dict[int, Tuple[bytes, bytes]] = {} + + def process_finalize( + proc: subprocess.Popen[Any], + stdout: bytes, + stderr: bytes, + ) -> None: + nonlocal index_current, index_count + index_count += 1 + + assert hasattr(proc, "my_index") + index = proc.my_index + assert hasattr(proc, "my_time") + time_orig = proc.my_time + + c = check_commands[index][0] + + time_delta = time.time() - time_orig + if USE_VERBOSE_PROGRESS: + percent = 100.0 * (index_count / len(check_commands)) + sys.stdout.flush() + sys.stdout.write("[{:s}] %: {:s} ({:.2f})\n".format( + ("{:.2f}".format(percent)).rjust(6), + os.path.relpath(c, source_dir), + time_delta, + )) + + while index == index_current: + log_fh.write(stderr) + log_fh.write(b"\n") + log_fh.write(stdout) + log_fh.write(b"\n") + + index_current += 1 + test_data = proc_results_by_index.pop(index_current, None) + if test_data is not None: + stdout, stderr = test_data + index += 1 + else: + proc_results_by_index[index] = stdout, stderr + + project_source_info.queue_processes( + process_functions, + process_finalize=process_finalize, + # job_total=4, + ) print("Finished!") +def cppcheck_generate_summary( + log_fh: IO[str], + log_summary_fh: IO[str], +) -> None: + source_dir = project_source_info.SOURCE_DIR + source_dir_source = os.path.join(source_dir, "source") + os.sep + source_dir_intern = os.path.join(source_dir, "intern") + os.sep + + # Avoids many duplicate lines generated by headers. + lines_unique = set() + + category: Dict[str, List[str]] = {} + re_match = re.compile(".* \\[([a-zA-Z_]+)\\]") + for line in log_fh: + if not ((source_dir_source in line) or (source_dir_intern in line)): + continue + + if (m := re_match.match(line)) is None: + continue + g = m.group(1) + if g in CHECKER_EXCLUDE_FROM_SUMMARY: + continue + + if line in lines_unique: + continue + lines_unique.add(line) + + try: + category_list = category[g] + except KeyError: + category_list = category[g] = [] + category_list.append(line) + + for key, value in sorted(category.items()): + log_summary_fh.write("\n\n{:s}\n".format(key)) + for line in value: + log_summary_fh.write(line) + + def main() -> None: + + cmake_dir = os.path.normpath(os.path.abspath(project_source_info.CMAKE_DIR)) + + cppcheck_dir = os.path.join(cmake_dir, "cppcheck") + os.makedirs(cppcheck_dir, exist_ok=True) + + filepath_output_log = os.path.join(cppcheck_dir, "cppcheck.log") + filepath_output_summary_log = os.path.join(cppcheck_dir, "cppcheck_summary.log") + + files_old = {} + + # Comparing logs is useful, keep the old ones (renamed). + for filepath in ( + filepath_output_log, + filepath_output_summary_log, + ): + if not os.path.exists(filepath): + continue + filepath_old = filepath.removesuffix(".log") + ".old.log" + if os.path.exists(filepath_old): + os.remove(filepath_old) + os.rename(filepath, filepath_old) + files_old[filepath] = filepath_old + with tempfile.TemporaryDirectory() as temp_dir: - cppcheck(temp_dir) + with open(filepath_output_log, "wb") as log_fh: + cppcheck(cppcheck_dir, temp_dir, log_fh) + + with ( + open(filepath_output_log, "r", encoding="utf-8") as log_fh, + open(filepath_output_summary_log, "w", encoding="utf-8") as log_summary_fh, + ): + cppcheck_generate_summary(log_fh, log_summary_fh) + + print("Written:") + for filepath in ( + filepath_output_log, + filepath_output_summary_log, + ): + print(" ", filepath, "<->", files_old.get(filepath, "")) if __name__ == "__main__": diff --git a/build_files/cmake/project_source_info.py b/build_files/cmake/project_source_info.py index fa085cc9210..5faab99eb4f 100644 --- a/build_files/cmake/project_source_info.py +++ b/build_files/cmake/project_source_info.py @@ -226,6 +226,17 @@ def build_defines_as_args() -> List[str]: ] +def process_make_non_blocking(proc: subprocess.Popen[Any]) -> subprocess.Popen[Any]: + import fcntl + for fh in (proc.stderr, proc.stdout): + if fh is None: + continue + fd = fh.fileno() + fl = fcntl.fcntl(fd, fcntl.F_GETFL) + fcntl.fcntl(fd, fcntl.F_SETFL, fl | os.O_NONBLOCK) + return proc + + # could be moved elsewhere!, this just happens to be used by scripts that also # use this module. def queue_processes( @@ -233,6 +244,7 @@ def queue_processes( *, job_total: int = -1, sleep: float = 0.1, + process_finalize: Optional[Callable[[subprocess.Popen[Any], bytes, bytes], Optional[int]]] = None, ) -> None: """ Takes a list of function arg pairs, each function must return a process """ @@ -248,15 +260,46 @@ def queue_processes( sys.stderr.flush() process = func(*args) - process.wait() + if process_finalize is not None: + data = process.communicate() + process_finalize(process, *data) else: import time - processes: List[subprocess.Popen[Any]] = [] + if process_finalize is not None: + def poll_and_finalize( + p: subprocess.Popen[Any], + stdout: List[bytes], + stderr: List[bytes], + ) -> Optional[int]: + assert p.stdout is not None + if data := p.stdout.read(): + stdout.append(data) + assert p.stderr is not None + if data := p.stderr.read(): + stderr.append(data) + + if (returncode := p.poll()) is not None: + data_stdout, data_stderr = p.communicate() + if data_stdout: + stdout.append(data_stdout) + if data_stderr: + stderr.append(data_stderr) + process_finalize(p, b"".join(stdout), b"".join(stderr)) + return returncode + else: + def poll_and_finalize( + p: subprocess.Popen[Any], + stdout: List[bytes], + stderr: List[bytes], + ) -> Optional[int]: + return p.poll() + + processes: List[Tuple[subprocess.Popen[Any], List[bytes], List[bytes]]] = [] for func, args in process_funcs: # wait until a thread is free while 1: - processes[:] = [p for p in processes if p.poll() is None] + processes[:] = [p_item for p_item in processes if poll_and_finalize(*p_item) is None] if len(processes) <= job_total: break @@ -265,11 +308,12 @@ def queue_processes( sys.stdout.flush() sys.stderr.flush() - processes.append(func(*args)) + processes.append((process_make_non_blocking(func(*args)), [], [])) # Don't return until all jobs have finished. while 1: - processes[:] = [p for p in processes if p.poll() is None] + processes[:] = [p_item for p_item in processes if poll_and_finalize(*p_item) is None] + if not processes: break time.sleep(sleep)