ci: S24.11 extend analyze-tidy + analyze-iwyu to platform code#460
Conversation
cpputest now ships include-what-you-use 0.23 (clang_19), matching the cpputest-clang build. cpputest-freertos and its cross sibling inherit IWYU by chaining FROM the rebased base, enabling the freertos-aware analyze-iwyu lane that subsequent commits on this branch add. - ghcr.io/davidcozens/cpputest: sha-18f19e1 -> sha-2c7b76b - ghcr.io/davidcozens/cpputest-freertos: sha-24de138 -> sha-a0c1c0a - ghcr.io/davidcozens/cpputest-freertos-cross: sha-24de138 -> sha-a0c1c0a
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (45)
📝 WalkthroughWalkthroughThis PR introduces FreeRTOS-aware analyzer lanes to the CI infrastructure by updating container images, adding two new analysis jobs, reorganizing header includes across platform and test files to resolve compilation issues, configuring lint rules for test code and MbedTls, enhancing IWYU filtering for FreeRTOS headers, introducing a MISRA suppression renumbering tool, and providing tiered local pre-PR check documentation. ChangesFreeRTOS analyzer lanes infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
…plustcp
Mirrors the existing analyze-tidy / analyze-iwyu lanes but runs on
cpputest-freertos so the Platform/{FreeRtos,PlusTcp,LwipRaw,MbedTls,
FatFs} trees and their Tests/* siblings enter the CMake configure.
The base lanes on cpputest silently skip those trees because
FREERTOS_KERNEL_PATH / MBEDTLS_DIR / LWIP_PATH / FATFS_PATH are not set.
The freertos iwyu lane overrides CMAKE_C_COMPILER explicitly to
clang-19 — cpputest-freertos ships the clang-19 binary but no `clang`
alternative (only cpputest-clang creates that alias). Could be cleaned
up later by adding the alternative to the cpputest gcc base.
Findings these lanes surface are addressed in subsequent commits on
this branch.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
The new analyze-iwyu-freertos-plustcp lane compiles mbedTLS / lwIP / FreeRTOS-Plus-TCP / FatFs INTERFACE-library TUs from /opt/* in the cpputest-freertos image, so include-what-you-use surfaces ~500 findings on those third-party trees. They are not actionable here — we don't own those includes — and they drown out the ~37 in-tree files where the report does want changes. Extends scripts/iwyu_filter.py with an EXTERNAL_PATH_PREFIXES tuple (currently /opt/) that drops whole file blocks before per-block filtering. The cpputest-clang IWYU lane is unaffected: it never sees /opt/* paths because that image doesn't carry the upstream sources.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Addresses the 11 findings analyze-tidy-freertos-plustcp surfaced on Tests/FreeRtos/CmsdkUartFake.c. Two distinct rule categories, two distinct treatments per discussion with David: * 10 × cppcoreguidelines-macro-to-enum / modernize-macro-to-enum on the CMSDK UART register-offset and status-bit defines — wrapped in NOLINTBEGIN/END with a rationale comment. The #define form mirrors the ARM datasheet so a future reader can grep `DATA_OFFSET` against vendor docs. Kept the rule active at tier level so real test code that should use enums still gets caught. * 1 × bugprone-easily-swappable-parameters on Fake_Write32 — disabled tier-wide in Tests/.clang-tidy. The rule fires routinely on legitimate patterns in test helpers (uintptr_t address + uint32_t value, pairs of comparison values) where the types carry semantic distinction the rule can't see. Real argument-order bugs in tests would be caught by the tests themselves.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Extends scripts/iwyu_filter.py with three new filter rules for FreeRTOS ecosystem conventions that IWYU doesn't model: * Never remove FreeRTOS.h or FreeRTOS_IP.h. Both are umbrella headers that set up the build environment (configMACRO_X from FreeRTOSConfig.h) before any sub-header is parsed. IWYU sees specific types in task.h / portmacro.h / FreeRTOS_DNS_Globals.h and decides the umbrella is redundant, but removing it breaks the upstream convention. * Never add FreeRTOSConfig.h. Direct inclusion from Platform/FreeRtos/ is a LayerGuard violation — the config is reached transitively via FreeRTOS.h. The freertos-aware analyze-iwyu lane surfaced this as the immediate post-apply LayerGuard failure on Platform/FreeRtos/Source/SolidSyslogFreeRtosSysUpTime.c. * Never add portmacro.h or FreeRTOS_DNS_Globals.h. Both are private to the FreeRTOS-Plus-TCP implementation; consumers reach BaseType_t and freertos_addrinfo via the umbrellas. Without these rules, iwyu-apply against the freertos image either breaks LayerGuard (FreeRTOSConfig.h add) or produces files that fail to parse (FreeRTOS_*.h sub-headers ahead of FreeRTOS.h). With them, the freertos-aware analyze-iwyu lane settles cleanly.
Drives include hygiene to clean in the previously-unanalysed trees that
analyze-iwyu-freertos-plustcp now sees. Generated via the iwyu-apply
target running with the FreeRTOS-umbrella filter rules in the previous
commit, then verified via:
cmake --preset debug -B build/iwyu-verify
cmake --build build/iwyu-verify --target junit BddTargetTests
inside cpputest-freertos:sha-a0c1c0a. All tests pass; analyze-iwyu lane
settles cleanly.
Touches 31 files across Platform/{FreeRtos,PlusTcp,LwipRaw}/Source/,
Tests/{FreeRtos,Lwip,MbedTls,MbedTlsIntegration}/, and Tests/Support/
fakes. The dominant pattern is adding stdint / stdbool / standard-C
headers that were transitively pulled in before, plus moving
"#include "X.h"" up to the natural sort position relative to system
headers.
No source-level behaviour change: the binary set produced is bit-for-
bit identical except for line-number debug info shifts.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
…mbers When a refactor or IWYU pass shifts source lines, the line-anchored entries in misra_suppressions.txt go stale and the cppcheck-misra gate fails on findings it has always passed — just at a new line. The clerical fix-up (re-edit the suppressions file by hand) has been a recurring tax on pre-PR time. scripts/misra_renumber.py automates it. Two passes: 1. Audit against unsuppressed findings (cppcheck --addon=misra without the suppressions list). For each (rule, file) pair where the suppression entries match the findings in count but not in lines, remap ascending-by-ascending — handles bulk refactors that touch many sites in one file. 2. Residual failures with suppressions in place. cppcheck dedupes per location, so when 11.2 and 11.3 both fire at the same line and only 11.2 is reported, pass 1 can't see 11.3. Pass 2 runs cppcheck WITH the suppressions list, picks up the residual failures, and updates single-entry (rule, file) suppressions. Conservative on ambiguity — bails on (rule, file) pairs whose suppression count and finding count diverge, so a real new finding or deleted suppression doesn't get silently mis-mapped. This commit also applies the tool's output to misra_suppressions.txt for the three shifts the IWYU apply in the previous commits introduced: misra-c2012-11.2:.../SolidSyslogPlusTcpAddress.c :9 -> :10 misra-c2012-11.3:.../SolidSyslogPlusTcpAddress.c :9 -> :10 misra-c2012-11.3:.../SolidSyslogPlusTcpDatagram.c :47 -> :49 cppcheck-misra now passes locally with the updated suppressions list.
Mirrors the suppression already in place on the host-side fake (Tests/FreeRtos/CmsdkUartFake.c) — same NOLINTBEGIN/END bracket, same rationale: register-offset, control-bit, and status-bit macros that mirror the ARM CMSDK UART hardware datasheet keep their #define form so a future reader can grep against vendor docs. Surfaced by the new analyze-tidy-freertos-plustcp lane on the cross- compiled BDD target build. Audit of inline cppcheck-suppress / NOLINT comments tier-wide is scheduled as a separate story under E10 (not S24.11).
The unconditional first iteration of the CAS loop assigns `previous` via InterlockedCompareExchange before reading it, so the initial 0 is never used. Moves the declaration into the loop body, which both satisfies cppcheck's unreadVariable rule and limits the variable's scope to where it's actually live. Pre-existing finding sitting silently in the cppcheck-report.xml artifact across many prior CI runs — S24.11 promoted reading the whole cppcheck output to a habit, which surfaced it.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
The SolidSyslogMbedTlsStreamConfig API takes integrator-supplied handles of upstream Mbed TLS types — `mbedtls_ctr_drbg_context`, `mbedtls_x509_crt`, `mbedtls_pk_context` — see project_mbedtls_di_handles. The Interface/ header forward-declares those structs to keep public consumers free of mbedTLS includes, which trips the project's StructCase + StructPrefix rules (snake_case `mbedtls_*` is neither CamelCase nor SolidSyslog-prefixed). Renaming is impossible — they're third-party. Adds Platform/MbedTls/.clang-tidy with a StructIgnoredRegexp override that re-states the root whitelist plus the mbedtls_ namespace. The config applies to both Interface/ and Source/ via clang-tidy's walk-up-the-tree resolution. Surfaced by analyze-tidy-freertos-plustcp; verified locally with `cmake --preset tidy && cmake --build`. clang-tidy CheckOption values do not merge across the hierarchy (latest-encountered wins), so the platform-level file restates the root pattern verbatim. Re-stating beats relying on regex-OR with the root, which would be ambiguous to read at the override site.
Codifies the agreement reached at the start of S24.11: full CI suite takes 10-15 minutes; running every lane locally before pushing burns human time for confidence we mostly already have from the previous push's CI result. Split into: - Tier A (per-commit, ~30-60 s): debug build + tests for matching preset - Tier B (pre-push, ~3-5 min, scoped): IWYU + format reflow + misra_renumber - CI (everything else): tidy, sanitize, coverage, Windows, BDD, integration Tier B runs only when production source changed — tests-only, docs-only or CMake-only diffs push immediately and let CI run. The path-gating is the engineer's judgement; the doc spells out the rule. The doc also captures the two non-obvious mechanics from this session: - `analyze-iwyu-freertos-plustcp` lane needs CMAKE_C_COMPILER=clang-19 override (the gcc base image carries clang-19 with no `clang` alias) - IWYU output should be read whole, not head/tail — easy to miss findings that sit in the middle of a long report Updates CLAUDE.md "Before raising a PR" section to point at the new doc rather than recite the (now-stale) preset list.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Closes #459. Extends the CMake-driven analyzer jobs (
analyze-tidy,analyze-iwyu) to cover the FreeRTOS / PlusTcp / MbedTls / FatFs / LwipRaw platform trees and their tests, which the base-image lanes silently skipped becauseFREERTOS_KERNEL_PATH/MBEDTLS_DIR/LWIP_PATH/FATFS_PATHaren't set incpputest:sha-*.analyze-cppcheck(and the cppcheck-misra step) already covered those trees via direct invocation — no change needed there.Also brings forward
scripts/misra_renumber.py(originally a closing deliverable) when the IWYU apply line-shifted some MISRA suppressions, anddocs/local-checks.mdcodifying the tiered pre-PR check budget agreed at session start.Change Description
CI lanes (the literal #459 scope)
analyze-tidy-freertos-plustcpandanalyze-iwyu-freertos-plustcponcpputest-freertos:sha-a0c1c0a— env vars are set, so Platform/{FreeRtos,PlusTcp,LwipRaw,MbedTls,FatFs} and their Tests/* siblings enter the CMake configure.-DCMAKE_C_COMPILER=clang-19explicitly: thecpputestgcc base shipsclang-19but noclangalternative. A future image bump can drop the override.Image bumps (cascade through the chain)
ghcr.io/davidcozens/cpputest:sha-18f19e1→sha-2c7b76b— addsinclude-what-you-use 0.23built from theclang_19upstream branch (same recipecpputest-clangalready used) so all three images ship the same IWYU binary.ghcr.io/davidcozens/cpputest-freertos:sha-24de138→sha-a0c1c0a— inherits IWYU automatically viaFROM cpputest.ghcr.io/davidcozens/cpputest-freertos-cross:sha-24de138→sha-a0c1c0a— inherits via chain.Image work landed via
CppUTestDocker#2andCppUTestFreertosDocker#4.IWYU filter hardening (
scripts/iwyu_filter.py)Three new filter rules so the freertos-aware lane settles cleanly:
/opt/*— third-party trees IWYU shouldn't be suggesting include changes for.FreeRTOS.handFreeRTOS_IP.h— both are umbrella headers that must be included first to set up the build environment fromFreeRTOSConfig.hbefore any FreeRTOS sub-header is parsed. IWYU sees specific types intask.h/portmacro.h/FreeRTOS_DNS_Globals.hand decides the umbrella is redundant, but removing it breaks the upstream convention.FreeRTOSConfig.h(LayerGuard violation),portmacro.h(private FreeRTOS header), andFreeRTOS_DNS_Globals.h(internal_Globalssuffix) — all are reached transitively through the umbrellas.IWYU applied to 31 project-tree files
After the filter rules, ran
cmake --build --preset iwyu --target iwyu-applyagainst the freertos image. Edits across Platform/{FreeRtos,PlusTcp,LwipRaw}/Source/, Tests/{FreeRtos,Lwip,MbedTls,MbedTlsIntegration}/, and Tests/Support/ fakes. Verified via a fullcmake --preset debug && cmake --build --target junit BddTargetTestsinsidecpputest-freertos:sha-a0c1c0a— all tests pass.Tidy treatment
Tests/.clang-tidy— adds tier-wide-bugprone-easily-swappable-parameters(rule fires too often for legitimate MMIO-helper / equality-assertion patterns; real arg-swap bugs in tests get caught by the test itself).Tests/FreeRtos/CmsdkUartFake.c— NOLINTBEGIN/END around the CMSDK register-offset and status-bit macros (cppcoreguidelines-macro-to-enum,modernize-macro-to-enum); the#defineform mirrors the ARM datasheet by design.Bdd/Targets/FreeRtos/Common/CmsdkUart.c— same NOLINTBEGIN/END pattern for the production driver counterpart.Platform/MbedTls/.clang-tidy(new) — extendsStructIgnoredRegexpto accept upstreammbedtls_[a-z0-9_]+struct names in forward declarations. Renaming is impossible (they're third-party). clang-tidy CheckOption values don't merge across the hierarchy, so the override file restates the root whitelist verbatim plus the mbedTLS namespace.David's preference for tier-wide over inline surfaced mid-session; the inline NOLINTs in CmsdkUartFake.c stay as the consistent local pattern. A full audit of inline
cppcheck-suppress/NOLINT*comments is queued under #461 — S24.12.MISRA tooling:
scripts/misra_renumber.pyTwo-pass script that automates the clerical "renumber stale suppression lines after a refactor / IWYU shift":
Conservative on ambiguity — bails on (rule, file) pairs whose suppression count and finding count diverge.
Used to fix the three line-shifts the IWYU apply introduced (
PlusTcpAddress.c:9→10for 11.2 and 11.3,PlusTcpDatagram.c:47→49for 11.3).Small fixes that came out of reading the cppcheck XML report whole
Platform/Windows/Source/SolidSyslogWindowsAtomicCounter.c— dropLONG previous = 0;initializer; the CAS loop unconditionally assigns it before any read. The finding had been sitting silently incppcheck-report.xmlacross many CI runs (that step doesn't carry--error-exitcode=1).Docs
docs/local-checks.md(new) — tiered pre-PR check budget: per-commitdebugbuild + tests (~30–60s); pre-push IWYU + format +misra_renumber.pyonly when production source changed (~3–5 min); CI owns everything else. Path-gating is the engineer's judgement.CLAUDE.md— "Before raising a PR" section now points at the doc rather than listing presets.DEVLOG.md— session entry capturing decisions, deferrals, open questions.Test Evidence
All seven analyzer / cppcheck lanes green on the final push:
analyze-tidy✓ (Core + base-image Platforms)analyze-tidy-freertos-plustcp✓ (new — FreeRTOS / PlusTcp / LwipRaw / MbedTls / FatFs)analyze-iwyu✓ (Core + base-image Platforms, cpputest-clang)analyze-iwyu-freertos-plustcp✓ (new — freertos image, postiwyu-apply)analyze-cppcheck✓ (incl. cppcheck-misra with renumbered suppressions)analyze-format✓Areas Affected
.github/workflows/ci.yml— 2 new jobs, image SHA bumps,summary.needsextended.devcontainer/docker-compose.yml,ci/docker-compose.bdd.yml,docs/containers.md— image SHA bumpsTests/.clang-tidy— tier-widebugprone-easily-swappable-parametersrelaxationPlatform/MbedTls/.clang-tidy(new) — mbedTLS struct-name exceptionTests/FreeRtos/CmsdkUartFake.c,Bdd/Targets/FreeRtos/Common/CmsdkUart.c— register-macro NOLINTBEGIN/ENDPlatform/Windows/Source/SolidSyslogWindowsAtomicCounter.c— drop dead initializer#includereorder / additions / removalsscripts/iwyu_filter.py— FreeRTOS-umbrella +/opt/*filter rulesscripts/misra_renumber.py(new)misra_suppressions.txt— three line renumbersdocs/local-checks.md(new),CLAUDE.md,DEVLOG.md🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores