Repository navigation
build: make cppcheck a real gate instead of a no-op - #123
Merged
Merged
Conversation
The cppcheck target could not fail and was not reporting what it looked like it was reporting: - the command ended in `|| true` and had no --error-exitcode, so the target succeeded no matter what. Run as-is it exits 0 on 531 findings, one of them an error. - --checkers-report was mistaken for the findings report. It writes the inventory of which checks ran; the findings go to stderr, which nothing captured. cppcheck-report.txt never held a single finding, including the "There were critical errors (syntaxError)" notice at the top of it. - the target passed src/ with no -D or -I, so cppcheck guessed the preprocessor configuration instead of analysing the code that ships, in a tree where 15 files branch on #ifdef PSP. - no --platform, so a 32-bit target was analysed with host (64-bit) type sizes. - --enable=all --inconclusive made 62% of the output structural noise: 96 unusedFunction plus 119 staticFunction findings, neither of which means anything for a library whose public API is by definition unused internally. - the suppressions only listed lib/ paths while the target scanned src/, so none of them could ever match. - it ran nowhere in CI. run-cppcheck.sh replaces it, built like run-uncrustify.sh: analysis driven by compile_commands.json (which carries -DPSP and the include paths), --platform=mips32, --enable=warning,performance,portability, --check-level=exhaustive, --error-exitcode=1, plus a --report flag that writes the actual findings and a file argument to narrow the run. Because the compilation database covers lib/ as well as src/, the third-party suppressions are load-bearing for the first time. src/vfpu.c gains a documented syntaxError suppression: cppcheck cannot parse GCC's explicit-register syntax and aborts the file, which would otherwise mask every real finding in it. The CMake target now fails the build on any finding, with cppcheck-report for the report, and a static-analysis job runs it in CI against a pinned cppcheck. Verified: the gate exits 1 on an injected null dereference and 0 once it is reverted, the file filter reports a defect only when the run includes the file holding it, and both CMake targets work.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44b6ffc70e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
An unwritable or missing report destination can fail silently instead of producing the requested report.
Review effort: Lite
Findings: None
What changed in this PR
Replaces the ineffective Cppcheck integration with a PSP-targeted static-analysis gate, reporting, and CI enforcement.
Changes:
- Adds compile-database-driven
run-cppcheck.shwith filtering and suppressions. - Updates CMake targets to enforce Cppcheck findings.
- Adds pinned Cppcheck CI execution and documented suppressions.
| File | Description |
|---|---|
run-cppcheck.sh |
Implements enforced analysis and reporting. |
CMakeLists.txt |
Adds functional Cppcheck targets. |
.github/workflows/Build.yml |
Adds pinned static-analysis CI. |
.cppcheck-suppress |
Documents third-party and parser-specific suppressions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… report Two problems the reviewers raised on #123. The compilation database path was hard-coded to build/, so a target created by `cmake -S . -B out` still analysed build/: a different, possibly stale database from the configuration it was invoked from, or one the script would helpfully configure from the preset. The script takes --build-dir now and both CMake targets pass ${CMAKE_BINARY_DIR}, so the analysis always matches the build it came from. CMAKE_EXPORT_COMPILE_COMMANDS is set in CMakeLists.txt rather than only in the preset, so every configuration produces a database; when a build directory has none, the script says so instead of silently falling back, except for the default build/ which it can still populate from the preset. Writing the report was `[ -n "$report" ] && cp ... && echo ...`. A cp failure in the middle of an AND-OR list is exempt from set -e, so an unwritable path printed cp's error and then carried on to "Cppcheck found no problems" and exit 0, telling the caller a report existed when it did not. It is an explicit if now, and a failed write exits 1. Verified: with the same defect injected into src/vram_mgr.c and that file removed from one database, the target in that build directory exits 0 while the one whose database still lists it exits 1, so each target really does read its own; --report on an unwritable path now exits 1, and on a writable one still exits 0 having written the file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f04857f46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
find_program() resolves cppcheck and gates target creation on it, but the
targets then invoked run-cppcheck.sh, which performed its own PATH lookup for a
bare `cppcheck`. The resolved executable was discarded, so configuring with
-DCPPCHECK=/path/to/cppcheck, or finding it through a PATH that is not in
effect at build time, either failed with exit 127 or -- worse -- silently
analysed with a different cppcheck than the one the project was configured
with, which is precisely what pinning a version is meant to prevent.
The script takes --cppcheck now, also honouring a CPPCHECK environment
variable, and both CMake targets pass ${CPPCHECK}. The existence check moved
after argument parsing so it reports the binary that will actually run.
Verified with a stub binary: configured with -DCPPCHECK pointing at it, the
target runs the stub rather than the one on PATH; the same holds through the
environment variable; a missing binary exits 127 naming the path; and the
default still resolves from PATH. The gate itself was re-checked in both
directions afterwards.
Reported by the Codex reviewer on #123.
The static-analysis job prepended the pinned build to PATH and then invoked run-cppcheck.sh bare, leaving the choice of binary to PATH precedence. That is the same fragility the previous commit removed from the CMake targets: it happens to work because the prepend wins, but a system cppcheck appearing on the runner would silently change which version gates the branch. The job names the binary and passes it through --cppcheck.
dogo
added a commit
that referenced
this pull request
Sep 24, 2026
… report Two problems the reviewers raised on #123. The compilation database path was hard-coded to build/, so a target created by `cmake -S . -B out` still analysed build/: a different, possibly stale database from the configuration it was invoked from, or one the script would helpfully configure from the preset. The script takes --build-dir now and both CMake targets pass ${CMAKE_BINARY_DIR}, so the analysis always matches the build it came from. CMAKE_EXPORT_COMPILE_COMMANDS is set in CMakeLists.txt rather than only in the preset, so every configuration produces a database; when a build directory has none, the script says so instead of silently falling back, except for the default build/ which it can still populate from the preset. Writing the report was `[ -n "$report" ] && cp ... && echo ...`. A cp failure in the middle of an AND-OR list is exempt from set -e, so an unwritable path printed cp's error and then carried on to "Cppcheck found no problems" and exit 0, telling the caller a report existed when it did not. It is an explicit if now, and a failed write exits 1. Verified: with the same defect injected into src/vram_mgr.c and that file removed from one database, the target in that build directory exits 0 while the one whose database still lists it exits 1, so each target really does read its own; --report on an unwritable path now exits 1, and on a writable one still exits 0 having written the file.
dogo
added a commit
that referenced
this pull request
Sep 24, 2026
find_program() resolves cppcheck and gates target creation on it, but the
targets then invoked run-cppcheck.sh, which performed its own PATH lookup for a
bare `cppcheck`. The resolved executable was discarded, so configuring with
-DCPPCHECK=/path/to/cppcheck, or finding it through a PATH that is not in
effect at build time, either failed with exit 127 or -- worse -- silently
analysed with a different cppcheck than the one the project was configured
with, which is precisely what pinning a version is meant to prevent.
The script takes --cppcheck now, also honouring a CPPCHECK environment
variable, and both CMake targets pass ${CPPCHECK}. The existence check moved
after argument parsing so it reports the binary that will actually run.
Verified with a stub binary: configured with -DCPPCHECK pointing at it, the
target runs the stub rather than the one on PATH; the same holds through the
environment variable; a missing binary exits 127 naming the path; and the
default still resolves from PATH. The gate itself was re-checked in both
directions afterwards.
Reported by the Codex reviewer on #123.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #122, which cleared the findings. This one makes sure they stay cleared.
The target could not fail
No
--error-exitcodeeither. Run as-is onmasterbefore #122 it exited 0 on 531 findings, one of which was anerror.The report was not the report
--checkers-reportwas taken for the findings output. It writes the inventory of which checks ran:The findings go to stderr, which nothing captured. So
cppcheck-report.txtnever contained a single finding — including the notice sitting at the top of it:That notice was about
src/vfpu.c, which has not been analysed at all. Nobody saw it, because the file people were reading was the wrong one.It was analysing code that does not ship
The target passed
src/with no-Dand no-I, in a tree where 15 files branch on#ifdef PSP. Without-DPSPcppcheck guesses the preprocessor configuration and reasons about the emulator branches instead of the PSP build. And with no--platform, a 32-bit target was analysed with host 64-bit type sizes.This is also why the bugs fixed in #122 went unnoticed for so long: with the flags corrected, they show up immediately.
62% of the output was structural noise
--enable=all --inconclusiveproduced 531 findings, of which 328 were structural: 96unusedFunctionand 119staticFunction. Neither means anything for a library — the public API is by definition unused internally. The real findings were buried under them.The suppressions could never match
.cppcheck-suppresslisted onlylib/paths while the target scannedsrc/only. Same dead-config shape as.uncrustifyignorein #121.And it ran nowhere in CI.
What replaces it
run-cppcheck.sh, built likerun-uncrustify.sh:Driven by
compile_commands.json, so-DPSPand the include paths are honoured;--platform=mips32;--enable=warning,performance,portability(noall, no--inconclusive);--check-level=exhaustive;--inline-suppr;--error-exitcode=1.Exhaustive costs 13s against 1.8s for normal and removes the
normalCheckLevelMaxBranchesgive-ups, where cppcheck silently stops analysing branch-heavy functions. Worth it at this size.The third-party suppressions become load-bearing for the first time: the compilation database covers
lib/(53 of its 102 entries) as well assrc/, so without them the vendored code would dominate the output.src/vfpu.cgets a documentedsyntaxErrorsuppression. Cppcheck cannot parse GCC's explicit-register syntax:It aborts the file, which would otherwise mask every real finding in it. It is a parser limitation, not a defect, and the suppression says so and says when to drop it.
CMake and CI
cppchecknow fails the build on any finding;cppcheck-reportwrites the findings. Astatic-analysisjob runs it in CI with cppcheck pinned to 2.22.0 — the exact version every check here was run against — built from the tag and cached, matching how #121 pins Uncrustify.The job spells out the PSPDEV setup arguments rather than reusing the
buildjob's YAML anchor: that step is matrix-driven, and passing no arguments falls back tolinux/x86_64, requestingpspdev-linux-x86_64.tar.gz— which is not among the release assets (pspdev-ubuntu-latest-x86_64.tar.gzis).Verification
A gate that only knows how to pass is the bug being fixed here, so it was tested in both directions:
vram_mgr.cerror: Null pointer dereference: p [nullPointer]messagebox.cvram_mgr.cmake cppcheck/make cppcheck-report