tsan: keep each access's line in the finding key

CHANGELOG.md

@@ -4,6 +4,13 @@
 
 ### Fixed
 
+- **Two races in one file under the same name are two findings.** TSan reports were
+  deduplicated by rule, file and top symbols, without the line, so two different races
+  whose inlined frames share a name (two `Drop` impls, both `drop`) became one finding
+  and one of them was counted as a repeat of the other. The key now carries each
+  access's line; the same race reported again, from either side, is still one finding.
+  Findings are also no longer merged across classifications (a harness race with a race
+  outside the extension), nor at line 1 when their primary location has no line.
 - **A mutator refilling another library's buffer is a harness race.** A mutator writing
   into a numpy array (`arr[:] = ...`) reaches the memory through numpy's copy loop, not
   CPython's, and was filed as the extension's `certain` race, failing the run. A copy

docs/ci.md

@@ -54,6 +54,8 @@ $ docker run --rm --security-opt seccomp=unconfined \
    module only with the GIL forced off.
 5. **Collect.** TSan logs are parsed, each report is attributed, and repeats of the same
    race — from either side of the pair, from any threads — collapse into one finding.
+   A repeat is the same rule, the same top symbols and the same source line for each
+   access; two races in one file under the same inlined name (two `drop`s) stay two.
 6. **Report.** Terminal summary, `--format json`, `--sarif`, `--junit`. The lint and the
    sanitizer write one SARIF format; only the rule id (`FT00x` vs `tsan/...`) and the
    `producer` property tell them apart.
@@ -154,7 +156,9 @@ Each report is placed by looking at where each conflicting access actually happe
 Two refinements: when the *other* access is an allocation into
 reused memory (`new_dict`, `PyList_New`, `_PyFreeList_Pop`, …), your code touched an object
 after it was freed — **yours**, even though the frames look like CPython's. And reports
-from many stacks that share one rule and one primary line are merged into one finding.
+from many stacks that share one rule and one primary line are merged into one finding,
+within one classification (a harness race never absorbs another). A primary location
+with no line is not merged this way.
 
 **Crashes are findings.** A `SEGV` (or other fatal signal) with your code on the stack,
 or with no stack captured at all, fails the run: a process that died while being driven

fixtures/racy/race-in-dependency-callbacks/expected.toml

@@ -1,4 +1,4 @@
 race = true
 rules = ["FT001"]
-symbols = ["count_dropped", "deferred", "count_retired"]
+symbols = ["drop", "deferred"]
 justification = "Unsynchronised read-modify-writes of shared counters in code the extension hands to a dependency: a message's Drop run by oneshot, a closure deferred to crossbeam-epoch's collector, and a thread-local destructor run at thread exit. The stacks pass through the same dependencies whose own reports ftcheck suppresses, so this pins that the suppressions stop at the dependency's code. FT001 sees only the closure, which is written inside the #[pyfunction]."

fixtures/racy/race-in-dependency-callbacks/src/lib.rs

@@ -22,19 +22,14 @@ static mut RETIRED: u64 = 0;
 /// Dropped by `oneshot` itself when the receiver goes away first.
 struct Receipt([u64; 32]);
 
+// Both `Drop` impls in this file are symbolised as `drop`: only their lines
+// tell the two races apart.
 impl Drop for Receipt {
     fn drop(&mut self) {
-        count_dropped(self.0[0]);
+        unsafe { DROPPED += self.0[0] };
     }
 }
 
-// The racy accesses are in functions of their own, so each race is reported
-// under its own name rather than as another `drop`.
-#[inline(never)]
-fn count_dropped(n: u64) {
-    unsafe { DROPPED += n };
-}
-
 #[pyfunction]
 fn undelivered(n: usize) -> usize {
     for _ in 0..n {
@@ -59,15 +54,10 @@ struct Retire;
 
 impl Drop for Retire {
     fn drop(&mut self) {
-        count_retired();
+        unsafe { RETIRED += 1 };
     }
 }
 
-#[inline(never)]
-fn count_retired() {
-    unsafe { RETIRED += 1 };
-}
-
 thread_local! {
     static ON_EXIT: Retire = const { Retire };
 }

fixtures/racy/two-drops-one-file/Cargo.toml

@@ -0,0 +1,12 @@
+[package]
+name = "two-drops-one-file"
+version = "0.0.0"
+edition = "2021"
+publish = false
+
+[lib]
+name = "two_drops_one_file"
+crate-type = ["cdylib", "rlib"]
+
+[dependencies]
+pyo3 = { version = "0.29", features = ["extension-module"] }

fixtures/racy/two-drops-one-file/expected.toml

@@ -0,0 +1,4 @@
+race = true
+rules = []
+symbols = ["drop"]
+justification = "Two `Drop` impls in one file each do an unsynchronised read-modify-write of a shared counter. Inlined, both accesses are symbolised as `drop` in `src/lib.rs`, so the file and the symbol cannot tell them apart: ThreadSanitizer must report two findings, one at each line (17 and 25). A deduplication key without the line merged them and hid one. The lint sees neither: the accesses are in `Drop` impls, not in a #[pyfunction]."

fixtures/racy/two-drops-one-file/src/lib.rs

@@ -0,0 +1,49 @@
+//! Racy: two different races in one file under one inlined name.
+//!
+//! Each `Drop` impl below does an unsynchronised read-modify-write of its own
+//! counter, shared by every thread. Inlined, both are symbolised as `drop` in
+//! this file, so only their lines tell the two races apart: both must be
+//! reported, each at its own line.
+
+use pyo3::prelude::*;
+
+static mut ADMITTED: u64 = 0;
+static mut RELEASED: u64 = 0;
+
+struct Admit(u64);
+
+impl Drop for Admit {
+    fn drop(&mut self) {
+        unsafe { ADMITTED += self.0 };
+    }
+}
+
+struct Release(u64);
+
+impl Drop for Release {
+    fn drop(&mut self) {
+        unsafe { RELEASED += self.0 };
+    }
+}
+
+#[pyfunction]
+fn admit(n: usize) -> usize {
+    for _ in 0..n {
+        drop(Admit(1));
+    }
+    n
+}
+
+#[pyfunction]
+fn release(n: usize) -> usize {
+    for _ in 0..n {
+        drop(Release(1));
+    }
+    n
+}
+
+#[pymodule]
+fn two_drops_one_file(m: &Bound<'_, PyModule>) -> PyResult<()> {
+    m.add_function(wrap_pyfunction!(admit, m)?)?;
+    m.add_function(wrap_pyfunction!(release, m)?)
+}

fixtures/racy/two-drops-one-file/tests/test_threads.py

@@ -0,0 +1,8 @@
+"""Run by `ftcheck ci` under pytest-run-parallel: this body runs in N threads at once."""
+import two_drops_one_file as m
+
+
+def test_admit_and_release_from_many_threads():
+    for _ in range(200):
+        m.admit(5)
+        m.release(5)

python/ftcheck/ci/tsan.py

@@ -478,6 +478,31 @@ def _primary(report: Report, extension_modules: set[str], crate_root: str) -> tu
     return first, first.file or first.module or "<unknown>"
 
 
+def _anchor(section: Section, shown: list[Frame], placed_in_extension: bool,
+            extension_modules: set[str], crate_root: str) -> str:
+    """Where one access happened, as part of the deduplication key.
+
+    The frame chosen is the one `_primary` would choose from this stack alone
+    (or, for a race placed where it happened, the stack's top), so the
+    finding's primary location is one of its anchors. Inlined frames
+    are named by their function alone — two `Drop` impls in one file are both
+    `drop` — so the line is what tells two races apart. A frame without one
+    (missing, or 0 for compiler-generated code) falls back to its file, which
+    is the key as it was before lines were part of it.
+    """
+    frame = shown[0] if shown else None
+    if placed_in_extension:
+        in_crate = [f for f in section.frames if f.file and _relative(f.file, crate_root) is not None]
+        beyond_ffi = [f for f in in_crate if "/ffi/" not in f.file]  # type: ignore[operator]
+        in_module = [f for f in section.frames if f.module in extension_modules]
+        located = [f for f in in_module if f.file]
+        frame = next(iter(beyond_ffi or in_crate or located or in_module), frame)
+    if frame is None:
+        return ""
+    where = frame.file or frame.module or ""
+    return f"{where}:{frame.line}" if frame.line else where
+
+
 def to_findings(
     reports: list[Report], extension_modules: set[str], crate_root: str
 ) -> tuple[list[dict], list[dict], list[dict]]:
@@ -488,7 +513,9 @@ def to_findings(
 
     The same race reported from either side of the pair is one problem: stacks
     are ordered by their top frame before the signature is taken, so "write
-    races previous read" and "read races previous write" collapse.
+    races previous read" and "read races previous write" collapse. The
+    signature is the rule, the top symbols and each access's line (see
+    `_anchor`): the symbols alone cannot tell two inlined `drop`s apart.
 
     A report of yours is shown from your first frame; the others are shown from
     where the access actually happened, so a race inside CPython reads as one.
@@ -531,11 +558,16 @@ def to_findings(
             frame, path = Frame("<no stack captured>", None), "<no stack captured>"
         rule = _rule(report.kind)
         tops = " / ".join(dict.fromkeys(s[1][0].symbol for s in stacks if s[1]))
-        signature = f"{rule}|{path}|{tops}"
+        # Sorted, so the same race reported from either side is one key.
+        anchors = sorted(
+            _anchor(section, frames, owner in (YOURS, HARNESS), extension_modules, crate_root)
+            for section, frames in stacks
+        )
+        signature = f"{rule}|{tops}|{' / '.join(anchors)}" if stacks else f"{rule}|{path}"
 
         bucket = buckets[owner]
         if signature in bucket:
-            bucket[signature]["occurrences"] += 1
+            bucket[signature][1]["occurrences"] += 1
             continue
 
         # Addresses differ on every run; a message that changes when nothing
@@ -558,7 +590,11 @@ def to_findings(
             HARNESS: "A mutator rewrote a buffer's contents while your extension read it — "
             "a race in the harness by the buffer protocol's contract, not in your code. ",
         }[owner]
-        bucket[signature] = {
+        # Findings merge on their primary line (below); a primary without a
+        # line would sit at line 1 and merge with anything else in the file,
+        # so it merges only on its own signature.
+        merge_key = (rule, path, frame.line) if frame.line else (rule, signature)
+        bucket[signature] = merge_key, {
             "rule": rule,
             "message": f"{prefix}ThreadSanitizer: {report.kind}. {described}".rstrip(),
             "confidence": "certain" if owner == YOURS else "likely",
@@ -581,17 +617,18 @@ def to_findings(
         }
     # One root cause reported from many stacks is one finding: merge findings
     # sharing a rule and a primary line (one bug otherwise shows up as many).
+    # Never across classifications: a harness race and a race outside the
+    # extension at the same line stay two findings.
     return (
         _merge(buckets[YOURS].values()),
         _merge(buckets[REACHED].values()),
-        _merge(list(buckets[EXTERNAL].values()) + list(buckets[HARNESS].values())),
+        _merge(buckets[EXTERNAL].values()) + _merge(buckets[HARNESS].values()),
     )
 
 
-def _merge(findings) -> list[dict]:
+def _merge(keyed) -> list[dict]:
     merged: dict[tuple, dict] = {}
-    for f in findings:
-        key = (f["rule"], f["primary"]["file"], f["primary"]["line"])
+    for key, f in keyed:
         if key in merged:
             merged[key]["occurrences"] += f["occurrences"]
         else: