Skip to content

Commit a69587a

Browse files
committed
Document Coverity Scan triage and pin Cython for stable scans
Backport of IntelPython/mkl_umath#266, adapted to mkl_fft. - Pin cython==3.3.0 in the Coverity workflow (only there, not in pyproject.toml) so the generated _pydfti.c stays byte-stable between scans and Coverity CIDs plus their triage survive. Works because the scan build uses --no-build-isolation. - Add coverity/README.md: where findings come from across mkl_fft's two translation units (template-generated mklfft.c, which is our DFTI logic and stays in scope, and Cython-generated _pydfti.c), the Cython-pin rationale, an opt-in Project Component, a review checklist, and the verified false-positive families for the mkl_fft project. The known false positives are grouped by checker + mechanism from the current scan: Cython-generated boilerplate (tp_traverse DEADCODE, version/ABI-guarded helper DEADCODE, a reference-cleanup UNUSED_VALUE, a CHECKED_RETURN) and two dead-by-construction families in our own .pyx (redundant is-NULL guards Cython already proves, and an intentional in-place placeholder stub) - all triaged Intentional / Ignore. The genuine __create_descriptor_1d UNUSED_VALUE defects were fixed in #365, not suppressed, so mklfft.c stays in scope. mkl_umath's own documented false positives were not ported blind; the table was rebuilt from mkl_fft's own scan and generated code.
1 parent d87b5c5 commit a69587a

3 files changed

Lines changed: 116 additions & 1 deletion

File tree

‎.github/workflows/coverity.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,9 @@ jobs:
3232
architecture: x64
3333

3434
- name: Install mkl_fft dependencies
35-
run: pip install meson-python ninja cmake cython "numpy>=2" mkl-devel
35+
# Cython is pinned here only (not in pyproject.toml) to keep the generated
36+
# code stable between scans, so Coverity CIDs and their triage survive
37+
run: pip install meson-python ninja cmake "cython==3.3.0" "numpy>=2" mkl-devel
3638

3739
- name: Download Coverity Build Tool
3840
timeout-minutes: 15

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111
### Changed
1212
* Multi-iterator constructors now return a status corresponding to allocation success or failure, and raise `MemoryError` instead of `ValueError` [gh-373](https://github.com/IntelPython/mkl_fft/pull/373)
1313
* `_direct_fftnd` now also checks the status returned by the backend instead of discarding it [gh-373](https://github.com/IntelPython/mkl_fft/pull/373)
14+
* Pinned Cython in the Coverity Scan workflow so generated code stays stable between scans, and added `coverity/README.md` documenting the known false-positive families and the scan review checklist [gh-374](https://github.com/IntelPython/mkl_fft/pull/374)
1415

1516
### Fixed
1617
* Declared `f_ndim` as a C `int` in `_allocate_result` so the buffer size is computed in C rather than through a Python object, resolving a Coverity out-of-bounds (OVERRUN) false positive [gh-364](https://github.com/IntelPython/mkl_fft/pull/364)

‎coverity/README.md‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
# Triaging Coverity Scan findings
2+
3+
Static analysis runs on [Coverity Scan](https://scan.coverity.com) via
4+
[`.github/workflows/coverity.yml`](../.github/workflows/coverity.yml) (weekly + on
5+
demand). Analysis runs on Black Duck's servers; triage is done in the Scan web UI.
6+
7+
Almost every finding to date is a false positive — either in Cython-generated
8+
boilerplate in `_pydfti.c`, or in a handful of branches in our own `.pyx` that are
9+
dead-by-construction (Cython already guarantees the guard, or the branch is an
10+
intentional placeholder). The one family of *genuine* defects — `UNUSED_VALUE` in
11+
the `__create_descriptor_1d` reallocate path in `mklfft.c.src` — was fixed in
12+
[gh-365](https://github.com/IntelPython/mkl_fft/pull/365). This guide records the
13+
verified findings and how to keep triage from resetting.
14+
15+
## Where findings come from
16+
17+
`cov-build` captures two C translation units (the `.h` files under
18+
`mkl_fft/src/` are `#include`d into them, not compiled on their own):
19+
20+
- **Template-generated** `mklfft.c` (from `mkl_fft/src/mklfft.c.src` via
21+
`_vendored/process_src_template.py`, wired up in `meson.build`). This is *not*
22+
Cython — it is our oneMKL DFTI descriptor/compute logic, just type-specialized
23+
for float32/float64/complex64/complex128. It pulls in the hand-written helpers
24+
in `mkl_fft/src/multi_iter.h` and `mkl_fft/src/mklfft.h`. A real bug can surface
25+
here, so treat it like hand-written code, **not** boilerplate — review every
26+
finding. The only genuine defects found so far lived here (the
27+
`__create_descriptor_1d` `UNUSED_VALUE`s, fixed in
28+
[gh-365](https://github.com/IntelPython/mkl_fft/pull/365)).
29+
- **Cython-generated** `_pydfti.c` (from `mkl_fft/_pydfti.pyx`): `__Pyx_*` /
30+
`__pyx_pw_*` / `__pyx_tp_*` helpers and wrappers are boilerplate — findings here
31+
are ~always false positives. `__pyx_pf_*` functions are the C translation of our
32+
`.pyx` bodies; a real `.pyx` bug could surface there, so keep them in scope
33+
(Coverity can't see Python-level invariants — see the `_allocate_result`
34+
OVERRUN fixed in [gh-364](https://github.com/IntelPython/mkl_fft/pull/364)).
35+
36+
## Keeping triage durable: the Cython pin
37+
38+
A Cython *version* bump regenerates `_pydfti.c` wholesale, which churns the
39+
Coverity CIDs and silently drops their triage — the same boilerplate then returns
40+
under new CIDs. So **Cython is pinned in `coverity.yml`** (not `pyproject.toml`,
41+
so shipped wheels are unaffected). The pin works only because the build runs with
42+
`--no-build-isolation`; bumping it means re-triaging the boilerplate.
43+
44+
## Reducing the noise: a Project Component
45+
46+
**Project Settings → Components** buckets defects by a path regex. Define one to
47+
group (not hide) the Cython unit so it can be filtered out of view — path-based,
48+
so it survives regeneration:
49+
50+
- **Name:** `Generated-code` **Path regex:** `.*_pydfti\.c`
51+
52+
Do **not** group `mklfft.c` (or the `mkl_fft/src/*.h` helpers) — that is our DFTI
53+
logic, not boilerplate. Group only — do **not** mark it *ignored*, as that also
54+
drops the `__pyx_pf_*` bodies (see [declined](#evaluated-and-declined)).
55+
56+
## Review checklist
57+
58+
Don't blanket-ignore the generated file — prioritise instead:
59+
60+
1. **Findings in `mklfft.c` and `mkl_fft/src/*.h`** — review every one; this is
61+
our type-specialized oneMKL DFTI code and its inlined iterator/cache helpers,
62+
not boilerplate.
63+
2. **High/Medium findings in `__pyx_pf_*`** — verify against `_pydfti.pyx`; if it's
64+
a Python-level invariant Coverity can't see, mark `False Positive` with a
65+
reason. If it's a genuine defect, fix the `.pyx` (as with the `_allocate_result`
66+
OVERRUN).
67+
3. **Known false-positive families below** — carry the recorded disposition;
68+
match on **checker + mechanism**, not CID (CIDs reset on a Cython bump or an
69+
engine upgrade).
70+
71+
## Known false-positive families
72+
73+
Match on **checker + mechanism**, not CID: the CIDs in parentheses are a current
74+
snapshot (Coverity Scan, Cython 3.3.0) and reset on a Cython bump or engine
75+
upgrade. Helper names are from Cython 3.3.0 and vary between versions. All Minor
76+
severity, no runtime or security impact. Every family below is triaged
77+
**Intentional / Ignore**.
78+
79+
### Cython-generated boilerplate (in `_pydfti.c` `__Pyx_*` / `__pyx_tp_*` helpers)
80+
81+
| Family | Checker | Why it's a false positive |
82+
| --- | --- | --- |
83+
| `tp_traverse` slots — `__Pyx_Coroutine_traverse`, `__Pyx_CyFunction_traverse`, and the genexpr/closure scope traversals for `_genexpr` and `_get_element_strides` (652767, 652770, 652768, 652769 — one mechanism) | DEADCODE | Cython emits a uniform base-type traversal preamble `e = __Pyx_call_type_traverse(o, 1, v, a); if (e) return e;`. The traversed object derives from a base whose traverse contributes nothing, so the helper returns 0 and the early-return is dead. The preamble *is* needed for objects deriving from a GC type. |
84+
| `__Pyx_VectorcallBuilder_AddArg`, `__Pyx_PyCode_New`, `__Pyx_ParseKeywordDict` (652764, 652766, 652771) | DEADCODE | Dead branches in version/ABI-guarded runtime helpers — arms selected out by the CPython version the wheel is built against, or by an argument shape the call site never produces. |
85+
| `_c2r_fft1d_impl` reference-cleanup epilogue (652759) | UNUSED_VALUE | Cython's generated `finally`/decref epilogue assigns a temp (e.g. resets a borrowed slot to `NULL`) that is never read again before the function returns. |
86+
| `__Pyx_Generator_Replace_StopIteration` (652760) | CHECKED_RETURN | Boilerplate deliberately drops the return value of a call whose result is not needed on that path. |
87+
88+
### Our `.pyx`, dead-by-construction (in `__pyx_pf_*` bodies)
89+
90+
| Family | Checker | Why it's a false positive |
91+
| --- | --- | --- |
92+
| Redundant `is NULL` guard after an array-returning call — `_process_arguments` and `_direct_fftnd` (652772, 652761) | DEADCODE | The `.pyx` has an explicit `if <array> is NULL:` guard, but because the variable is typed `cnp.ndarray`, Cython already emits its own NULL/error check right after the returning call (e.g. `PyArray_CheckFromAny`). By the time our guard runs the value is provably non-NULL, so our guard body is dead. Harmless defensive code — kept for readability. |
93+
| `_c2r_fft1d_impl` in-place stub (652765) | DEADCODE | `in_place = 0` is assigned immediately before `if in_place:`, so the `# TODO: Provide in-place functionality` branch is provably unreachable. Intentional placeholder for future in-place `irfft` support. |
94+
95+
## Genuine defects (fixed, not suppressed)
96+
97+
Not every finding was a false positive. The `UNUSED_VALUE` reports in the
98+
`__create_descriptor_1d` reallocate path of `mklfft.c.src` (652763, 652773, 652774,
99+
652775) were real — a status value overwritten before it could be checked — and
100+
were **fixed** in [gh-365](https://github.com/IntelPython/mkl_fft/pull/365), not
101+
triaged away. This is why `mklfft.c` stays in review scope (see the checklist).
102+
103+
## Evaluated and declined
104+
105+
- **Modeling files** correct the behavior of *called* functions; our FPs are
106+
intraprocedural (dead branches, compile-time-constant guards, `#if`), which
107+
models can't reach.
108+
- **Dropping the generated unit** (hard exclude), e.g. after `cov-build`:
109+
```bash
110+
cov-manage-emit --dir cov-int --tu-pattern "file('.*_pydfti\\.c')" delete
111+
```
112+
Also drops the `__pyx_pf_*` bodies, so it's disabled in favour of the checklist.

0 commit comments

Comments
 (0)