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 c622e916..d2630b57 100644 --- a/prebindgen-proc-macro/src/lib.rs +++ b/prebindgen-proc-macro/src/lib.rs @@ -36,7 +36,13 @@ //! //! See also: [`prebindgen`](https://docs.rs/prebindgen) for the main processing library. //! -use std::{collections::HashMap, fs::OpenOptions}; +use std::{ + collections::HashMap, + ffi::{OsStr, OsString}, + hash::{Hash, Hasher}, + path::{Path, PathBuf}, + sync::{Arc, Mutex, OnceLock}, +}; use prebindgen::{get_prebindgen_out_dir, Record, RecordKind, SourceLocation, DEFAULT_GROUP_NAME}; use proc_macro::TokenStream; @@ -135,51 +141,208 @@ 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()); +#[derive(Default)] +struct CaptureState { + initialized: bool, } -/// Get the full path to `{group}_{pid}_{thread_id}.jsonl` generated in OUT_DIR. -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; +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(); + +/// 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 +/// 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` 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 { + 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()); } - 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() + + 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=")) + } + _ => arg + .strip_prefix("-C") + .and_then(|arg| arg.strip_prefix("metadata=")) + .or_else(|| arg.strip_prefix("--codegen=metadata=")), }; - 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; + if let Some(value) = value { + components.extend(value.split_whitespace()); } - random_value = Some(rand::random::()); - }; - JSONL_PATHS.with(|path| { - path.borrow_mut() - .insert(group.to_string(), new_path.clone()); - }); - new_path + } + components +} + +/// 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 { + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + value.hash(&mut hasher); + format!("{:016x}", hasher.finish()) +} + +/// Get the full path to `{group}_{unit_id}.jsonl` in OUT_DIR. +fn get_prebindgen_jsonl_path(group: &str) -> std::path::PathBuf { + 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_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() + )); + } + } + } + + 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. @@ -307,12 +470,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 @@ -439,3 +599,226 @@ 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 std::{ + collections::BTreeSet, + sync::{Arc, Barrier}, + }; + + 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(); + 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_every_rustc_codegen_spelling_and_hashes_the_value() { + let args = [ + "rustc", + "-Cmetadata=first", + "--codegen=metadata=second", + "-C", + "metadata=third/path\\unit", + ] + .map(Into::into); + let unit_id = unit_id_from_args(&args); + 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"][..])); + } + + #[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] + 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 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; + + 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(); + } + + #[test] + 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(dir).unwrap(); + } +} diff --git a/prebindgen/src/api/utils/jsonl.rs b/prebindgen/src/api/utils/jsonl.rs index 2b252ca3..340181d3 100644 --- a/prebindgen/src/api/utils/jsonl.rs +++ b/prebindgen/src/api/utils/jsonl.rs @@ -14,15 +14,17 @@ pub fn write_to_jsonl_file, R: Borrow>( file_path: P, records: &[R], ) -> Result<(), Box> { + // 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()); }; - // Check if file is empty (just created or was deleted) - for record in records { - let json_line = record.borrow().to_jsonl_string()?; - writeln!(file, "{json_line}")?; - } - file.flush()?; + file.write_all(buf.as_bytes())?; Ok(()) }