From 442416fc4b26e6e3d719c050861bc14bb3f2a9c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:24:01 +0200 Subject: [PATCH 01/11] fix(codegen): give each .ll compile a private temp dir so it can be deleted (#7144) Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- crates/perry-codegen/src/linker.rs | 268 ++++++++++++--- .../src/linker_temp_lifecycle_tests.rs | 317 ++++++++++++++++++ 2 files changed, 547 insertions(+), 38 deletions(-) create mode 100644 crates/perry-codegen/src/linker_temp_lifecycle_tests.rs diff --git a/crates/perry-codegen/src/linker.rs b/crates/perry-codegen/src/linker.rs index b460827dc8..dc7e0ed723 100644 --- a/crates/perry-codegen/src/linker.rs +++ b/crates/perry-codegen/src/linker.rs @@ -46,9 +46,77 @@ static CLANG_PROBE: OnceLock> = OnceLock::new(); /// may stay in every *output* name, where it costs nothing and still closes /// #509. The one caveat is `PERRY_DEBUG_SYMBOLS`, which adds `-g` and pulls the /// absolute `.ll` path plus `DW_AT_comp_dir` into DWARF — objects built with it -/// are reproducible only for a fixed `TMPDIR` and working directory. +/// are reproducible only for a fixed `TMPDIR` and working directory. That +/// caveat is also why the two layouts in `LlLayout` exist. static TEMP_NONCE_COUNTER: AtomicU64 = AtomicU64::new(0); +/// Which temp-file layout one `.ll` uses — and, inseparably, whether the file +/// survives the compile (#7144). +/// +/// The rule, stated once: **the `.ll` lives at a path whose stability matches +/// what the emitted object records about it.** +/// +/// * `Scratch` (default): the object records the translation unit's *basename* +/// and nothing else — the table above, measured, not assumed. So the basename +/// must be content-addressed and the *directory* is free. Give every call its +/// own directory and remove it when the compile succeeds. Nothing is shared, +/// so the unlink cannot race a sibling worker that computed the same content +/// hash. That race is why #7135 stopped deleting the `.ll` at all, and why +/// #7144 then measured one leftover per *distinct IR ever compiled* — 1627 +/// files / 951.8 MB on one dev box after a day, 29 GB on another. +/// * `DebugShared` (`PERRY_DEBUG_SYMBOLS`): clang also gets `-g`, which puts +/// the `.ll`'s **absolute** path plus `DW_AT_comp_dir` into DWARF. The file is +/// then referenced by the shipped object, so it must (a) keep a stable +/// absolute path — flat in the temp root, content-addressed, exactly the +/// pre-#7144 layout — and (b) outlive the compile, or a debugger stepping +/// into generated code has nothing to resolve. These are retained on purpose. +/// They are bounded by the distinct IR compiled *with `-g`*, and a debug build +/// asking for debug info is the one case where keeping the source is the +/// point rather than a leak. +/// +/// The layout and clang's `-g` flag must never disagree: deleting a `.ll` that +/// DWARF names by absolute path silently breaks source-level debugging. Both +/// are therefore derived from one `TempFilePolicy` value at a single call site, +/// and `debug_symbols_layout_and_g_flag_agree` pins that. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LlLayout { + /// Per-call directory, removed on success. + Scratch, + /// Flat, content-addressed, retained — the object points at it. + DebugShared, +} + +/// The two environment inputs that decide what happens to the temp files. +/// +/// Read once, in `compile_ll_to_object`, and threaded down rather than probed +/// where they are used: the lifecycle is then testable without a test mutating +/// process-wide environment underneath every other test in the binary. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct TempFilePolicy { + /// `PERRY_LLVM_KEEP_IR` — retain every intermediate and write the compile + /// plan alongside it. + keep: bool, + /// `PERRY_DEBUG_SYMBOLS` — clang gets `-g`; see `LlLayout::DebugShared`. + debug_symbols: bool, +} + +impl TempFilePolicy { + fn from_env() -> Self { + Self { + keep: env::var_os("PERRY_LLVM_KEEP_IR").is_some(), + debug_symbols: env::var_os("PERRY_DEBUG_SYMBOLS").is_some(), + } + } + + fn layout(self) -> LlLayout { + if self.debug_symbols { + LlLayout::DebugShared + } else { + LlLayout::Scratch + } + } +} + /// FNV-1a 64-bit over `ll_text`. Stable across platforms and rustc versions /// (unlike `DefaultHasher`), used only to content-address temp IR filenames so /// clang embeds a deterministic source path (#7131). Collision risk is @@ -81,6 +149,11 @@ fn ll_content_hash(ll_text: &str) -> u64 { /// same-source compiles failed with "Failed to read clang output … No such /// file or directory". This is #509 again, one scope out. /// +/// Under `LlLayout::Scratch` the two names additionally sit inside a directory +/// that belongs to this call alone (#7144). The directory name is *not* +/// content-addressed — it must not be, or two callers would share it again — +/// and it does not have to be: only the basename reaches the object. +/// /// `pid` and `counter` are parameters rather than read in here so the property /// above is testable without spawning processes. fn llvm_temp_paths_for( @@ -88,21 +161,53 @@ fn llvm_temp_paths_for( ll_text: &str, pid: u32, counter: u64, -) -> (PathBuf, PathBuf) { + layout: LlLayout, +) -> LlvmTempPaths { let hash = ll_content_hash(ll_text); - let ll_path = tmp_dir.join(format!("perry_llvm_{hash:016x}.ll")); - let obj_path = tmp_dir.join(format!("perry_llvm_{hash:016x}_{pid:x}_{counter:x}.o")); - (ll_path, obj_path) + let ll_name = format!("perry_llvm_{hash:016x}.ll"); + // The `.o` keeps its uniquifiers even inside a private directory. They cost + // nothing, and the day someone flattens the layout again the object must + // not silently go back to colliding across processes (#7140). + let obj_name = format!("perry_llvm_{hash:016x}_{pid:x}_{counter:x}.o"); + match layout { + LlLayout::Scratch => { + let scratch = tmp_dir.join(format!("perry_llvm_scratch_{pid:x}_{counter:x}")); + LlvmTempPaths { + ll_path: scratch.join(&ll_name), + obj_path: scratch.join(&obj_name), + scratch_dir: Some(scratch), + } + } + LlLayout::DebugShared => LlvmTempPaths { + ll_path: tmp_dir.join(&ll_name), + obj_path: tmp_dir.join(&obj_name), + scratch_dir: None, + }, + } +} + +/// Temp paths for one `compile_ll_to_object` call. +#[derive(Debug, Clone)] +struct LlvmTempPaths { + /// Directory owned exclusively by this call, removed once the object bytes + /// have been read. `None` under `LlLayout::DebugShared`, where the `.ll` is + /// shared between callers and deliberately retained. + scratch_dir: Option, + ll_path: PathBuf, + obj_path: PathBuf, } /// `llvm_temp_paths_for` with this process's pid and the next counter value. -/// Returns `(ll_path, obj_path, pid, counter)` — the last two also name the +/// Returns the paths plus `(pid, counter)` — the last two also name the /// atomic-write staging file. -fn llvm_temp_paths(tmp_dir: &Path, ll_text: &str) -> (PathBuf, PathBuf, u32, u64) { +fn llvm_temp_paths(tmp_dir: &Path, ll_text: &str, layout: LlLayout) -> (LlvmTempPaths, u32, u64) { let pid = std::process::id(); let counter = TEMP_NONCE_COUNTER.fetch_add(1, Ordering::Relaxed); - let (ll_path, obj_path) = llvm_temp_paths_for(tmp_dir, ll_text, pid, counter); - (ll_path, obj_path, pid, counter) + ( + llvm_temp_paths_for(tmp_dir, ll_text, pid, counter, layout), + pid, + counter, + ) } /// Staging name for the atomic `.ll` write. Must be unique per process for the @@ -300,6 +405,7 @@ fn build_clang_compile_plan( target_triple: Option<&str>, ll_byte_size: usize, ll_fn_count: usize, + debug_symbols: bool, ) -> ClangCompilePlan { let effective_target = target_triple .map(|s| s.to_string()) @@ -335,7 +441,11 @@ fn build_clang_compile_plan( }; let mut clang_args = vec!["-c".to_string(), opt_flag.to_string()]; - if std::env::var("PERRY_DEBUG_SYMBOLS").is_ok() { + // `-g` and `LlLayout::DebugShared` are two consequences of ONE decision + // (`TempFilePolicy::debug_symbols`) and are passed the same value from the + // same call site. If they ever disagree, we delete a `.ll` that DWARF names + // by absolute path and source-level debugging breaks silently (#7144). + if debug_symbols { clang_args.push("-g".to_string()); } clang_args.push("-fno-math-errno".to_string()); @@ -389,6 +499,40 @@ fn build_clang_compile_plan( /// resulting `.o`, and clean up both on success. On failure the temp files /// are left behind for debugging — the caller can `grep /tmp/perry_llvm_*`. pub fn compile_ll_to_object(ll_text: &str, target_triple: Option<&str>) -> Result> { + compile_ll_to_object_in( + &env::temp_dir(), + ll_text, + target_triple, + TempFilePolicy::from_env(), + ) +} + +/// `compile_ll_to_object` with the temp root and the file-lifetime policy as +/// arguments instead of process state. +/// +/// Both are parameters so the temp-file *lifecycle* — the subject of #7144 — is +/// testable: a test can hand this an empty directory of its own and assert what +/// is left in it, without racing every other test in the binary over `TMPDIR` +/// or `PERRY_LLVM_KEEP_IR`. +/// +/// Cleanup policy, in one place: +/// +/// * **success**: the object is read into memory and removed; under +/// `LlLayout::Scratch` the whole per-call directory goes with it, so a +/// compile leaves nothing behind (#7144). +/// * **failure** (clang non-zero, or the object cannot be read): everything is +/// left on disk. The error message names the `.ll`, and a failed compile is +/// exactly when someone wants to look at the IR that produced it. +/// * **`PERRY_LLVM_KEEP_IR`**: everything is kept and its location printed, +/// plus the compile plan as JSON. +/// * **`PERRY_DEBUG_SYMBOLS`**: the `.ll` is kept even on success — the object's +/// DWARF references it by absolute path. See `LlLayout`. +fn compile_ll_to_object_in( + tmp_dir: &Path, + ll_text: &str, + target_triple: Option<&str>, + policy: TempFilePolicy, +) -> Result> { // Validate the toolchain before creating the potentially large `.ll` // scratch file. Unsupported clang releases should fail without leaving // an artifact that was never passed to the compiler. @@ -414,10 +558,21 @@ pub fn compile_ll_to_object(ll_text: &str, target_triple: Option<&str>) -> Resul })?; ensure_supported_clang(&clang)?; - let tmp_dir = env::temp_dir(); // #7131: content-address the `.ll` basename (clang embeds it into the // object on ELF). #509: keep the `.o` unique via the per-call counter. - let (ll_path, obj_path, write_pid, write_nonce) = llvm_temp_paths(&tmp_dir, ll_text); + // #7144: put both under a directory this call owns, so the `.ll` can be + // deleted again without racing a sibling that hashed the same IR. + let layout = policy.layout(); + let (paths, write_pid, write_nonce) = llvm_temp_paths(tmp_dir, ll_text, layout); + let LlvmTempPaths { + scratch_dir, + ll_path, + obj_path, + } = paths; + if let Some(dir) = &scratch_dir { + fs::create_dir_all(dir) + .with_context(|| format!("Failed to create temp dir at {}", dir.display()))?; + } write_ll_atomically(&ll_path, ll_text, write_pid, write_nonce)?; let plan = build_clang_compile_plan( @@ -427,6 +582,7 @@ pub fn compile_ll_to_object(ll_text: &str, target_triple: Option<&str>) -> Resul target_triple, ll_text.len(), count_ll_functions(ll_text), + policy.debug_symbols, ); // Pre-flight probe: capture clang's default Target: line once per process, @@ -499,15 +655,21 @@ pub fn compile_ll_to_object(ll_text: &str, target_triple: Option<&str>) -> Resul let bytes = fs::read(&obj_path) .with_context(|| format!("Failed to read clang output at {}", obj_path.display()))?; - // Clean up on success. The content-addressed `.ll` is **shared** across - // concurrent `compile_ll_to_object` calls with identical IR (#7131), so we - // must NOT delete it here — a sibling worker may still have clang open on - // that path (CodeRabbit). Only the unique `.o` is removed. The `.ll` is - // left in the process temp dir (same content is reused on cache misses; - // OS temp cleanup / `PERRY_LLVM_KEEP_IR` handle the rest). When KEEP_IR is - // set we also retain the object and write compile metadata. - let keep = env::var_os("PERRY_LLVM_KEEP_IR").is_some(); - if keep { + // Clean up on success. + // + // #7135 could not delete the `.ll`: it had just made the name a pure + // function of the IR, so two workers holding identical IR *shared* that + // path and either could unlink it in the window between the other computing + // the path and clang opening it. The consequence (#7144) was that nothing + // ever deleted them — one file per distinct IR ever compiled, measured at + // 29 GB on a dev box. + // + // The fix is not a more careful delete, it is removing the sharing: the + // `.ll` now sits in a directory that belongs to this call, so unlinking it + // is unobservable to anyone else and there is no window to lose. The name + // clang records — the basename — is untouched, so emission stays + // deterministic (#7131). + if policy.keep { let _ = fs::write(&plan.stderr_remarks_path, &output.stderr); let metadata_path = PathBuf::from(format!("{}.compile-plan.json", plan.obj_path.display())); write_compile_plan_metadata(&plan, &metadata_path)?; @@ -519,6 +681,19 @@ pub fn compile_ll_to_object(ll_text: &str, target_triple: Option<&str>) -> Resul ); } else { let _ = fs::remove_file(&plan.obj_path); + match &scratch_dir { + // Ours alone: `remove_dir_all` rather than unlinking the two names + // we know about, so anything clang chose to drop beside them goes + // too and the directory cannot survive as an empty husk. + Some(dir) => { + let _ = fs::remove_dir_all(dir); + } + // `LlLayout::DebugShared`: the object's DWARF names this `.ll` by + // absolute path. Deleting it would leave a debug build that cannot + // show the source it was built from — the one case where retaining + // the IR is the feature. + None => {} + } } Ok(bytes) @@ -1367,6 +1542,7 @@ mod tests { None, 0, 0, + false, ); assert!(plan.clang_args.contains(&"-fno-math-errno".to_string())); // Small module → optimized at -O3 (#4880). @@ -1394,6 +1570,7 @@ mod tests { None, huge, many_funcs, + false, ); assert!(plan.clang_args.contains(&"-Os".to_string())); assert!(!plan.clang_args.contains(&"-O3".to_string())); @@ -1413,6 +1590,7 @@ mod tests { None, huge, 2, // ~3 MB/fn — far above the density cap + false, ); assert!(plan.clang_args.contains(&"-O0".to_string())); assert!(!plan.clang_args.contains(&"-O3".to_string())); @@ -1428,6 +1606,7 @@ mod tests { Some("x86_64-unknown-linux-gnu"), 0, 0, + false, ); assert_eq!(plan.effective_target, "x86_64-unknown-linux-gnu"); assert_eq!(plan.native_tuning_arg, None); @@ -1520,6 +1699,7 @@ mod tests { Some("x86_64-unknown-linux-gnu"), 0, 0, + false, ); write_compile_plan_metadata(&plan, &temp).unwrap(); let text = fs::read_to_string(&temp).unwrap(); @@ -1570,8 +1750,10 @@ mod tests { // basename still differs via the counter. let tmp = env::temp_dir(); let ir = "define void @f() {\n ret void\n}\n"; - let (ll_a, obj_a, _, _) = llvm_temp_paths(&tmp, ir); - let (ll_b, obj_b, _, _) = llvm_temp_paths(&tmp, ir); + let (a, _, _) = llvm_temp_paths(&tmp, ir, LlLayout::Scratch); + let (b, _, _) = llvm_temp_paths(&tmp, ir, LlLayout::Scratch); + let (ll_a, obj_a) = (&a.ll_path, &a.obj_path); + let (ll_b, obj_b) = (&b.ll_path, &b.obj_path); assert_eq!( ll_a.file_name(), ll_b.file_name(), @@ -1583,8 +1765,12 @@ mod tests { ".o basenames must stay unique across calls (#509)" ); // Different IR → different .ll basename. - let (ll_c, _, _, _) = llvm_temp_paths(&tmp, "define void @g() {\n ret void\n}\n"); - assert_ne!(ll_a.file_name(), ll_c.file_name()); + let (c, _, _) = llvm_temp_paths( + &tmp, + "define void @g() {\n ret void\n}\n", + LlLayout::Scratch, + ); + assert_ne!(ll_a.file_name(), c.ll_path.file_name()); // No pid / wall-clock digits of variable width — only hex hash. let name = ll_a.file_name().unwrap().to_string_lossy(); assert!( @@ -1616,8 +1802,10 @@ mod tests { let ir = "define void @f() {\n ret void\n}\n"; // Same IR, same counter, DIFFERENT process. - let (ll_p1, obj_p1) = llvm_temp_paths_for(&tmp, ir, 1111, 0); - let (ll_p2, obj_p2) = llvm_temp_paths_for(&tmp, ir, 2222, 0); + let p1 = llvm_temp_paths_for(&tmp, ir, 1111, 0, LlLayout::Scratch); + let p2 = llvm_temp_paths_for(&tmp, ir, 2222, 0, LlLayout::Scratch); + let (ll_p1, obj_p1) = (&p1.ll_path, &p1.obj_path); + let (ll_p2, obj_p2) = (&p2.ll_path, &p2.obj_path); assert_eq!( ll_p1.file_name(), ll_p2.file_name(), @@ -1633,29 +1821,26 @@ mod tests { // Same process, different call: the counter still has to separate // in-process rayon workers. - let (_, obj_c0) = llvm_temp_paths_for(&tmp, ir, 1111, 0); - let (_, obj_c1) = llvm_temp_paths_for(&tmp, ir, 1111, 1); - assert_ne!(obj_c0.file_name(), obj_c1.file_name()); + let c0 = llvm_temp_paths_for(&tmp, ir, 1111, 0, LlLayout::Scratch); + let c1 = llvm_temp_paths_for(&tmp, ir, 1111, 1, LlLayout::Scratch); + assert_ne!(c0.obj_path.file_name(), c1.obj_path.file_name()); // The atomic-write staging name needs the same separation: both // processes reach it with the same hash and the same counter, and // `File::create` truncates. assert_ne!( - ll_staging_path(&ll_p1, 1111, 0).file_name(), - ll_staging_path(&ll_p1, 2222, 0).file_name(), + ll_staging_path(ll_p1, 1111, 0).file_name(), + ll_staging_path(ll_p1, 2222, 0).file_name(), "staging .tmp name must be per-process" ); assert_ne!( - ll_staging_path(&ll_p1, 1111, 0).file_name(), - ll_staging_path(&ll_p1, 1111, 1).file_name(), + ll_staging_path(ll_p1, 1111, 0).file_name(), + ll_staging_path(ll_p1, 1111, 1).file_name(), "staging .tmp name must be per-call" ); // …and the staging file must never be mistaken for the real `.ll`. - assert_ne!( - ll_staging_path(&ll_p1, 1111, 0).file_name(), - ll_p1.file_name() - ); + assert_ne!(ll_staging_path(ll_p1, 1111, 0).file_name(), ll_p1.file_name()); } #[test] @@ -1665,3 +1850,10 @@ mod tests { assert_eq!(ll_content_hash("a"), 0xaf63_dc4c_8601_ec8c); } } + +/// Temp-file *lifecycle* — who owns the `.ll`, and when it is removed (#7144). +/// A sibling file only because of the 2,000-line cap; `use super::*` gives it +/// the same view of this module as the block above. +#[cfg(test)] +#[path = "linker_temp_lifecycle_tests.rs"] +mod linker_temp_lifecycle_tests; diff --git a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs new file mode 100644 index 0000000000..d6edaaea3a --- /dev/null +++ b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs @@ -0,0 +1,317 @@ +//! Temp-file lifecycle for the clang driver (#7144), and the path-shape +//! properties it has to hold constant (#7131, #7140, #509). +//! +//! Split out of `linker.rs` for the 2,000-line file cap, not because it is a +//! different subject: everything here is about the two temp files one +//! `.ll` → `.o` compile creates, who may see them, and when they are removed. +//! +//! The end-to-end tests drive the real `clang` into a temp root of their own +//! and assert on what is left in that root. That is the only way a leak is +//! observable — every path-shape test in this file passes just as happily +//! against a compiler that never deletes anything, which is exactly how #7144 +//! shipped inside a green #7135. + +use super::*; + +#[test] +fn scratch_dir_is_per_call_and_per_process_but_the_ll_basename_is_not() { + // #7144's shape, and the reason it does not undo #7131: the *directory* + // carries every uniquifier, the *basename* carries none. clang records + // the basename into the object and nothing else (no `-g`), so the two + // properties do not compete — a call can own its `.ll` outright and + // still emit the same object bytes as any other call with the same IR. + let tmp = Path::new("/tmp"); + let ir = "define void @f() {\n ret void\n}\n"; + + let a = llvm_temp_paths_for(tmp, ir, 1111, 0, LlLayout::Scratch); + let b = llvm_temp_paths_for(tmp, ir, 1111, 1, LlLayout::Scratch); // same process + let c = llvm_temp_paths_for(tmp, ir, 2222, 0, LlLayout::Scratch); // other process + + let dirs = [&a, &b, &c].map(|p| { + p.scratch_dir + .clone() + .expect("Scratch layout must allocate a private directory") + }); + assert_ne!(dirs[0], dirs[1], "two calls must not share a directory"); + assert_ne!(dirs[0], dirs[2], "two processes must not share a directory"); + + for p in [&a, &b, &c] { + let dir = p.scratch_dir.as_ref().unwrap(); + assert_eq!( + p.ll_path.parent(), + Some(dir.as_path()), + "the .ll must live inside the directory that gets removed" + ); + assert_eq!( + p.obj_path.parent(), + Some(dir.as_path()), + "the .o must go with it, so one remove_dir_all cleans up" + ); + assert_eq!( + p.ll_path.file_name(), + a.ll_path.file_name(), + "the recorded name — the basename — must stay content-only (#7131)" + ); + } +} + +#[test] +fn debug_symbols_keep_a_stable_absolute_ll_path() { + // Under `-g` the object names the `.ll` by ABSOLUTE path in DWARF, so + // the whole path has to be a function of the IR (else `-g` builds stop + // being reproducible for a fixed TMPDIR) and the file has to outlive the + // compile (else the debugger has nothing to open). Both follow from + // "no scratch directory": #7144 exempts this layout on purpose. + let tmp = Path::new("/tmp"); + let ir = "define void @f() {\n ret void\n}\n"; + + let a = llvm_temp_paths_for(tmp, ir, 1111, 0, LlLayout::DebugShared); + let b = llvm_temp_paths_for(tmp, ir, 2222, 7, LlLayout::DebugShared); + assert!( + a.scratch_dir.is_none() && b.scratch_dir.is_none(), + "a per-call directory would put pid/counter into DWARF" + ); + assert_eq!( + a.ll_path, b.ll_path, + "the DWARF-referenced path must be content-addressed end to end" + ); + assert_eq!(a.ll_path.parent(), Some(tmp)); + // The object still may not collide across processes (#7140/#509). + assert_ne!(a.obj_path.file_name(), b.obj_path.file_name()); +} + +#[test] +fn debug_symbols_layout_and_g_flag_agree() { + // The hazard this pins: if the layout said "scratch, delete it" while + // clang was still passed `-g`, every debug build would ship DWARF + // pointing at a file that no longer exists — and nothing would fail + // loudly. One `TempFilePolicy` decides both; this is that contract. + let with_g = TempFilePolicy { + keep: false, + debug_symbols: true, + }; + let without = TempFilePolicy { + keep: false, + debug_symbols: false, + }; + assert_eq!(with_g.layout(), LlLayout::DebugShared); + assert_eq!(without.layout(), LlLayout::Scratch); + + for policy in [with_g, without] { + let plan = build_clang_compile_plan( + PathBuf::from("clang"), + PathBuf::from("/tmp/input.ll"), + PathBuf::from("/tmp/output.o"), + None, + 0, + 0, + policy.debug_symbols, + ); + let has_g = plan.clang_args.iter().any(|a| a == "-g"); + assert_eq!( + has_g, + policy.layout() == LlLayout::DebugShared, + "`-g` is passed iff the `.ll` is retained at a stable path: {policy:?}" + ); + } +} + +// ── Temp-file lifecycle (#7144) ──────────────────────────────────────── +// +// These drive the real `clang`, into a temp root of their own, and assert +// on what is left in that root. A leak is only observable end-to-end: every +// path-shape test above passes just as happily against a compiler that +// never deletes anything, which is exactly how #7144 shipped. + +/// A fresh, empty directory to use as the temp root, or `None` when this +/// host has no usable clang and the compile step cannot run at all. +fn temp_root_if_clang_available(tag: &str) -> Option { + let Some(clang) = find_clang() else { + eprintln!("[linker tests] skipping {tag}: no clang on this host"); + return None; + }; + if let Err(err) = ensure_supported_clang(&clang) { + eprintln!("[linker tests] skipping {tag}: unusable clang ({err:#})"); + return None; + } + let root = env::temp_dir().join(format!( + "perry_linker_test_{tag}_{}_{:x}", + std::process::id(), + TEMP_NONCE_COUNTER.fetch_add(1, Ordering::Relaxed) + )); + let _ = fs::remove_dir_all(&root); + fs::create_dir_all(&root).expect("failed to create test temp root"); + Some(root) +} + +fn entries(root: &Path) -> Vec { + let mut names: Vec = fs::read_dir(root) + .expect("temp root vanished") + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + names +} + +fn test_ir(nth: u32) -> String { + format!("\ndefine i32 @perry_temp_lifecycle_{nth}() {{\nentry:\n ret i32 {nth}\n}}\n") +} + +const CLEAN: TempFilePolicy = TempFilePolicy { + keep: false, + debug_symbols: false, +}; + +#[test] +fn successful_compile_leaves_nothing_behind() { + // THE #7144 regression test. Before the fix each compile left one + // `perry_llvm_.ll` in the temp dir forever — bounded by distinct + // IR ever compiled, which in practice means every rebuild of the + // compiler. 1627 files / 951.8 MB on one dev box after a day. + let Some(root) = temp_root_if_clang_available("clean") else { + return; + }; + + for nth in 0..3 { + let bytes = compile_ll_to_object_in(&root, &test_ir(nth), None, CLEAN) + .unwrap_or_else(|e| panic!("compile {nth} failed: {e:#}")); + assert!(!bytes.is_empty(), "compile {nth} produced no object bytes"); + assert_eq!( + entries(&root), + Vec::::new(), + "compile {nth} left temp files behind (#7144)" + ); + } + let _ = fs::remove_dir_all(&root); +} + +#[test] +fn concurrent_compiles_of_identical_ir_both_succeed_and_leave_nothing() { + // The race that made #7135 stop deleting: two workers holding the SAME + // IR agree on the content-addressed name, so one could unlink it in the + // window between the other computing the path and clang opening it. + // + // This is why the fix is a private directory rather than a more careful + // unlink — there is no shared name left to race over. A "just delete it + // again" fix passes the path-shape tests above and fails here, but only + // sometimes, which is worse than failing. + use std::thread; + + let Some(root) = temp_root_if_clang_available("race") else { + return; + }; + let ir = test_ir(42); + + let results: Vec>> = thread::scope(|s| { + let handles: Vec<_> = (0..8) + .map(|_| { + let root = root.clone(); + let ir = ir.clone(); + s.spawn(move || compile_ll_to_object_in(&root, &ir, None, CLEAN)) + }) + .collect(); + handles.into_iter().map(|h| h.join().unwrap()).collect() + }); + + let mut first: Option> = None; + for (i, r) in results.into_iter().enumerate() { + let bytes = r.unwrap_or_else(|e| panic!("concurrent compile {i} failed: {e:#}")); + match &first { + // Identical IR must still give identical objects — the sharing + // this fix removed was never what made emission deterministic + // (#7131); the content-addressed basename is (#7140). + Some(expected) => assert_eq!(&bytes, expected, "compile {i} emitted other bytes"), + None => first = Some(bytes), + } + } + assert_eq!( + entries(&root), + Vec::::new(), + "8 concurrent identical-IR compiles left temp files behind" + ); + let _ = fs::remove_dir_all(&root); +} + +#[test] +fn failed_compile_keeps_the_ll_for_diagnosis() { + // Stated policy: failures retain their IR. The error message names the + // file, and a compile that just failed is precisely when someone wants + // to read the IR that produced it. + let Some(root) = temp_root_if_clang_available("failure") else { + return; + }; + + let err = compile_ll_to_object_in(&root, "this is not LLVM IR\n", None, CLEAN) + .expect_err("clang must reject non-IR input"); + let message = format!("{err:#}"); + assert!( + message.contains("LLVM IR left at:"), + "the failure must say where the IR is; got: {message}" + ); + + let left = entries(&root); + assert_eq!(left.len(), 1, "expected one retained scratch dir: {left:?}"); + let kept: Vec = entries(&root.join(&left[0])); + assert!( + kept.iter().any(|n| n.ends_with(".ll")), + "the .ll must survive a failed compile: {kept:?}" + ); + let _ = fs::remove_dir_all(&root); +} + +#[test] +fn keep_ir_retains_the_whole_scratch_dir() { + let Some(root) = temp_root_if_clang_available("keep") else { + return; + }; + let policy = TempFilePolicy { + keep: true, + debug_symbols: false, + }; + compile_ll_to_object_in(&root, &test_ir(7), None, policy).expect("compile failed"); + + let left = entries(&root); + assert_eq!(left.len(), 1, "expected one kept scratch dir: {left:?}"); + let kept = entries(&root.join(&left[0])); + for want in [".ll", ".o", ".compile-plan.json"] { + assert!( + kept.iter().any(|n| n.ends_with(want)), + "PERRY_LLVM_KEEP_IR must retain the {want}: {kept:?}" + ); + } + let _ = fs::remove_dir_all(&root); +} + +#[test] +fn debug_symbols_retain_the_ll_at_the_path_dwarf_names() { + // (b) of #7144, as executable policy rather than a sentence in a doc: + // `-g` puts the `.ll`'s absolute path into DWARF, so this layout keeps + // the file — flat in the temp root, content-addressed, at exactly the + // path the object points at. The `.o` is still cleaned up. + let Some(root) = temp_root_if_clang_available("debug") else { + return; + }; + let policy = TempFilePolicy { + keep: false, + debug_symbols: true, + }; + let ir = test_ir(9); + compile_ll_to_object_in(&root, &ir, None, policy).expect("compile failed"); + + let left = entries(&root); + let expected = llvm_temp_paths_for(&root, &ir, 0, 0, LlLayout::DebugShared); + let expected_name = expected.ll_path.file_name().unwrap().to_string_lossy(); + assert_eq!( + left, + vec![expected_name.to_string()], + "a -g build must retain exactly the .ll its DWARF references" + ); + + // …and re-running must not accumulate a second copy: the name is a + // function of the IR, so a `-g` temp dir is bounded by distinct IR + // compiled with `-g`, not by the number of compiles. + compile_ll_to_object_in(&root, &ir, None, policy).expect("second compile failed"); + assert_eq!(entries(&root), left); + let _ = fs::remove_dir_all(&root); +} From bfd81750b9da533fb0c3ed553169c6d9afe522f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:27:54 +0200 Subject: [PATCH 02/11] test(codegen): layout-agnostic failure-path assertion; state what the race test proves Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- .../src/linker_temp_lifecycle_tests.rs | 51 +++++++++++++++---- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs index d6edaaea3a..c47bd6e0c2 100644 --- a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs +++ b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs @@ -192,10 +192,17 @@ fn concurrent_compiles_of_identical_ir_both_succeed_and_leave_nothing() { // IR agree on the content-addressed name, so one could unlink it in the // window between the other computing the path and clang opening it. // - // This is why the fix is a private directory rather than a more careful - // unlink — there is no shared name left to race over. A "just delete it - // again" fix passes the path-shape tests above and fails here, but only - // sometimes, which is worse than failing. + // What this test does and does not prove, stated because a race test that + // is trusted for more than it shows is worse than none. It cannot *decide* + // the race: sabotaged to the naive shape (one shared flat `.ll`, unlinked + // after use) it went red in one full-suite run and green in the next three + // — which is the definition of the window being narrow, not absent. The + // guarantee comes from `scratch_dir_is_per_call_and_per_process_…`: no two + // calls are ever handed the same `.ll` path, so there is nothing to race + // over and no window to lose. This test is the end-to-end complement — + // under real concurrency, 8 identical-IR compiles must all succeed, emit + // the same bytes, and leave nothing — and it will occasionally catch a + // regression the structural test somehow passed. use std::thread; let Some(root) = temp_root_if_clang_available("race") else { @@ -250,16 +257,42 @@ fn failed_compile_keeps_the_ll_for_diagnosis() { "the failure must say where the IR is; got: {message}" ); - let left = entries(&root); - assert_eq!(left.len(), 1, "expected one retained scratch dir: {left:?}"); - let kept: Vec = entries(&root.join(&left[0])); + // Asserted on the whole tree rather than on the scratch directory: the + // claim is "the IR survives a failed compile", and it has to keep meaning + // that if the layout is ever rearranged again. + let surviving = ll_files_under(&root); + assert_eq!( + surviving.len(), + 1, + "exactly the failed compile's .ll must survive, found: {surviving:?}" + ); assert!( - kept.iter().any(|n| n.ends_with(".ll")), - "the .ll must survive a failed compile: {kept:?}" + message.contains(&surviving[0].display().to_string()), + "the failure must name the file it left: {message}" ); let _ = fs::remove_dir_all(&root); } +/// Every `.ll` anywhere under `root`, so a lifetime assertion does not have to +/// know which layout produced the file. +fn ll_files_under(root: &Path) -> Vec { + let mut found = Vec::new(); + let mut stack = vec![root.to_path_buf()]; + while let Some(dir) = stack.pop() { + let Ok(read) = fs::read_dir(&dir) else { continue }; + for entry in read.flatten() { + let path = entry.path(); + if path.is_dir() { + stack.push(path); + } else if path.extension().is_some_and(|e| e == "ll") { + found.push(path); + } + } + } + found.sort(); + found +} + #[test] fn keep_ir_retains_the_whole_scratch_dir() { let Some(root) = temp_root_if_clang_available("keep") else { From fa69a2c3364bef479426abae3f389ef590403785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:30:15 +0200 Subject: [PATCH 03/11] =?UTF-8?q?test(harness):=20census-temp-hygiene=20ga?= =?UTF-8?q?te=20=E2=80=94=20compiling=20must=20leave=20the=20temp=20dir=20?= =?UTF-8?q?empty=20(#7144)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- .github/workflows/test.yml | 16 ++ scripts/compiler_output_harness/cli.py | 30 +++ .../repsel_temp_hygiene.py | 231 ++++++++++++++++++ 3 files changed, 277 insertions(+) create mode 100644 scripts/compiler_output_harness/repsel_temp_hygiene.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d91f3d9239..94ed5eb98d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1218,6 +1218,7 @@ jobs: python3 scripts/compiler_output_regression.py census-self-test python3 scripts/compiler_output_regression.py census-knob-isolation-self-test python3 scripts/compiler_output_regression.py census-determinism-self-test + python3 scripts/compiler_output_regression.py census-temp-hygiene-self-test python3 -m unittest tests.test_repsel_census - name: Build compiler @@ -1235,6 +1236,21 @@ jobs: --repeat 2 \ --jobs 4 + # #7144. The other consequence of content-addressing the `.ll`: workers + # holding identical IR share the name, so #7135 stopped deleting it and + # nothing else did — one leftover per distinct IR ever compiled. CI never + # saw it (runner temp dirs are reclaimed) while developer machines + # reached 29 GB. This step compiles with `TMPDIR` pointed at an empty + # directory and asserts it is still empty afterwards; note that a + # repeat-and-compare check would NOT have caught it, because identical + # IR reuses the identical name. + - name: Temp-directory hygiene + run: | + python3 scripts/compiler_output_regression.py census-temp-hygiene \ + --perry target/debug/perry \ + --repeat 2 \ + --jobs 4 + - name: Promotion census run: | python3 scripts/compiler_output_regression.py census \ diff --git a/scripts/compiler_output_harness/cli.py b/scripts/compiler_output_harness/cli.py index c8148207e3..7b6d5a0e89 100644 --- a/scripts/compiler_output_harness/cli.py +++ b/scripts/compiler_output_harness/cli.py @@ -11,6 +11,9 @@ from .repsel_determinism import self_test as determinism_self_test from .repsel_knob_isolation import check_isolation from .repsel_knob_isolation import self_test as isolation_self_test +from .repsel_temp_hygiene import DEFAULT_REPEAT as DEFAULT_HYGIENE_REPEAT +from .repsel_temp_hygiene import check_temp_hygiene +from .repsel_temp_hygiene import self_test as temp_hygiene_self_test from .spec import WORKLOADS @@ -179,6 +182,33 @@ def build_parser() -> argparse.ArgumentParser: ) det_self_p.set_defaults(func=determinism_self_test) + # Temp-directory hygiene (#7144). The other half of the #7131 story: the + # `.ll` name became a function of the IR, which meant workers shared it, + # which meant nothing deleted it — one file per distinct IR ever compiled, + # forever, on every developer machine. + hyg_p = sub.add_parser( + "census-temp-hygiene", + help="assert compiling leaves no files behind in the temp directory", + ) + hyg_p.add_argument("--perry") + hyg_p.add_argument("--baseline") + hyg_p.add_argument("--workload", action="append", help="restrict to named workload(s)") + hyg_p.add_argument( + "--repeat", + type=int, + default=DEFAULT_HYGIENE_REPEAT, + help="compiles per workload; >1 puts identical IR in flight concurrently", + ) + hyg_p.add_argument("--compile-timeout", type=int, default=300) + hyg_p.add_argument("--jobs", type=int, default=4, help="parallel compiles") + hyg_p.set_defaults(func=check_temp_hygiene) + + hyg_self_p = sub.add_parser( + "census-temp-hygiene-self-test", + help="check the temp-hygiene verdict logic without compiling", + ) + hyg_self_p.set_defaults(func=temp_hygiene_self_test) + return parser diff --git a/scripts/compiler_output_harness/repsel_temp_hygiene.py b/scripts/compiler_output_harness/repsel_temp_hygiene.py new file mode 100644 index 0000000000..0fe8044629 --- /dev/null +++ b/scripts/compiler_output_harness/repsel_temp_hygiene.py @@ -0,0 +1,231 @@ +"""Temp-directory hygiene check (#7144). + +`compile_ll_to_object` writes the module's LLVM IR to a temp `.ll`, hands the +path to `clang -c`, and reads back the object. #7131 made that name a pure +function of the IR — it had to, because clang records a translation unit's +source basename into the ELF object — and #7135, shipping that fix, stopped +deleting the file: workers holding identical IR now *shared* the path, so a +per-call unlink could race a sibling that had computed the path but not yet +opened it. + +Nothing else deleted them. The file count is bounded by the number of +**distinct IR contents ever compiled** on the machine, which sounds benign +until you notice that iterating on the compiler changes the IR on essentially +every rebuild: + + leftover perry_llvm_*.ll files: 1627, total 951.8 MB (one dev box, one day) + ~29,000 files, 29 GB (another, a month) + +#7144 removed the sharing instead of the deletion: the `.ll` now lives in a +directory that belongs to one call, so unlinking it is unobservable to anyone +else, and the *basename* clang records is untouched — `census-determinism` is +the check that the second half still holds. + +What this module checks is the first half, end to end, on the real compiler: +compile the census corpus with `TMPDIR` pointed at an empty directory of our +own, then look in that directory. It must be empty. + +Two design notes, because both alternatives were tried and are wrong: + +* **"No growth run-over-run" is not the property.** Compiling the same corpus + twice leaves the same content-addressed names, so a repeat-and-compare check + is *green on the broken compiler*. Growth needs new IR, which is why the leak + was invisible in CI and only ever showed up on developer machines. The + property that goes red immediately is the absolute one: nothing left at all. +* **`TMPDIR` isolation is not politeness, it is what makes the check sound.** + Counting entries in the shared system temp directory measures every other + process on the box — on a machine running several compiles at once, that is + noise large enough to swamp the signal in either direction. + +The `PERRY_DEBUG_SYMBOLS` layout is deliberately exempt: under `-g` the object's +DWARF names the `.ll` by absolute path, so the file has to outlive the compile. +This check runs without it, which is how every other gate compiles too. +""" + +from __future__ import annotations + +import argparse +import platform +import shutil +import tempfile +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from typing import Any, Callable + +from .capture import resolve_perry +from .common import HarnessError, REPO_ROOT +from .repsel_census import DEFAULT_BASELINE, compile_and_census, load_baseline + + +#: Compiles per workload. Two, not one, so the corpus contains pairs of +#: compiles with *identical* IR — the case that shares a content-addressed name +#: and the reason #7135 could not simply keep deleting the file. They run +#: concurrently for the same reason (#7140: a serial check never opens the +#: window). +DEFAULT_REPEAT = 2 + +#: Cap on how many leftover paths a failure prints. A leaking compiler leaks one +#: per compile, and 52 lines of the same shape teaches nothing the first 12 did +#: not. +MAX_REPORTED = 12 + + +def leftovers_under(root: Path) -> list[str]: + """Every path under `root`, relative to it, deepest entries first. + + Directories are reported too: the pre-#7144 failure is a stray *file*, but + a half-finished cleanup that leaves empty scratch directories behind is the + same defect wearing a smaller coat, and it is just as unbounded. + """ + if not root.exists(): + raise HarnessError(f"the isolated temp root vanished during the run: {root}") + found = [p.relative_to(root).as_posix() for p in root.rglob("*")] + found.sort() + return found + + +def verdict( + leftovers: list[str], + *, + compiles: int, + printer: Callable[[str], None] = print, +) -> int: + """Turn "what was left in the temp dir" into an exit code. + + Split from the compile loop so the decision can be exercised without a + compiler (CLAUDE.md failure mode 4: a gate nobody can re-check is not a + gate), and so a run that compiled *nothing* is a harness error rather than + a green line — an empty directory is exactly what you get from doing no + work at all. + """ + if compiles <= 0: + raise HarnessError( + "no compiles ran, so an empty temp directory proves nothing; " + "this run checked nothing" + ) + if not leftovers: + printer( + f"Temp directory is clean: {compiles} compile(s) left 0 files behind. " + "The #7144 leak is not present." + ) + return 0 + + shown = leftovers[:MAX_REPORTED] + printer( + f"TEMP FILES LEAKED: {compiles} compile(s) left {len(leftovers)} " + f"entr{'y' if len(leftovers) == 1 else 'ies'} in a temp directory that " + "started empty.\n" + ) + for name in shown: + printer(f" {name}") + if len(leftovers) > len(shown): + printer(f" … and {len(leftovers) - len(shown)} more") + printer( + "\n" + " This is #7144. The `.ll` handed to `clang -c` is content-addressed\n" + " (#7131 — clang records its basename into the ELF object), so the\n" + " leftovers are bounded by DISTINCT IR EVER COMPILED, not by compiles:\n" + " a repeat-and-compare check stays green while a developer machine\n" + " fills up. Measured before the fix: 1627 files / 951.8 MB after a day\n" + " of compiler work; 29 GB on a longer-lived box.\n" + "\n" + " The fix is not a more careful unlink — that races a sibling worker\n" + " holding the same IR, which is why #7135 stopped deleting at all. It\n" + " is to stop sharing: `crates/perry-codegen/src/linker.rs` gives each\n" + " compile a private scratch directory and removes it on success, while\n" + " the basename inside it stays a pure function of the IR so\n" + " `census-determinism` keeps passing.\n" + "\n" + " Exempt by design: `PERRY_DEBUG_SYMBOLS` builds keep their `.ll` —\n" + " the object's DWARF names it by absolute path. If this fired under\n" + " `-g`, that is the expected behaviour and not this gate's business." + ) + return 1 + + +def check_temp_hygiene(args: argparse.Namespace) -> int: + """Compile the census corpus with an isolated `TMPDIR` and inspect it.""" + perry = resolve_perry(getattr(args, "perry", None)) + baseline = load_baseline(Path(args.baseline) if args.baseline else DEFAULT_BASELINE) + workloads: list[dict[str, Any]] = baseline["workloads"] + if getattr(args, "workload", None): + wanted = set(args.workload) + workloads = [w for w in workloads if w["name"] in wanted] + missing = wanted - {w["name"] for w in workloads} + if missing: + raise HarnessError(f"unknown workload(s): {', '.join(sorted(missing))}") + if not workloads: + raise HarnessError("no workloads selected") + + repeat = max(1, int(args.repeat)) + + print("Temp-directory hygiene (#7144)") + print("==============================\n") + print(f"compiler: {' '.join(perry)}") + print(f"host: {platform.system()} {platform.machine()}") + print(f"corpus: {len(workloads)} workload(s) x {repeat} compile(s)\n") + + outer = Path(tempfile.mkdtemp(prefix="repsel-temp-hygiene-")) + # The directory the *compiler* will treat as its temp dir. Separate from + # `outer` so the harness's own scratch never counts as a leftover. + isolated = outer / "compiler-tmp" + isolated.mkdir() + try: + jobs = [(w, i) for w in workloads for i in range(repeat)] + + def run(job: tuple[dict[str, Any], int]) -> None: + workload, _index = job + compile_and_census( + perry, + REPO_ROOT / workload["source"], + timeout=args.compile_timeout, + # TMP/TEMP alongside TMPDIR so the check means the same thing + # if this ever runs on Windows, where `env::temp_dir()` reads + # those instead. + extra_env={ + "TMPDIR": str(isolated), + "TMP": str(isolated), + "TEMP": str(isolated), + }, + ) + + with ThreadPoolExecutor(max_workers=max(1, args.jobs)) as pool: + list(pool.map(run, jobs)) + + left = leftovers_under(isolated) + return verdict(left, compiles=len(jobs)) + finally: + shutil.rmtree(outer, ignore_errors=True) + + +def self_test(_args: argparse.Namespace) -> int: + """Prove the verdict can go red, and that it refuses a vacuous run.""" + quiet: Callable[[str], None] = lambda _line: None + + assert verdict([], compiles=52, printer=quiet) == 0 + assert verdict(["perry_llvm_2791e842224ea99c.ll"], compiles=52, printer=quiet) == 1 + # An empty scratch directory left behind is the same defect, smaller. + assert verdict(["perry_llvm_scratch_1a2b_0"], compiles=1, printer=quiet) == 1 + + # A run that compiled nothing finds an empty directory for the wrong + # reason. It must not be able to report success. + try: + verdict([], compiles=0, printer=quiet) + except HarnessError: + pass + else: # pragma: no cover - the raise below is the failure report + raise AssertionError("verdict() called a zero-compile run clean") + + lines: list[str] = [] + verdict(["a.ll", "b.ll"], compiles=2, printer=lines.append) + report = "\n".join(lines) + for expected in ("#7144", "#7131", "PERRY_DEBUG_SYMBOLS", "linker.rs"): + assert expected in report, f"failure report must mention {expected}: {report}" + + # The truncation must announce itself rather than quietly dropping paths. + lines = [] + verdict([f"f{i}.ll" for i in range(MAX_REPORTED + 5)], compiles=1, printer=lines.append) + assert "and 5 more" in "\n".join(lines) + + print("repsel temp-hygiene self-test OK") + return 0 From 6033b449e85401b47d56242190ce37c9880bdebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:30:49 +0200 Subject: [PATCH 04/11] style: rustfmt Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- crates/perry-codegen/src/linker.rs | 5 ++++- crates/perry-codegen/src/linker_temp_lifecycle_tests.rs | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/perry-codegen/src/linker.rs b/crates/perry-codegen/src/linker.rs index dc7e0ed723..f2d100e7aa 100644 --- a/crates/perry-codegen/src/linker.rs +++ b/crates/perry-codegen/src/linker.rs @@ -1840,7 +1840,10 @@ mod tests { ); // …and the staging file must never be mistaken for the real `.ll`. - assert_ne!(ll_staging_path(ll_p1, 1111, 0).file_name(), ll_p1.file_name()); + assert_ne!( + ll_staging_path(ll_p1, 1111, 0).file_name(), + ll_p1.file_name() + ); } #[test] diff --git a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs index c47bd6e0c2..77815dcb76 100644 --- a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs +++ b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs @@ -279,7 +279,9 @@ fn ll_files_under(root: &Path) -> Vec { let mut found = Vec::new(); let mut stack = vec![root.to_path_buf()]; while let Some(dir) = stack.pop() { - let Ok(read) = fs::read_dir(&dir) else { continue }; + let Ok(read) = fs::read_dir(&dir) else { + continue; + }; for entry in read.flatten() { let path = entry.path(); if path.is_dir() { From 8dc46616c9d1bbf443b0d834a8f39ab4d635fd47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:34:34 +0200 Subject: [PATCH 05/11] docs(census): document the temp-hygiene gate and the -g exemption (#7144) Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- benchmarks/repsel_census/README.md | 41 ++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/benchmarks/repsel_census/README.md b/benchmarks/repsel_census/README.md index 0a54f586b0..2f203b6f01 100644 --- a/benchmarks/repsel_census/README.md +++ b/benchmarks/repsel_census/README.md @@ -286,6 +286,47 @@ canonical-i32 draws from. A withdrawn proof cannot be selected. A knob that `--jobs` compiles arms in parallel; the whole corpus × 6 knobs × 4 arms is about 20 s on an M1. +## Temp-directory hygiene (#7144) + +The other consequence of content-addressing that `.ll`: two workers holding +identical IR now *share* the path, so a per-call unlink can race a sibling that +computed the path but has not yet handed it to clang. #7135 responded by not +deleting it — and then nothing did. The leftovers are bounded by **distinct IR +ever compiled** on the machine, which is fine in CI (runner temp dirs are +reclaimed) and not fine anywhere a compiler is being worked on, where the IR +changes on every rebuild: 1627 files / 951.8 MB after one day on one dev box, +~29 GB on another. + +#7144 removed the sharing rather than the deletion. Each `.ll` → `.o` compile +gets a private directory under the temp root and removes it on success; the +*basename* inside it is still a pure function of the IR, so the determinism +property above is untouched — the directory is not recorded in the object, only +the basename is. + +```bash +python3 scripts/compiler_output_regression.py census-temp-hygiene \ + --perry --repeat 2 --jobs 4 +``` + +It compiles the corpus with `TMPDIR` pointed at an empty directory of its own +and asserts the directory is still empty. Two things worth knowing before +changing it: + +* **"No growth run-over-run" would have been green on the broken compiler.** + Compiling the same corpus twice produces the same content-addressed names, so + a repeat-and-compare check sees a flat count while the machine fills up. The + property that goes red is the absolute one: *nothing* left behind. +* **The `TMPDIR` isolation is load-bearing**, not politeness. Counting entries + in the shared system temp dir measures every other process on the box. + +Exempt on purpose: under `PERRY_DEBUG_SYMBOLS` clang gets `-g` and puts the +`.ll`'s **absolute** path into DWARF, so those builds keep their `.ll` at a +stable, content-addressed path in the temp root — deleting it would ship debug +info pointing at a file that no longer exists. That layout is bounded by +distinct IR compiled with `-g`, and `debug_symbols_layout_and_g_flag_agree` pins +the two halves of the decision together so a future edit cannot delete a file +DWARF still names. + ## Editing the fixtures Don't tidy them. Every one is written against a specific collector's rules and From 7205959a1f7a6c698ea0c5e46e74d10c6532a16a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 08:47:10 +0200 Subject: [PATCH 06/11] test(harness): scope temp-hygiene to the clang driver's own files; report the driver's perry-objs leak Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- .../repsel_temp_hygiene.py | 95 ++++++++++++++++--- 1 file changed, 84 insertions(+), 11 deletions(-) diff --git a/scripts/compiler_output_harness/repsel_temp_hygiene.py b/scripts/compiler_output_harness/repsel_temp_hygiene.py index 0fe8044629..dea2bbd6dc 100644 --- a/scripts/compiler_output_harness/repsel_temp_hygiene.py +++ b/scripts/compiler_output_harness/repsel_temp_hygiene.py @@ -69,6 +69,34 @@ #: not. MAX_REPORTED = 12 +#: Every temp name `crates/perry-codegen/src/linker.rs` creates — the `.ll`/`.o` +#: pair and its scratch directory (`perry_llvm_*`), the multi-codegen-unit +#: staging objects (`perry_cgu_*`, #5391), and the bitcode-link intermediates +#: (`perry_bc_*`). This gate's subject is that module's file lifecycle, so it +#: fails on these and only these. +#: +#: Anything else found is REPORTED, loudly, and does not fail: as of #7144 the +#: compile driver leaks a `perry-objs--/` staging directory on the +#: `--no-link` path (`run_pipeline.rs` removes it on both *link* exits and there +#: is no third one), which is a real defect but a different module's, and a gate +#: that goes red for someone else's bug gets muted rather than fixed. Widen this +#: to "nothing at all" once the driver's path is closed. +OWNED_PREFIXES = ("perry_llvm", "perry_cgu", "perry_bc") + + +def classify(leftovers: list[str]) -> tuple[list[str], list[str]]: + """Split leftovers into "this gate's subject" and "somebody else's". + + Classified on the FIRST path component: everything under a leaked scratch + directory is leaked by whoever leaked the directory. + """ + owned: list[str] = [] + other: list[str] = [] + for rel in leftovers: + top = rel.split("/", 1)[0] + (owned if top.startswith(OWNED_PREFIXES) else other).append(rel) + return owned, other + def leftovers_under(root: Path) -> list[str]: """Every path under `root`, relative to it, deepest entries first. @@ -103,23 +131,46 @@ def verdict( "no compiles ran, so an empty temp directory proves nothing; " "this run checked nothing" ) - if not leftovers: + owned, other = classify(leftovers) + + if other: + shown = other[:MAX_REPORTED] + printer( + f"Not this gate's subject — {len(other)} entr" + f"{'y' if len(other) == 1 else 'ies'} left by a module other than " + "perry-codegen's clang driver:" + ) + for name in shown: + printer(f" {name}") + if len(other) > len(shown): + printer(f" … and {len(other) - len(shown)} more") + printer( + " `perry-objs--/` is the compile driver's object " + "staging dir;\n" + " `run_pipeline.rs` removes it on both *link* exits and `--no-link` " + "returns\n" + " before either. Reported, not failed: a gate that goes red for " + "another\n" + " module's defect gets muted rather than fixed.\n" + ) + + if not owned: printer( - f"Temp directory is clean: {compiles} compile(s) left 0 files behind. " - "The #7144 leak is not present." + f"Temp directory is clean: {compiles} compile(s) left 0 clang-driver " + "files behind. The #7144 leak is not present." ) return 0 - shown = leftovers[:MAX_REPORTED] + shown = owned[:MAX_REPORTED] printer( - f"TEMP FILES LEAKED: {compiles} compile(s) left {len(leftovers)} " - f"entr{'y' if len(leftovers) == 1 else 'ies'} in a temp directory that " + f"TEMP FILES LEAKED: {compiles} compile(s) left {len(owned)} " + f"entr{'y' if len(owned) == 1 else 'ies'} in a temp directory that " "started empty.\n" ) for name in shown: printer(f" {name}") - if len(leftovers) > len(shown): - printer(f" … and {len(leftovers) - len(shown)} more") + if len(owned) > len(shown): + printer(f" … and {len(owned) - len(shown)} more") printer( "\n" " This is #7144. The `.ll` handed to `clang -c` is content-addressed\n" @@ -206,6 +257,24 @@ def self_test(_args: argparse.Namespace) -> int: assert verdict(["perry_llvm_2791e842224ea99c.ll"], compiles=52, printer=quiet) == 1 # An empty scratch directory left behind is the same defect, smaller. assert verdict(["perry_llvm_scratch_1a2b_0"], compiles=1, printer=quiet) == 1 + # …and a file inside one is attributed to whoever leaked the directory. + assert verdict(["perry_llvm_scratch_1a2b_0/x.ll"], compiles=1, printer=quiet) == 1 + for owned in ("perry_cgu_1_2_0.o", "perry_bc_1_2_linked.bc"): + assert verdict([owned], compiles=1, printer=quiet) == 1, owned + + # Another module's leftovers are reported, not failed — see OWNED_PREFIXES. + assert verdict(["perry-objs-9-1/m.o"], compiles=1, printer=quiet) == 0 + lines: list[str] = [] + assert verdict(["perry-objs-9-1/m.o"], compiles=1, printer=lines.append) == 0 + joined = "\n".join(lines) + assert "perry-objs" in joined and "run_pipeline.rs" in joined, joined + # A mixture still fails, and the failure is about the owned half. + assert verdict(["perry-objs-9-1/m.o", "perry_llvm_a.ll"], compiles=1, printer=quiet) == 1 + + assert classify(["perry_llvm_a.ll", "perry-objs-9-1/m.o"]) == ( + ["perry_llvm_a.ll"], + ["perry-objs-9-1/m.o"], + ) # A run that compiled nothing finds an empty directory for the wrong # reason. It must not be able to report success. @@ -216,15 +285,19 @@ def self_test(_args: argparse.Namespace) -> int: else: # pragma: no cover - the raise below is the failure report raise AssertionError("verdict() called a zero-compile run clean") - lines: list[str] = [] - verdict(["a.ll", "b.ll"], compiles=2, printer=lines.append) + lines = [] + verdict(["perry_llvm_a.ll", "perry_llvm_b.ll"], compiles=2, printer=lines.append) report = "\n".join(lines) for expected in ("#7144", "#7131", "PERRY_DEBUG_SYMBOLS", "linker.rs"): assert expected in report, f"failure report must mention {expected}: {report}" # The truncation must announce itself rather than quietly dropping paths. lines = [] - verdict([f"f{i}.ll" for i in range(MAX_REPORTED + 5)], compiles=1, printer=lines.append) + verdict( + [f"perry_llvm_{i}.ll" for i in range(MAX_REPORTED + 5)], + compiles=1, + printer=lines.append, + ) assert "and 5 more" in "\n".join(lines) print("repsel temp-hygiene self-test OK") From 868bca491a886e1b722d9b44991815258b6510d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 09:00:36 +0200 Subject: [PATCH 07/11] =?UTF-8?q?fix(codegen):=20delete=20the=20PERRY=5FDE?= =?UTF-8?q?BUG=5FSYMBOLS=20exemption=20=E2=80=94=20-g=20records=20nothing?= =?UTF-8?q?=20about=20the=20.ll?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/perry-codegen/src/linker.rs | 183 ++++-------- .../src/linker_temp_lifecycle_tests.rs | 279 +++++++++++------- 2 files changed, 240 insertions(+), 222 deletions(-) diff --git a/crates/perry-codegen/src/linker.rs b/crates/perry-codegen/src/linker.rs index f2d100e7aa..ef12529cc2 100644 --- a/crates/perry-codegen/src/linker.rs +++ b/crates/perry-codegen/src/linker.rs @@ -44,49 +44,25 @@ static CLANG_PROBE: OnceLock> = OnceLock::new(); /// /// That is the whole reason only the `.ll` basename had to change: the counter /// may stay in every *output* name, where it costs nothing and still closes -/// #509. The one caveat is `PERRY_DEBUG_SYMBOLS`, which adds `-g` and pulls the -/// absolute `.ll` path plus `DW_AT_comp_dir` into DWARF — objects built with it -/// are reproducible only for a fixed `TMPDIR` and working directory. That -/// caveat is also why the two layouts in `LlLayout` exist. -static TEMP_NONCE_COUNTER: AtomicU64 = AtomicU64::new(0); - -/// Which temp-file layout one `.ll` uses — and, inseparably, whether the file -/// survives the compile (#7144). -/// -/// The rule, stated once: **the `.ll` lives at a path whose stability matches -/// what the emitted object records about it.** +/// #509. And because the *directory* is recorded nowhere, #7144 could put every +/// compile's `.ll` in a directory of its own and delete it again. /// -/// * `Scratch` (default): the object records the translation unit's *basename* -/// and nothing else — the table above, measured, not assumed. So the basename -/// must be content-addressed and the *directory* is free. Give every call its -/// own directory and remove it when the compile succeeds. Nothing is shared, -/// so the unlink cannot race a sibling worker that computed the same content -/// hash. That race is why #7135 stopped deleting the `.ll` at all, and why -/// #7144 then measured one leftover per *distinct IR ever compiled* — 1627 -/// files / 951.8 MB on one dev box after a day, 29 GB on another. -/// * `DebugShared` (`PERRY_DEBUG_SYMBOLS`): clang also gets `-g`, which puts -/// the `.ll`'s **absolute** path plus `DW_AT_comp_dir` into DWARF. The file is -/// then referenced by the shipped object, so it must (a) keep a stable -/// absolute path — flat in the temp root, content-addressed, exactly the -/// pre-#7144 layout — and (b) outlive the compile, or a debugger stepping -/// into generated code has nothing to resolve. These are retained on purpose. -/// They are bounded by the distinct IR compiled *with `-g`*, and a debug build -/// asking for debug info is the one case where keeping the source is the -/// point rather than a leak. -/// -/// The layout and clang's `-g` flag must never disagree: deleting a `.ll` that -/// DWARF names by absolute path silently breaks source-level debugging. Both -/// are therefore derived from one `TempFilePolicy` value at a single call site, -/// and `debug_symbols_layout_and_g_flag_agree` pins that. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum LlLayout { - /// Per-call directory, removed on success. - Scratch, - /// Flat, content-addressed, retained — the object points at it. - DebugShared, -} +/// **`PERRY_DEBUG_SYMBOLS` is not an exception**, contrary to what the comment +/// here used to say. `-g` was assumed to pull the `.ll`'s absolute path plus +/// `DW_AT_comp_dir` into DWARF, which would have made the file part of the +/// shipped object and forced it to persist at a stable path. 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 at all**. Perry's codegen +/// emits no `DICompileUnit`/`DIFile`/`!dbg` metadata, and `clang -g` on a `.ll` +/// lowers debug info that is in the IR rather than synthesising a compile unit +/// for the input file. So `-g` records nothing about where the `.ll` lived, and +/// the temp-file lifetime does not depend on it — see +/// `debug_symbols_do_not_change_what_the_object_records`. (Not measured on +/// COFF/Windows.) +static TEMP_NONCE_COUNTER: AtomicU64 = AtomicU64::new(0); -/// The two environment inputs that decide what happens to the temp files. +/// The environment inputs that decide what happens to the temp files. /// /// Read once, in `compile_ll_to_object`, and threaded down rather than probed /// where they are used: the lifecycle is then testable without a test mutating @@ -94,9 +70,11 @@ enum LlLayout { #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct TempFilePolicy { /// `PERRY_LLVM_KEEP_IR` — retain every intermediate and write the compile - /// plan alongside it. + /// plan alongside it. The only input that changes the files' lifetime. keep: bool, - /// `PERRY_DEBUG_SYMBOLS` — clang gets `-g`; see `LlLayout::DebugShared`. + /// `PERRY_DEBUG_SYMBOLS` — clang gets `-g`. Carried here only so the flag + /// is a parameter rather than an env probe buried in plan construction; it + /// deliberately does **not** affect cleanup (see `TEMP_NONCE_COUNTER`). debug_symbols: bool, } @@ -107,14 +85,6 @@ impl TempFilePolicy { debug_symbols: env::var_os("PERRY_DEBUG_SYMBOLS").is_some(), } } - - fn layout(self) -> LlLayout { - if self.debug_symbols { - LlLayout::DebugShared - } else { - LlLayout::Scratch - } - } } /// FNV-1a 64-bit over `ll_text`. Stable across platforms and rustc versions @@ -149,40 +119,26 @@ fn ll_content_hash(ll_text: &str) -> u64 { /// same-source compiles failed with "Failed to read clang output … No such /// file or directory". This is #509 again, one scope out. /// -/// Under `LlLayout::Scratch` the two names additionally sit inside a directory -/// that belongs to this call alone (#7144). The directory name is *not* -/// content-addressed — it must not be, or two callers would share it again — -/// and it does not have to be: only the basename reaches the object. +/// Both names additionally sit inside a `scratch_dir` that belongs to this call +/// alone (#7144). The directory name is *not* content-addressed — it must not +/// be, or two callers would share it again and the unlink would be racing a +/// sibling, which is the corner #7135 painted itself into. And it does not have +/// to be: only the basename reaches the object. /// /// `pid` and `counter` are parameters rather than read in here so the property /// above is testable without spawning processes. -fn llvm_temp_paths_for( - tmp_dir: &Path, - ll_text: &str, - pid: u32, - counter: u64, - layout: LlLayout, -) -> LlvmTempPaths { +fn llvm_temp_paths_for(tmp_dir: &Path, ll_text: &str, pid: u32, counter: u64) -> LlvmTempPaths { let hash = ll_content_hash(ll_text); let ll_name = format!("perry_llvm_{hash:016x}.ll"); // The `.o` keeps its uniquifiers even inside a private directory. They cost // nothing, and the day someone flattens the layout again the object must // not silently go back to colliding across processes (#7140). let obj_name = format!("perry_llvm_{hash:016x}_{pid:x}_{counter:x}.o"); - match layout { - LlLayout::Scratch => { - let scratch = tmp_dir.join(format!("perry_llvm_scratch_{pid:x}_{counter:x}")); - LlvmTempPaths { - ll_path: scratch.join(&ll_name), - obj_path: scratch.join(&obj_name), - scratch_dir: Some(scratch), - } - } - LlLayout::DebugShared => LlvmTempPaths { - ll_path: tmp_dir.join(&ll_name), - obj_path: tmp_dir.join(&obj_name), - scratch_dir: None, - }, + let scratch = tmp_dir.join(format!("perry_llvm_scratch_{pid:x}_{counter:x}")); + LlvmTempPaths { + ll_path: scratch.join(&ll_name), + obj_path: scratch.join(&obj_name), + scratch_dir: scratch, } } @@ -190,9 +146,8 @@ fn llvm_temp_paths_for( #[derive(Debug, Clone)] struct LlvmTempPaths { /// Directory owned exclusively by this call, removed once the object bytes - /// have been read. `None` under `LlLayout::DebugShared`, where the `.ll` is - /// shared between callers and deliberately retained. - scratch_dir: Option, + /// have been read. + scratch_dir: PathBuf, ll_path: PathBuf, obj_path: PathBuf, } @@ -200,11 +155,11 @@ struct LlvmTempPaths { /// `llvm_temp_paths_for` with this process's pid and the next counter value. /// Returns the paths plus `(pid, counter)` — the last two also name the /// atomic-write staging file. -fn llvm_temp_paths(tmp_dir: &Path, ll_text: &str, layout: LlLayout) -> (LlvmTempPaths, u32, u64) { +fn llvm_temp_paths(tmp_dir: &Path, ll_text: &str) -> (LlvmTempPaths, u32, u64) { let pid = std::process::id(); let counter = TEMP_NONCE_COUNTER.fetch_add(1, Ordering::Relaxed); ( - llvm_temp_paths_for(tmp_dir, ll_text, pid, counter, layout), + llvm_temp_paths_for(tmp_dir, ll_text, pid, counter), pid, counter, ) @@ -441,10 +396,10 @@ fn build_clang_compile_plan( }; let mut clang_args = vec!["-c".to_string(), opt_flag.to_string()]; - // `-g` and `LlLayout::DebugShared` are two consequences of ONE decision - // (`TempFilePolicy::debug_symbols`) and are passed the same value from the - // same call site. If they ever disagree, we delete a `.ll` that DWARF names - // by absolute path and source-level debugging breaks silently (#7144). + // A parameter rather than an env probe so a test can pin what `-g` does + // and does not reach — measured in #7144: on a Perry `.ll` it produces a + // byte-identical object with no `.debug_*` sections, because Perry's + // codegen emits no DI metadata for clang to lower. See `TEMP_NONCE_COUNTER`. if debug_symbols { clang_args.push("-g".to_string()); } @@ -517,16 +472,16 @@ pub fn compile_ll_to_object(ll_text: &str, target_triple: Option<&str>) -> Resul /// /// Cleanup policy, in one place: /// -/// * **success**: the object is read into memory and removed; under -/// `LlLayout::Scratch` the whole per-call directory goes with it, so a -/// compile leaves nothing behind (#7144). +/// * **success**: the object is read into memory, and the per-call directory +/// goes with it — a compile leaves nothing behind (#7144). /// * **failure** (clang non-zero, or the object cannot be read): everything is /// left on disk. The error message names the `.ll`, and a failed compile is /// exactly when someone wants to look at the IR that produced it. /// * **`PERRY_LLVM_KEEP_IR`**: everything is kept and its location printed, /// plus the compile plan as JSON. -/// * **`PERRY_DEBUG_SYMBOLS`**: the `.ll` is kept even on success — the object's -/// DWARF references it by absolute path. See `LlLayout`. +/// * **`PERRY_DEBUG_SYMBOLS`**: no effect on any of the above. It was believed +/// to put the `.ll`'s absolute path into DWARF; measured, it does not put +/// anything there at all. See `TEMP_NONCE_COUNTER`. fn compile_ll_to_object_in( tmp_dir: &Path, ll_text: &str, @@ -562,17 +517,14 @@ fn compile_ll_to_object_in( // object on ELF). #509: keep the `.o` unique via the per-call counter. // #7144: put both under a directory this call owns, so the `.ll` can be // deleted again without racing a sibling that hashed the same IR. - let layout = policy.layout(); - let (paths, write_pid, write_nonce) = llvm_temp_paths(tmp_dir, ll_text, layout); + let (paths, write_pid, write_nonce) = llvm_temp_paths(tmp_dir, ll_text); let LlvmTempPaths { scratch_dir, ll_path, obj_path, } = paths; - if let Some(dir) = &scratch_dir { - fs::create_dir_all(dir) - .with_context(|| format!("Failed to create temp dir at {}", dir.display()))?; - } + fs::create_dir_all(&scratch_dir) + .with_context(|| format!("Failed to create temp dir at {}", scratch_dir.display()))?; write_ll_atomically(&ll_path, ll_text, write_pid, write_nonce)?; let plan = build_clang_compile_plan( @@ -680,20 +632,13 @@ fn compile_ll_to_object_in( metadata_path.display() ); } else { - let _ = fs::remove_file(&plan.obj_path); - match &scratch_dir { - // Ours alone: `remove_dir_all` rather than unlinking the two names - // we know about, so anything clang chose to drop beside them goes - // too and the directory cannot survive as an empty husk. - Some(dir) => { - let _ = fs::remove_dir_all(dir); - } - // `LlLayout::DebugShared`: the object's DWARF names this `.ll` by - // absolute path. Deleting it would leave a debug build that cannot - // show the source it was built from — the one case where retaining - // the IR is the feature. - None => {} - } + // Ours alone: `remove_dir_all` rather than unlinking the two names we + // know about, so anything clang chose to drop beside them goes too and + // the directory cannot survive as an empty husk. + // + // Unconditional, including under `PERRY_DEBUG_SYMBOLS`: `-g` records + // nothing about this path, measured — see `TEMP_NONCE_COUNTER`. + let _ = fs::remove_dir_all(&scratch_dir); } Ok(bytes) @@ -1750,8 +1695,8 @@ mod tests { // basename still differs via the counter. let tmp = env::temp_dir(); let ir = "define void @f() {\n ret void\n}\n"; - let (a, _, _) = llvm_temp_paths(&tmp, ir, LlLayout::Scratch); - let (b, _, _) = llvm_temp_paths(&tmp, ir, LlLayout::Scratch); + let (a, _, _) = llvm_temp_paths(&tmp, ir); + let (b, _, _) = llvm_temp_paths(&tmp, ir); let (ll_a, obj_a) = (&a.ll_path, &a.obj_path); let (ll_b, obj_b) = (&b.ll_path, &b.obj_path); assert_eq!( @@ -1765,11 +1710,7 @@ mod tests { ".o basenames must stay unique across calls (#509)" ); // Different IR → different .ll basename. - let (c, _, _) = llvm_temp_paths( - &tmp, - "define void @g() {\n ret void\n}\n", - LlLayout::Scratch, - ); + let (c, _, _) = llvm_temp_paths(&tmp, "define void @g() {\n ret void\n}\n"); assert_ne!(ll_a.file_name(), c.ll_path.file_name()); // No pid / wall-clock digits of variable width — only hex hash. let name = ll_a.file_name().unwrap().to_string_lossy(); @@ -1802,8 +1743,8 @@ mod tests { let ir = "define void @f() {\n ret void\n}\n"; // Same IR, same counter, DIFFERENT process. - let p1 = llvm_temp_paths_for(&tmp, ir, 1111, 0, LlLayout::Scratch); - let p2 = llvm_temp_paths_for(&tmp, ir, 2222, 0, LlLayout::Scratch); + let p1 = llvm_temp_paths_for(&tmp, ir, 1111, 0); + let p2 = llvm_temp_paths_for(&tmp, ir, 2222, 0); let (ll_p1, obj_p1) = (&p1.ll_path, &p1.obj_path); let (ll_p2, obj_p2) = (&p2.ll_path, &p2.obj_path); assert_eq!( @@ -1821,8 +1762,8 @@ mod tests { // Same process, different call: the counter still has to separate // in-process rayon workers. - let c0 = llvm_temp_paths_for(&tmp, ir, 1111, 0, LlLayout::Scratch); - let c1 = llvm_temp_paths_for(&tmp, ir, 1111, 1, LlLayout::Scratch); + let c0 = llvm_temp_paths_for(&tmp, ir, 1111, 0); + let c1 = llvm_temp_paths_for(&tmp, ir, 1111, 1); assert_ne!(c0.obj_path.file_name(), c1.obj_path.file_name()); // The atomic-write staging name needs the same separation: both diff --git a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs index 77815dcb76..8f1170ecce 100644 --- a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs +++ b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs @@ -13,38 +13,44 @@ use super::*; +// ── Path shape ───────────────────────────────────────────────────────────── + #[test] fn scratch_dir_is_per_call_and_per_process_but_the_ll_basename_is_not() { // #7144's shape, and the reason it does not undo #7131: the *directory* - // carries every uniquifier, the *basename* carries none. clang records - // the basename into the object and nothing else (no `-g`), so the two - // properties do not compete — a call can own its `.ll` outright and - // still emit the same object bytes as any other call with the same IR. + // carries every uniquifier, the *basename* carries none. The object records + // the basename and nothing else, so the two properties do not compete — a + // call can own its `.ll` outright and still emit the same object bytes as + // any other call with the same IR. + // + // This is the structural guarantee the whole fix rests on: no two calls are + // ever handed the same `.ll` path, so the unlink has nothing to race. let tmp = Path::new("/tmp"); let ir = "define void @f() {\n ret void\n}\n"; - let a = llvm_temp_paths_for(tmp, ir, 1111, 0, LlLayout::Scratch); - let b = llvm_temp_paths_for(tmp, ir, 1111, 1, LlLayout::Scratch); // same process - let c = llvm_temp_paths_for(tmp, ir, 2222, 0, LlLayout::Scratch); // other process + let a = llvm_temp_paths_for(tmp, ir, 1111, 0); + let b = llvm_temp_paths_for(tmp, ir, 1111, 1); // same process, next call + let c = llvm_temp_paths_for(tmp, ir, 2222, 0); // other process, same counter - let dirs = [&a, &b, &c].map(|p| { - p.scratch_dir - .clone() - .expect("Scratch layout must allocate a private directory") - }); - assert_ne!(dirs[0], dirs[1], "two calls must not share a directory"); - assert_ne!(dirs[0], dirs[2], "two processes must not share a directory"); + assert_ne!( + a.scratch_dir, b.scratch_dir, + "two calls must not share a directory" + ); + assert_ne!( + a.scratch_dir, c.scratch_dir, + "two processes must not share a directory — every process starts the \ + counter at 0, so only the pid can separate them (#7140)" + ); for p in [&a, &b, &c] { - let dir = p.scratch_dir.as_ref().unwrap(); assert_eq!( p.ll_path.parent(), - Some(dir.as_path()), + Some(p.scratch_dir.as_path()), "the .ll must live inside the directory that gets removed" ); assert_eq!( p.obj_path.parent(), - Some(dir.as_path()), + Some(p.scratch_dir.as_path()), "the .o must go with it, so one remove_dir_all cleans up" ); assert_eq!( @@ -53,75 +59,16 @@ fn scratch_dir_is_per_call_and_per_process_but_the_ll_basename_is_not() { "the recorded name — the basename — must stay content-only (#7131)" ); } + // …and the objects must still not collide, directory or no directory. + assert_ne!(a.obj_path.file_name(), c.obj_path.file_name()); } -#[test] -fn debug_symbols_keep_a_stable_absolute_ll_path() { - // Under `-g` the object names the `.ll` by ABSOLUTE path in DWARF, so - // the whole path has to be a function of the IR (else `-g` builds stop - // being reproducible for a fixed TMPDIR) and the file has to outlive the - // compile (else the debugger has nothing to open). Both follow from - // "no scratch directory": #7144 exempts this layout on purpose. - let tmp = Path::new("/tmp"); - let ir = "define void @f() {\n ret void\n}\n"; - - let a = llvm_temp_paths_for(tmp, ir, 1111, 0, LlLayout::DebugShared); - let b = llvm_temp_paths_for(tmp, ir, 2222, 7, LlLayout::DebugShared); - assert!( - a.scratch_dir.is_none() && b.scratch_dir.is_none(), - "a per-call directory would put pid/counter into DWARF" - ); - assert_eq!( - a.ll_path, b.ll_path, - "the DWARF-referenced path must be content-addressed end to end" - ); - assert_eq!(a.ll_path.parent(), Some(tmp)); - // The object still may not collide across processes (#7140/#509). - assert_ne!(a.obj_path.file_name(), b.obj_path.file_name()); -} - -#[test] -fn debug_symbols_layout_and_g_flag_agree() { - // The hazard this pins: if the layout said "scratch, delete it" while - // clang was still passed `-g`, every debug build would ship DWARF - // pointing at a file that no longer exists — and nothing would fail - // loudly. One `TempFilePolicy` decides both; this is that contract. - let with_g = TempFilePolicy { - keep: false, - debug_symbols: true, - }; - let without = TempFilePolicy { - keep: false, - debug_symbols: false, - }; - assert_eq!(with_g.layout(), LlLayout::DebugShared); - assert_eq!(without.layout(), LlLayout::Scratch); - - for policy in [with_g, without] { - let plan = build_clang_compile_plan( - PathBuf::from("clang"), - PathBuf::from("/tmp/input.ll"), - PathBuf::from("/tmp/output.o"), - None, - 0, - 0, - policy.debug_symbols, - ); - let has_g = plan.clang_args.iter().any(|a| a == "-g"); - assert_eq!( - has_g, - policy.layout() == LlLayout::DebugShared, - "`-g` is passed iff the `.ll` is retained at a stable path: {policy:?}" - ); - } -} - -// ── Temp-file lifecycle (#7144) ──────────────────────────────────────── +// ── Temp-file lifecycle (#7144) ──────────────────────────────────────────── // -// These drive the real `clang`, into a temp root of their own, and assert -// on what is left in that root. A leak is only observable end-to-end: every -// path-shape test above passes just as happily against a compiler that -// never deletes anything, which is exactly how #7144 shipped. +// These drive the real `clang`, into a temp root of their own, and assert on +// what is left in that root. A leak is only observable end-to-end: every +// path-shape test above passes just as happily against a compiler that never +// deletes anything, which is exactly how #7144 shipped. /// A fresh, empty directory to use as the temp root, or `None` when this /// host has no usable clang and the compile step cannot run at all. @@ -319,11 +266,15 @@ fn keep_ir_retains_the_whole_scratch_dir() { } #[test] -fn debug_symbols_retain_the_ll_at_the_path_dwarf_names() { - // (b) of #7144, as executable policy rather than a sentence in a doc: - // `-g` puts the `.ll`'s absolute path into DWARF, so this layout keeps - // the file — flat in the temp root, content-addressed, at exactly the - // path the object points at. The `.o` is still cleaned up. +fn debug_symbols_do_not_change_the_temp_file_lifetime() { + // #7144 question (b), answered by measurement rather than by inheriting the + // premise. `PERRY_DEBUG_SYMBOLS` was believed to make the `.ll` part of the + // shipped object — "`-g` pulls the absolute `.ll` path plus `DW_AT_comp_dir` + // into DWARF" — which would have required keeping the file at a stable path + // for as long as the object lived. It does not: see + // `debug_symbols_do_not_change_what_the_object_records` below and the note + // on `TEMP_NONCE_COUNTER`. So `-g` cleans up like everything else, and there + // is no second layout left sitting untested. let Some(root) = temp_root_if_clang_available("debug") else { return; }; @@ -331,22 +282,148 @@ fn debug_symbols_retain_the_ll_at_the_path_dwarf_names() { keep: false, debug_symbols: true, }; - let ir = test_ir(9); - compile_ll_to_object_in(&root, &ir, None, policy).expect("compile failed"); + for nth in 0..2 { + compile_ll_to_object_in(&root, &test_ir(9 + nth), None, policy) + .unwrap_or_else(|e| panic!("-g compile {nth} failed: {e:#}")); + assert_eq!( + entries(&root), + Vec::::new(), + "a -g compile must leave nothing behind either" + ); + } + let _ = fs::remove_dir_all(&root); +} + +// ── What the object actually records about the `.ll` ─────────────────────── + +/// The property the whole design rests on, as a test rather than a comment. +/// +/// Everything above is safe *because* an emitted object records the `.ll`'s +/// **basename** and never its **directory** — that is what lets a per-call +/// directory coexist with byte-identical emission (#7131). Until now that was +/// measured by hand, once, on a Raspberry Pi (#7140), and written down. A +/// property this load-bearing that no test can restate is one that quietly +/// stops being true. +/// +/// ELF is the format that records the basename at all (Mach-O does not, which +/// is why this defect class is invisible on the machine most of this project's +/// work happens on), so this cross-compiles: the embedding is a property of the +/// ELF writer, not of the host or the arch. +#[test] +fn the_ll_directory_is_not_recorded_in_the_object_but_the_basename_is() { + let Some(root) = temp_root_if_clang_available("elf-record") else { + return; + }; + let clang = find_clang().unwrap(); + let ir = test_ir(1); - let left = entries(&root); - let expected = llvm_temp_paths_for(&root, &ir, 0, 0, LlLayout::DebugShared); - let expected_name = expected.ll_path.file_name().unwrap().to_string_lossy(); + for target in ["x86_64-unknown-linux-gnu", "aarch64-unknown-linux-gnu"] { + // Same basename, two different directories. + let mut objs = Vec::new(); + for dir_name in ["dirA", "dirB"] { + let dir = root.join(format!("{target}-{dir_name}")); + fs::create_dir_all(&dir).unwrap(); + let ll = dir.join("perry_llvm_00000000cafe0001.ll"); + fs::write(&ll, &ir).unwrap(); + let Some(obj) = elf_compile(&clang, &ll, target, &dir) else { + eprintln!("[linker tests] skipping {target}: clang cannot target it"); + return; + }; + objs.push(obj); + } + assert_eq!( + objs[0], objs[1], + "{target}: the .ll's DIRECTORY must not reach the object — a \ + per-call scratch dir would otherwise break emission determinism" + ); + + // Control: the instrument must be able to see a difference at all. If + // this ever stops differing, the assertion above proves nothing. + let dir = root.join(format!("{target}-control")); + fs::create_dir_all(&dir).unwrap(); + let other = dir.join("perry_llvm_00000000cafe0002.ll"); + fs::write(&other, &ir).unwrap(); + let control = elf_compile(&clang, &other, target, &dir).unwrap(); + assert_ne!( + objs[0], control, + "{target}: a different .ll BASENAME must change the object — if it \ + does not, this test cannot detect the directory leaking either" + ); + } + let _ = fs::remove_dir_all(&root); +} + +#[test] +fn debug_symbols_do_not_change_what_the_object_records() { + // The measurement that retired the `PERRY_DEBUG_SYMBOLS` exemption. Perry's + // codegen emits no `DICompileUnit`/`DIFile`/`!dbg` metadata, and `clang -g` + // on a `.ll` lowers debug info that is *in* the IR rather than synthesising + // a compile unit for the input file — so `-g` adds no DWARF, and in + // particular no record of where the `.ll` was. If Perry ever does emit debug + // metadata this goes red, and the temp-file lifetime has to be revisited. + let Some(root) = temp_root_if_clang_available("elf-debug") else { + return; + }; + let clang = find_clang().unwrap(); + let target = "x86_64-unknown-linux-gnu"; + + let dir = root.join("g"); + fs::create_dir_all(&dir).unwrap(); + let ll = dir.join("perry_llvm_00000000cafe0003.ll"); + fs::write(&ll, test_ir(2)).unwrap(); + + let Some(plain) = elf_compile(&clang, &ll, target, &dir) else { + eprintln!("[linker tests] skipping: clang cannot target {target}"); + return; + }; + let with_g = elf_compile_with(&clang, &ll, target, &dir, &["-g"]).unwrap(); assert_eq!( - left, - vec![expected_name.to_string()], - "a -g build must retain exactly the .ll its DWARF references" + plain, with_g, + "-g changed the emitted object. Perry's IR has gained debug metadata, \ + or this clang synthesises a compile unit for .ll input. Either way the \ + `.ll` may now be referenced by the object and #7144's decision to \ + delete it unconditionally has to be re-taken." ); - // …and re-running must not accumulate a second copy: the name is a - // function of the IR, so a `-g` temp dir is bounded by distinct IR - // compiled with `-g`, not by the number of compiles. - compile_ll_to_object_in(&root, &ir, None, policy).expect("second compile failed"); - assert_eq!(entries(&root), left); + // Control: this comparison must be capable of failing. `-O0` is a flag that + // definitely changes the bytes; if even that compares equal, the harness is + // not really building two objects. + let control = elf_compile_with(&clang, &ll, target, &dir, &["-O0"]).unwrap(); + assert_ne!( + plain, control, + "the object comparison cannot distinguish two different compiles, so \ + the -g assertion above proves nothing" + ); let _ = fs::remove_dir_all(&root); } + +fn elf_compile(clang: &Path, ll: &Path, target: &str, cwd: &Path) -> Option> { + elf_compile_with(clang, ll, target, cwd, &[]) +} + +/// `clang -c` for an explicit target, returning the object bytes. `None` when +/// this clang has no backend for `target` (then the caller must skip, not pass). +fn elf_compile_with( + clang: &Path, + ll: &Path, + target: &str, + cwd: &Path, + extra: &[&str], +) -> Option> { + let obj = cwd.join(format!("out{}.o", extra.join(""))); + let run = Command::new(clang) + .current_dir(cwd) + .args(["-c", "-O3"]) + .args(extra) + .arg("-target") + .arg(target) + .arg(ll) + .arg("-o") + .arg(&obj) + .output() + .ok()?; + if !run.status.success() { + return None; + } + fs::read(&obj).ok() +} From cf6eda532a4105bf9274067f30d9de9159b1e584 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 09:04:12 +0200 Subject: [PATCH 08/11] test(codegen): assert directly that the -g object carries no DWARF Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- .../src/linker_temp_lifecycle_tests.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs index 8f1170ecce..4b372241b8 100644 --- a/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs +++ b/crates/perry-codegen/src/linker_temp_lifecycle_tests.rs @@ -385,6 +385,16 @@ fn debug_symbols_do_not_change_what_the_object_records() { delete it unconditionally has to be re-taken." ); + // Said directly as well as comparatively: no DWARF is emitted at all, so + // there is nowhere for a path to have been recorded. An equality assertion + // can be defeated by an edit to itself; "this object contains no `.debug_` + // section name" is a claim about the artifact. + assert!( + !contains(&with_g, b".debug_"), + "the -g object has .debug_ sections — DWARF is being emitted now, and \ + `DW_AT_comp_dir` / `DW_AT_name` may point at the temp `.ll`" + ); + // Control: this comparison must be capable of failing. `-O0` is a flag that // definitely changes the bytes; if even that compares equal, the harness is // not really building two objects. @@ -401,6 +411,11 @@ fn elf_compile(clang: &Path, ll: &Path, target: &str, cwd: &Path) -> Option bool { + haystack.windows(needle.len()).any(|w| w == needle) +} + /// `clang -c` for an explicit target, returning the object bytes. `None` when /// this clang has no backend for `target` (then the caller must skip, not pass). fn elf_compile_with( From 81a549786cef4c738ec47c1d4f221f51fed583cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 09:05:10 +0200 Subject: [PATCH 09/11] docs: retire the PERRY_DEBUG_SYMBOLS exemption from the census README and harness Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- benchmarks/repsel_census/README.md | 28 ++++++++++++++----- .../repsel_temp_hygiene.py | 15 ++++++---- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/benchmarks/repsel_census/README.md b/benchmarks/repsel_census/README.md index 2f203b6f01..4b5a170029 100644 --- a/benchmarks/repsel_census/README.md +++ b/benchmarks/repsel_census/README.md @@ -319,13 +319,27 @@ changing it: * **The `TMPDIR` isolation is load-bearing**, not politeness. Counting entries in the shared system temp dir measures every other process on the box. -Exempt on purpose: under `PERRY_DEBUG_SYMBOLS` clang gets `-g` and puts the -`.ll`'s **absolute** path into DWARF, so those builds keep their `.ll` at a -stable, content-addressed path in the temp root — deleting it would ship debug -info pointing at a file that no longer exists. That layout is bounded by -distinct IR compiled with `-g`, and `debug_symbols_layout_and_g_flag_agree` pins -the two halves of the decision together so a future edit cannot delete a file -DWARF still names. +No exemption for `PERRY_DEBUG_SYMBOLS`, and that is a change of belief rather +than a change of policy. `-g` was documented as pulling the `.ll`'s **absolute** +path plus `DW_AT_comp_dir` into DWARF, which would have made the file part of +the shipped object and forced it to persist. 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 has **no `.debug_*` sections at all**. Perry's codegen emits no +`DICompileUnit`/`DIFile`/`!dbg` metadata, and `clang -g` on a `.ll` lowers debug +info that is already in the IR rather than synthesising a compile unit for the +input file. So there is one layout, not two, and +`debug_symbols_do_not_change_what_the_object_records` in +`linker_temp_lifecycle_tests.rs` goes red the day that stops being true. (Not +measured on COFF/Windows.) + +That test has a sibling worth knowing about: +`the_ll_directory_is_not_recorded_in_the_object_but_the_basename_is` compiles +one `.ll` under the same basename from two different directories, for both +Linux ELF targets, and asserts the objects are identical — with a control that +a *different* basename does change them. That is the property this whole +directory rests on, and until #7144 it lived only in a comment and a hand +measurement taken once on a Raspberry Pi. ## Editing the fixtures diff --git a/scripts/compiler_output_harness/repsel_temp_hygiene.py b/scripts/compiler_output_harness/repsel_temp_hygiene.py index dea2bbd6dc..e7f941f808 100644 --- a/scripts/compiler_output_harness/repsel_temp_hygiene.py +++ b/scripts/compiler_output_harness/repsel_temp_hygiene.py @@ -37,9 +37,12 @@ process on the box — on a machine running several compiles at once, that is noise large enough to swamp the signal in either direction. -The `PERRY_DEBUG_SYMBOLS` layout is deliberately exempt: under `-g` the object's -DWARF names the `.ll` by absolute path, so the file has to outlive the compile. -This check runs without it, which is how every other gate compiles too. +`PERRY_DEBUG_SYMBOLS` is *not* exempt, though it was going to be. `-g` was +documented as putting the `.ll`'s absolute path into DWARF; measured on a real +Perry module it emits no DWARF at all (Perry's codegen produces no DI metadata, +and `clang -g` on a `.ll` lowers what the IR already has rather than +synthesising a compile unit). One layout, no exemption — see +`debug_symbols_do_not_change_what_the_object_records`. """ from __future__ import annotations @@ -187,9 +190,9 @@ def verdict( " the basename inside it stays a pure function of the IR so\n" " `census-determinism` keeps passing.\n" "\n" - " Exempt by design: `PERRY_DEBUG_SYMBOLS` builds keep their `.ll` —\n" - " the object's DWARF names it by absolute path. If this fired under\n" - " `-g`, that is the expected behaviour and not this gate's business." + " `PERRY_DEBUG_SYMBOLS` is not an exemption: measured, `-g` emits no\n" + " DWARF from a Perry `.ll` at all, so nothing records where the file\n" + " was and nothing needs to outlive the compile." ) return 1 From b56b7c0d1506b647d7f8bb6b4f0d866149513fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 09:06:13 +0200 Subject: [PATCH 10/11] docs: point the temp-hygiene reporting at #7167 Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- benchmarks/repsel_census/README.md | 5 +++++ scripts/compiler_output_harness/repsel_temp_hygiene.py | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/benchmarks/repsel_census/README.md b/benchmarks/repsel_census/README.md index 4b5a170029..aa3a3b6f16 100644 --- a/benchmarks/repsel_census/README.md +++ b/benchmarks/repsel_census/README.md @@ -319,6 +319,11 @@ changing it: * **The `TMPDIR` isolation is load-bearing**, not politeness. Counting entries in the shared system temp dir measures every other process on the box. +It fails on the clang driver's own temp names (`perry_llvm_*`, `perry_cgu_*`, +`perry_bc_*`) and merely *reports* anything else — today that is the compile +driver's `perry-objs--/` staging directory, which `--no-link` never +cleans up (#7167). Widen `OWNED_PREFIXES` to "everything" once that is closed. + No exemption for `PERRY_DEBUG_SYMBOLS`, and that is a change of belief rather than a change of policy. `-g` was documented as pulling the `.ll`'s **absolute** path plus `DW_AT_comp_dir` into DWARF, which would have made the file part of diff --git a/scripts/compiler_output_harness/repsel_temp_hygiene.py b/scripts/compiler_output_harness/repsel_temp_hygiene.py index e7f941f808..9d47b722e0 100644 --- a/scripts/compiler_output_harness/repsel_temp_hygiene.py +++ b/scripts/compiler_output_harness/repsel_temp_hygiene.py @@ -80,8 +80,8 @@ #: #: Anything else found is REPORTED, loudly, and does not fail: as of #7144 the #: compile driver leaks a `perry-objs--/` staging directory on the -#: `--no-link` path (`run_pipeline.rs` removes it on both *link* exits and there -#: is no third one), which is a real defect but a different module's, and a gate +#: `--no-link` path (#7167 — `run_pipeline.rs` removes it on both *link* exits and +#: there is no third one), which is a real defect but a different module's, and a gate #: that goes red for someone else's bug gets muted rather than fixed. Widen this #: to "nothing at all" once the driver's path is closed. OWNED_PREFIXES = ("perry_llvm", "perry_cgu", "perry_bc") @@ -152,8 +152,8 @@ def verdict( "staging dir;\n" " `run_pipeline.rs` removes it on both *link* exits and `--no-link` " "returns\n" - " before either. Reported, not failed: a gate that goes red for " - "another\n" + " before either (#7167). Reported, not failed: a gate that goes " + "red for another\n" " module's defect gets muted rather than fixed.\n" ) From 8bd0d306610453ee210cfd18532f16b563241fb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 09:13:47 +0200 Subject: [PATCH 11/11] docs: changelog fragment for #7144 Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- changelog.d/7168-ll-temp-file-lifecycle.md | 78 ++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 changelog.d/7168-ll-temp-file-lifecycle.md diff --git a/changelog.d/7168-ll-temp-file-lifecycle.md b/changelog.d/7168-ll-temp-file-lifecycle.md new file mode 100644 index 0000000000..dba59d2633 --- /dev/null +++ b/changelog.d/7168-ll-temp-file-lifecycle.md @@ -0,0 +1,78 @@ +**Fixed:** the temp directory no longer accumulates one LLVM IR file per distinct +module ever compiled (#7144). + +`compile_ll_to_object` stopped unlinking its temp `.ll` in #7135, deliberately: +that change 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 **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, and because the name is content-addressed the leftovers are bounded +by *distinct IR ever compiled on the machine* — which grows without limit in +practice, since working on the compiler changes the IR on nearly every rebuild. +1627 files / 951.8 MB after a day on one box; 1069 files / 635 MB still sitting in +the temp dir of the machine this was fixed on. + +The fix removes the sharing rather than making the deletion more careful. Every +`.ll` → `.o` compile now gets a directory it owns — +`perry_llvm_scratch__/` — and the directory is removed once the +object bytes have been read: + +* no two calls, in one process or across processes, are handed the same `.ll` + path, so the unlink has nothing to race and there is no narrow window left to + lose. That 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 added here went red in one full-suite run and green in the next three; +* the *basename* inside the directory is still `perry_llvm_.ll`, and + the object records the basename and nothing else — so emission determinism + (#7131) is untouched, and the `.o` keeps the pid + counter that #7140 restored; +* failures keep their IR, and the error message names it. `PERRY_LLVM_KEEP_IR` + keeps everything, now collected in one directory (`.ll`, `.o`, `.clang-stderr`, + compile plan) instead of scattered across the temp root. + +**`PERRY_DEBUG_SYMBOLS` is not an exemption, and the reason it was believed to be +one was wrong.** `-g` was documented as pulling the `.ll`'s absolute path plus +`DW_AT_comp_dir` into DWARF, which would have made the file part of the shipped +object. 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 has **no `.debug_*` sections at +all**. Perry's codegen emits no `DICompileUnit`/`DIFile`/`!dbg` metadata, and +`clang -g` on a `.ll` lowers debug info already in the IR rather than synthesising +a compile unit for the input file. The second temp-file layout that premise +justified was implemented and then deleted — a mode justified by something that +does not happen is an unexercised configuration, not a feature. Not measured on +COFF/Windows. + +**New checks**, all sabotage-verified in both directions: + +* `census-temp-hygiene` (`scripts/compiler_output_regression.py`) compiles the + census corpus with `TMPDIR` pointed at an empty directory and asserts the + directory is still empty. Red on the old compiler (27 leftovers from 54 + compiles), green on the new one; wired into the `repsel-census` CI job with a + verdict self-test. Note the shape of that number — repeats share a content hash + and so share a filename, which means a *"no growth run-over-run"* check would + have been **green on the broken compiler**. That is why CI never saw this while + developer machines filled up, and why the gate asserts the absolute property. +* `the_ll_directory_is_not_recorded_in_the_object_but_the_basename_is` compiles + one `.ll` under the same basename from two different directories, for both Linux + ELF targets, and asserts the objects are identical — with a live control that a + *different* basename does change them. This is the property the whole design + rests on; until now it lived in a comment and one hand measurement taken on a + Raspberry Pi. It cross-compiles, because the embedding is a property of the ELF + writer rather than of the host, so it also runs on macOS where this defect class + is otherwise invisible. +* `debug_symbols_do_not_change_what_the_object_records` pins the measurement + above, including a direct assertion that the `-g` object carries no `.debug_` + section name. + +Verification: 27/27 census workloads emit **byte-identical objects** before and +after (this renames files and nothing else); `census-determinism --repeat 3 +--jobs 4` and `--repeat 4 --jobs 8` all green; 24 concurrent `perry` processes +compiling one identical source, three times, 0 failures and one object hash. + +Filed separately as **#7167**: the compile driver leaks its +`perry-objs--/` object staging directory on the `--no-link` path +(`run_pipeline.rs` removes it on both *link* exits and there is no third one). +That one is unbounded in the number of compiles rather than in distinct IR. The +new gate reports it without failing on it — a gate that goes red for another +module's defect gets muted rather than fixed — and widening it once #7167 lands is +a one-line change.