From 1bc1c2f29fb869f052dd92b0ac55e9ba8a664042 Mon Sep 17 00:00:00 2001 From: Benjamin Leggett Date: Wed, 5 Aug 2026 14:01:12 -0400 Subject: [PATCH 1/2] fix(cache): invalidate cache for .include and .incbin directives --- src/compiler/c.rs | 456 ++++++++++++++++++++++++++++- src/compiler/compiler.rs | 27 +- src/compiler/preprocessor_cache.rs | 2 +- src/compiler/rust.rs | 1 + tests/system.rs | 202 +++++++++++++ 5 files changed, 670 insertions(+), 18 deletions(-) diff --git a/src/compiler/c.rs b/src/compiler/c.rs index 7cf84b5059..a421a404d0 100644 --- a/src/compiler/c.rs +++ b/src/compiler/c.rs @@ -32,7 +32,7 @@ use crate::util::{ use async_trait::async_trait; use fs_err as fs; use std::borrow::Cow; -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::ffi::{OsStr, OsString}; use std::fmt; use std::hash::Hash; @@ -508,6 +508,9 @@ where ); return Ok(HashResult { key, + // Preprocessor cache mode is explicitly disabled for any + // translation unit containing an `.incbin`. + cacheable: Cacheable::Yes, compilation: Box::new(CCompilation { parsed_args: self.parsed_args.clone(), is_locally_preprocessed: false, @@ -620,6 +623,78 @@ where ) }; + // Files named by `.incbin` and `.include` are read by the assembler, so + // they are not part of the preprocessor output and have to be hashed + // separately. `.include` names assembly source, which can name further + // files in turn, so the queue grows as those are read. + let mut extra_hashes = extra_hashes; + let mut cacheable = Cacheable::Yes; + let mut pending: VecDeque<_> = find_asm_dependencies(&preprocessor_output).into(); + let mut included = HashSet::new(); + while let Some(dependency) = pending.pop_front() { + let (tag, name, assembled_in_place) = match &dependency { + AsmDependency::Embedded(name) => ("incbin", name, false), + AsmDependency::Source(name) => ("include", name, true), + AsmDependency::Unparsable => { + debug!( + "[{}]: Not cacheable: an assembler directive names a file we cannot identify", + self.parsed_args.output_pretty() + ); + cacheable = Cacheable::No; + break; + } + }; + + // gas resolves a relative operand against the working directory before + // consulting its own include path, so that is the only location we can + // resolve without replicating the assembler's search. An operand we + // cannot parse or a file we cannot hash makes the translation unit + // explicitly non-cacheable. + let path = decode_path(name).ok().map(|name| { + if name.is_absolute() { + name + } else { + cwd.join(name) + } + }); + + let hash = match &path { + // Included assembly is read rather than streamed, because the + // directives inside it have to be followed as well. + Some(path) if assembled_in_place => { + if !included.insert(path.clone()) { + // Already hashed and scanned: a repeated or cyclic include. + continue; + } + match fs::read(path) { + Ok(bytes) => { + pending.extend(find_asm_dependencies(&bytes)); + Digest::reader_sync(&bytes[..]).ok() + } + Err(_) => None, + } + } + Some(path) => Digest::file(path, pool).await.ok(), + None => None, + }; + + match hash { + // Tagged so these digests cannot collide with each other or with an + // entry contributed by `extra_hash_files`. + Some(hash) => extra_hashes.push(format!("{tag}:{hash}")), + None => { + debug!( + "[{}]: Not cacheable: cannot hash {:?} named by a .{tag} directive", + self.parsed_args.output_pretty(), + path.as_deref() + .unwrap_or(Path::new("")) + ); + cacheable = Cacheable::No; + break; + } + } + } + // Create an argument vector containing both common and arch args, to // use in creating a hash key let mut common_and_arch_args = self.parsed_args.common_args.clone(); @@ -667,6 +742,7 @@ where ); Ok(HashResult { key, + cacheable, compilation: Box::new(CCompilation { parsed_args: self.parsed_args.clone(), is_locally_preprocessed: true, @@ -702,6 +778,174 @@ const PRAGMA_GCC_PCH_PREPROCESS: &[u8] = b"pragma GCC pch_preprocess"; const HASH_31_COMMAND_LINE_NEWLINE: &[u8] = b"# 31 \"\"\n"; const HASH_32_COMMAND_LINE_2_NEWLINE: &[u8] = b"# 32 \"\" 2\n"; const INCBIN_DIRECTIVE: &[u8] = b".incbin"; +const INCLUDE_DIRECTIVE: &[u8] = b".include"; + +/// Upper bound on the length of a directive operand we are willing to parse. +/// A file name longer than this is treated as unparsable rather than scanned to +/// the end of the translation unit. +const OPERAND_MAX_LEN: usize = 4096; + +/// A file an assembler directive pulls in, which the preprocessor never reports. +#[derive(Debug, PartialEq, Eq)] +enum AsmDependency { + /// `.incbin "file"`: the file's bytes are embedded verbatim. + Embedded(Vec), + /// `.include "file"`: the file is assembly source assembled in place, so it + /// can pull in further files itself. + Source(Vec), + /// A directive whose operand is not a plain quoted file name, leaving the + /// file it names unknown. + Unparsable, +} + +/// The operand of an assembler directive that names a file. +#[derive(Debug, PartialEq, Eq)] +enum FileOperand { + /// The file name, exactly as written in the source. + Name(Vec), + /// The operand could not be parsed as a plain quoted file name. + Unparsable, +} + +/// Whether the quotes around a directive operand are backslash-escaped. +#[derive(Copy, Clone)] +enum OperandQuoting { + /// `"file"`, as written directly in assembly. + Plain, + /// `\"file\"`, as written inside a C string literal in inline assembly. + Escaped, +} + +/// Skips whitespace between an assembler directive and its operand. +/// +/// Inside a C string literal the whitespace may be spelled as a two-character +/// escape sequence rather than appearing literally, so both forms are accepted. +fn skip_asm_whitespace(bytes: &[u8]) -> (&[u8], bool) { + let mut rest = bytes; + let mut skipped = false; + loop { + rest = match rest { + [b' ' | b'\t', tail @ ..] => tail, + [b'\\', b't' | b'n' | b'r' | b'f' | b'v' | b' ', tail @ ..] => tail, + _ => return (rest, skipped), + }; + skipped = true; + } +} + +/// Parses the file operand of a directive from the bytes that immediately follow +/// the directive name, also returning how many of those bytes it spans. +/// +/// Returns `None` if the name did not end where the caller thought it did, as in +/// `.incbinary`. +fn parse_file_operand(after_directive: &[u8]) -> Option<(FileOperand, usize)> { + // A directive name continuing past the one we matched is a different directive. + if after_directive + .first() + .is_some_and(|c| c.is_ascii_alphanumeric() || matches!(c, b'_' | b'.' | b'$')) + { + return None; + } + + let (rest, skipped_whitespace) = skip_asm_whitespace(after_directive); + let (quoting, body) = if let Some(body) = rest.strip_prefix(b"\\\"") { + (OperandQuoting::Escaped, body) + } else if let Some(body) = rest.strip_prefix(b"\"") { + (OperandQuoting::Plain, body) + } else if skipped_whitespace { + // gas requires a quoted file name. Not finding one means the operand was + // produced some other way, so we cannot tell what this depends on. + return Some((FileOperand::Unparsable, 0)); + } else { + return None; + }; + + let terminator: &[u8] = match quoting { + OperandQuoting::Plain => b"\"", + OperandQuoting::Escaped => b"\\\"", + }; + let window = &body[..body.len().min(OPERAND_MAX_LEN)]; + let mut end = None; + let mut i = 0; + while i < window.len() { + if window[i..].starts_with(terminator) { + end = Some(i); + break; + } + // A quote, backslash or newline before the terminator means the operand + // is escaped, concatenated or otherwise not a plain file name. + if matches!(window[i], b'"' | b'\\' | b'\n') { + break; + } + i += 1; + } + + Some(match end { + Some(0) | None => (FileOperand::Unparsable, 0), + Some(end) => { + let spanned = after_directive.len() - body.len() + end + terminator.len(); + (FileOperand::Name(window[..end].to_vec()), spanned) + } + }) +} + +/// Matches an assembler directive naming a file at the start of `slice`, also +/// returning how many bytes of `slice` it spans. +/// +/// Returns `None` if no such directive starts there. +fn asm_dependency_at(slice: &[u8]) -> Option<(AsmDependency, usize)> { + let (wrap, directive_len, after_directive): (fn(Vec) -> AsmDependency, _, _) = + if let Some(rest) = slice.strip_prefix(INCBIN_DIRECTIVE) { + (AsmDependency::Embedded, INCBIN_DIRECTIVE.len(), rest) + } else if let Some(rest) = slice.strip_prefix(INCLUDE_DIRECTIVE) { + (AsmDependency::Source, INCLUDE_DIRECTIVE.len(), rest) + } else { + return None; + }; + + Some(match parse_file_operand(after_directive)? { + (FileOperand::Name(name), spanned) => (wrap(name), directive_len + spanned), + // The operand's extent is unknown, so resume right after the directive + // name; a spurious extra match is harmless once we are not caching. + (FileOperand::Unparsable, _) => (AsmDependency::Unparsable, directive_len), + }) +} + +/// Finds every file an assembler directive pulls into `bytes`, in the order the +/// directives appear. +/// +/// `.incbin` and `.include` are handled by the assembler, not the preprocessor, +/// so the files they name never appear in preprocessor output. Hashing that +/// output alone therefore misses the dependency entirely: the named file can +/// change while the cache key stays the same, and a stale object gets served, +/// which we do not want. +/// +/// `.include` names assembly source, so its contents have to be scanned in turn. +/// That is the caller's job, since it is the one reading the files. +fn find_asm_dependencies(bytes: &[u8]) -> Vec { + let mut dependencies = Vec::new(); + let mut offset = 0; + while offset < bytes.len() { + let Some(pos) = [INCBIN_DIRECTIVE, INCLUDE_DIRECTIVE] + .iter() + .filter_map(|directive| memchr::memmem::find(&bytes[offset..], directive)) + .min() + else { + break; + }; + let start = offset + pos; + match asm_dependency_at(&bytes[start..]) { + // Resume past the operand, so a file name that itself contains + // ".incbin" or ".include" is not mistaken for another directive. + Some((dependency, spanned)) => { + dependencies.push(dependency); + offset = start + spanned; + } + None => offset = start + 1, + } + } + dependencies +} /// Remember the include files in the preprocessor output if it can be cached. /// Returns `false` if preprocessor cache mode should be disabled. @@ -779,18 +1023,13 @@ fn process_preprocessed_file( continue; } } - } else if slice - .strip_prefix(INCBIN_DIRECTIVE) - .filter(|slice| { - slice.starts_with(b"\"") || slice.starts_with(b" \"") || slice.starts_with(b" \\\"") - }) - .is_some() - { - // An assembler .inc bin (without the space) statement, which could be - // part of inline assembly, refers to an external file. If the file - // changes, the hash should change as well, but finding out what file to - // hash is too hard for sccache, so just bail out. - debug!("Found potential unsupported .inc bin directive in source code"); + } else if asm_dependency_at(slice).is_some() { + // An assembler .inc bin or .include statement, which could be part of + // inline assembly, refers to an external file. Preprocessor cache mode + // keys on the include files alone and never sees the preprocessed + // output, so it cannot account for that file; `generate_hash_key` + // handles it for the normal path instead. + debug!("Found potential unsupported assembler file directive in source code"); return Ok(false); } else if slice.starts_with(b"___________") && (start == 0 || bytes[start - 1] == b'\n') { // Unfortunately the distcc-pump wrapper outputs standard output lines: @@ -1449,7 +1688,7 @@ impl pkg::ToolchainPackager for CToolchainPackager { } /// The cache is versioned by the inputs to `HashKeyParams::compute`. -pub const CACHE_VERSION: &[u8] = b"12"; +pub const CACHE_VERSION: &[u8] = b"13"; /// Environment variables that are factored into the cache key. static CACHED_ENV_VARS: LazyLock> = LazyLock::new(|| { @@ -1870,6 +2109,195 @@ mod test { t("Cu"); } + /// Convenience for asserting on a source fragment holding a single directive. + fn asm_dependency(source: &str) -> Option { + let mut dependencies = find_asm_dependencies(source.as_bytes()); + assert!(dependencies.len() <= 1, "expected at most one dependency"); + dependencies.pop() + } + + fn embedded_file(source: &str) -> Option { + match asm_dependency(source) { + Some(AsmDependency::Embedded(name)) => Some(String::from_utf8(name).unwrap()), + _ => None, + } + } + + #[test] + fn test_asm_dependency_plain_assembly() { + // As written directly in a .S file. + assert_eq!( + embedded_file("\t.incbin \"certs/signing_key.x509\"\n"), + Some("certs/signing_key.x509".to_string()) + ); + // Tab between the directive and its operand, as in the kernel's rmpiggy.S. + assert_eq!( + embedded_file("\t.incbin\t\"realmode.bin\"\n"), + Some("realmode.bin".to_string()) + ); + // gas does not require any separator before a quoted operand. + assert_eq!( + embedded_file(".incbin\"blob.bin\""), + Some("blob.bin".to_string()) + ); + // A skip/count suffix does not change which file is embedded. + assert_eq!( + embedded_file(".incbin \"blob.bin\",16,32\n"), + Some("blob.bin".to_string()) + ); + } + + #[test] + fn test_asm_dependency_include_is_source() { + // `.include` pulls in assembly source rather than embedding bytes, so it is + // distinguished: its contents have to be scanned for further directives. + assert_eq!( + asm_dependency("\t.include \"macros.s\"\n"), + Some(AsmDependency::Source(b"macros.s".to_vec())) + ); + assert_eq!( + asm_dependency("\"\t.include \\\"macros.s\\\"\\n\""), + Some(AsmDependency::Source(b"macros.s".to_vec())) + ); + // A longer directive that merely starts with ".include". + assert_eq!(asm_dependency(".included \"macros.s\""), None); + // Embedding and including the same file must not hash the same way. + assert_ne!( + asm_dependency(".incbin \"x\"\n"), + asm_dependency(".include \"x\"\n") + ); + } + + #[test] + fn test_asm_dependency_in_c_string_literal() { + // Inline assembly, where the operand's quotes are backslash-escaped. This + // is how the Linux kernel's kernel/configs.c embeds config_data.gz. + assert_eq!( + embedded_file("\"\t.incbin \\\"kernel/config_data.gz\\\"\t\\n\""), + Some("kernel/config_data.gz".to_string()) + ); + // Whitespace spelled as an escape sequence rather than appearing literally. + assert_eq!( + embedded_file("\"\\t.incbin\\t\\\"blob.bin\\\"\\n\""), + Some("blob.bin".to_string()) + ); + } + + #[test] + fn test_asm_dependency_not_a_directive() { + // A longer directive that merely starts with ".incbin". + assert_eq!(asm_dependency(".incbinary \"blob.bin\""), None); + assert_eq!(asm_dependency(".incbin_data \"blob.bin\""), None); + // The bare word with no operand at all is not something we can attribute a + // file to, but neither is it a directive we recognize. + assert_eq!(asm_dependency(".incbin"), None); + } + + #[test] + fn test_asm_dependency_unparsable_operands() { + let unparsable = |source| { + assert_eq!( + asm_dependency(source), + Some(AsmDependency::Unparsable), + "{source:?}" + ); + }; + // Not a quoted string: assembled from something we cannot evaluate. + unparsable(".incbin BLOB_PATH\n"); + unparsable(".include MACRO_PATH\n"); + // Operand split across two C string literals, which the compiler + // concatenates but we cannot. + unparsable("\".incbin \\\"bl\" \"ob.bin\\\"\\n\""); + // Never terminated. + unparsable(".incbin \"blob.bin\n"); + // Empty file name. + unparsable(".incbin \"\"\n"); + // An escape inside the file name. + unparsable(".incbin \"bl\\ob.bin\"\n"); + // Longer than we are willing to scan. + let long = "x".repeat(OPERAND_MAX_LEN + 1); + unparsable(&format!(".incbin \"{long}\"\n")); + } + + #[test] + fn test_asm_dependency_multiple_directives() { + // certs/system_certificates.S embeds two files in one translation unit, + // and both have to be accounted for. + let source = "__module_cert_start:\n\t.incbin \"certs/signing_key.x509\"\n\ + __module_cert_end:\n\t.incbin \"certs/x509_certificate_list\"\n\ + \t.include \"trailer.s\"\n"; + assert_eq!( + find_asm_dependencies(source.as_bytes()), + vec![ + AsmDependency::Embedded(b"certs/signing_key.x509".to_vec()), + AsmDependency::Embedded(b"certs/x509_certificate_list".to_vec()), + AsmDependency::Source(b"trailer.s".to_vec()), + ] + ); + } + + #[test] + fn test_asm_dependency_directive_name_inside_operand() { + // The scan resumes past the operand, so a file whose own name contains a + // directive name does not look like a second directive. + assert_eq!( + find_asm_dependencies(b"\t.incbin \"payload.incbin\"\n"), + vec![AsmDependency::Embedded(b"payload.incbin".to_vec())] + ); + assert_eq!( + find_asm_dependencies(b"\t.include \"a.include\"\n\t.incbin \"b.bin\"\n"), + vec![ + AsmDependency::Source(b"a.include".to_vec()), + AsmDependency::Embedded(b"b.bin".to_vec()), + ] + ); + } + + #[test] + fn test_asm_dependency_absent() { + assert!(find_asm_dependencies(b"int main(void) { return 0; }\n").is_empty()); + assert!(find_asm_dependencies(b"").is_empty()); + } + + #[test] + fn test_process_preprocessed_file_asm_directive_disables_preprocessor_cache() { + // Preprocessor cache mode keys on include files only and never sees the + // preprocessed output, so it cannot account for a file named by an + // assembler directive and has to be turned off for every form the scanner + // recognizes. + for source in [ + "\t.incbin \"blob.bin\"\n", + "\t.incbin\t\"blob.bin\"\n", + "\"\t.incbin \\\"blob.bin\\\"\t\\n\"\n", + ".incbin BLOB_PATH\n", + "\t.include \"macros.s\"\n", + "\t.include\t\"macros.s\"\n", + "\"\t.include \\\"macros.s\\\"\t\\n\"\n", + ".include MACRO_PATH\n", + ] { + // Pad past the 7-byte minimum the scan loop requires. + let mut bytes = format!("# 1 \"test.c\"\n{source} ").into_bytes(); + let success = process_preprocessed_file( + Path::new("test.c"), + Path::new(""), + &mut bytes, + &mut HashMap::new(), + PreprocessorCacheModeConfig { + use_preprocessor_cache_mode: true, + skip_system_headers: true, + ..Default::default() + }, + std::time::SystemTime::now(), + StandardFsAbstraction, + ) + .unwrap(); + assert!( + !success, + "{source:?} should disable preprocessor cache mode" + ); + } + } + #[test] fn test_process_preprocessed_file() { env_logger::builder() diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs index 78bb5a4332..ceb0cf3476 100644 --- a/src/compiler/compiler.rs +++ b/src/compiler/compiler.rs @@ -556,7 +556,7 @@ where out_pretty, fmt_duration_as_secs(&start.elapsed()) ); - let (key, compilation, weak_toolchain_key) = match result { + let (key, key_cacheable, compilation, weak_toolchain_key) = match result { Err(e) => { return match e.downcast::() { Ok(ProcessError(output)) => { @@ -568,15 +568,19 @@ where } Ok(HashResult { key, + cacheable, compilation, weak_toolchain_key, - }) => (key, compilation, weak_toolchain_key), + }) => (key, cacheable, compilation, weak_toolchain_key), }; debug!("[{}]: Hash key: {}", out_pretty, key); // If `ForceRecache` is enabled, we won't check the cache. let start = Instant::now(); let cache_status = async { - if cache_control == CacheControl::ForceNoCache { + // A key that does not cover every dependency must not be looked up, + // an entry stored under it may have been produced from different + // inputs. + if key_cacheable == Cacheable::No || cache_control == CacheControl::ForceNoCache { Ok(Cache::None) } else if cache_control == CacheControl::ForceRecache { Ok(Cache::Recache) @@ -746,6 +750,18 @@ where compiler_result, )); } + if key_cacheable != Cacheable::Yes { + // The hash key does not cover every dependency, so we cannot cache it. + debug!( + "[{}]: Compiled in {}, but the hash key is not cacheable", + out_pretty, + fmt_duration_as_secs(&duration_compilation) + ); + return Ok(( + CompileResult::NotCacheable(dist_type, duration_compilation), + compiler_result, + )); + } if miss_type == MissType::ForcedNoCache { // Do not cache debug!( @@ -1151,6 +1167,11 @@ where { /// The hash key of the inputs. pub key: String, + /// Whether `key` accounts for everything the compilation depends on. + /// + /// `Cacheable::No` means a dependency was found that the key cannot cover, + /// so the cache must be neither read nor written for this compilation. + pub cacheable: Cacheable, /// An object to use for the actual compilation, if necessary. pub compilation: Box + 'static>, /// A weak key that may be used to identify the toolchain diff --git a/src/compiler/preprocessor_cache.rs b/src/compiler/preprocessor_cache.rs index 61cc889f6d..880a38d5a2 100644 --- a/src/compiler/preprocessor_cache.rs +++ b/src/compiler/preprocessor_cache.rs @@ -41,7 +41,7 @@ use super::Language; /// The current format is 1 header byte for the version + bincode encoding /// of the [`PreprocessorCacheEntry`] struct. -const FORMAT_VERSION: u8 = 0; +const FORMAT_VERSION: u8 = 1; const MAX_PREPROCESSOR_CACHE_ENTRIES: usize = 100; const MAX_PREPROCESSOR_CACHE_FILE_INFO_ENTRIES: usize = 10000; diff --git a/src/compiler/rust.rs b/src/compiler/rust.rs index 9952713399..956649bfda 100644 --- a/src/compiler/rust.rs +++ b/src/compiler/rust.rs @@ -1679,6 +1679,7 @@ where Ok(HashResult { key: m.finish(), + cacheable: Cacheable::Yes, compilation: Box::new(RustCompilation { executable: self.executable.clone(), host: self.host.clone(), diff --git a/tests/system.rs b/tests/system.rs index fbbaeca96b..60bbaea797 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -567,6 +567,205 @@ int main(int argc, char** argv) { }); } +/// The assembler reads the file named by `.incbin` directly, so its contents never +/// appear in preprocessor output and hashing that output alone does not see them. +/// Changing the embedded file has to change the cache key, or a stale object gets +/// served for the new contents. +fn test_incbin_embedded_file_changes(compiler: Compiler, tempdir: &Path) { + let Compiler { + name, + exe, + env_vars, + } = compiler; + println!("test_incbin_embedded_file_changes: {}", name); + zero_stats(); + + const SRC: &str = "incbin.c"; + const BLOB: &str = "blob.bin"; + write_source( + tempdir, + SRC, + "__asm__(\".incbin \\\"blob.bin\\\"\\n\"); +int main(int argc, char** argv) { + return 0; +} +", + ); + write_source(tempdir, BLOB, "embedded contents, take one"); + + let args = compile_cmdline(name, exe, SRC, OUTPUT, Vec::new()); + let compile = |env_vars: Vec<(OsString, OsString)>| { + sccache_command() + .args(&args) + .current_dir(tempdir) + .envs(env_vars) + .assert() + .success(); + }; + + trace!("compile with the original blob"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(0, info.stats.cache_hits.all()); + assert_eq!(1, info.stats.cache_misses.all()); + }); + + // Nothing changed, so an embedded file must not cost us the cache entirely. + trace!("compile again unchanged"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(1, info.stats.cache_hits.all()); + assert_eq!(1, info.stats.cache_misses.all()); + }); + + // The source preprocesses identically, so only the blob's contents distinguish + // this compilation from the previous one. + trace!("compile with a changed blob"); + write_source(tempdir, BLOB, "embedded contents, take two"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(1, info.stats.cache_hits.all()); + assert_eq!(2, info.stats.cache_misses.all()); + }); + + // Keyed on the blob's contents rather than merely noticing that it changed. + trace!("compile with the original blob restored"); + write_source(tempdir, BLOB, "embedded contents, take one"); + compile(env_vars); + get_stats(|info| { + assert_eq!(2, info.stats.cache_hits.all()); + assert_eq!(2, info.stats.cache_misses.all()); + }); +} + +/// `.include` pulls in assembly source, which is likewise absent from preprocessor +/// output, and which can name further files of its own. Every file reachable that +/// way has to reach the cache key, however deep. +fn test_include_transitive_dependency_changes(compiler: Compiler, tempdir: &Path) { + let Compiler { + name, + exe, + env_vars, + } = compiler; + println!("test_include_transitive_dependency_changes: {}", name); + zero_stats(); + + const SRC: &str = "include.c"; + const OUTER: &str = "outer.s"; + const INNER: &str = "inner.s"; + const BLOB: &str = "nested_blob.bin"; + write_source( + tempdir, + SRC, + "__asm__(\".include \\\"outer.s\\\"\\n\"); +int main(int argc, char** argv) { + return 0; +} +", + ); + // The source names outer.s, which names inner.s, which embeds the blob. None of + // the three appears anywhere in the preprocessed output. + write_source(tempdir, OUTER, "\t.include \"inner.s\"\n"); + write_source(tempdir, INNER, "\t.incbin \"nested_blob.bin\"\n"); + write_source(tempdir, BLOB, "nested contents, take one"); + + let args = compile_cmdline(name, exe, SRC, OUTPUT, Vec::new()); + let compile = |env_vars: Vec<(OsString, OsString)>| { + sccache_command() + .args(&args) + .current_dir(tempdir) + .envs(env_vars) + .assert() + .success(); + }; + + trace!("compile the include chain"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(0, info.stats.cache_hits.all()); + assert_eq!(1, info.stats.cache_misses.all()); + }); + + trace!("compile again unchanged"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(1, info.stats.cache_hits.all()); + assert_eq!(1, info.stats.cache_misses.all()); + }); + + // Two levels down from the translation unit, reachable only by following + // `.include` into `.incbin`. + trace!("compile with the transitively embedded blob changed"); + write_source(tempdir, BLOB, "nested contents, take two"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(1, info.stats.cache_hits.all()); + assert_eq!(2, info.stats.cache_misses.all()); + }); + + // One level down: the included source itself, rather than what it embeds. + trace!("compile with the intermediate included source changed"); + write_source(tempdir, INNER, "\t.incbin \"nested_blob.bin\"\n\tnop\n"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(1, info.stats.cache_hits.all()); + assert_eq!(3, info.stats.cache_misses.all()); + }); + + // Restoring the whole chain returns us to a key seen before. + trace!("compile with the chain restored"); + write_source(tempdir, INNER, "\t.incbin \"nested_blob.bin\"\n"); + write_source(tempdir, BLOB, "nested contents, take one"); + compile(env_vars); + get_stats(|info| { + assert_eq!(2, info.stats.cache_hits.all()); + assert_eq!(3, info.stats.cache_misses.all()); + }); +} + +/// An `.incbin` operand we cannot resolve to a file leaves us unable to tell what +/// the translation unit depends on, so it must not be cached at all. Caching it +/// under a key that ignores the embedded file is what produces stale objects. +fn test_incbin_unresolvable_operand_not_cacheable(compiler: Compiler, tempdir: &Path) { + let Compiler { + name, + exe, + env_vars, + } = compiler; + println!("test_incbin_unresolvable_operand_not_cacheable: {}", name); + zero_stats(); + + const SRC: &str = "incbin_split.c"; + const BLOB: &str = "blob.bin"; + // The compiler concatenates the adjacent string literals and assembles + // `.incbin "blob.bin"`; we only see an operand split across two literals. + write_source( + tempdir, + SRC, + "__asm__(\".incbin \\\"bl\" \"ob.bin\\\"\\n\"); +int main(int argc, char** argv) { + return 0; +} +", + ); + write_source(tempdir, BLOB, "embedded contents"); + + let args = compile_cmdline(name, exe, SRC, OUTPUT, Vec::new()); + for _ in 0..2 { + sccache_command() + .args(&args) + .current_dir(tempdir) + .envs(env_vars.clone()) + .assert() + .success(); + } + get_stats(|info| { + assert_eq!(0, info.stats.cache_hits.all()); + assert_eq!(0, info.stats.cache_misses.all()); + assert_eq!(2, info.stats.non_cacheable_compilations); + }); +} + /* test case like this: echo "int test(){}" > test.cc mkdir o1 o2 @@ -755,6 +954,9 @@ fn run_sccache_command_tests(compiler: Compiler, tempdir: &Path, preprocessor_ca test_gcc_clang_no_warnings_from_macro_expansion(compiler.clone(), tempdir); test_split_dwarf_object_generate_output_dir_changes(compiler.clone(), tempdir); test_gcc_clang_depfile(compiler.clone(), tempdir); + test_incbin_embedded_file_changes(compiler.clone(), tempdir); + test_include_transitive_dependency_changes(compiler.clone(), tempdir); + test_incbin_unresolvable_operand_not_cacheable(compiler.clone(), tempdir); } if compiler.name == "clang++" { test_clang_multicall(compiler.clone(), tempdir); From 972312ea69716f3ef4d40ab39ed3f3c8765d2d11 Mon Sep 17 00:00:00 2001 From: Benjamin Leggett Date: Wed, 5 Aug 2026 16:56:25 -0400 Subject: [PATCH 2/2] fix(cache): also fix a few more similar issues --- src/compiler/c.rs | 45 +++++++++++++++++--- src/compiler/clang.rs | 44 ++++++++++++++++++++ src/compiler/gcc.rs | 96 ++++++++++++++++++++++++++++++++++++++++++- src/compiler/msvc.rs | 1 + tests/system.rs | 80 ++++++++++++++++++++++++++++++++++++ 5 files changed, 260 insertions(+), 6 deletions(-) diff --git a/src/compiler/c.rs b/src/compiler/c.rs index a421a404d0..2cf9762bd7 100644 --- a/src/compiler/c.rs +++ b/src/compiler/c.rs @@ -27,7 +27,7 @@ use crate::dist::pkg; use crate::mock_command::CommandCreatorSync; use crate::util::{ Digest, HashToDigest, MetadataCtimeExt, TimeMacroFinder, Timestamp, decode_path, encode_path, - hash_all, strip_basedirs, + strip_basedirs, }; use async_trait::async_trait; use fs_err as fs; @@ -378,7 +378,28 @@ where ) -> Result> { let start_of_compilation = std::time::SystemTime::now(); - let extra_hashes = hash_all(&self.parsed_args.extra_hash_files, &pool.clone()).await?; + // An argument may name a file whose contents the compiler reads directly, + // making it an input the preprocessor never reports. A file we cannot hash + // leaves us unable to tell what the compilation depends on, so it must not + // be cached. + let mut cacheable = Cacheable::Yes; + let mut extra_hashes = Vec::with_capacity(self.parsed_args.extra_hash_files.len()); + for path in &self.parsed_args.extra_hash_files { + match Digest::file(path, pool).await { + Ok(hash) => extra_hashes.push(hash), + Err(e) => { + debug!( + "[{}]: Not cacheable: cannot hash {:?}: {}", + self.parsed_args.output_pretty(), + path, + e + ); + cacheable = Cacheable::No; + break; + } + } + } + // Create an argument vector containing both preprocessor and arch args, to // use in creating a hash key let mut preprocessor_and_arch_args = self.parsed_args.preprocessor_args.clone(); @@ -409,9 +430,13 @@ where let needs_preprocessing = self.parsed_args.language.needs_c_preprocessing(); let use_preprocessor_cache_mode = if needs_preprocessing { + // Preprocessor cache mode maps include files to a hash key computed + // elsewhere, so it must not record one for a compilation whose key we + // already know is untrustworthy. let can_use_preprocessor_cache_mode = preprocessor_cache_mode_config .use_preprocessor_cache_mode - && !too_hard_for_preprocessor_cache_mode; + && !too_hard_for_preprocessor_cache_mode + && cacheable == Cacheable::Yes; let mut use_preprocessor_cache_mode = can_use_preprocessor_cache_mode; @@ -627,8 +652,6 @@ where // they are not part of the preprocessor output and have to be hashed // separately. `.include` names assembly source, which can name further // files in turn, so the queue grows as those are read. - let mut extra_hashes = extra_hashes; - let mut cacheable = Cacheable::Yes; let mut pending: VecDeque<_> = find_asm_dependencies(&preprocessor_output).into(); let mut included = HashSet::new(); while let Some(dependency) = pending.pop_front() { @@ -1704,6 +1727,18 @@ static CACHED_ENV_VARS: LazyLock> = LazyLock::new(|| { "WATCHOS_DEPLOYMENT_TARGET", "SDKROOT", "CCC_OVERRIDE_OPTIONS", + // Selects which cc1/as the driver runs, so it changes the generated code + // without changing the driver binary we hash. + "COMPILER_PATH", + // Turns on -fcompare-debug, which changes what the compiler does. + "GCC_COMPARE_DEBUG", + // Adds include directories. Preprocessing reflects these, but preprocessor + // cache mode skips preprocessing and would keep matching the files it + // recorded before the search path changed. + "CPATH", + "C_INCLUDE_PATH", + "CPLUS_INCLUDE_PATH", + "OBJC_INCLUDE_PATH", ] .iter() .map(OsStr::new) diff --git a/src/compiler/clang.rs b/src/compiler/clang.rs index bd7a980092..012e09178b 100644 --- a/src/compiler/clang.rs +++ b/src/compiler/clang.rs @@ -223,6 +223,14 @@ counted_array!(pub static ARGS: [ArgInfo; _] = [ take_arg!("-fprofile-instr-use", PathBuf, Concatenated(b'='), ClangProfileUse), // Note: this overrides the -fprofile-use option in gcc.rs. take_arg!("-fprofile-use", PathBuf, Concatenated(b'='), ClangProfileUse), + // The seed file's contents decide struct field ordering, so two builds that + // differ only in this file must not share an object. + take_arg!( + "-frandomize-layout-seed-file", + PathBuf, + Concatenated(b'='), + ExtraHashFile + ), take_arg!("-fsanitize-blacklist", PathBuf, Concatenated(b'='), ExtraHashFile), take_arg!("-fsanitize-ignorelist", PathBuf, Concatenated(b'='), ExtraHashFile), flag!("-fuse-ctor-homing", PassThroughFlag), @@ -1011,6 +1019,42 @@ mod test { ); } + /// The seed file decides struct field ordering under randstruct, so two builds + /// differing only in its contents must not share an object. + #[test] + fn test_parse_frandomize_layout_seed_file() { + let a = parses!( + "-c", + "foo.c", + "-o", + "foo.o", + "-frandomize-layout-seed-file=seed.txt" + ); + assert_eq!( + ovec!["-frandomize-layout-seed-file=seed.txt"], + a.common_args + ); + assert_eq!( + ovec![std::env::current_dir().unwrap().join("seed.txt")], + a.extra_hash_files + ); + } + + /// The string form carries its value in the argument itself, which is already + /// hashed, so it must not be mistaken for the file form. + #[test] + fn test_parse_frandomize_layout_seed_string_not_a_file() { + let a = parses!( + "-c", + "foo.c", + "-o", + "foo.o", + "-frandomize-layout-seed=abcdef" + ); + assert!(a.extra_hash_files.is_empty()); + assert_eq!(ovec!["-frandomize-layout-seed=abcdef"], a.common_args); + } + #[test] fn test_parse_fsanitize_blacklist() { let a = parses!( diff --git a/src/compiler/gcc.rs b/src/compiler/gcc.rs index 8a832b5d68..321843e632 100644 --- a/src/compiler/gcc.rs +++ b/src/compiler/gcc.rs @@ -161,6 +161,11 @@ ArgData! { pub Coverage, ModuleOnlyFlag, ExtraHashFile(PathBuf), + // For arguments naming a file whose contents are an input, where the value is + // a path only when it contains a directory separator; otherwise the compiler + // resolves the bare name against a search path of its own that we cannot + // reproduce. Used by -fplugin and -specs. + ExtraHashFileRequiringPath(PathBuf), // Only valid for clang, but this needs to be here since clang shares gcc's arg parsing. // For -fmodule-file which can be either "path" or "name=path" ExtraHashFileClangModuleFile(OsString), @@ -186,6 +191,9 @@ counted_array!(pub static ARGS: [ArgInfo; _] = [ flag!("--save-temps=cwd", TooHardFlag), flag!("--save-temps=obj", TooHardFlag), take_arg!("--serialize-diagnostics", PathBuf, Separated, SerializeDiagnostics), + // A spec file rewrites the command line, so it can change anything about the + // compilation while the arguments we hash stay the same. + take_arg!("--specs", PathBuf, CanBeConcatenated(b'='), ExtraHashFileRequiringPath), take_arg!("--sysroot", PathBuf, Separated, PassThroughPath), take_arg!("-A", OsString, Separated, PassThrough), take_arg!("-B", PathBuf, CanBeSeparated, PassThroughPath), @@ -227,7 +235,11 @@ counted_array!(pub static ARGS: [ArgInfo; _] = [ flag!("-fno-profile-generate", TooHardFlag), flag!("-fno-profile-use", TooHardFlag), flag!("-fno-working-directory", PreprocessorArgumentFlag), - flag!("-fplugin=libcc1plugin", TooHardFlag), + // A plugin's code runs as part of the compilation, so its contents are an + // input rather than just the path naming it. This also covers the one plugin + // that used to be singled out here, libcc1plugin, which is passed as a bare + // name and so lands in the same bucket as any other name we cannot resolve. + take_arg!("-fplugin", PathBuf, CanBeConcatenated(b'='), ExtraHashFileRequiringPath), flag!("-fprofile-arcs", ProfileGenerate), flag!("-fprofile-generate", ProfileGenerate), take_arg!("-fprofile-use", OsString, Concatenated, TooHard), @@ -260,6 +272,7 @@ counted_array!(pub static ARGS: [ArgInfo; _] = [ flag!("-save-temps", TooHardFlag), flag!("-save-temps=cwd", TooHardFlag), flag!("-save-temps=obj", TooHardFlag), + take_arg!("-specs", PathBuf, CanBeConcatenated(b'='), ExtraHashFileRequiringPath), take_arg!("-std", OsString, Concatenated(b'='), Standard), take_arg!("-stdlib", OsString, Concatenated(b'='), PreprocessorArgument), flag!("-trigraphs", PreprocessorArgumentFlag), @@ -420,6 +433,7 @@ where serialize_diagnostics = Some(path.clone()); } Some(ExtraHashFile(_)) + | Some(ExtraHashFileRequiringPath(_)) | Some(ExtraHashFileClangModuleFile(_)) | Some(PassThroughFlag) | Some(PreprocessorArgumentFlag) @@ -502,6 +516,19 @@ where extra_hash_files.push(cwd.join(path)); &mut common_args } + Some(ExtraHashFileRequiringPath(path)) => { + // Without a directory separator the compiler resolves this name + // itself, against its plugin directory or its spec search path. We + // cannot reproduce that, and guessing at the working directory + // could hash an unrelated file that happens to share the name. + // (Note: ccache queries the compiler with -print-file-name to resolve + // spec files, we settle for not caching.) + if path.components().count() < 2 { + cannot_cache!("file argument without a path"); + } + extra_hash_files.push(cwd.join(path)); + &mut common_args + } Some(ExtraHashFileClangModuleFile(val)) => { // -fmodule-file can be either "path" or "name=path" let val_str = val.to_string_lossy(); @@ -571,6 +598,7 @@ where | Some(ClangModuleOutput(_)) | Some(TooHardFlag) | Some(XClang(_)) + | Some(ExtraHashFileRequiringPath(_)) | Some(TooHard(_)) => cannot_cache!( arg.flag_str() .unwrap_or("Can't handle complex arguments through clang",) @@ -1809,6 +1837,72 @@ mod test { ); } + /// A gcc plugin's code runs as part of the compilation, so the object depends + /// on the plugin's contents, not just on the path naming it. + #[test] + fn test_parse_arguments_fplugin_hashes_the_plugin() { + for args in [ + stringvec!["-c", "foo.c", "-o", "foo.o", "-fplugin=./plugin.so"], + stringvec!["-c", "foo.c", "-o", "foo.o", "-fplugin", "./plugin.so"], + stringvec![ + "-c", + "foo.c", + "-o", + "foo.o", + "-fplugin=scripts/gcc-plugins/structleak_plugin.so" + ], + ] { + let a = match parse_arguments_(args.clone(), false) { + CompilerArguments::Ok(a) => a, + o => panic!("Got unexpected parse result: {o:?} for {args:?}"), + }; + assert_eq!(1, a.extra_hash_files.len(), "{args:?}"); + } + } + + /// A spec file rewrites the command line, so its contents are an input even + /// though the arguments we hash do not change with it. + #[test] + fn test_parse_arguments_specs_hashes_the_spec_file() { + for args in [ + stringvec!["-c", "foo.c", "-o", "foo.o", "-specs=./hardened.spec"], + stringvec!["-c", "foo.c", "-o", "foo.o", "-specs", "./hardened.spec"], + stringvec![ + "-c", + "foo.c", + "-o", + "foo.o", + "--specs=/usr/lib/rpm/cc1.spec" + ], + ] { + let a = match parse_arguments_(args.clone(), false) { + CompilerArguments::Ok(a) => a, + o => panic!("Got unexpected parse result: {o:?} for {args:?}"), + }; + assert_eq!(1, a.extra_hash_files.len(), "{args:?}"); + } + } + + /// A plugin or spec file named without a path is resolved by the compiler + /// against a search path of its own, so there is nothing for us to hash. + /// libcc1plugin is one of these. + #[test] + fn test_parse_arguments_file_argument_without_path_refused() { + for arg in [ + "-fplugin=libcc1plugin", + "-fplugin=structleak_plugin.so", + "-specs=hardened.spec", + ] { + let args = stringvec!["-c", "foo.c", "-o", "foo.o", arg]; + match parse_arguments_(args, false) { + CompilerArguments::CannotCache(reason, None) => { + assert_eq!(reason, "file argument without a path", "{arg}"); + } + o => panic!("Got unexpected parse result: {o:?} for {arg}"), + } + } + } + #[test] fn test_parse_arguments_too_hard() { let too_hard_flags = stringvec![ diff --git a/src/compiler/msvc.rs b/src/compiler/msvc.rs index 900501f6a6..ccb9f6ea21 100644 --- a/src/compiler/msvc.rs +++ b/src/compiler/msvc.rs @@ -690,6 +690,7 @@ pub fn parse_arguments( | Some(ClangModuleOutput(_)) | Some(ExtraHashFileClangModuleFile(_)) | Some(ModuleOnlyFlag) + | Some(ExtraHashFileRequiringPath(_)) | Some(TooHard(_)) => cannot_cache!( arg.flag_str() .unwrap_or("Can't handle complex arguments through clang",) diff --git a/tests/system.rs b/tests/system.rs index 60bbaea797..46559395a2 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -638,6 +638,83 @@ int main(int argc, char** argv) { }); } +/// The randstruct seed file decides struct field ordering. Nothing about it reaches +/// the preprocessor, so two builds differing only in the seed preprocess to the same +/// bytes, and so without hashing its contents they share an object and one of them gets a +/// struct layout the compiler never chose for it, which Is Bad. +fn test_randomize_layout_seed_file_changes(compiler: Compiler, tempdir: &Path) { + let Compiler { + name, + exe, + env_vars, + } = compiler; + println!("test_randomize_layout_seed_file_changes: {}", name); + + const SRC: &str = "randlayout.c"; + const SEED: &str = "randstruct.seed"; + write_source( + tempdir, + SRC, + "struct __attribute__((randomize_layout)) S { int a; long b; char c; void *d; }; +int off(void) { return __builtin_offsetof(struct S, d); } +int main(int argc, char** argv) { + return 0; +} +", + ); + write_source(tempdir, SEED, "seed one aaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"); + + // Older compilers do not support randstruct at all. Probe the compiler + // directly, so the probe does not populate the cache we are about to measure. + let probe = Command::new(&exe) + .args(["-fsyntax-only", SRC]) + .arg(format!("-frandomize-layout-seed-file={}", SEED)) + .current_dir(tempdir) + .envs(env_vars.clone()) + .output() + .expect("Failed to probe for randstruct support"); + if !probe.status.success() { + println!(" compiler does not support randstruct, skipping"); + return; + } + zero_stats(); + + let mut args = compile_cmdline(name, exe, SRC, OUTPUT, Vec::new()); + args.push(format!("-frandomize-layout-seed-file={}", SEED).into()); + + let compile = |env_vars: Vec<(OsString, OsString)>| { + sccache_command() + .args(&args) + .current_dir(tempdir) + .envs(env_vars) + .assert() + .success(); + }; + + trace!("compile with the first seed"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(0, info.stats.cache_hits.all()); + assert_eq!(1, info.stats.cache_misses.all()); + }); + + trace!("compile again unchanged"); + compile(env_vars.clone()); + get_stats(|info| { + assert_eq!(1, info.stats.cache_hits.all()); + assert_eq!(1, info.stats.cache_misses.all()); + }); + + // The source is untouched, so only the seed distinguishes this compilation. + trace!("compile with a changed seed"); + write_source(tempdir, SEED, "seed two bbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"); + compile(env_vars); + get_stats(|info| { + assert_eq!(1, info.stats.cache_hits.all()); + assert_eq!(2, info.stats.cache_misses.all()); + }); +} + /// `.include` pulls in assembly source, which is likewise absent from preprocessor /// output, and which can name further files of its own. Every file reachable that /// way has to reach the cache key, however deep. @@ -958,6 +1035,9 @@ fn run_sccache_command_tests(compiler: Compiler, tempdir: &Path, preprocessor_ca test_include_transitive_dependency_changes(compiler.clone(), tempdir); test_incbin_unresolvable_operand_not_cacheable(compiler.clone(), tempdir); } + if compiler.name == "clang" { + test_randomize_layout_seed_file_changes(compiler.clone(), tempdir); + } if compiler.name == "clang++" { test_clang_multicall(compiler.clone(), tempdir); }