Add a ThreadSanitizer config to enforce the main-thread macro-name copy #2
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
bcox/tymbal#2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The plugin keeps a main-thread-owned copy of the macro names,
macro_names_,specifically so that
paramsInfonever reads them out of the liveinstrument_— which the audio thread may be swapping under it via thepending_ready_handover.That discipline is currently held by review and a comment, not by the
build. Nothing in the suite can observe it: a single-threaded test loads,
installs and then reads, so reading straight out of
instrument_would passtoo.
plugin/host_smoke_test.ccsays so in a comment rather than pretendingotherwise:
A ThreadSanitizer configuration would convert it into a property the build
enforces.
Shape of the test
Activate the plugin, run
process()on a second thread in a loop, and from themain thread repeatedly call
preset_load->from_location()followed byparams->get_info(). Assert TSan reports no race.The current design is expected to be clean under it. The check that it has
teeth: change
paramsInfoto readplayground::MacroName(instrument_.parameters(), index)and TSan should flagthe read against
AdoptPatch's write. If it does not, the test is notexercising what it claims to.
What this does not cover
The
pending_single-writer guard inInstallPatchBytesis a differentproperty and TSan would likely not catch it without a dedicated stress
test — the window is one audio block. That guard has its own deterministic,
single-threaded test
(
RefusesASecondPresetWhileAHandoverIsStillInFlight), which is sufficient forthe guard's behaviour even though it says nothing about the race itself.
Cost
This needs a
--config=tsanin.bazelrcand a way to keep TSan off thedefault lane — it is slow, and it needs a test that spawns a thread, which no
existing test does. Reasonable to defer until CI exists, since a sanitizer lane
nobody runs is worth nothing.
Depends on the CI issue.
This is a comment.
Landed on main:
c82aef1,28395b8,977fb6e.--config=tsanis in.bazelrc, scoped the same way asan is and for the same rules_cc_autoconf reason.//plugin:preset_race_testrunsprocess()on a second thread with a voice sounding while the main thread hands over presets and reads back.//plugin:patch_format_testis taggedno-tsan— 1611s under TSan against 449s under asan, single-threaded throughout, so there is nothing there to find; the tag is a claim about threads, not speed. The lane also carries--test_timeout=180,600,1800,3600, because//dsp:fm_pair_layer_testis an honestsmallthat intermittently ran past 60s under instrumentation.The injection this issue specified works: with
paramsInforeadingMacroName(instrument_.parameters(), index), TSan reports it againstAdoptPatch's write, 20 runs in 20.Two real defects turned up while building it, both fixed here.
paramsValuereadinstrument_.parameters().macro[id]— the same arrayAdoptPatchoverwrites andWriteParameterstores into on every automation event insideprocess(). That is the racemacro_names_exists to avoid, one field over.macro_names_was not the right shape to copy: a name has one writer so a main-thread copy suffices, a position has two, somacro_values_is an array of relaxed atomics and that is what the host is told about.stateSavehanded the whole liveinstrument_toWritePatch. TSan withhalt_on_error=0enumerated thirty-odd distinct races across layer count, layer kinds, the FM pair, the mod matrix, the LFOs, the frame curves, macros, macro names and trim. It now picks an instrument rather than a field, usingpending_ready_— which can only go true→false whilestateSaveruns, since both it and the only store oftrueare [main-thread].The lane is the belt, not the braces. Both defects were also wrong answers on one thread, in the window between a preset load the host was told succeeded and the block that installs it. Three deterministic tests in
host_smoke_test.ccpin that window and fail in every lane; they are the gate. TSan covers what they cannot — a race that leaves the right answer behind, which an aligned 8-byte load always does. Injected detection rates are recorded inpreset_race_test.cc's header: 20/20 for state, 19–20/20 for names, 14/20 for values. That 14 is why it is not the gate.What the issue anticipated held up. The
pending_single-writer guard is still not covered here and still has its deterministic test; and the note that a sanitizer lane nobody runs is worth nothing is still true — #1 stays open for the CI trigger.