dt-lockcheck: a tool to scan for locking issues (sharing data between threads)

[kofa73]

Claude, Codex and Gemini have been working on updating the dev-docs. During that work, as a side-effect, they identified a number of bugs, some of them I have filed, some of them not yet.

Today I read about https://github.com/JordyZomer/lemmalog, developed by a security researcher (see article). I asked Claude whether it would be useful to trace calls during our bug hunts. While it said lemmalog does bring some advantages (more about that below), it proposed to create a Python tool to deterministically and very quickly scan for locking issues. The result is the tool on this branch. It's not a 100% accurate magical oracle, but it does report findings quickly and fairly accurately (this was measured against how many of the already found bugs it reported, as well as quickly evaluating some of the new entries in its findings to see if they are real or false positives -- investigation was not very deep, but I'll work on that later, as it's quite promising).

One thing I'm considering is a permanently checked-in false-positive list, which would be invalidated when evidence (line numbers etc) change, or manually rescanned and re-evaluated every now and then). This is work in progress, I'd love to have your feedback.

I don't know whether tools is the right directory - please advise.

The following is a Claude-generated description.

======

tools/dt-lockcheck: a static check for gui_data thread safety

State. The script works, the current gui_data audit round is driven from
it, and nothing in the build depends on it: it reads src/iop/*.c and *.cc
and prints a report. What is still moving is the tuning, the checked-in
false-positive list, and how much of its output has actually been read by a
human. This section describes where all three stand today, on this branch.

The short version: 74 findings over 87 analysed modules, of which 52 name a
field that an already-filed report also names and 22 nobody has ruled on. Two
new upstream issues were filed off the back of the first review round, and that
round also overturned a defect that three independent readers had initially
written off.

What it is for

Each IOP module keeps its per-instance GUI state in a
dt_iop_<module>_gui_data_t, reached as g->field. The gui_* callbacks and
widget handlers run on the GTK main thread; process() and its neighbours run
on a pixelpipe worker. A field both sides touch has to be accessed inside
dt_iop_gui_enter/leave_critical_section(), or under a private mutex.

This is a recurring defect family and an awkward one to spot by review: the
write is in process(), the read is in a draw callback, and the two are three
call frames apart in a 4000-line file. Consequences run from a mis-drawn overlay
to a use-after-free. Finding them by hand is slow and almost entirely
mechanical — which is the argument for a tool.

How it works

Four stages, all lexical. There is no compiler front end and no dataflow
analysis; the tool matches source text and propagates labels.

1. Extraction. For each source: the gui_data struct and its field types,
   every g->field access with its line and its mode, every critical section,
   and the call graph within that one file.
2. Thread labelling. Every function gets gtk, pipe or either. The
   module API entry points are ground truth; static helpers inherit the label of
   their callers along the in-file call graph; callbacks are labelled from how
   their address is taken rather than from their name — passed to a callee
   is the registrar's thread, passed to g_idle_add/g_signal_connect is gtk,
   stored into a struct member or a file-scope table is either because it
   escapes the file. either is the interesting label: commit_params(),
   init_pipe(), distort_transform() and the two colorspace callbacks are all
   reachable from GTK-thread code, so the tool refuses to claim they are on a
   worker thread, and counts them on both sides of the cross-thread test.
3. Rules. Five of them, over those facts (below). A field is reported under
   at most one rule.
4. Suppression. The checked-in false-positive list is applied after the
   rules, so it changes what is reported and not what is derived.

A full run over src/iop takes about a second and a half. Python 3.8+,
standard library only, nothing to install.

What it reports today

dt-lockcheck: 97 sources in .../src/iop (darktable root above $PWD); 87 with a gui_data struct, 10 skipped
dt-lockcheck: false positives: 2 suppressed; from .../tools/dt-lockcheck/false-positives.json

rule	default	findings today	what it checks
widget_from_pipe	yes	4 (5 before suppression)	a Gtk*-typed field touched from a pipe or either function. GTK may only be called from the main thread, so the lock is beside the point
violation	yes	34	the module locks the field somewhere and accesses it unlocked from the other side. The module's own code is the evidence that the lock is needed
no_lock_share	yes	35 (36)	written and shared across both threads, and the module never locks it at all
pointer_share	yes	1	a pointer field shared across both threads with every access correctly locked. The section protects the pointer load, not the object the other thread may free under the reader
discipline_gap	no	59	locked somewhere, accessed unlocked, but no cross-thread share was proven. Mostly noise — this is why it is off by default

74 reported by default (76 before the false-positive list), 133 with every
rule (135 before). Each finding names the locked sites — the module's own
evidence that the field is meant to be protected — and the unlocked sites that
are the suspected defect, with line numbers and the thread of each function.

pointer_share is the newest rule and the odd one out: every input to it is a
correctly locked field, and what it says is that locking alone cannot fix the
shape. Its single hit tree-wide is colorreconstruction's frozen bilateral
grid, copied out under gui_lock, dereferenced after the release and freed by
the preview pipe — #22060. It is bounded by the shared-pointer population
(11 fields), which is what keeps it in the default set.

What the findings have been worth so far

Coverage. 52 of the 76 default-settings findings name a field that one of
the 23 reports of the ongoing gui_data audit already names (#21915-#21919,
#21974, #22005-#22009, #22057-#22064 and #22066-#22069). The other 24 had not
been ruled on either way.

First review round. 23 of the findings were then assessed independently and
blind by three agents (Claude, Codex, Gemini), and the disagreements debated.
21 verdicts agreed from the start; both disputes were resolved unanimously, and
one of them mattered:

• toneequal's pipe_order was called a false positive by two of the three
  rounds, on the grounds that both pipes write the same value and the guarded
  block is idempotent. It is neither. The comparison at toneequal.c:1036 sits
  outside the section it guards and is never re-tested inside it, so both pipes
  can enter; the preview pipe publishes luminance_valid = TRUE and the full
  pipe then re-runs the stale invalidation, leaving a valid mask marked invalid.
  Every GTK reader gated on that flag — histogram, cursor readout, mask
  display — goes silently inert until the preview pipe happens to run again.
  g->pipe_order is initialised to 0 and iop_order never is, so both pipes
  see the mismatch on the module's first darkroom run; no reordering is
  needed to reach it.

Two further issues came out of the same round, neither of them a locking
defect, both found while reading a finding: #22080 (the channelmixerrgb ΔE
buffer keeps the size of the previous colour checker — a heap out-of-bounds
read and write when switching between a 24- and a 48-patch checker) and
#22081 (a retouch OpenCL failure path strands the auto-levels handshake).
A third — the preview pipe freeing delta_E_label_text unlocked while the GTK
thread passes it to gtk_label_set_markup(), with reload_defaults() making
that pairing a double free — was folded into #22058, which needs a section
around the same call site.

One methodological result worth passing on. Three independent passes at
matching findings against already-filed reports gave three different answers
(55/19, 51/23, 47/27); reconciling them by hand gave 52/22. Every error was a
substring match — a field name inside a fenced code block in an unrelated
issue, or the English words "colour checker". Dedup errors of that kind are
silent: a wrongly-deduped finding is assessed by nobody and leaves no trace in
any report. Two of the three passes dropped what turned out to be the round's
second most serious defect that way. If you re-run such a comparison, do it per
finding, against the issue text only, and require the document to be about that
module's that field.

The checked-in false-positive list

tools/dt-lockcheck/false-positives.json records findings a human has read and
judged not to be defects; suppression is on by default. The risk in any such
list is that a suppression outlives the code it was true about, so every entry
carries a key, and a source change that could alter the judgement invalidates
it: the entry goes stale, the finding comes back, and the reason recorded for
it is printed alongside. An entry that matches no finding at all is named as an
orphan. The stderr banner always reports these counts, and -q does not
silence it — the list may not hide its own size.

The key is the finding's facts (field type, the thread of every function
that touches it, and the (function, lock, mode) of every access) plus the
verbatim text of every access line. Each half catches what the other misses,
and this was measured rather than guessed, over 6105 finding-transitions across
80 commits touching src/iop:

candidate key	fires	missed a rule change	missed a site change
whole-file hash	9.6%	0	0
facts only	0.16%	0	7
function:line sites	5.6%	0	2
facts + access-line text	0.28%	0	0

The file hash is not "overkill but safe": it is 34× noisier for no extra safety,
and noise is what makes people re-confirm an entry without reading it.

Nobody edits the file by hand:

./dt-lockcheck.py --confirm-false-positive rgblevels:params \
                  --false-positive-reason "why this is not a defect"

A reason is required for a new entry, because that sentence is what gets printed
back when the entry goes stale. A bare --confirm-false-positive rgblevels
re-confirms every stale entry in that module but deliberately cannot add
entries, so a whole module cannot be silenced with one word.

It holds two entries today — colorharmonizer's auto_detect (the pipe
side only takes a g_object_ref() and hands the widget to
gdk_threads_add_idle(), which is the correct idiom) and rgblevels' params
(ownership passes through a state machine whose transitions are taken under the
lock). Nine further findings were agreed to be false positives in the review
round and have not been recorded yet; that is the next thing to land in this
file, not a claim that the rest are defects.

CI and build integration

Two mutually exclusive gates, both off by default:

• --fail-on-findings — exit 1 when anything is reported. Red on this tree
  today (74), and it is meant to be: it passes only once every finding is
  either fixed or recorded as a false positive. Not something to wire into CI
  yet.
• --fail-on-stale-false-positives-only — exit 1 only when a recorded judgement
  has gone stale. Green today, and the one a job or a pre-commit hook can use
  now.

There is deliberately no new-findings gate. That would need a checked-in
baseline of all 74, which is a different artifact with different invalidation
needs, and it is not in this branch.

Limitations

• Findings are candidates, not bugs. The README lists every false-positive
  cause we know of and asks people to confirm one by reading the code before
  filing.
• It is a lexical analysis, not a race detector. No dataflow inside a
  function: a pointer copied out of gui_data under the lock and dereferenced
  after the release is invisible to four of the five rules, which is exactly
  what pointer_share exists to work around, and it does so by firing on the
  shape rather than by proving the escape.
• One file at a time. The call graph is propagated within a source file
  only. A defect whose two halves sit in src/iop and src/develop — such as
  exposure's effective_exposure in exposure disagrees with itself about the exposure it applies #21974 — is out of reach by
  construction.
• The rules were written while looking at known defects, so recall against
  those defects is a best case and not a prediction for a tree it has not been
  tuned on. Precision is unaffected: the findings beyond the known fields were
  not known to anyone when the tool produced them.
• No false-positive rate has been measured. The honest statement is that 52
  of 76 findings land on ground somebody already confirmed, 24 are unruled, and
  a first review round found the great majority of the ones it read to be real.
  discipline_gap is visibly worse than the other four, which is why it is not
  in the default set.
• A clean run is not an all-clear. It means these rules found nothing in
  these files.

Layout and usage

tools/dt-lockcheck/
  dt-lockcheck.py         the tool
  rules.lemma             the same rules as Datalog (optional)
  false-positives.json    judged-harmless findings, maintained through the tool
  README.md               usage, the thread model, the rules, and the caveats

./dt-lockcheck.py                       # anywhere in a checkout; finds src/iop itself
./dt-lockcheck.py --module toneequal
./dt-lockcheck.py --rules ALL           # including the noisy tier
./dt-lockcheck.py --format csv > findings.csv
./dt-lockcheck.py --format json         # the extracted facts, before the rules

Every invocation mistake is an exit-2 error rather than a quiet no-op: an
unknown rule name, a --module that names no source or one with no gui_data
struct, --include-false-positives with a format the list never touches, a
malformed list file, both gates at once.

Optional: proof trees

rules.lemma expresses the same rules as Datalog, for
lemmalog. With --why, each finding
is followed by the proof behind it, bottoming out in the extracted facts — which
is how you see that a finding rests on a thread inference you disagree with. It
is off by default and needs nothing installed unless you ask for it. The two
implementations are checked to produce the same finding set, rule by rule and
field by field. --format lemmalog emits the facts alone, which is the way to
add a rule of your own — the audit bookkeeping above (which findings are already
filed, which modules the rules never mention) was written as Datalog queries
rather than as a script.

Related

There are pending changes to dev-doc/GUI_Threading.md covering the same
ground — which callbacks run on which thread, and the locking rules for
gui_data. This tool encodes that model directly, so the two should move
together: #21912

[kofa73]

@da-phil @jenshannoschwalm @ralfbrown @TurboGit do you think this is worth pursuing?

[kofa73]

Update: reach the callbacks, and the pointers a lock cannot protect

Two blind spots in the scanner, closed in two commits. The first was a reach
problem — a third of the functions touching gui_data had no thread label, so
whole defects were invisible. The second is a kind problem — one defect shape
has every access correctly locked and is a use-after-free anyway.

	before	after
unlabelled functions touching gui_data	33	0
findings (default rules)	72	76
findings lost	—	0
findings whose rule changed	—	0

---

1. Label the callbacks the naming conventions miss

dt-lockcheck inferred a thread for each function from its name plus the
intra-file call graph. Functions registered as callbacks fell outside both:
their names follow no convention, and nothing in the file calls them. They came
out unlabelled, counted on neither side of the cross-thread test, and so could
never make a field look shared — every defect reachable only through one was
invisible.

33 of them touched gui_data. After this change, 0 do.

The names say why it mattered:

exposure:  _exposure_proxy_get_effective_exposure, _exposure_proxy_handle_event
toneequal: _toneeq_preview_resized, auto_adjust_exposure_boost, auto_adjust_contrast_boost

Those sit behind issues #21974, #22008 and #22068 — already-confirmed defects
that a scan could not have found.

What changed

Callbacks are now labelled from how the address is taken, which is
observable, rather than from the name, which is not:

handover	label	why
passed to a callee as an argument	the registrar's thread	the callee calls it synchronously
passed to g_idle_add / g_signal_connect and friends	gtk	the main loop runs it, whoever registered it
stored in a struct member or file-scope table	either	it escapes the file; the call site is unknown

The first row is what recovers most of the 33. The previous code gated on a list
of registration spellings that did not include darktable's own helpers, so
dt_gui_connect_motion(g->area, area_motion_notify, area_enter_notify, area_leave_notify, self) registered three handlers and labelled none of them. A
bare identifier naming a file-local function in an argument list is a callback
— C decays a function designator to a pointer, and there is nothing else it can
be — so the gate is gone.

The third row is how the dev->proxy accessors get labelled. GUI_Threading.md
puts it plainly: the caller sits in another module and has no way to take your
gui_lock. either is the honest label, and not a free one — it counts on both
sides of the cross-thread test.

Three smaller fixes came out of the same work:

• an optional cast before a callback argument is accepted
  (gtk_tree_model_foreach(model, (GtkTreeModelForeachFunc)_list_match_string, g));
• file-scope initialiser tables are scanned, which is how dt_action_def_t
  registers shortcut handlers positionally;
• (^|_)action_process is labelled gtk, since shortcut dispatch runs on the
  main loop.

A latent bug this exposed

Propagation could overwrite a label that came from the API tables. An imprecise
call-graph edge into colorequal's process() demoted it from pipe to
either — and because either counts on both sides, that manufactures
sharing rather than losing it.

Labels from the PIPE/EITHER/GTK sets are ground truth: GUI_Threading.md
tabulates which callback runs where. They are now pinned, and propagation only
fills in or refines what was inferred.

Effect on findings

72 findings to 75, none lost, no rule reassigned. The three new ones are all
widget_from_pipe on exposure: _exposure_proxy_handle_event() calls
dt_bauhaus_slider_get() on g->black, g->exposure and
g->deflicker_target_level — all GtkWidget * — from a proxy accessor another
module invokes on a thread of its own. Two of the three fields are named by
#21974.

---

2. pointer_share: the lock that is not enough

Every rule here asks whether an access was locked. That misses the shape of
#22060, a confirmed use-after-free, where every access is locked:

can = g->can;                                    // :634  under gui_lock
dt_iop_gui_leave_critical_section(self);
b = dt_iop_colorreconstruct_bilateral_thaw(can); // :641  lock released
...
dt_iop_colorreconstruct_bilateral_dump(g->can);  // :659  preview pipe frees it

The critical section protects the pointer load, not the object. GUI_Threading.md
states the rule: "a scalar hands over cleanly; an allocation does not."

Proving the escape needs dataflow inside a function, which is out of reach. The
type is not. Field declarations now keep their * — it binds to the declarator,
so in float *buf, count; only buf is a pointer — and pointer_share fires on
a pointer-typed field shared across the two threads with no unlocked access
anywhere. It is last in the cascade, so it claims only fields no other rule did.

It finds nothing new, and that is the point

One finding tree-wide, and it is #22060 — a defect already filed. gui_data
holds 11 non-widget shared pointer fields and the other three default rules
already report 10 of them. This is a safeguard, not a source of candidates, and
the PR should be read that way.

What it guards is the fix path. Lock every access to zonesystem's
in_preview_buffer, colormapping's buffer, ashift's buf, exposure's
deflicker_histogram, channelmixerrgb's checker or toneequal's
full_preview_buf — the obvious fix, and the wrong one for a pointer — and every
other rule goes silent on a field that is still use-after-free shaped. Verified by
fact injection: all six go silent under the previous rule set, all six stay
flagged now.

Why it is a default rule

It proves less than the other three: a shape, not an escape. A module that fixes
a field correctly — copying the data, holding the lock through the last use, or
transferring ownership — would still trip it, and the rule cannot tell. That
argues for the discipline_gap treatment.

What settles it the other way is that the rule is bounded. Its output cannot
exceed the shared-pointer population: 11 fields on a 91-module tree, and 10 even
in the worst case where every open finding in this tool is "fixed" the lock-only
way. A ten-item ceiling is a readable list, not a noise tier — discipline_gap
returns 59. widget_from_pipe is already default while carrying a permanent
documented false positive, so gating the more precise rule was the inconsistency.
If the count ever stops being a readable list, move it out.

A rule that was tried and rejected

The obvious companion — a rule for gui_data shared between two pipe threads,
which GUI_Threading.md documents under Passing Values Between Pipes Through
gui_data — was built as far as validation and dropped. Recorded so it is not
re-proposed:

• The whole population is the four modules calling dt_dev_sync_pixelpipe_hash()
  — colorreconstruction, hazeremoval, levels, globaltonemap. grep -l
  enumerates them faster than any rule.
• All four already lock every pipe-side access to payload and hash, so the
  confirmed defect (colorreconstruction) and the clean example (levels) are
  indistinguishable in the fact base. No lock-presence rule can separate them.
• The candidate discriminator fails. &self->gui_lock passed into a callee looks
  like a handover marker, but five modules have one and the fifth is toneequal's
  hash_set_get() — the full pipe memoizing against a hash it wrote itself, which
  the dev-doc explicitly calls not a handover. It alone contributed 8 of the
  candidate rule's 15 fields, all already reported.
• The complement is empty: fields touched on pipe and never on gtk number
  2 tree-wide, one of them a documented false positive.

Hand-audited outcome for the four: hazeremoval, levels and globaltonemap
are clean — every pipe-side access under gui_lock, the primitive always called
with the lock released, payloads scalar. colorreconstruction is the defect, and
pointer_share is what reaches it.

Effect on findings

75 findings to 76, none lost, no rule reassigned; the addition is
colorreconstruction.can. The fact base is otherwise unchanged: accesses, thread
labels and field names are identical in every module, and the only difference is
882 field types gaining a * across 34 distinct pointer types.

---

Verification

• Output is byte-identical across runs.
• The default run's first 75 findings are unchanged in module, field, rule and
  every site.
• The Python and the Datalog in rules.lemma derive the same findings under all
  five rules — 5 / 34 / 36 / 59 / 1.

Documentation

The module docstring and README.md gain the address-taken table, the new rule
with its caveat, the ptr_type predicate, refreshed statistics (52 of the 76
default-rule findings name a field an existing report already names), and the
limits below. docs/agents/gui-data-lock-hunt.md records the rejected
pipe-to-pipe rule with its measurements, so the next round does not re-derive it.

A limit written down, not fixed

Widening the file glob would not help, which is worth recording so it is not
re-proposed. Outside src/iop, a *_gui_data_t struct exists in exactly four
sources — imageio/format/{jxl,jpeg,webp}.c and imageio/storage/piwigo.c — and
all four are correct by construction: write_image() takes a params snapshot and
never reaches gui_data. src/libs uses a different convention entirely and has
no IOP callbacks to key thread inference off. The one file worth adding by name
is src/develop/preview_data.c, the only participant in the same lock discipline
outside src/iop, and it needs a different extractor because it has no
gui_data struct.

Not addressed

Intra-function dataflow — "pointer copied under the lock, then dereferenced after
releasing it" — is still out of reach. pointer_share reaches the shape that
permits it, not the escape itself, which is why every one of its hits has to be
read before it is believed.

[da-phil]

I just skimmed over the posts to get a rough understanding, but need to re-read for full understanding. But what I see is a essentially a linter / static-code analyis tool which detects one category of bugs in our "darktable bug ontology". Is that something we should include in our CI to catch newly introduced issues before they even get merged, or is the tool not robust enough (low FP rate) for that?

Another - more general - question: would you see any benefit in running our unit-tests in a build configuration which contains the thread sanitizer, which will more broadly catch multi-threading issues (dynamic analysis)? I'm asking because I'm working on including a bunch of sensible sanitizers into our build config, so that you could easily just start a build with them and run a test-suite to check for potential issue.

[kofa73]

Yes, it's a cheap heuristic, not a match for actually running stuff. While the bots worked on updating the GUI dev-doc (PR #21912), they found a bunch of issues, but spent a lot of tokens on the analysis; at one point, Claude Code came up with this script, which is intended to be a quick pre-filter to find suspicious points, bug candidates. It has blind spots as well as false positives; the 2nd of which can be mitigated using the bundled false-positives.json.

They have now finished going through the reported candidates and filed a 2 new bugs, updated 2 previously existing reports, and added a bunch of false positive exclusions. In theory, with the --fail-on-stale-false-positives-only flag, it could now be added to CI: it will only exit with RC=1 if one of the filed false positives becomes invalidated, requiring analysis, and either a fix, or re-confirming the false positive. (It still reports all findings, though.)

I'm not familiar with darktable's pipeline and with C multithreading / the thread sanitizer. I'm a Java guy. If you think it is useful, I'm all for it, it sounds like a useful addition.

[kofa73]

Are there unit tests that are exercising UI interactions? I know there are integration tests that export images and compare them with a reference/baseline, but those are unlikely to help with UI threading issues.

[da-phil]

I'm not familiar with darktable's pipeline and with C multithreading / the thread sanitizer. I'm a Java guy. If you think it is useful, I'm all for it, it sounds like a useful addition.

I think it's going to be very useful, just had a quick run using address & undefined behaviour sanitizers on a couple of integration tests and they already found a ton of issues, didn't even use the thread sanitizer yet...
Will post a PR in the next few days.

Are there unit tests that are exercising UI interactions

Nope, not that I'm aware but of, unfortunately, this would actually also benefit the sanitizer coverage, as GUI issues are completely out of scope for them.
Seems that we need to brainstorm how we can actually establish a new group of integration tests which utilise the GUI. I know there are a couple of test frameworks people use for GUI testing, that make GUI interactions and assertions quite easy.

[wpferguson]

Are there unit tests that are exercising UI interactions

Working on it. I can't do mouse interactions but I can exercise controls, presets, styles, etc.

[kofa73]

False positives filed for all rules and rebased on master.

$ ./tools/dt-lockcheck/dt-lockcheck.py --rules ALL --fail-on-stale-false-positives-only
dt-lockcheck: 97 sources in /home/kofa/dt-claude/workspace/dt-master/src/iop (darktable root above $PWD); 87 with a gui_data struct, 10 skipped
dt-lockcheck: false positives: 73 suppressed; from /home/kofa/dt-claude/workspace/dt-master/tools/dt-lockcheck/false-positives.json

0 stale of 62 findings; the rest are not what this gate asks about.

The 73 suppressed findings (field-level) are the those we have issues about, they will go away as issues get fixed.

[da-phil]

Can you re-use regex CALL_RE here?

[kofa73]

Not directly (only the identifier part matches, it's followed by different characters, but Claude did some cleanup:

• dead code removed (FUNC_HEAD, gui_ty, an unused index); constant regexes hoisted out of the per-line loops that were rebuilding them (CALLBACK_ARG_RE, OTHER_G, STATIC_REF_RE, LOCK_ARG); the hand-rolled _write_cache replaced with functools.lru_cache; repeated idioms collapsed into one named thing each -- fp_ident() for the (module, field, rule) triple that appeared at eight sites, _module_of() for the path->module rule, a local join() for thethread-lattice update written out twice, and reuse of CALL_RE, _as_iop_dir() and next(...) where the logic had been spelled out a second time.
• --format lemmalog deduped: it deduped over a line number the access(...) fact does not print, so the same assertion was emitted once per source line; deduping over what is actually emitted cuts the output ~37%

[da-phil]

I'm going to introduce a folder tools/sanitizer which also contains SW quality tooling. I wonder whether we should create a common folder for SW quality tools like dt-lockcheck and the sanitizers? Maybe tools/quality or tools/sw_quality?

It's funny that after long analysis sessions of sanitizer output files, I saw patterns and started writing deterministic static code analysis / linter scripts too, which basically just parse the instrumented build tree to find easy to spot issues almost effortless, without even running the instrumented code, which can be at a 20x runtime penality, as for the thread sanitizer.

Anyway, good job with this tool, it looks very promising!
I'm really looking forward to bring some of those tools into our CI eventually and proactively prevent bugs from even entering the code base instead of retroactively triaging and fixing them.

I also wanted to ask you if you have a special skill / instruction for your LLM models to create those nicely structured github issues? I'm planning to do the same for the sanitizer findings.

[kofa73]

At first, I was against tools/sw_quality ("Of course it's software..."), but it could be misunderstood as measurements of "image quality" (whatever that is), so I think tools/sw_quality is fine.

Emerging patterns: most static analysis is about patterns.

dt-lockcheck seems to have run its course for now; I think I have a few more chains to verify and possibly report, then it can transition into a build quality gate tool.

As for the issue style: I think I told them once or twice to structure the output and avoid walls of text. Since then, they are just copying the existing style (I have a copy of these reports in a local directory, and when I tell Claude to add a new report there, it checks the existing ones - it does that anyway, as I also tell it to check if the issue it's about to report is already covered by something, or should be added to an existing report, rather than raising a new one).

I'm considering to tell it to use mermaid diagrams e.g. for threading issues, instead of textual descriptions. I'll have to see if that makes the reports more readable or just "fancier".

[kofa73]

Added experimental alternative reports with diagrams to #22064, #22133