From ab97deb843f538c5e37bd46f3f89780853cbf7a5 Mon Sep 17 00:00:00 2001 From: Michael Ilyin Date: Tue, 11 Aug 2026 02:35:52 +0200 Subject: [PATCH 1/4] Stop leaking a JSONL file per rustc invocation (#201) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `#[prebindgen]` named its capture file `{group}_{pid}_{thread}.jsonl`, so every rustc process wrote a fresh one. `build.rs` wipes the directory, but only when Cargo re-runs the build script — plain `cargo check` / `build` / `test` / `clippy` cycles re-run rustc against an untouched directory, so the files piled up forever (a full copy of the crate's captured items each time). Key the file on the compilation unit instead: rustc's own `-C metadata` hash, which is stable across rebuilds of a unit and distinct between units built concurrently (lib vs. test). rustdoc, which compiles doctests without `-C metadata`, falls back to a hash of its command line. The file is reset once per process, on the first record written, so a rebuild replaces the unit's records rather than appending them again. Since threads of one rustc now share a file, write each batch of records with a single `write_all` — a line split across writes could interleave. --- prebindgen-proc-macro/src/lib.rs | 129 ++++++++++++++++++++---------- prebindgen/src/api/utils/jsonl.rs | 10 ++- 2 files changed, 93 insertions(+), 46 deletions(-) diff --git a/prebindgen-proc-macro/src/lib.rs b/prebindgen-proc-macro/src/lib.rs index c622e916..388c76e7 100644 --- a/prebindgen-proc-macro/src/lib.rs +++ b/prebindgen-proc-macro/src/lib.rs @@ -36,7 +36,11 @@ //! //! See also: [`prebindgen`](https://docs.rs/prebindgen) for the main processing library. //! -use std::{collections::HashMap, fs::OpenOptions}; +use std::{ + collections::HashSet, + hash::{Hash, Hasher}, + sync::{Mutex, OnceLock}, +}; use prebindgen::{get_prebindgen_out_dir, Record, RecordKind, SourceLocation, DEFAULT_GROUP_NAME}; use proc_macro::TokenStream; @@ -135,51 +139,59 @@ impl Parse for PrebindgenArgs { } } -thread_local! { - static THREAD_ID: std::cell::RefCell> = const { std::cell::RefCell::new(None) }; - static JSONL_PATHS: std::cell::RefCell> = std::cell::RefCell::new(HashMap::new()); +/// Groups whose JSONL file this process has already reset. +static STARTED_GROUPS: OnceLock>> = OnceLock::new(); + +/// Identify the compilation unit this expansion belongs to. +/// +/// `build.rs` clears the prebindgen directory, but only when Cargo re-runs the +/// build script — plain `cargo check` / `build` / `test` / `clippy` cycles +/// re-run *rustc* against an untouched directory. A per-process file name would +/// therefore leave one stale JSONL file behind per rustc invocation, forever +/// (#201). The id has to be stable across rebuilds of the same unit (so the +/// file is overwritten) yet differ between units built concurrently, e.g. lib +/// vs. test: rustc's own `-C metadata` hash is exactly that. rustdoc, which +/// compiles doctests without `-C metadata`, gets a hash of its command line +/// instead — equally stable per unit. +fn unit_id() -> &'static str { + static UNIT_ID: OnceLock = OnceLock::new(); + UNIT_ID.get_or_init(|| { + let args: Vec = std::env::args().collect(); + if let Some(metadata) = + args.iter() + .enumerate() + .find_map(|(i, arg)| match arg.strip_prefix("-C") { + Some("") => args.get(i + 1)?.strip_prefix("metadata="), + Some(rest) => rest.strip_prefix("metadata="), + None => None, + }) + { + return metadata.to_string(); + } + if args.is_empty() { + // No command line to key off at all — stay unique per process. + return format!("{}_{}", std::process::id(), rand::random::()); + } + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + args.hash(&mut hasher); + format!("{:016x}", hasher.finish()) + }) } -/// Get the full path to `{group}_{pid}_{thread_id}.jsonl` generated in OUT_DIR. +/// Get the full path to `{group}_{unit_id}.jsonl` in OUT_DIR, truncating it on +/// the first write of this process so re-running the same unit replaces its +/// records instead of appending them again. fn get_prebindgen_jsonl_path(group: &str) -> std::path::PathBuf { - if let Some(p) = JSONL_PATHS.with(|path| path.borrow().get(group).cloned()) { - return p; + let path = get_prebindgen_out_dir().join(format!("{group}_{}.jsonl", unit_id())); + let first_write = STARTED_GROUPS + .get_or_init(Mutex::default) + .lock() + .map(|mut started| started.insert(group.to_string())) + .unwrap_or(false); + if first_write { + let _ = std::fs::remove_file(&path); } - let process_id = std::process::id(); - let thread_id = if let Some(in_thread_id) = THREAD_ID.with(|id| *id.borrow()) { - in_thread_id - } else { - let new_id = rand::random::(); - THREAD_ID.with(|id| *id.borrow_mut() = Some(new_id)); - new_id - }; - let mut random_value = None; - // Try to really create file and repeat until success - // to avoid collisions in extremely rare case when two threads got - // the same random value - let new_path = loop { - let postfix = if let Some(rv) = random_value { - format!("_{rv}") - } else { - "".to_string() - }; - let path = get_prebindgen_out_dir() - .join(format!("{group}_{process_id}_{thread_id}{postfix}.jsonl")); - if OpenOptions::new() - .create_new(true) - .write(true) - .open(&path) - .is_ok() - { - break path; - } - random_value = Some(rand::random::()); - }; - JSONL_PATHS.with(|path| { - path.borrow_mut() - .insert(group.to_string(), new_path.clone()); - }); - new_path + path } /// Attribute macro that exports FFI definitions for use in language-specific binding crates. @@ -439,3 +451,36 @@ pub fn manifest_dir(_input: TokenStream) -> TokenStream { let lit = syn::LitStr::new(&dir, proc_macro2::Span::call_site()); TokenStream::from(quote! { #lit }) } + +#[cfg(test)] +mod tests { + use super::get_prebindgen_jsonl_path; + + /// The same unit must reuse one file: dropped what a previous run of this + /// unit left there, then appended to within the run. + #[test] + fn jsonl_file_is_reset_once_per_run() { + let out_dir = std::env::temp_dir().join(format!("prebindgen-test-{}", std::process::id())); + std::fs::create_dir_all(out_dir.join("prebindgen")).unwrap(); + std::env::set_var("OUT_DIR", &out_dir); + + let stale = out_dir + .join("prebindgen") + .join(format!("test_group_{}.jsonl", super::unit_id())); + std::fs::write(&stale, "from a previous run\n").unwrap(); + + let path = get_prebindgen_jsonl_path("test_group"); + assert_eq!(path, stale); + assert!(!path.exists(), "records of the previous run were kept"); + + std::fs::write(&path, "first record\n").unwrap(); + assert_eq!(get_prebindgen_jsonl_path("test_group"), path); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "first record\n", + "records of this run were dropped" + ); + + std::fs::remove_dir_all(&out_dir).unwrap(); + } +} diff --git a/prebindgen/src/api/utils/jsonl.rs b/prebindgen/src/api/utils/jsonl.rs index 2b252ca3..758488b4 100644 --- a/prebindgen/src/api/utils/jsonl.rs +++ b/prebindgen/src/api/utils/jsonl.rs @@ -17,12 +17,14 @@ pub fn write_to_jsonl_file, R: Borrow>( let Ok(mut file) = OpenOptions::new().create(true).append(true).open(file_path) else { return Err("Failed to open file".into()); }; - // Check if file is empty (just created or was deleted) + // One write for the whole batch: several rustc threads can append to the + // same file, and a line split across writes would interleave into garbage. + let mut buf = String::new(); for record in records { - let json_line = record.borrow().to_jsonl_string()?; - writeln!(file, "{json_line}")?; + buf.push_str(&record.borrow().to_jsonl_string()?); + buf.push('\n'); } - file.flush()?; + file.write_all(buf.as_bytes())?; Ok(()) } From 574a98b3cd78e2faf3153309899679a183f4225c Mon Sep 17 00:00:00 2001 From: Michael Ilyin Date: Thu, 13 Aug 2026 17:54:46 +0200 Subject: [PATCH 2/4] Serialize capture initialization with writes Keep each capture file's first reset and every append under one per-file lock, so a concurrent macro expansion cannot have its record unlinked. Propagate reset and lock failures as compiler diagnostics and cover the race with synchronized writers.\n\nAlso parse rustc unit metadata from OS-native arguments, accept every codegen-option spelling, and drop the no-longer-needed rand dependency. --- Cargo.toml | 1 - prebindgen-proc-macro/Cargo.toml | 1 - prebindgen-proc-macro/src/lib.rs | 256 ++++++++++++++++++++++-------- prebindgen/src/api/utils/jsonl.rs | 10 +- 4 files changed, 197 insertions(+), 71 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4b5f2b0c..4688d05e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,4 +52,3 @@ itertools = "0.14.0" if_rust_version = "1.0" konst = "0.3.0" # consumer crates need konst too: the generated file emits a konst::assertc_eq! guard toml = "0.9.5" -rand = "0.9.2" diff --git a/prebindgen-proc-macro/Cargo.toml b/prebindgen-proc-macro/Cargo.toml index 2303b0e3..aecbce3a 100644 --- a/prebindgen-proc-macro/Cargo.toml +++ b/prebindgen-proc-macro/Cargo.toml @@ -26,7 +26,6 @@ syn = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } prebindgen = { workspace = true } -rand = { workspace = true } [features] debug = ["prebindgen/debug"] diff --git a/prebindgen-proc-macro/src/lib.rs b/prebindgen-proc-macro/src/lib.rs index 388c76e7..3d0ae6aa 100644 --- a/prebindgen-proc-macro/src/lib.rs +++ b/prebindgen-proc-macro/src/lib.rs @@ -37,9 +37,11 @@ //! See also: [`prebindgen`](https://docs.rs/prebindgen) for the main processing library. //! use std::{ - collections::HashSet, + collections::HashMap, + ffi::OsString, hash::{Hash, Hasher}, - sync::{Mutex, OnceLock}, + path::{Path, PathBuf}, + sync::{Arc, Mutex, OnceLock}, }; use prebindgen::{get_prebindgen_out_dir, Record, RecordKind, SourceLocation, DEFAULT_GROUP_NAME}; @@ -139,8 +141,16 @@ impl Parse for PrebindgenArgs { } } -/// Groups whose JSONL file this process has already reset. -static STARTED_GROUPS: OnceLock>> = OnceLock::new(); +#[derive(Default)] +struct CaptureState { + initialized: bool, +} + +type SharedCaptureState = Arc>; + +/// One lock per capture file. Macro expansions can run on several rustc +/// threads, but unrelated groups do not need to block one another. +static CAPTURE_STATES: OnceLock>> = OnceLock::new(); /// Identify the compilation unit this expansion belongs to. /// @@ -155,43 +165,80 @@ static STARTED_GROUPS: OnceLock>> = OnceLock::new(); /// instead — equally stable per unit. fn unit_id() -> &'static str { static UNIT_ID: OnceLock = OnceLock::new(); - UNIT_ID.get_or_init(|| { - let args: Vec = std::env::args().collect(); - if let Some(metadata) = - args.iter() - .enumerate() - .find_map(|(i, arg)| match arg.strip_prefix("-C") { - Some("") => args.get(i + 1)?.strip_prefix("metadata="), - Some(rest) => rest.strip_prefix("metadata="), - None => None, - }) - { - return metadata.to_string(); - } - if args.is_empty() { - // No command line to key off at all — stay unique per process. - return format!("{}_{}", std::process::id(), rand::random::()); - } - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - args.hash(&mut hasher); - format!("{:016x}", hasher.finish()) - }) + UNIT_ID.get_or_init(|| unit_id_from_args(&std::env::args_os().collect::>())) +} + +fn unit_id_from_args(args: &[OsString]) -> String { + // rustc accepts both short/long and joined/separate codegen options. Use + // the last occurrence, matching how repeated compiler options are applied. + if let Some(metadata) = args + .iter() + .enumerate() + .filter_map(|(index, arg)| { + let arg = arg.to_str()?; + match arg { + "-C" | "--codegen" => args.get(index + 1)?.to_str()?.strip_prefix("metadata="), + _ => arg + .strip_prefix("-C") + .and_then(|arg| arg.strip_prefix("metadata=")) + .or_else(|| arg.strip_prefix("--codegen=metadata=")), + } + }) + .rfind(|metadata| !metadata.is_empty()) + { + return metadata.to_string(); + } + + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + args.hash(&mut hasher); + format!("{:016x}", hasher.finish()) } -/// Get the full path to `{group}_{unit_id}.jsonl` in OUT_DIR, truncating it on -/// the first write of this process so re-running the same unit replaces its -/// records instead of appending them again. +/// Get the full path to `{group}_{unit_id}.jsonl` in OUT_DIR. fn get_prebindgen_jsonl_path(group: &str) -> std::path::PathBuf { - let path = get_prebindgen_out_dir().join(format!("{group}_{}.jsonl", unit_id())); - let first_write = STARTED_GROUPS + get_prebindgen_out_dir().join(format!("{group}_{}.jsonl", unit_id())) +} + +fn capture_state(file_path: &Path) -> std::result::Result { + let mut states = CAPTURE_STATES .get_or_init(Mutex::default) .lock() - .map(|mut started| started.insert(group.to_string())) - .unwrap_or(false); - if first_write { - let _ = std::fs::remove_file(&path); + .map_err(|_| "prebindgen capture-state registry lock was poisoned".to_string())?; + Ok(Arc::clone( + states.entry(file_path.to_path_buf()).or_default(), + )) +} + +/// Reset a compilation unit's capture on its first record, then serialize all +/// initialization and appends to that file. Keeping both operations under the +/// same per-file lock prevents a first writer from unlinking a record that a +/// second rustc thread has already appended. +fn write_capture_record(file_path: &Path, record: &Record) -> std::result::Result<(), String> { + let state = capture_state(file_path)?; + let mut state = state.lock().map_err(|_| { + format!( + "prebindgen capture lock for {} was poisoned", + file_path.display() + ) + })?; + + if !state.initialized { + match std::fs::remove_file(file_path) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => { + return Err(format!( + "failed to reset prebindgen capture {}: {error}", + file_path.display() + )); + } + } } - path + + prebindgen::utils::write_to_jsonl_file(file_path, &[record]) + .map_err(|error| format!("failed to append {}: {error}", file_path.display()))?; + state.initialized = true; + Ok(()) } /// Attribute macro that exports FFI definitions for use in language-specific binding crates. @@ -319,12 +366,9 @@ pub fn prebindgen(args: TokenStream, input: TokenStream) -> TokenStream { parsed_args.cfg.clone(), ); - // Get the full path to the JSONL file let file_path = get_prebindgen_jsonl_path(&group); - if prebindgen::utils::write_to_jsonl_file(&file_path, &[&new_record]).is_err() { - return TokenStream::from(quote! { - compile_error!("Failed to write prebindgen record"); - }); + if let Err(error) = write_capture_record(&file_path, &new_record) { + return syn::Error::new(span, error).to_compile_error().into(); } // Re-emit the original item, optionally prepending `#[cfg(...)]` (from the @@ -454,33 +498,117 @@ pub fn manifest_dir(_input: TokenStream) -> TokenStream { #[cfg(test)] mod tests { - use super::get_prebindgen_jsonl_path; + use std::{ + collections::BTreeSet, + sync::{Arc, Barrier}, + }; + + use super::{unit_id_from_args, write_capture_record, Record, RecordKind}; + + fn record(name: impl Into) -> Record { + let name = name.into(); + Record::new( + RecordKind::Struct, + name.clone(), + format!("pub struct {name};"), + Default::default(), + None, + ) + } + + fn test_dir(name: &str) -> std::path::PathBuf { + std::env::temp_dir().join(format!( + "prebindgen-proc-macro-{}-{name}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )) + } + + #[test] + fn unit_id_accepts_rustc_codegen_spellings_and_uses_the_last() { + let args = [ + "rustc", + "-Cmetadata=first", + "--codegen=metadata=second", + "-C", + "metadata=third", + ] + .map(Into::into); + assert_eq!(unit_id_from_args(&args), "third"); + + let args = ["rustdoc", "--codegen", "metadata=long-form"].map(Into::into); + assert_eq!(unit_id_from_args(&args), "long-form"); + } + + #[test] + fn unit_id_fallback_is_stable_without_metadata() { + let args = ["rustdoc", "--test", "src/lib.rs"].map(Into::into); + assert_eq!(unit_id_from_args(&args), unit_id_from_args(&args)); + assert_ne!( + unit_id_from_args(&args), + unit_id_from_args(&["rustdoc", "src/lib.rs"].map(Into::into)) + ); + } + + #[test] + fn concurrent_first_writes_reset_once_without_losing_records() { + const WRITERS: usize = 32; + + let dir = test_dir("concurrent-first-writes"); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("group_unit.jsonl"); + prebindgen::utils::write_to_jsonl_file(&path, &[record("Stale")]).unwrap(); + + let barrier = Arc::new(Barrier::new(WRITERS)); + let handles = (0..WRITERS) + .map(|index| { + let barrier = Arc::clone(&barrier); + let path = path.clone(); + std::thread::spawn(move || { + let record = record(format!("Record{index}")); + barrier.wait(); + write_capture_record(&path, &record).unwrap(); + }) + }) + .collect::>(); + for handle in handles { + handle.join().unwrap(); + } + + // A later expansion in the same process must append, not reset again. + write_capture_record(&path, &record("LastRecord")).unwrap(); + + let records = prebindgen::utils::read_jsonl_file(&path).unwrap(); + assert_eq!(records.len(), WRITERS + 1); + let names = records + .into_iter() + .map(|record| record.name) + .collect::>(); + let expected = (0..WRITERS) + .map(|index| format!("Record{index}")) + .chain(["LastRecord".to_string()]) + .collect::>(); + assert_eq!(names, expected); + + std::fs::remove_dir_all(dir).unwrap(); + } - /// The same unit must reuse one file: dropped what a previous run of this - /// unit left there, then appended to within the run. #[test] - fn jsonl_file_is_reset_once_per_run() { - let out_dir = std::env::temp_dir().join(format!("prebindgen-test-{}", std::process::id())); - std::fs::create_dir_all(out_dir.join("prebindgen")).unwrap(); - std::env::set_var("OUT_DIR", &out_dir); - - let stale = out_dir - .join("prebindgen") - .join(format!("test_group_{}.jsonl", super::unit_id())); - std::fs::write(&stale, "from a previous run\n").unwrap(); - - let path = get_prebindgen_jsonl_path("test_group"); - assert_eq!(path, stale); - assert!(!path.exists(), "records of the previous run were kept"); - - std::fs::write(&path, "first record\n").unwrap(); - assert_eq!(get_prebindgen_jsonl_path("test_group"), path); - assert_eq!( - std::fs::read_to_string(&path).unwrap(), - "first record\n", - "records of this run were dropped" + fn unexpected_reset_error_is_reported() { + let dir = test_dir("reset-error"); + let path = dir.join("capture-is-a-directory"); + std::fs::create_dir_all(&path).unwrap(); + + let error = write_capture_record(&path, &record("Record")).unwrap_err(); + assert!( + error.contains("failed to reset prebindgen capture"), + "{error}" ); + assert!(error.contains(&path.display().to_string()), "{error}"); - std::fs::remove_dir_all(&out_dir).unwrap(); + std::fs::remove_dir_all(dir).unwrap(); } } diff --git a/prebindgen/src/api/utils/jsonl.rs b/prebindgen/src/api/utils/jsonl.rs index 758488b4..340181d3 100644 --- a/prebindgen/src/api/utils/jsonl.rs +++ b/prebindgen/src/api/utils/jsonl.rs @@ -14,16 +14,16 @@ pub fn write_to_jsonl_file, R: Borrow>( file_path: P, records: &[R], ) -> Result<(), Box> { - let Ok(mut file) = OpenOptions::new().create(true).append(true).open(file_path) else { - return Err("Failed to open file".into()); - }; - // One write for the whole batch: several rustc threads can append to the - // same file, and a line split across writes would interleave into garbage. + // Serialize the complete batch before touching the output, then minimize + // the number of append operations used for it. let mut buf = String::new(); for record in records { buf.push_str(&record.borrow().to_jsonl_string()?); buf.push('\n'); } + let Ok(mut file) = OpenOptions::new().create(true).append(true).open(file_path) else { + return Err("Failed to open file".into()); + }; file.write_all(buf.as_bytes())?; Ok(()) } From fea397d51c2848cfc7d0f0a5546c363d7aafa96a Mon Sep 17 00:00:00 2001 From: Michael Ilyin Date: Thu, 13 Aug 2026 19:09:20 +0200 Subject: [PATCH 3/4] Hash rustc metadata before naming captures --- prebindgen-proc-macro/src/lib.rs | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/prebindgen-proc-macro/src/lib.rs b/prebindgen-proc-macro/src/lib.rs index 3d0ae6aa..350710c3 100644 --- a/prebindgen-proc-macro/src/lib.rs +++ b/prebindgen-proc-macro/src/lib.rs @@ -186,11 +186,18 @@ fn unit_id_from_args(args: &[OsString]) -> String { }) .rfind(|metadata| !metadata.is_empty()) { - return metadata.to_string(); + // rustc accepts arbitrary strings here, including slashes and backslashes. + // Hash the value before using it as a path component so a caller's + // metadata cannot introduce directories or platform-specific names. + return hash_unit_id(metadata); } + hash_unit_id(args) +} + +fn hash_unit_id(value: &(impl Hash + ?Sized)) -> String { let mut hasher = std::collections::hash_map::DefaultHasher::new(); - args.hash(&mut hasher); + value.hash(&mut hasher); format!("{:016x}", hasher.finish()) } @@ -503,7 +510,7 @@ mod tests { sync::{Arc, Barrier}, }; - use super::{unit_id_from_args, write_capture_record, Record, RecordKind}; + use super::{hash_unit_id, unit_id_from_args, write_capture_record, Record, RecordKind}; fn record(name: impl Into) -> Record { let name = name.into(); @@ -528,19 +535,23 @@ mod tests { } #[test] - fn unit_id_accepts_rustc_codegen_spellings_and_uses_the_last() { + fn unit_id_accepts_rustc_codegen_spellings_hashes_and_uses_the_last() { let args = [ "rustc", "-Cmetadata=first", "--codegen=metadata=second", "-C", - "metadata=third", + "metadata=third/path\\unit", ] .map(Into::into); - assert_eq!(unit_id_from_args(&args), "third"); + let unit_id = unit_id_from_args(&args); + assert_eq!(unit_id, hash_unit_id("third/path\\unit")); + assert_eq!(unit_id.len(), 16); + assert!(unit_id.bytes().all(|byte| byte.is_ascii_hexdigit())); + assert!(!unit_id.contains(['/', '\\'])); let args = ["rustdoc", "--codegen", "metadata=long-form"].map(Into::into); - assert_eq!(unit_id_from_args(&args), "long-form"); + assert_eq!(unit_id_from_args(&args), hash_unit_id("long-form")); } #[test] From e6e2a33c4fec4660affd46fc418a1742fea394e6 Mon Sep 17 00:00:00 2001 From: Michael Ilyin Date: Fri, 14 Aug 2026 23:12:47 +0200 Subject: [PATCH 4/4] Key captures on the whole unit, not the last metadata word MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review blockers, both cases where the capture id was coarser than the compilation unit — the failure mode that loses records rather than files. `-C metadata` is a list, not a scalar: rustc parses it with `parse_list`, so occurrences accumulate and each contributes its whitespace-separated words, and `StableCrateId::new` sorts them. Hashing only the last occurrence mapped `unit_a shared` and `unit_b shared` onto one file, so one unit reset the other's capture. Reproduced with `RUSTFLAGS=-Cmetadata=custom`, which reaches every unit: the lib and test units of example-flat then shared one file, and a `#[cfg(test)]`-only item captured by the test unit was silently erased by the following build. Collect every component, sort as rustc does, hash the list. The rustdoc fallback hashed the whole command line, including the doctest filter Cargo forwards as `--test-args`, so `cargo test --doc alpha` and `--doc beta` minted a capture pair each. Drop the options that only run the tests or render diagnostics before hashing. The exclusion is a deny-list of provably run-time-only options rather than an allow-list: an unknown option stays in the id, where it can cost an extra file but never a record. Verified `-C metadata` list semantics (accumulate, whitespace-split, sorted, empty ignored) against probe-crate symbol hashes on 1.85.0 and 1.97.1. --- prebindgen-proc-macro/src/lib.rs | 257 +++++++++++++++++++++++++++---- 1 file changed, 228 insertions(+), 29 deletions(-) diff --git a/prebindgen-proc-macro/src/lib.rs b/prebindgen-proc-macro/src/lib.rs index 350710c3..d2630b57 100644 --- a/prebindgen-proc-macro/src/lib.rs +++ b/prebindgen-proc-macro/src/lib.rs @@ -38,7 +38,7 @@ //! use std::{ collections::HashMap, - ffi::OsString, + ffi::{OsStr, OsString}, hash::{Hash, Hasher}, path::{Path, PathBuf}, sync::{Arc, Mutex, OnceLock}, @@ -152,6 +152,35 @@ type SharedCaptureState = Arc>; /// threads, but unrelated groups do not need to block one another. static CAPTURE_STATES: OnceLock>> = OnceLock::new(); +/// Options that describe how the compiled tests are *run*, or how diagnostics +/// are *rendered*, and take a value (`--opt value` or `--opt=value`). +/// +/// None of them changes what is compiled, and Cargo varies them from one +/// invocation to the next: `cargo test --doc ` forwards the filter (and +/// everything after `--`) to rustdoc as `--test-args`. Feeding them to the +/// fallback id would mint a fresh capture file per invocation — exactly the +/// leak this is fixing (#201). +/// +/// The list is deliberately a deny-list of provably run-time-only options +/// rather than an allow-list of compile-identifying ones: dropping an +/// argument that *does* identify a unit would let two distinct units share one +/// capture file and reset each other's records, which loses items silently. +/// Everything not listed here is kept, so an unknown option can only ever cost +/// an extra file, never a lost record. +const RUNTIME_ONLY_VALUE_OPTIONS: &[&str] = &[ + "--test-args", + "--test-run-directory", + "--runtool", + "--runtool-arg", + "--color", + "--error-format", + "--json", + "--diagnostic-width", +]; + +/// Run-time-only options that take no value; see [`RUNTIME_ONLY_VALUE_OPTIONS`]. +const RUNTIME_ONLY_FLAGS: &[&str] = &["--nocapture"]; + /// Identify the compilation unit this expansion belongs to. /// /// `build.rs` clears the prebindgen directory, but only when Cargo re-runs the @@ -160,39 +189,107 @@ static CAPTURE_STATES: OnceLock>> = O /// therefore leave one stale JSONL file behind per rustc invocation, forever /// (#201). The id has to be stable across rebuilds of the same unit (so the /// file is overwritten) yet differ between units built concurrently, e.g. lib -/// vs. test: rustc's own `-C metadata` hash is exactly that. rustdoc, which -/// compiles doctests without `-C metadata`, gets a hash of its command line -/// instead — equally stable per unit. +/// vs. test: rustc's own `-C metadata` is exactly that. rustdoc, which compiles +/// doctests without `-C metadata`, gets a hash of the compile-identifying part +/// of its command line instead — equally stable per unit. +/// +/// Both halves err towards *more* ids: an id that is finer than the unit only +/// leaves a stale file behind, while an id shared by two units would let one +/// unit reset the other's capture and silently drop its items. fn unit_id() -> &'static str { static UNIT_ID: OnceLock = OnceLock::new(); UNIT_ID.get_or_init(|| unit_id_from_args(&std::env::args_os().collect::>())) } fn unit_id_from_args(args: &[OsString]) -> String { - // rustc accepts both short/long and joined/separate codegen options. Use - // the last occurrence, matching how repeated compiler options are applied. - if let Some(metadata) = args - .iter() - .enumerate() - .filter_map(|(index, arg)| { - let arg = arg.to_str()?; - match arg { - "-C" | "--codegen" => args.get(index + 1)?.to_str()?.strip_prefix("metadata="), - _ => arg - .strip_prefix("-C") - .and_then(|arg| arg.strip_prefix("metadata=")) - .or_else(|| arg.strip_prefix("--codegen=metadata=")), + let mut metadata = metadata_components(args); + // rustc sorts the list before folding it into the crate id, so argument + // order alone does not make a different unit. + metadata.sort_unstable(); + if !metadata.is_empty() { + // rustc accepts arbitrary strings here, including slashes and + // backslashes. Hash the components before using them as a path + // component so a caller's metadata cannot introduce directories or + // platform-specific names. + return hash_unit_id(metadata.as_slice()); + } + + hash_unit_id(compile_identifying_args(args).as_slice()) +} + +/// Collect every `-C metadata` component. +/// +/// rustc declares this option as `metadata: Vec` parsed by `parse_list`, +/// so repeated occurrences **accumulate** (they are not last-value-wins) and each +/// occurrence contributes its whitespace-separated words; `StableCrateId::new` +/// then sorts them. `-Cmetadata=a b`, `-Cmetadata=a -Cmetadata=b` and +/// `-C metadata=b --codegen=metadata=a` therefore all describe the same unit, +/// while `-Cmetadata=b` alone describes a different one, and an empty value +/// contributes nothing. Verified on rustc 1.85.0 and 1.97.1 by comparing the +/// symbol hash / crate disambiguator of a probe crate. +/// +/// Hashing only the last occurrence — as this did before — would map +/// `unit_a shared` and `unit_b shared` onto one capture file, letting one unit +/// reset the other's records. +fn metadata_components(args: &[OsString]) -> Vec<&str> { + let mut components = Vec::new(); + let mut index = 0; + while index < args.len() { + let arg = args[index].to_str(); + index += 1; + let Some(arg) = arg else { continue }; + // rustc accepts the short and long spelling of the codegen flag, each + // with the value joined or in the following argument. + let value = match arg { + "-C" | "--codegen" => { + let value = args.get(index).and_then(|arg| arg.to_str()); + // The next argument is this option's value whatever it holds. + index += 1; + value.and_then(|value| value.strip_prefix("metadata=")) } - }) - .rfind(|metadata| !metadata.is_empty()) - { - // rustc accepts arbitrary strings here, including slashes and backslashes. - // Hash the value before using it as a path component so a caller's - // metadata cannot introduce directories or platform-specific names. - return hash_unit_id(metadata); + _ => arg + .strip_prefix("-C") + .and_then(|arg| arg.strip_prefix("metadata=")) + .or_else(|| arg.strip_prefix("--codegen=metadata=")), + }; + if let Some(value) = value { + components.extend(value.split_whitespace()); + } } + components +} - hash_unit_id(args) +/// The arguments that identify *what* is compiled: everything except the +/// run-time-only options above. +fn compile_identifying_args(args: &[OsString]) -> Vec<&OsStr> { + let mut kept = Vec::with_capacity(args.len()); + let mut index = 0; + while index < args.len() { + let arg = &args[index]; + index += 1; + // A non-UTF-8 argument is none of the options below, so it is kept. + let Some(text) = arg.to_str() else { + kept.push(arg.as_os_str()); + continue; + }; + if RUNTIME_ONLY_FLAGS.contains(&text) { + continue; + } + if RUNTIME_ONLY_VALUE_OPTIONS.contains(&text) { + // Drop the value carried by the following argument as well. + index += 1; + continue; + } + let joined_value = RUNTIME_ONLY_VALUE_OPTIONS.iter().any(|option| { + text.strip_prefix(option) + .is_some_and(|rest| rest.starts_with('=')) + }); + if joined_value { + continue; + } + kept.push(arg.as_os_str()); + } + kept } fn hash_unit_id(value: &(impl Hash + ?Sized)) -> String { @@ -510,7 +607,10 @@ mod tests { sync::{Arc, Barrier}, }; - use super::{hash_unit_id, unit_id_from_args, write_capture_record, Record, RecordKind}; + use super::{ + compile_identifying_args, hash_unit_id, unit_id_from_args, write_capture_record, Record, + RecordKind, + }; fn record(name: impl Into) -> Record { let name = name.into(); @@ -535,7 +635,7 @@ mod tests { } #[test] - fn unit_id_accepts_rustc_codegen_spellings_hashes_and_uses_the_last() { + fn unit_id_accepts_every_rustc_codegen_spelling_and_hashes_the_value() { let args = [ "rustc", "-Cmetadata=first", @@ -545,13 +645,60 @@ mod tests { ] .map(Into::into); let unit_id = unit_id_from_args(&args); - assert_eq!(unit_id, hash_unit_id("third/path\\unit")); + assert_eq!( + unit_id, + hash_unit_id(&["first", "second", "third/path\\unit"][..]) + ); assert_eq!(unit_id.len(), 16); assert!(unit_id.bytes().all(|byte| byte.is_ascii_hexdigit())); assert!(!unit_id.contains(['/', '\\'])); let args = ["rustdoc", "--codegen", "metadata=long-form"].map(Into::into); - assert_eq!(unit_id_from_args(&args), hash_unit_id("long-form")); + assert_eq!(unit_id_from_args(&args), hash_unit_id(&["long-form"][..])); + } + + #[test] + fn unit_id_hashes_every_metadata_component_not_only_the_last() { + // rustc's `-C metadata` is a list: every occurrence contributes to the + // unit, so two units sharing only their trailing component are still + // distinct and must not share a capture file. + let unit_a = ["rustc", "-Cmetadata=unit_a", "-Cmetadata=shared"].map(Into::into); + let unit_b = ["rustc", "-Cmetadata=unit_b", "-Cmetadata=shared"].map(Into::into); + assert_ne!(unit_id_from_args(&unit_a), unit_id_from_args(&unit_b)); + assert_ne!( + unit_id_from_args(&unit_a), + unit_id_from_args(&["rustc", "-Cmetadata=shared"].map(Into::into)) + ); + + // Argument order alone is not a different unit: rustc sorts the list + // before folding it into the crate id (verified against the symbol + // hashes of a probe crate on 1.85.0 and 1.97.1). + assert_eq!( + unit_id_from_args(&["rustc", "-Cmetadata=a", "-Cmetadata=b"].map(Into::into)), + unit_id_from_args(&["rustc", "-Cmetadata=b", "-Cmetadata=a"].map(Into::into)) + ); + } + + #[test] + fn unit_id_follows_rustc_list_parsing_of_metadata() { + // `parse_list` splits each value on whitespace and appends it, so these + // spellings describe one and the same unit. + assert_eq!( + unit_id_from_args(&["rustc", "-Cmetadata=a b"].map(Into::into)), + unit_id_from_args(&["rustc", "-Cmetadata=a", "-Cmetadata=b"].map(Into::into)) + ); + // An empty value contributes no component. + assert_eq!( + unit_id_from_args(&["rustc", "-Cmetadata=first", "-Cmetadata="].map(Into::into)), + unit_id_from_args(&["rustc", "-Cmetadata=first"].map(Into::into)) + ); + // ... and on its own it leaves nothing to key the unit on, so the + // argument fallback takes over. + let only_empty = ["rustc", "-Cmetadata=", "src/lib.rs"].map(Into::into); + assert_eq!( + unit_id_from_args(&only_empty), + hash_unit_id(compile_identifying_args(&only_empty).as_slice()) + ); } #[test] @@ -564,6 +711,58 @@ mod tests { ); } + #[test] + fn unit_id_fallback_ignores_arguments_that_only_run_the_tests() { + // Cargo forwards a doctest filter (and anything after `--`) as + // `--test-args`; rustdoc compiles the same unit either way. + let doctest = |extra: &[&str]| { + let mut args = vec![ + "rustdoc".to_string(), + "--edition=2021".to_string(), + "--crate-type".to_string(), + "lib".to_string(), + "--crate-name".to_string(), + "example_flat".to_string(), + "--test".to_string(), + "examples/example-flat/src/lib.rs".to_string(), + ]; + args.extend(extra.iter().map(|arg| arg.to_string())); + unit_id_from_args(&args.into_iter().map(Into::into).collect::>()) + }; + + let plain = doctest(&[]); + assert_eq!(plain, doctest(&["--test-args", "alpha"])); + assert_eq!(plain, doctest(&["--test-args", "beta"])); + assert_eq!(plain, doctest(&["--test-args=beta"])); + assert_eq!( + plain, + doctest(&["--test-args", "beta", "--test-args", "--nocapture"]) + ); + assert_eq!(plain, doctest(&["--nocapture"])); + assert_eq!( + plain, + doctest(&[ + "--test-run-directory", + "examples/example-flat", + "--color", + "always", + "--error-format=json", + ]) + ); + + // What is compiled still separates units. + assert_ne!(plain, doctest(&["--cfg", "feature=\"unstable\""])); + assert_ne!( + plain, + unit_id_from_args( + &["rustdoc", "--crate-name", "other", "--test", "src/lib.rs"].map(Into::into) + ) + ); + // An unknown option is kept: an extra file is acceptable, a lost + // record is not. + assert_ne!(plain, doctest(&["--never-heard-of-it", "value"])); + } + #[test] fn concurrent_first_writes_reset_once_without_losing_records() { const WRITERS: usize = 32;