Repository navigation
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jserv
added a commit
that referenced
this pull request
Aug 6, 2026
Sleeping did not guarantee that B completed both read-modify-writes before A's compare-exchange, and the exit status added earlier in this series turned that scheduling variance into a nonzero exit, so an oversubscribed CI runner could fail the job for no reason. Let B signal completion and have A wait on it. The interleaving is now pinned by the two flags rather than by a timeout, the compare-exchange cannot lose its race, and the four lines the manuscript quotes come out on every run. Dropping the one-second sleep also takes 300 runs from five minutes to under two seconds. Reported by cubic-dev-ai on #28.
jserv
added a commit
that referenced
this pull request
Aug 6, 2026
`uname -m` reports the machine running make, so a cross build from x86-64 to anything else still forced -mcx16 onto the target command line, and the libatomic probe answered for the host toolchain rather than the one actually in use. Ask the compiler for __x86_64__ instead, and run the probe with the same flags the ABA build uses. Reported by cubic-dev-ai on #28.
jserv
added a commit
that referenced
this pull request
Aug 6, 2026
A glob drops a deleted file from the prerequisites, so removing an example the document still inputs would leave an existing PDF looking up to date and make would never run latexmk to report the broken \inputminted. Name the three files the manuscript embeds. Reported by cubic-dev-ai on #28.
jserv
added a commit
that referenced
this pull request
Aug 6, 2026
The comment said clearing `initialized` invites the caller to destroy the pool, but the same branch sets head to NULL and tpool_destroy dereferences head->prev unconditionally, so taking that invitation would crash. init already undoes everything it did; say that instead. Reported by cubic-dev-ai on #28.
jserv
added a commit
that referenced
this pull request
Aug 6, 2026
Rewriting this listing earlier in the series turned its job-pointer load into a relaxed one; it had been a sequentially consistent atomic_load. The loop follows what it loads, reading job->prev to compute the next candidate and dereferencing the job once the claim succeeds, and those fields reach the worker only through the plain stores the employer made before publishing the pointer. A release on the successful compare-exchange orders the claiming thread's later writes, not its earlier reads, so nothing synchronized with the publisher and the listing described a data race. Make the load and the failure order acquires, and say why this loop is not the case where every load can be relaxed. Reported by cubic-dev-ai on #28.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jserv
added a commit
that referenced
this pull request
Aug 6, 2026
The previous wording said the fields the loop follows were published by the thread that added the job. That holds only for a queue of one. add_job relinks jobs that are already queued, writing head->job.next->prev with an ordinary store, while head->prev is published atomically only when the queue was empty. So a worker can acquire head->prev and then read a job->prev that a later insertion wrote with nothing to synchronize against. The acquire is still necessary, since a release on the claiming CAS orders that thread's later writes rather than its earlier reads. It is just not sufficient, and the example continues to rely on jobs being added only while the workers are idle. Say that rather than implying the ordering is complete. Reported by cubic-dev-ai on #28.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A failed compare_exchange writes the current value back into the expected operand, so the reloaded pointer can be the idle sentinel once another worker drains the queue. That sentinel is allocated with plain malloc and its func and future fields are never initialized, so claiming it dereferences indeterminate pointers. Under a sanitizer the previous code aborts in roughly 2 runs out of 30 with a heap-use-after-free. Rule the sentinel out on every iteration rather than once before the loop, and update the two listings in the manuscript that quote it. The versioned variant carries the same defect and is repaired separately, because there it cannot be separated from how the queue head is published.
Two defects that cannot be separated in this file. The worker had the same sentinel bug as its unversioned twin: a failed compare-exchange reloads the expected operand, so the reloaded pointer can be the idle job, whose func and future are never initialized. And add_job wrote the pointer and its version as separate stores, even though the union exists so the pair can be swung with one double-width compare-and-swap. A worker compare-exchanging between the two observes the new job against the stale version, which is the state the version number exists to make unrepresentable, and the increment was itself a plain read-modify-write on a location workers update concurrently. Fixing either alone makes the example worse rather than better, which is why they land together: the new claim loop against the old split publication hangs in roughly 2 runs out of 40, and the old claim loop against the new atomic publication fails outright. Together they are clean over the same sample. The store remains unsynchronized, relying on the employer only adding jobs while the pool is idle, so the comment says that rather than claiming nothing races it.
tpool_destroy reports on a pool it cancels while it is still running, and main reached it as soon as the last future completed, which does not mean the last worker has published idle. The window is narrow enough that the pool examples do not print the diagnostic today, but it widens as soon as the claim loop is touched, and later commits gate CI on this output matching what the manuscript quotes. wait_until is already there for the same purpose earlier in main.
Workers pop from the back and free as they go, but nothing ever updates head->job.next, so once the queue has drained it still names the last job freed and the next add_job writes through that pointer. It cannot fire while main adds a single batch, but the ABA scenario in the manuscript narrates precisely a second batch, so the text describes a flow the code cannot survive. A two-batch driver under a sanitizer aborts in 20 runs out of 20 inside add_job before this change and none after; what remains is the documented use-after-free in worker, which the book keeps deliberately.
The struct carries a cache line of padding and a comment saying it avoids false sharing, but it was allocated with plain malloc, so it landed on a 16-byte boundary and the hot pointer shared a line with whatever preceded it. Padding sizes a struct; only alignment places it. In the versioned variant the same alignment is what the double-width compare-and-swap needs. Declaring it makes sizeof a multiple of the alignment, which is what aligned_alloc requires of its size argument.
thrd_create was unchecked, so a failure left an indeterminate thread identifier that tpool_destroy would later join. The allocation failure paths leaked the identifier array and, more awkwardly, returned false with the initialized flag still set, which wedged the pool object against any retry. Check every create, join the workers already hired, release the flag, and leave no dangling pointer behind. Nothing is left for tpool_destroy to reclaim on this path, so the comment says the caller must not call it rather than inviting them to.
Cancelling a pool with jobs still queued freed each job but neither the future it owns nor that future's result. Workers are all joined by this point, so the queue is single-threaded here and can be drained safely. The cancellation warning also fired for any non-zero prior state rather than for a running pool specifically, which is what the manuscript says it reports.
add_job returns NULL when it cannot allocate, and bbp returns NULL when it cannot allocate its result; both were then dereferenced unchecked. Guarding them without also failing would be worse than the crash, because the program prints a value the manuscript quotes exactly, and a run missing a term would otherwise report success while contradicting the book. A failed init returns non-zero too, having previously returned 0 from a run that did no work. Bailing out mid-loop has to drain the queue first: tpool_destroy frees the futures of jobs still queued, but a worker that has already claimed one frees the job and not its future. Wait for the workers to park before destroying either way, since tpool_destroy reports on a pool it cancels while running and a completed future does not mean the last worker has published idle.
threadA looped while its compare-exchange kept succeeding, so it ran forever and the program only terminated because main slept two seconds and returned out from under it. Whether the fourth line was printed before the process died was a race, and nothing ordered A's load before B's updates, so B could finish first and leave no ABA to observe at all. Two flags pin the interleaving instead: B waits until A has read v, and A waits until B has changed it and changed it back. The four lines the manuscript quotes now come out on every run, and dropping the sleep takes 300 runs from five minutes to under two seconds. A compare-exchange that fails means the run demonstrates nothing, so it reports that and exits non-zero rather than printing a shorter story. Both creates are checked, since each thread spins on a flag only the other one sets.
The listing is introduced as the worker from the thread pool section, but it referenced a job->args field that does not exist, detected idleness by testing that field against NULL rather than comparing against the sentinel, and called free on it: in the real program that argument points into an array on main's stack. It was also C code in a C++ listing environment. The ordering it showed was wrong in three ways as well. The load of the job pointer has to be an acquire, because the loop follows what it reads and a release on the successful compare-exchange orders the claiming thread's earlier work rather than the reads that follow it. The reload a failed compare-exchange performs needs the same. And the test for running announces a queue the employer has just filled, unlike the test for cancelled, so that load cannot be relaxed either: add_job publishes the head pointer atomically only for the first job of an empty queue, which leaves the store of running as the only edge carrying the rest.
A single all target relinked all three programs on every invocation and hardcoded -mcx16, which does not exist outside x86-64 and fails the build outright elsewhere. Ask the compiler for its target rather than asking uname about the host, so a cross build is judged on what it is producing, and say so when the compiler cannot answer instead of quietly dropping the flag. -mcx16 does not make libatomic unnecessary either: some versions still route 16-byte atomic load and store through it, which is the link failure the manuscript footnotes. Probing beats guessing, and the probe carries the flags of the link it is deciding for. check now compares stdout against the text the manuscript quotes, since asserting only the exit status lets the book and its own programs drift apart while CI stays green. Flags are set with override, and the binaries depend on a stamp naming the toolchain, so caller flags compose and changing any of them rebuilds rather than leaving check to assert on stale binaries.
The examples reach the document through inputminted, which hands them to pygmentize rather than opening them as TeX inputs, so latexmk never records them as dependencies and examples/ appears nowhere in its database. Editing one left the PDF untouched and the book shipped a stale copy of its own code. Naming them as prerequisites is not enough on its own, because latexmk then consults that database and declares the PDF up to date, so force one pass when make has decided something changed. The list is read out of the document rather than globbed or spelled out here, since the inputminted directives are the only record of what the book actually embeds.
clean removed the minted cache under its minted v2 name while the current toolchain writes elsewhere, and left the LuaLaTeX intermediate behind, so roughly a megabyte survived it. The example binaries were neither cleaned nor ignored.
The programs need a libc with C11 threads, which rules out macOS, and the versioned example needs a double-width compare-and-swap. Neither was written down. Also note that the first build fetches a style file over the network.
checkout and cache were a major version behind. The TeX cache had no restore-keys, so its key changed on nearly every push and it never once restored; a prefix fallback is what makes it do anything. Add the minted output to it, which is the expensive part of the build, one pygmentize process per snippet. Both the key and that fallback carry the workflow hash, because the container digest lives there: minted names its entries by listing content alone, so after an image bump a bare prefix would serve every untouched listing from the previous pygments and the PDF would mix two highlighting styles. The style file stays out of the cache entirely, since the Makefile fetches it from a floating URL and a cached copy would pin the build to whichever revision landed first. Superseded pull request runs still cancel, but a push to main no longer does, since that run publishes the release and moves the tag.
One script per check, each runnable by hand rather than only through CI, and driven by the file list git already has. Adds a trailing-newline check, a sweep for banned functions and hardcoded secrets, and cppcheck alongside the formatting gate. clang-format is pinned to one release. These files are printed verbatim in the book, so the accepted style has to be a single version rather than whichever major a machine happens to carry, and the script says so instead of silently reformatting to a different one. The security sweep folds spliced lines before scanning and replaces comments with a space, matching translation phases 2 and 3, so that a banned name written in prose cannot fail the build and a definition split by a comment cannot hide from it. Quoted literals are copied out untouched, since a string holding a comment opener would otherwise swallow the code after it and the secret pattern matches on literals anyway. cppcheck runs with warnings enabled on top of the error checks that always run. That does not exempt the use-after-free the manuscript keeps on purpose; cppcheck simply does not see it, because the free and the stale read are in different threads rather than along one path through a function.
The job only checked formatting, so an example that no longer compiled, no longer terminated, or no longer printed what the manuscript quotes would still be typeset into the PDF and published. Run the checks, and make the release depend on that job: latexmk typesets the examples from source and succeeds either way, so nothing else can catch it. The toolchain is installed rather than taken from the runner image, which means the image no longer decides the accepted style, and the runner is pinned so what does decide it is reproducible. The release also accepts a manual run against main. Runs get lost to queue backlogs and cancellations, and without this the only way to publish that commit is to push another one. The ref check is what keeps it honest: dispatching on any other branch still skips the job, exactly as a pull request does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Refined CI and build to compile and run the C examples, rebuild the PDF when examples change, and block releases on failing code. Improved CI diagnostics and tightened security scanning.
New Features
.ci/check-newline.sh,.ci/check-format.sh(pinnedclang-format-20),.ci/check-security.sh,.ci/check-cppcheck.sh, thenmake -C examples check;releasenow depends onbuildandexamples; PR runs can cancel, pushes tomainnever do;workflow_dispatchcan republish.\inputmintedand runninglatexmk -g; cached TeX and_mintedkeyed on the pinned container with restore keys; cleaned/ignored generated artifacts.examples/Makefile: per-target rules withoverride CFLAGS; detect x86-64 via compiler macros to pass-mcx16; probe-latomic; keep callerCPPFLAGS/LDFLAGS; rebuild when the toolchain/flags change via a signature stamp that includes the compiler version and safely escapes quotes;checkasserts exact stdout;formatusesCLANG_FORMAT; README documents C11 threads and-mcx16/libatomic.Bug Fixes
v_prev); dropped the stale front link when the queue drains; in the versioned variant, claimed with a guarded loop and published pointer+version in one 16-byte store.thrd_createand unwound cleanly; freed unclaimed futures (and results) on cancel; waited for workers to publishidlebeforetpool_destroy; failed runs on init/add_job/result allocation errors and reported via exit status..ci/check-security.sh(perform translation phase 2 first, preserve line numbers, replace comments with spaces, no SIGPIPE; detect#undef _FORTIFY_SOURCE/__SSP__);.ci/check-format.sh,.ci/check-newline.sh,.ci/check-cppcheck.shprint actionable errors and clean temp files.Written for commit 9a714cf. Summary will update on new commits.