fix(codegen): give each .ll compile a private temp dir so it can be deleted again (#7144) - #7168
Conversation
… race test proves Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
…mp dir empty (#7144) Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
…port the driver's perry-objs leak Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
…othing about the .ll Measured on a real Perry module (Apple clang 21, -target x86_64-unknown-linux-gnu and aarch64-unknown-linux-gnu): the -g object is byte-identical to the one without it and carries no .debug_* sections. Perry's codegen emits no DICompileUnit/DIFile/!dbg metadata, and clang -g on a .ll lowers debug info present in the IR rather than synthesising a compile unit for the input file. The inherited claim that -g pulls the absolute .ll path plus DW_AT_comp_dir into DWARF is therefore false here, and the second temp-file layout it justified was a mode nobody could exercise. One layout now. Both halves of the measurement are tests: the .ll directory never reaches the object (with a live control), and -g does not change the emitted bytes. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fixes #7144.
compile_ll_to_objectstopped unlinking its temp.llin #7135, on purpose:that PR had just made the name a pure function of the IR (#7131 — clang records
a translation unit's basename into the ELF object), so two workers holding
identical IR now shared the path and a per-call unlink could race a sibling
that had computed the path but not yet handed it to clang.
Nothing else deleted them. Because the name is content-addressed, the leftovers
are bounded by distinct IR ever compiled on the machine — which sounds benign
until you notice that working on the compiler changes the IR on essentially every
rebuild. 1627 files / 951.8 MB after one day on one box; the temp dir of the Mac
this was written on still held 1069 files / 635 MB from before the fix.
The fix: stop sharing, rather than delete more carefully
Every
.ll→.ocompile gets a directory it owns:and
remove_dir_alls it once the object bytes are in memory.Why this survives the identical-IR race, where a delete-after-use does not.
The race needs a shared name. There isn't one any more: the directory carries
every uniquifier, so no two calls — in one process or across processes — are ever
handed the same
.llpath, and unlinking is unobservable to anyone else. Thereis no window to lose rather than a narrow one, which matters because a narrow
window is not testable: sabotaged to the naive shape (one flat shared
.ll,unlinked after use), the 8-way concurrent test in this PR went red in one
full-suite run and green in the next three. The structural property is what is
asserted; the concurrency test is the end-to-end complement, and says so in its
own comment.
Why it does not undo #7131. The directory carries the uniquifiers, the
basename carries none — and the object records the basename and nothing else.
That was measured by hand once, on a Pi, in #7140. It is now a test
(
the_ll_directory_is_not_recorded_in_the_object_but_the_basename_is): compileone
.llunder the same basename from two directories, for both Linux ELFtargets, assert the objects are identical, with a live control that a different
basename does change them. Cross-compiling because the embedding is a property of
the ELF writer, not of the host — so this runs on the Mac where the defect class
is otherwise invisible.
PERRY_DEBUG_SYMBOLSis not an exemption — the premise was falseThe issue asks whether
-gbuilds should keep the file, since they "embed theabsolute
.llpath in DWARF". They do not. Measured on a real Perry module(Apple clang 21,
-target x86_64-unknown-linux-gnuandaarch64-unknown-linux-gnu):-gobject vs no--gobject.debug_*sections in the-gobjectDICompileUnit/DIFile/!dbgin Perry's IRclang -gon a.lllowers debug info that is in the IR; it does notsynthesise a compile unit for the input file. So nothing records where the
.llwas, and nothing needs to outlive the compile.
I had first implemented the exemption — a second
DebugSharedlayout — and thendeleted it, because a mode justified by something that does not happen is exactly
the "unexercised mode nobody has verified" CLAUDE.md says to remove. One layout
now. Both halves of the measurement are tests, including a direct assertion that
the
-gobject contains no.debug_section name (an equality assertion can bedefeated by an edit to itself; a claim about the artifact cannot). Not measured
on COFF/Windows — stated in the code.
Failure policy
Unchanged and now pinned: a failed compile keeps its IR. The error already
names the file;
failed_compile_keeps_the_ll_for_diagnosisasserts exactly one.llsurvives anywhere under the temp root and that the message names it. Samefor
PERRY_LLVM_KEEP_IR, which now keeps.ll,.o,.clang-stderrand thecompile plan together in one directory instead of scattered across the temp root.
Red then green
census-temp-hygiene(new): compile the corpus withTMPDIRpointed at an emptydirectory, then look in it. Two real arms, built sequentially from one target dir,
distinct binary hashes:
df7214b0d+ main'slinker.rsNote the shape of the number: 27, not 54. Repeats share a content hash and so
share a filename. "No growth run-over-run" would have been green on the broken
compiler — which is why CI never saw this and developer machines filled up. The
gate asserts the absolute property instead.
The other two acceptance criteria
Determinism holds —
census-determinism --repeat 3 --jobs 4: 27/27 workloadsbyte-identical;
--repeat 4 --jobs 8twice more, same. #7140's concurrencyproperty holds — 24 concurrent
perryprocesses compiling one identicalsource, three runs: 0/24 failures, one distinct object hash each time.
No behavioural change: across all 27 census workloads the objects emitted by
main and by this branch are byte-identical, 0/27 differences — including the
raced-repeat fixture, whose hash
94f09ed6782d4917is the same on both arms.Sabotage-verified, both directions
Every check was made to fail on purpose before being believed:
.ll, unlinked after use (the naive fix).llbasename (re-break #7131)PERRY_LLVM_KEEP_IRdeletes anyway-target(falls back to Mach-O)-gobject does have.debug_-gtest REDFound on the way, filed not fixed: #7167
The hygiene gate went red on its first real run — on someone else's files.
run_pipeline.rscreates aperry-objs-<pid>-<nanos>/object staging directoryand removes it on both link exits;
--no-linkreturns before either, so every--no-linkcompile leaks the directory and its objects. Unlike #7144 that isunbounded in compiles, not in distinct IR, and the objects are much larger.
Filed as #7167 and left alone: it is
crates/perry/src/commands/compile/, notthis module.
So this gate fails on the clang driver's own temp names (
perry_llvm_*,perry_cgu_*,perry_bc_*) and reports anything else without failing — a gatethat goes red for another module's defect gets muted rather than fixed. Widening
OWNED_PREFIXESto "nothing at all" is a one-line change once #7167 lands, andthe comment there says so.
What I could not verify
mDNS), and the Mini belongs to another agent. So
census-determinismwas notre-run on real ELF hardware. What that check would add over what is here: the
ELF-specific risk this change introduces is "does the new per-call directory
reach the object", and that is measured directly above, by cross-compiling to
both Linux targets — the embedding is a property of the ELF writer, not of the
host. The corpus-level Linux run is still worth doing when a Pi is free.
-gbehaviour is not measured, only ELF and Mach-O.perry_llvm_*.llfiles from the compile path: an age-based sweep can unlink afile a concurrently running old binary is about to reuse via its
"already exists, same content" fast path, and several worktrees on this machine
are doing exactly that. One-time reclaim, when the box is quiet:
find "$TMPDIR" -maxdepth 1 -name 'perry_llvm_*.ll' -delete.mainis independently red:manifest_consistency(missing API manifestentries for
iovalkey,dgram::sendto,sqlite::{serialize,deserialize}) andthe addr-class ratchet (
child_process/value_util.rs,fs/dirent.rs). Neitherfile is in this diff. Judge CI against main's, not against green.
byte-identical-objects result is the direct evidence.
Note on the file split
linker.rscrossed the 2,000-line cap, so the new tests live inlinker_temp_lifecycle_tests.rsvia#[path](repo precedent:object_cache_tests.rs,type_analysis_tests.rs).linker.rsis 1,803 lines.