From dee2b81067cd7f44d55eed2d8a11ded0e7f7a4c0 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 10 Sep 2026 15:07:28 +0200 Subject: [PATCH] tools: add a checker for missing argument name comments CODING_STYLE asks for a /* name= */ comment whenever NULL or another value meaning "unset" is passed to a function. Nothing enforces it. clang-tidy's bugprone-argument-comment only validates comments that exist, and its option to demand them for literals never sees NULL behind its cast in C and flags every size and mode along the way. Add tools/check-argument-comments.py. It parses each file with libclang and the flags from compile_commands.json, with every set of them for a file that is built more than once, takes the parameter names from the first declaration the caller sees, the one clang-tidy compares comments against, and reports arguments spelled as a bare NULL, 0, false or true, in the file and in the in-tree headers it includes. The arguments are read from the text of the call. So a literal with a comment after it is still bare, and a comment at the end of the previous line belongs to the previous argument. Like clang-tidy, only a /* name= */ comment names a parameter, and case and leading or trailing underscores don't matter. A /* name */ that lost its '=' is reported too. Calls into libc are skipped, and so are calls produced by macro bodies since the caller never spelled their arguments. A call written inside a macro argument is checked, as is a call through a macro that only renames the function, the shims in src/include/override for example, and a call through the sym_* pointers of the dlopen() wrappers. --diff REV limits the report to lines that differ from a revision, and --fix inserts the comments. tools/check-argument-comments.py --diff "$(git merge-base main HEAD)" src The builtin headers, stddef.h and friends, are located by asking the clang binary of the same major version. libclang derives their location from its own, and in the Fedora rawhide tools tree that comes out as the relative ../lib/clang/22, so every file fails to parse with 'stddef.h' file not found. Needs python3-clang and libclang 19 or newer. Older versions return the file location from clang_getSpellingLocation(), so calls produced by macro bodies would be checked as if the caller had spelled them. --- docs/CODING_STYLE.md | 10 + tools/check-argument-comments.py | 1001 ++++++++++++++++++++++++++++++ 2 files changed, 1011 insertions(+) create mode 100755 tools/check-argument-comments.py diff --git a/docs/CODING_STYLE.md b/docs/CODING_STYLE.md index 378de8248e2..6b497a8d2f8 100644 --- a/docs/CODING_STYLE.md +++ b/docs/CODING_STYLE.md @@ -286,6 +286,16 @@ SPDX-License-Identifier: LGPL-2.1-or-later ``` This guidance should be applied tree-wide, including in test files. + `tools/check-argument-comments.py` enforces it for every bare `NULL`, `0`, + `true` and `false`, not only the ones meaning "unset", passed for a parameter + with a telling name. Single-letter names, `argN`, reserved `__names`, calls + into libc and other code outside the tree, and calls produced by macro bodies + are exempt. Calls through the `sym_*` pointers of the dlopen() wrappers use + the names from the library's header. It also checks that an existing comment + names the right parameter. `meson test -C build --suite=argument-comments` + checks the whole tree. Once `meson compile -C build gensources` has run, + `tools/check-argument-comments.py -p build --diff "$(git merge-base main HEAD)" src` + checks only what a branch changed, and `--fix` inserts the missing comments. - Please do not introduce new circular dependencies between header files. Effectively this means that if a.h includes b.h, then b.h cannot include a.h, diff --git a/tools/check-argument-comments.py b/tools/check-argument-comments.py new file mode 100755 index 00000000000..1e100e4fd5c --- /dev/null +++ b/tools/check-argument-comments.py @@ -0,0 +1,1001 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: LGPL-2.1-or-later +"""Report function arguments passed as a bare NULL, 0, false or true without a /* name= */ comment. + +docs/CODING_STYLE.md asks for the argument name in a comment whenever NULL or another value meaning +"unset" is passed to a function. clang-tidy's bugprone-argument-comment cannot check that for C code: +it never sees NULL behind its cast, and asking it to comment integer literals flags every size and +mode as well. This walks the AST with libclang instead, takes the parameter names from the actual +prototypes and looks only at the literals the rule is about. A comment that names a different +parameter is reported too, those go stale when parameters are renamed. As for bugprone-argument-comment +only a /* name= */ comment names a parameter, case and surrounding underscores aside, a /* name */ that +lost its '=' is reported as well, and any other comment, in front of the literal or after it, leaves it +bare. + +The names come from the first declaration the caller sees, usually the header, which is also the one +bugprone-argument-comment compares against. Parameters whose name says nothing, a single letter, argN +or a reserved __name copied from a libc declaration, are left alone. Calls to functions declared outside +the source tree and the build directory, e.g. libc, are skipped unless --system is given; calls through +the sym_* pointers of the dlopen() wrappers are ours and are checked against the function the pointer +was declared from, except that a comment naming none of its parameters is only warned about, library +headers rename parameters between versions. Calls produced by a macro body are skipped, the caller never +spelled the argument, while a call written inside a macro argument, assert_se(f(NULL)), is checked, and +so is a call through a macro that only renames the function, #define f f_shim. The in-tree headers a +file includes are checked from its translation unit, which covers their static inline functions, and a +file built more than once, for userspace and for the boot loader say, is checked with each set of flags. +With --diff REV only lines that differ from REV, and files git does not track, are reported, which is +what a review wants. Rust code is not checked. + +Usage: tools/check-argument-comments.py [-p BUILDDIR] [--root DIR] [--diff REV] [--fix] [-j JOBS] [--system] PATH... + +PATH is a C file or a directory to search for them, BPF programs are skipped. The compile flags come +from compile_commands.json in the build directory, the generated headers must exist ("meson compile -C +BUILDDIR gensources"); a file that is not in there, or does not parse, is an error and fails the run, +except that --diff skips files the build does not compile. --fix leaves the comments that name the +wrong parameter and files whose translation unit did not parse to a human. libclang is the one the +bindings find, else the one LIBCLANG_PATH names, the library or its directory, else the newest one +installed, and has to be 19 or newer. Exits with 1 when something is left to fix, 2 on errors and 77 +when the libclang python bindings are missing. +""" + +import argparse +import bisect +import glob +import json +import os +import re +import shlex +import shutil +import signal +import subprocess +import sys +from collections.abc import Iterator +from concurrent.futures import BrokenExecutor, ProcessPoolExecutor +from ctypes import POINTER, byref, c_uint, c_void_p, cast +from typing import Any, NamedTuple, Optional + +try: + import clang.cindex as ci # type: ignore +except ImportError: + print('python3-clang is not installed', file=sys.stderr) + sys.exit(77) + +# The argument lexer steps over comments, string and character literals and preprocessor lines, which can +# hold anything, and splits on the commas outside of nested brackets. +TOKEN_RE = re.compile( + rb'/\*.*?\*/|//(?:\\\n|[^\n])*|"(?:\\.|[^"\\\n])*"|\'(?:\\.|[^\'\\\n])*\'|^[ \t]*#(?:\\\n|[^\n])*|[()\[\]{},]', + re.DOTALL | re.MULTILINE, +) +# What may surround an argument, a newline on its own so that a preprocessor line can start after it +GAP_RE = re.compile( + rb'^[ \t]*#(?:\\\n|[^\n])*|[ \t\r\f\v]+|\n|/\*.*?\*/|//(?:\\\n|[^\n])*', re.DOTALL | re.MULTILINE +) +LITERAL_RE = re.compile(rb'(?:NULL|0|false|true)(?![\w.])') +# [] is how an array parameter is declared +NAME_COMMENT_RE = re.compile(rb'/\*\s*(\w+)(?:\[\])?\s*(=)?\s*\*/') +# The callee, maybe in parentheses, up to the parenthesis that opens the arguments +CALL_RE = re.compile(rb'((?:\(\s*)*)([A-Za-z_]\w*)\s*((?:\)\s*)*)\(') +ALIAS_RE = re.compile( + rb'[ \t]*#[ \t]*define[ \t]+(\w+)[ \t]+(\w+)[ \t]*(?:/\*[^\n]*?\*/[ \t]*)*(?://[^\n]*)?(?=\n|\Z)' +) +INCLUDE_RE = re.compile(rb'^[ \t]*#[ \t]*include[ \t]*[<"]([^>"\n]+)[>"]', re.MULTILINE) +HUNK_RE = re.compile(r'@@ -\S+ \+(\d+)(?:,(\d+))? @@') +UNINFORMATIVE_RE = re.compile(r'^(?:[a-z]|arg\d*|__.*)$', re.IGNORECASE) +# gcc-only flags of the EFI build that libclang rejects, .clang-tidy drops the same ones through RemovedArgs +DROPPED_FLAGS = frozenset({'-fwide-exec-charset=UCS2', '-maccumulate-outgoing-args'}) +LAUNCHERS = frozenset({'ccache', 'sccache', 'distcc', 'icecc'}) +# A cross compiler named after its target, which clang's tooling turns into --target= as well +CROSS_COMPILER_RE = re.compile( + r'((?:aarch64|alpha|arc|arm|csky|hppa|i[3-6]86|ia64|loongarch|m68k|microblaze|mips|or1k|powerpc|ppc|riscv' + r'|s390|sh|sparc|x86_64|xtensa)\w*(?:-\w+)+)-(?:gcc|cc|clang)(?:-[\d.]+)?$' +) + +CALL_EXPR = ci.CursorKind.CALL_EXPR.value +DECL_REF_EXPR = ci.CursorKind.DECL_REF_EXPR.value + +Command = tuple[str, tuple[str, ...]] +Edit = tuple[int, int, str] + + +class Finding(NamedTuple): + file: str + line: int + column: int + offset: int + callee: str + param: str + literal: str + comment: Optional[str] # None for a bare literal, else the text of the comment in front of it + fix: Optional[Edit] # what adds or completes the comment, None when a human has to look + + +class Result(NamedTuple): + path: str + findings: list[Finding] + errors: list[str] + warnings: list[str] + + +class Item(NamedTuple): + path: str + root: str + build_dir: str + command: Command + include_system: bool + only: Optional[dict[str, list[int]]] + resources: Optional[str] + + +class GitError(Exception): + pass + + +def newest_first(libraries: list[str]) -> list[str]: + return sorted( + libraries, key=lambda p: [int(x) if x.isdigit() else x for x in re.split(r'(\d+)', p)], reverse=True + ) + + +def setup_libclang(library_file: Optional[str] = None) -> Optional[str]: + """Loads libclang and returns the library file the workers are to load, None when the bindings find it + on their own. A worker loads the one the parent passes.""" + if ci.Config.loaded: + return library_file # a forked worker inherits the parent's library + if library_file is not None: + ci.Config.set_library_file(library_file) + ci.conf.lib + return library_file + try: + ci.conf.lib + return None + except ci.LibclangError as e: + error = e + candidates = [] + path = os.environ.get('LIBCLANG_PATH') + if path: + path = os.path.realpath(path) # a worker resolves relative paths elsewhere + if os.path.isdir(path): # the bindgen convention + candidates += newest_first(glob.glob(os.path.join(glob.escape(path), 'libclang*.so*'))) + else: + candidates.append(path) + candidates += newest_first( + glob.glob('/usr/lib/llvm-*/lib/libclang-*.so.1') + + glob.glob('/usr/lib*/libclang.so*') + + glob.glob('/usr/lib/*/libclang*.so*') + ) + for candidate in candidates: + ci.Config.set_library_file(candidate) # refused only once a library has loaded + try: + ci.conf.lib + return candidate + except ci.LibclangError: + continue + print(f'error: cannot load libclang: {error}', file=sys.stderr) + sys.exit(2) + + +def setup_worker(library_file: Optional[str]) -> None: + signal.signal(signal.SIGINT, signal.SIG_IGN) # Ctrl-C is for the parent, which cancels the run + setup_libclang(library_file) + + +# The libclang 19 bindings only have the callbacks dictionary +VISIT = getattr(ci, 'cursor_visit_callback', None) or ci.callbacks['cursor_visit'] + + +def _children(child: Any, parent: Any, out: list[Any]) -> int: + out.append(child) + return 1 # CXChildVisit_Continue + + +def _calls(child: Any, parent: Any, out: list[Any]) -> int: + if child._kind_id == CALL_EXPR: + out.append(child) + return 2 # CXChildVisit_Recurse + + +def _decl_refs(child: Any, parent: Any, out: list[Any]) -> int: + if child._kind_id == DECL_REF_EXPR: + out.append(child) + return 2 + + +CHILDREN, CALLS, DECL_REFS = VISIT(_children), VISIT(_calls), VISIT(_decl_refs) + + +def visit(cursor: Any, callback: Any) -> list[Any]: + """The cursors below cursor the callback picks. libclang does the walking, which is a lot cheaper than + walk_preorder() calling into it for every node, and does not recurse in python.""" + out: list[Any] = [] + ci.conf.lib.clang_visitChildren(cursor, callback, out) + for c in out: + c._tu = cursor._tu # keeps the translation unit alive, as the bindings do + return out + + +LOCATIONS: dict[str, Any] = {} + + +def location_function(name: str) -> Any: + """clang_getFileLocation() gives where a token was spelled if it came from a macro argument and the macro + invocation if it came from a macro body, clang_getSpellingLocation() the #define for the latter and + clang_getExpansionLocation() always the invocation. The bindings only wrap the expansion location.""" + fn = LOCATIONS.get(name) + if fn is None: + fn = LOCATIONS[name] = getattr(ci.conf.lib, name) + fn.argtypes = [ + ci.SourceLocation, + POINTER(ci.c_object_p), + POINTER(c_uint), + POINTER(c_uint), + POINTER(c_uint), + ] + fn.restype = None + return fn + + +SOURCES: dict[str, Optional[bytes]] = {} +LINES: dict[str, list[int]] = {} +REAL_PATHS: dict[tuple[str, str], str] = {} +ARGUMENTS: dict[tuple[str, int], dict[int, tuple[bytes, list[tuple[int, int]]]]] = {} + + +def read(path: str) -> Optional[bytes]: + if path not in SOURCES: + try: + with open(path, 'rb') as f: + SOURCES[path] = f.read() # libclang offsets are byte offsets + except OSError: + SOURCES[path] = None + return SOURCES[path] + + +def position(path: str, offset: int) -> tuple[int, int]: + """(line, column) of a byte offset.""" + starts = LINES.get(path) + if starts is None: + starts = LINES[path] = [0] + [m.end() for m in re.finditer(rb'\n', read(path) or b'')] + line = bisect.bisect_right(starts, offset) + return line, offset - starts[line - 1] + 1 + + +def touches(lines: list[int], first: int, last: int) -> bool: + """Whether one of the sorted lines lies in [first, last].""" + i = bisect.bisect_left(lines, first) + return i < len(lines) and lines[i] <= last + + +def real_path(directory: str, name: str) -> str: + key = (directory, name) + path = REAL_PATHS.get(key) + if path is None: + path = REAL_PATHS[key] = os.path.realpath(os.path.join(directory, name)) + return path + + +def source_path(path: str) -> str: + """A file to check, its directories resolved but not the file itself. test-bus-vtable-cc.cc is a + symlink to test-bus-vtable.c and a C++ translation unit of its own.""" + path = os.path.abspath(path) + return os.path.join(os.path.realpath(os.path.dirname(path)), os.path.basename(path)) + + +def display(path: str, root: str) -> str: + return os.path.relpath(path, root) if path.startswith(root + os.sep) else path + + +def split_arguments(source: bytes, open_paren: int) -> Optional[list[tuple[int, int]]]: + """[start, end) of each argument of the call whose argument list opens at open_paren, None when the + brackets do not match up.""" + ranges: list[tuple[int, int]] = [] + depth, start = 0, open_paren + 1 + for m in TOKEN_RE.finditer(source, open_paren): + token = m.group() + if len(token) != 1: + continue # a comment, a literal or a preprocessor line + if token in b'([{': + depth += 1 + elif token in b')]}': + depth -= 1 + if depth == 0: + ranges.append((start, m.start())) + return ranges if token == b')' else None + elif token == b',' and depth == 1: + ranges.append((start, m.start())) + start = m.end() + return None + + +def skip_gap(source: bytes, pos: int, end: int, comments: Optional[list[tuple[int, int]]] = None) -> int: + """Steps over whitespace, comments and preprocessor lines and collects the block comments.""" + while pos < end: + m = GAP_RE.match(source, pos, end) + if not m: + break + if comments is not None and source.startswith(b'/*', pos): + comments.append((pos, m.end())) + pos = m.end() + return pos + + +def bare_literal(source: bytes, start: int, end: int) -> Optional[tuple[int, bytes, list[tuple[int, int]]]]: + """(offset, literal, block comments in front) when the argument in [start, end) is nothing but NULL, 0, + false or true, in parentheses or not.""" + comments: list[tuple[int, int]] = [] + pos = arg = skip_gap(source, start, end, comments) + depth = 0 + while pos < end and source[pos] == ord('('): + depth += 1 + pos = skip_gap(source, pos + 1, end) + m = LITERAL_RE.match(source, pos, end) + if not m: + return None + pos = skip_gap(source, m.end(), end) + while depth and pos < end and source[pos] == ord(')'): + depth -= 1 + pos = skip_gap(source, pos + 1, end) + return (arg, m.group(), comments) if depth == 0 and pos == end else None + + +def literal_arguments( + path: str, source: bytes, open_paren: int +) -> dict[int, tuple[bytes, list[tuple[int, int]]]]: + """The arguments of a call that are written as a bare literal, by offset. Headers come up in every file + that includes them, so this is kept for the whole run of a worker.""" + key = (path, open_paren) + found = ARGUMENTS.get(key) + if found is None: + found = ARGUMENTS[key] = {} + for start, end in split_arguments(source, open_paren) or []: + literal = bare_literal(source, start, end) + if literal: + found[literal[0]] = (literal[1], literal[2]) + return found + + +def name_comment( + source: bytes, comments: list[tuple[int, int]], arg: int +) -> Optional[tuple[str, bool, int, int]]: + """The parameter the comments in front of an argument name: (name, whether the '=' is there, start and end + of the comment). A /* name= */ counts anywhere after the previous argument, a /* name */ only right in + front of this one and any other comment names nothing.""" + for start, end in reversed(comments): + m = NAME_COMMENT_RE.fullmatch(source, start, end) + if m and m.group(2): + return m.group(1).decode(), True, start, end + if comments: + start, end = comments[-1] + m = NAME_COMMENT_RE.fullmatch(source, start, end) + if m and not source[end:arg].strip(b' \t'): + return m.group(1).decode(), False, start, end + return None + + +def same_name(a: str, b: str) -> bool: + """The comparison of bugprone-argument-comment without StrictMode.""" + return a.strip('_').lower() == b.strip('_').lower() + + +class Checker: + """The calls of one translation unit.""" + + def __init__(self, item: Item, tu: Any): + self.item = item + self.tu = tu + self.directory = item.command[0] + self.root = item.root + os.sep + self.build_dir = item.build_dir + os.sep + self.main = os.path.realpath(item.path) + self.paths: dict[Optional[int], str] = {} + self.ours_by_name: dict[str, bool] = {} + self.params_by_name: dict[str, list[str]] = {} + self.syms: dict[str, Optional[Any]] = {} + self.functions: Optional[dict[str, Any]] = None + self.done: set[tuple[str, int, str]] = set() + self.findings: list[Finding] = [] + self.warnings: list[str] = [] + + def locate(self, function: str, location: Any) -> tuple[Optional[str], int]: + """(real path, offset) of a location, location_function() tells which one.""" + f, offset = ci.c_object_p(), c_uint() + location_function(function)(location, byref(f), None, None, byref(offset)) + if not f: + return None, 0 + key = cast(f, c_void_p).value + path = self.paths.get(key) + if path is None: + path = self.paths[key] = real_path(self.directory, ci.File(f).name) + return path, offset.value + + def ours(self, path: str) -> bool: + """Whether a file is the one checked or one of the in-tree headers it includes.""" + return path == self.main or (path.startswith(self.root) and not path.startswith(self.build_dir)) + + def top_level(self) -> Iterator[Any]: + """The definitions in the file and in the in-tree headers it includes. Under --diff only those that + span a changed line.""" + only = self.item.only + for c in visit(self.tu.cursor, CHILDREN): + path, _ = self.locate('clang_getExpansionLocation', c.location) + if path is None or not self.ours(path): + continue + if only is not None: + lines = only.get(path) + if not lines: + continue + extent = c.extent + start = self.locate('clang_getExpansionLocation', extent.start)[1] + end = self.locate('clang_getExpansionLocation', extent.end)[1] + if not touches(lines, position(path, start)[0], position(path, end)[0]): + continue + yield c + + def declared_by_us(self, function: Any) -> bool: + """Whether a function's first declaration is in the source tree, or in a header generated into the + build directory, which need not be below it.""" + ours = self.ours_by_name.get(function.spelling) + if ours is None: + path, _ = self.locate('clang_getExpansionLocation', function.canonical.location) + ours = path is not None and (path.startswith(self.root) or path.startswith(self.build_dir)) + self.ours_by_name[function.spelling] = ours + return ours + + def parameters(self, function: Any) -> list[str]: + params = self.params_by_name.get(function.spelling) + if params is None: + params = self.params_by_name[function.spelling] = [ + p.spelling for p in function.canonical.get_arguments() + ] + return params + + def sym_function(self, var: Any) -> Optional[Any]: + """The function a sym_* pointer was declared from with typeof(&function), which also sees through a + library renaming its functions with macros, pcre2 does.""" + if var.spelling not in self.syms: + function = None + for ref in visit(var, DECL_REFS): + target = ref.referenced + if target is not None and target.kind == ci.CursorKind.FUNCTION_DECL: + function = target + break + if function is None: + if self.functions is None: + self.functions = { + c.spelling: c + for c in visit(self.tu.cursor, CHILDREN) + if c.kind == ci.CursorKind.FUNCTION_DECL + } + function = self.functions.get(var.spelling[4:]) + self.syms[var.spelling] = function + return self.syms[var.spelling] + + def alias(self, path: Optional[str], offset: int, name: bytes, target: str) -> bool: + """Whether the token at offset is the body of '#define name target', a macro that only renames.""" + source = read(path) if path else None + if source is None: + return False + m = ALIAS_RE.match(source, source.rfind(b'\n', 0, offset) + 1) + return ( + m is not None and m.group(1) == name and m.group(2) == target.encode() and m.start(2) == offset + ) + + def callee(self, call: Any, name: bytes) -> Optional[tuple[Any, bool]]: + """The function a call names and whether it does so through a sym_* pointer. None for calls through + an expression, calls a macro body produced and functions that are not ours.""" + children = visit(call, CHILDREN) + expr = children[0] if children else None + while expr is not None and expr.kind in (ci.CursorKind.UNEXPOSED_EXPR, ci.CursorKind.PAREN_EXPR): + inner = visit(expr, CHILDREN) + expr = inner[0] if len(inner) == 1 else None + if expr is None or expr._kind_id != DECL_REF_EXPR: + return None # the pointer some expression yields, lookup(x)(NULL) say + decl = expr.referenced + if decl is None: + return None + spelled = self.locate('clang_getSpellingLocation', expr.location) + if spelled != self.locate('clang_getFileLocation', expr.location) and not self.alias( + *spelled, name, decl.spelling + ): + return None # the product of a macro body, the caller never spelled the arguments + if decl.kind == ci.CursorKind.FUNCTION_DECL: + if not self.item.include_system and not self.declared_by_us(decl): + return None + return decl, False + if decl.kind == ci.CursorKind.VAR_DECL and decl.spelling.startswith('sym_'): + function = self.sym_function(decl) + return (function, True) if function is not None else None + return None + + def check(self, call: Any) -> None: + path, offset = self.locate('clang_getFileLocation', call.location) + if path is None or not self.ours(path): + return + only_lines = None + if self.item.only is not None: + only_lines = self.item.only.get(path) + if not only_lines: + return + source = read(path) + if source is None: + return + m = CALL_RE.match(source, offset) + if not m or m.group(1).count(b'(') != m.group(3).count(b')'): + return # the call is not spelled out here, a macro body produced it + open_paren = m.end() - 1 + literals = literal_arguments(path, source, open_paren) + if not literals: + return + callee = self.callee(call, m.group(2)) + if callee is None: + return + function, via_sym = callee + key = (path, open_paren, function.spelling) + if key in self.done: + return # a macro such as ASSERT_EQ() has its argument more than once + self.done.add(key) + name = m.group(2).decode() + params = self.parameters(function) + for i, arg in enumerate(call.get_arguments()): + if i >= len(params): + break # the rest is variadic + param = params[i] + if arg is None or not param or UNINFORMATIVE_RE.match(param): + continue + arg_path, arg_offset = self.locate('clang_getFileLocation', arg.location) + found = literals.get(arg_offset) if arg_path == path else None + if found is None: + continue # not written as a bare literal, or not at this position of the call + line, column = position(path, arg_offset) + if only_lines is not None and not touches(only_lines, line, line): + continue + literal, comments = found + named = name_comment(source, comments, arg_offset) + comment: Optional[str] = None + fix: Optional[Edit] = None + if named is None: + fix = (arg_offset, arg_offset, f'/* {param}= */ ') + else: + text, has_equals, start, end = named + if has_equals and same_name(text, param): + continue + comment = source[start + 2 : end - 2].strip().decode(errors='replace') + if via_sym and not any(p and same_name(text, p) for p in params): + self.warnings.append( + f'{display(path, self.item.root)}:{line}:{column}: {name}(): ' + f"'{comment}' names no parameter of {function.spelling}(), " + f"its header calls argument {i + 1} '{param}'" + ) + continue + if not has_equals and same_name(text, param): + fix = (start, end, f'/* {param}= */') + self.findings.append( + Finding(path, line, column, arg_offset, name, param, literal.decode(), comment, fix) + ) + + +def describe(diagnostic: Any, root: str) -> str: + location = diagnostic.location + if not location.file: + return str(diagnostic.spelling) + return f'{display(os.path.realpath(location.file.name), root)}:{location.line}: {diagnostic.spelling}' + + +def check_file(item: Item) -> Result: + directory, flags = item.command + os.chdir(directory) + args = ([f'-resource-dir={item.resources}'] if item.resources else []) + list(flags) + try: + tu = ci.Index.create().parse(item.path, args=args) + except ci.TranslationUnitLoadError as e: + return Result(item.path, [], [str(e)], []) + errors = [describe(d, item.root) for d in tu.diagnostics if d.severity >= ci.Diagnostic.Error] + checker = Checker(item, tu) + for top in checker.top_level(): + for call in visit(top, CALLS): + checker.check(call) + return Result(item.path, checker.findings, errors, checker.warnings) + + +def worker(item: Item) -> Result: + try: + return check_file(item) + except Exception as e: # a compile directory that is gone, say, fails this file and not the run + return Result(item.path, [], [f'{type(e).__name__}: {e}'], []) + + +def expand_response_files(argv: list[str], directory: str, depth: int = 0) -> list[str]: + out: list[str] = [] + for a in argv: + if a.startswith('@') and depth < 8: + try: + with open(os.path.join(directory, a[1:])) as f: + out += expand_response_files(shlex.split(f.read()), directory, depth + 1) + continue + except OSError: + pass + out.append(a) + return out + + +def compile_flags(argv: list[str], directory: str, path: str) -> tuple[str, ...]: + """The flags of a compile command minus what only matters to a real compile, with the target a cross + compiler implies made explicit, as clang's tooling does.""" + while len(argv) > 1 and os.path.basename(argv[0]) in LAUNCHERS: + argv = argv[1:] + argv = expand_response_files(argv, directory) + flags: list[str] = [] + skip = False + for a in argv[1:]: + if skip: + skip = False + elif a in ('-o', '-MF', '-MQ', '-MT'): + skip = True + elif a in ('-c', '-MD', '-MMD') or a in DROPPED_FLAGS or a.startswith('-fdiagnostics'): + continue + elif a.startswith('-Wp,'): + if not any(p.startswith('-M') for p in a[4:].split(',')): + flags.append(a) # -D, -U and -include for the preprocessor, but no dependency files + elif a.startswith('-W'): + continue + elif ( + a.startswith('-') + or os.path.basename(a) != os.path.basename(path) + or source_path(os.path.join(directory, a)) != path + ): + flags.append(a) + m = CROSS_COMPILER_RE.match(os.path.basename(argv[0])) + if m and not any(f.startswith('--target=') or f == '-target' for f in flags): + flags.insert(0, f'--target={m.group(1)}') + return tuple(flags) + + +def compile_commands(build_dir: str, wanted: set[str]) -> dict[str, list[Command]]: + """file -> each distinct (directory, flags) it is compiled with, for the files asked for.""" + with open(os.path.join(build_dir, 'compile_commands.json')) as f: + entries = json.load(f) + commands: dict[str, list[Command]] = {} + for e in entries: + path = source_path(os.path.join(e['directory'], e['file'])) + if path not in wanted: + continue + argv = e['arguments'] if 'arguments' in e else shlex.split(e['command']) + command = (e['directory'], compile_flags(argv, e['directory'], path)) + if command not in commands.setdefault(path, []): + commands[path].append(command) + return commands + + +def libclang_major() -> Optional[int]: + try: + fn = ci.conf.lib.clang_getClangVersion # the bindings do not declare its return type + fn.restype = ci._CXString + version = str(ci._CXString.from_result(fn())) + except AttributeError: + return None + m = re.search(r'clang version (\d+)', version) + return int(m.group(1)) if m else None + + +def resource_dir() -> Optional[str]: + """Where clang's builtin headers, stddef.h and friends, live. libclang derives that from its own location, + which goes wrong when the library does not sit next to them, so ask the clang binary of the same major + version instead. An unversioned clang only counts when it is that version, the headers of another one + need not parse with this libclang.""" + major = libclang_major() + if major is None: + return None + for name in (f'clang-{major}', 'clang'): + clang = shutil.which(name) + if not clang: + continue + r = subprocess.run([clang, '-print-resource-dir'], capture_output=True, text=True) + path = r.stdout.strip() + if ( + r.returncode == 0 + and os.path.basename(path).split('.')[0] == str(major) + and os.path.exists(os.path.join(path, 'include', 'stddef.h')) + ): + return path + return None + + +def source_root(path: str) -> Optional[str]: + """The tree the file belongs to, which need not be the one this script lives in.""" + try: + r = subprocess.run( + ['git', '-C', os.path.dirname(path), 'rev-parse', '--show-toplevel'], + capture_output=True, + text=True, + ) + except FileNotFoundError: + return None + return os.path.realpath(r.stdout.strip()) if r.returncode == 0 else None + + +def twin_header(path: str) -> Optional[str]: + """The header a .c file is paired with.""" + header = path[:-2] + '.h' + return header if os.path.exists(header) else None + + +def includes(path: str, header: str) -> bool: + name = os.path.basename(header).encode() + return any(os.path.basename(m.group(1)) == name for m in INCLUDE_RE.finditer(read(path) or b'')) + + +def git(cwd: str, *args: str) -> str: + try: + r = subprocess.run( + ['git', '-c', 'core.quotePath=false', *args], + cwd=cwd, + capture_output=True, + text=True, + errors='surrogateescape', + ) + except OSError as e: + raise GitError(f'git: {e}') from e + if r.returncode != 0: + raise GitError(r.stderr.strip() or f'git {args[0]} failed') + return r.stdout + + +def unquote(name: str) -> str: + """Undoes git's C-style quoting of unusual file names.""" + if not name.startswith('"'): + return name + escapes = {'a': 7, 'b': 8, 't': 9, 'n': 10, 'v': 11, 'f': 12, 'r': 13} + raw, out, i = name[1:-1].encode(errors='surrogateescape'), bytearray(), 0 + while i < len(raw): + if raw[i] == ord('\\') and i + 1 < len(raw): + c = chr(raw[i + 1]) + if c in '01234567': + out.append(int(raw[i + 1 : i + 4], 8)) + i += 4 + else: + out.append(escapes.get(c, raw[i + 1])) + i += 2 + else: + out.append(raw[i]) + i += 1 + return out.decode(errors='surrogateescape') + + +def changed_lines(root: str, rev: str, paths: list[str]) -> dict[str, list[int]]: + """The lines of the files below paths that differ from rev, all of them for files git does not track. + One diff over all of them pairs up renames, and git's settings must not widen the hunks.""" + if rev.startswith('-'): + raise GitError(f'{rev}: not a revision') + try: + git(root, 'rev-parse', '--verify', rev + '^{commit}') + except GitError as e: + raise GitError(f'{rev}: {e}') from e + top = os.path.realpath(git(root, 'rev-parse', '--show-toplevel').strip()) + diff = git( + root, + 'diff', + '-U0', + '--inter-hunk-context=0', + '--no-color', + '--no-ext-diff', + '--no-textconv', + '--no-relative', + '--src-prefix=a/', + '--dst-prefix=b/', + '-M', + rev, + '--', + *paths, + ) + lines: dict[str, set[int]] = {} + current: Optional[set[int]] = None + header = False + for line in diff.split('\n'): + if line.startswith('diff '): + header, current = True, None + elif header and line.startswith('+++ '): + name = unquote(line[4:]) + current = None + if name.startswith('b/'): + current = lines.setdefault(os.path.realpath(os.path.join(top, name[2:])), set()) + elif line.startswith('@@'): + header = False + m = HUNK_RE.match(line) + if m and current is not None: + start = int(m.group(1)) + current.update(range(start, start + (int(m.group(2)) if m.group(2) is not None else 1))) + untracked = git(root, 'ls-files', '-z', '--full-name', '--others', '--exclude-standard', '--', *paths) + for name in untracked.split('\0'): + if name: + path = os.path.realpath(os.path.join(top, name)) + lines[path] = set(range(1, (read(path) or b'').count(b'\n') + 2)) + return {path: sorted(numbers) for path, numbers in lines.items() if numbers} + + +def diff_selection( + files: list[str], only: dict[str, list[int]], commands: dict[str, list[Command]] +) -> list[str]: + """The files --diff parses: the changed ones and those paired with a changed header. A changed header + that no compiled file of its own includes is checked from whichever file includes it, so then all.""" + paired = set() + for f in files: + header = twin_header(f) + if header and f in commands and includes(f, header): + paired.add(os.path.realpath(header)) + if any(path.endswith('.h') and path not in paired for path in only): + return files + return [f for f in files if os.path.realpath(f) in only or os.path.realpath(twin_header(f) or f) in only] + + +def fix_files(edits: list[tuple[str, Edit]]) -> None: + by_file: dict[str, list[Edit]] = {} + for path, edit in edits: + by_file.setdefault(path, []).append(edit) + for path, items in by_file.items(): + with open(path, 'rb') as f: + source = f.read() + for start, end, text in sorted(items, reverse=True): + source = source[:start] + text.encode() + source[end:] + with open(path, 'wb') as f: + f.write(source) + + +def default_jobs() -> int: + try: + return len(os.sched_getaffinity(0)) + except (AttributeError, OSError): + return os.cpu_count() or 1 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.split('\n\n')[0] if __doc__ else None) + parser.add_argument( + '-p', '--build-dir', default='build', help='build directory with compile_commands.json' + ) + parser.add_argument( + '--root', + help='source tree whose functions are checked, the git toplevel of the first PATH by default', + ) + parser.add_argument('-j', '--jobs', type=int, default=default_jobs(), help='parallel jobs') + parser.add_argument('--diff', metavar='REV', help='only report lines that differ from this git revision') + parser.add_argument('--fix', action='store_true', help='insert the missing comments in place') + parser.add_argument( + '--system', action='store_true', help='also check calls to functions declared outside the tree' + ) + parser.add_argument( + 'paths', nargs='+', metavar='PATH', help='C files, or directories to search for them' + ) + args = parser.parse_args() + if args.diff == '': + print('error: --diff needs a revision', file=sys.stderr) + return 2 + + library = setup_libclang() + major = libclang_major() + if major is not None and major < 19: + # Before 19 clang_getSpellingLocation() returns the file location, callee() needs the real one + print(f'error: libclang {major} is too old, the checker needs 19 or newer', file=sys.stderr) + return 2 + build_dir = os.path.realpath(args.build_dir) + files: list[str] = [] + for p in args.paths: + real = os.path.realpath(p) + if os.path.isdir(real): + found = glob.glob(os.path.join(glob.escape(real), '**', '*.c'), recursive=True) + files += [source_path(f) for f in sorted(found)] + elif os.path.isfile(real) and p.endswith('.c'): + files.append(source_path(p)) + else: + problem = 'not a .c file or a directory' if os.path.exists(real) else 'no such file or directory' + print(f'{p}: error: {problem}', file=sys.stderr) + return 2 + files = list( + dict.fromkeys(f for f in files if not f.endswith('.bpf.c')) + ) # BPF programs are not ours to style, and libclang cannot parse them + if not files: + print('nothing to check', file=sys.stderr) + return 2 + root = os.path.realpath(args.root) if args.root else source_root(files[0]) + if root is None: + print(f'cannot tell which source tree {files[0]} belongs to, use --root', file=sys.stderr) + return 2 + try: + commands = compile_commands(build_dir, set(files)) + except (OSError, ValueError, KeyError) as e: + print(f'error: cannot read {build_dir}/compile_commands.json: {e!r}', file=sys.stderr) + return 2 + only = None + if args.diff is not None: + dirs = [os.path.realpath(p) for p in args.paths if os.path.isdir(p)] + twins = [h for h in map(twin_header, files) if h and not any(h.startswith(d + os.sep) for d in dirs)] + try: + only = changed_lines(root, args.diff, [os.path.realpath(p) for p in args.paths] + twins) + except GitError as e: + print(f'error: {e}', file=sys.stderr) + return 2 + files = diff_selection(files, only, commands) + resources = resource_dir() + n_errors = 0 + checked = [] + items = [] + for f in files: + if f in commands: + checked.append(f) + items += [Item(f, root, build_dir, c, args.system, only, resources) for c in commands[f]] + elif args.diff is not None: + print( + f'{os.path.relpath(f, root)}: not in {build_dir}/compile_commands.json, skipping', + file=sys.stderr, + ) + else: + print( + f'{os.path.relpath(f, root)}: error: not in {build_dir}/compile_commands.json', + file=sys.stderr, + ) + n_errors += 1 + + # A header is checked from every file that includes it, so collect first and act on each argument once. + # A finding only counts as fixable when a translation unit that parsed cleanly produced it. + findings: dict[tuple[str, int], tuple[Finding, bool]] = {} + warnings: set[str] = set() + if items: + items.sort(key=lambda item: -os.path.getsize(item.path)) # the big ones first keep every worker busy + broken: Optional[BaseException] = None + try: + with ProcessPoolExecutor( + max(1, min(args.jobs, len(items))), initializer=setup_worker, initargs=(library,) + ) as pool: + try: + for future in [pool.submit(worker, item) for item in items]: + try: + result = future.result() + except BrokenExecutor as e: + broken = e + continue + for error in result.errors: + print(f'{os.path.relpath(result.path, root)}: error: {error}', file=sys.stderr) + n_errors += len(result.errors) + warnings.update(result.warnings) + for finding in result.findings: + key = (finding.file, finding.offset) + if key not in findings or (findings[key][1] and not result.errors): + findings[key] = (finding, bool(result.errors)) + except KeyboardInterrupt: + # Wait here, the with's shutdown resets cancel_futures before the executor acts on it + pool.shutdown(cancel_futures=True) + raise + except BrokenExecutor as e: + broken = e + if broken is not None: + # A worker that dies, libclang crashing on a file say, ends the run instead of hanging it. + print(f'error: {broken}', file=sys.stderr) + n_errors += 1 + + for w in sorted(warnings): + print(f'warning: {w}', file=sys.stderr) + + fixable = [x for x, dirty in findings.values() if x.fix is not None and not dirty] + if args.fix: + fix_files([(x.file, x.fix) for x in fixable if x.fix is not None]) + left = [x for x, dirty in findings.values() if x.fix is None or dirty] + else: + left = [x for x, _ in findings.values()] + for x in sorted(left, key=lambda x: (x.file, x.offset)): + prefix = f"{os.path.relpath(x.file, root)}:{x.line}:{x.column}: {x.callee}(): argument '{x.param}'" + if x.comment is None: + print(f'{prefix} passed as bare {x.literal}, comment it as /* {x.param}= */ {x.literal}') + elif x.fix is not None: + print(f"{prefix} is commented as '{x.comment}', comment it as /* {x.param}= */ {x.literal}") + else: + print(f"{prefix} is commented as '{x.comment}'") + + print( + (f'fixed {len(fixable)} argument(s), ' if args.fix else '') + + f'{len(left)} argument(s) without a matching comment in {len(checked)} file(s)' + + (f', {n_errors} error(s)' if n_errors else ''), + file=sys.stderr, + ) + return 2 if n_errors else 1 if left else 0 + + +if __name__ == '__main__': + sys.exit(main())