From 81500b236065a338dd40f3473ac1f83c78abca1b Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Mon, 29 Jun 2026 13:43:02 +0300 Subject: [PATCH 1/2] gcc/clang: cache and distribute builds using quoted @response files Compilations that pass arguments via a `@file` response file were silently treated as uncacheable whenever the file contained a single or double quote. Change response-file parsing to handle quoting correctly. Add unit tests covering the tokenizer edge cases and an end-to-end test verifying a response file with a quoted argument parses and stays cacheable. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/ResponseFiles.md | 4 +- src/compiler/gcc.rs | 162 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 154 insertions(+), 12 deletions(-) diff --git a/docs/ResponseFiles.md b/docs/ResponseFiles.md index bb20a62bf0..a776d58a98 100644 --- a/docs/ResponseFiles.md +++ b/docs/ResponseFiles.md @@ -13,8 +13,8 @@ As defined by the [gcc docs](https://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Overal 5. The response file may itself contain additional @file options; any such options will be processed recursively. Implementation details: -- The gcc implementation in sccache supports all of these **except** #3. If a response file contains **any** quotations (`"` or `'`), the @file arg is treated literally and not removed (and its content not processed). -- Additionally, sccache will not expand concatenated arguments such as `-include@foo` (see [#150](https://github.com/mozilla/sccache/issues/150#issuecomment-318586953) for more on this). +- The gcc implementation in sccache supports all of these. +- sccache will not expand concatenated arguments such as `-include@foo` (see [#150](https://github.com/mozilla/sccache/issues/150#issuecomment-318586953) for more on this). - Recursive files are processed depth-first; when an @file option is encountered, its contents are read and each option is evaluated in-place before continuing to options following the @file. ## MSVC diff --git a/src/compiler/gcc.rs b/src/compiler/gcc.rs index 7bd4f96891..bb7f9cc3e9 100644 --- a/src/compiler/gcc.rs +++ b/src/compiler/gcc.rs @@ -1099,13 +1099,18 @@ impl Iterator for ExpandIncludeFile<'_> { // recursively. // // So here we interpret any I/O errors as "just return this - // argument". Currently we don't implement handling of arguments - // with quotes, so if those are encountered we just pass the option - // through literally anyway. + // argument". On a successful read the contents are tokenized using + // the same rules GCC and Clang apply (quotes group an argument and + // a backslash escapes the next character) and spliced into the + // argument stream in place of the original `@file`. Because the + // local and distributed commands are later reconstructed from the + // parsed (expanded) arguments, the response file never needs to + // exist on a remote machine, so such compilations can be both + // cached and distributed. // - // At this time we interpret all `@` arguments above as non - // cacheable, so if we fail to interpret this we'll just call the - // compiler anyway. + // If the read fails we return the original `@file` argument, which + // the parser treats as `TooHard` and refuses to cache, so we fall + // back to invoking the compiler directly. // // [1]: https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#Overall-Options let mut contents = String::new(); @@ -1114,13 +1119,68 @@ impl Iterator for ExpandIncludeFile<'_> { debug!("failed to read @-file `{}`: {}", file.display(), e); return Some(arg); } - if contents.contains('"') || contents.contains('\'') { - return Some(arg); + let new_args = split_gnu_response_file_args(&contents); + self.stack.extend(new_args.into_iter().rev()); + } + } +} + +/// Split the contents of a GCC/Clang `@response` file into arguments. +/// +/// This mirrors `llvm::cl::TokenizeGNUCommandLine`, the routine Clang (and, +/// equivalently, GCC) uses to parse response files: +/// +/// - Arguments are separated by whitespace. +/// - A single- or double-quoted string is part of a single argument; the +/// surrounding quotes are removed and may abut unquoted text (`a"b"c` is one +/// argument `abc`). +/// - A backslash escapes the following character, which is taken literally. +/// Backslashes are literal inside single quotes, but still escape inside +/// double quotes. +/// - Empty quoted strings (`""`) produce no argument, matching the compiler. +pub fn split_gnu_response_file_args(contents: &str) -> Vec { + let mut args = Vec::new(); + let mut token = String::new(); + let mut chars = contents.chars(); + + while let Some(c) = chars.next() { + match c { + c if c.is_whitespace() => { + if !token.is_empty() { + args.push(OsString::from(std::mem::take(&mut token))); + } } - let new_args = contents.split_whitespace().collect::>(); - self.stack.extend(new_args.iter().rev().map(|s| s.into())); + '\\' => { + // A backslash escapes the next character, if any. + if let Some(next) = chars.next() { + token.push(next); + } + } + '\'' | '"' => { + let quote = c; + while let Some(qc) = chars.next() { + if qc == quote { + break; + } + // Backslash escapes inside double-quoted strings only. + if quote == '"' && qc == '\\' { + if let Some(next) = chars.next() { + token.push(next); + } + } else { + token.push(qc); + } + } + } + c => token.push(c), } } + + if !token.is_empty() { + args.push(OsString::from(token)); + } + + args } #[cfg(test)] @@ -2417,6 +2477,88 @@ mod test { assert!(!msvc_show_includes); } + #[test] + fn test_split_gnu_response_file_args() { + // Plain whitespace separation, including newlines and tabs. + assert_eq!( + ovec!["-c", "foo.c", "-o", "foo.o"], + split_gnu_response_file_args("-c foo.c\n\t-o foo.o\n") + ); + // Quotes group an argument and are stripped. + assert_eq!( + ovec!["-I", "/a path/with spaces", "-DFOO=bar baz"], + split_gnu_response_file_args("-I \"/a path/with spaces\" '-DFOO=bar baz'") + ); + // Quotes may abut unquoted text. + assert_eq!( + ovec!["-DA=a b c"], + split_gnu_response_file_args("-DA=\"a b c\"") + ); + // Backslash escapes the next character outside quotes. + assert_eq!( + ovec!["a b", "c\\d"], + split_gnu_response_file_args("a\\ b c\\\\d") + ); + // Backslash escapes inside double quotes but not single quotes. + assert_eq!( + ovec!["a\"b", "c\\d"], + split_gnu_response_file_args("\"a\\\"b\" 'c\\d'") + ); + // Empty quoted strings produce no argument. + assert_eq!( + ovec!["-c", "foo.c"], + split_gnu_response_file_args("-c \"\" foo.c") + ); + // Empty or whitespace-only input produces no arguments. + assert!(split_gnu_response_file_args("").is_empty()); + assert!(split_gnu_response_file_args(" \n\t").is_empty()); + // A trailing backslash with nothing to escape is dropped. + assert_eq!(ovec!["foo"], split_gnu_response_file_args("foo\\")); + // A trailing backslash at the end of a double-quoted string is dropped. + assert_eq!(ovec!["x"], split_gnu_response_file_args("\"x\\")); + // An unterminated quote consumes the rest of the input as one argument. + assert_eq!(ovec!["abc"], split_gnu_response_file_args("\"abc")); + assert_eq!(ovec!["a b"], split_gnu_response_file_args("'a b")); + } + + #[test] + fn test_parse_arguments_response_file_with_quotes() { + // A response file containing quoted arguments (common with Clang) must + // be expanded and remain cacheable. + let td = tempfile::Builder::new() + .prefix("sccache") + .tempdir() + .unwrap(); + File::create(td.path().join("args")) + .unwrap() + .write_all(b"-c foo.c -o foo.o \"-DGREETING=hello world\"\n") + .unwrap(); + let arg = format!("@{}", td.path().join("args").display()); + let ParsedArguments { + input, + language, + outputs, + common_args, + .. + } = match parse_arguments_(vec![arg], false) { + CompilerArguments::Ok(args) => args, + o => panic!("Got unexpected parse result: {:?}", o), + }; + assert_eq!(Some("foo.c"), input.to_str()); + assert_eq!(Language::C, language); + assert_map_contains!( + outputs, + ( + "obj", + ArtifactDescriptor { + path: "foo.o".into(), + optional: false + } + ) + ); + assert_eq!(ovec!["-DGREETING=hello world"], common_args); + } + #[test] fn test_compile_simple() { let creator = new_creator(); From 7506d2eba3f9e1f333d0470e041d7a4c936432af Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Mon, 29 Jun 2026 14:08:54 +0300 Subject: [PATCH 2/2] integration: convert cmake 4.x modules XFAIL test to a passing test The `test-cmake-modules-v4` integration test was written as an XFAIL that tracked exactly the bug fixed in the previous commit: cmake 4.x drives C++20 modules through `@response` files (the generated `.modmap` files), which sccache's `@` handling rejected as non-cacheable. Now that response files are expanded and spliced into the argument list, those compilations are cacheable, so the test's XFAIL assumption no longer holds. Its single build can only ever produce cache misses (never hits) on a cold cache, and with the `@` files no longer rejected it fell through to the catch-all "FAIL: Unexpected failure" branch -- which is the CI failure observed. Convert it into a real passing test, mirroring the v3 modules test: build once (cold cache), then again, and assert cache hits. Keep the diagnostic output (grep for `@` in build.ninja, `ninja -v` command dump) since it remains useful, and additionally fail loudly if any `@` argument is reported as non-cacheable. Drop the "(xfail)" label from the Makefile help text. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/integration/Makefile | 2 +- .../scripts/test-cmake-modules-v4.sh | 36 +++++++++++++------ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/tests/integration/Makefile b/tests/integration/Makefile index ab4d13c232..c3463f7bf1 100644 --- a/tests/integration/Makefile +++ b/tests/integration/Makefile @@ -95,7 +95,7 @@ help: @echo "Build System Tests:" @echo " make test-cmake Run CMake integration test" @echo " make test-cmake-modules Run CMake C++20 modules test" - @echo " make test-cmake-modules-v4 Run CMake 4.x C++20 modules test (xfail)" + @echo " make test-cmake-modules-v4 Run CMake 4.x C++20 modules test" @echo " make test-autotools Run Autotools integration test" @echo "" @echo "Advanced Tests:" diff --git a/tests/integration/scripts/test-cmake-modules-v4.sh b/tests/integration/scripts/test-cmake-modules-v4.sh index eff43c5070..2237596a30 100755 --- a/tests/integration/scripts/test-cmake-modules-v4.sh +++ b/tests/integration/scripts/test-cmake-modules-v4.sh @@ -1,14 +1,15 @@ #!/bin/bash set -euo pipefail -# XFAIL: cmake 4.x generates arguments that trigger sccache's @ response file -# rejection in gcc.rs:349. This test tracks the issue and captures the actual -# compiler commands for debugging. +# cmake 4.x drives C++20 modules through @response files (e.g. the generated +# `.modmap` files). These used to be rejected by sccache's @ response-file +# handling in gcc.rs, making such builds non-cacheable. Now that response files +# are expanded and spliced into the argument list, these builds are cacheable. SCCACHE="${SCCACHE_PATH:-/sccache/target/debug/sccache}" echo "==========================================" -echo "Testing: CMake 4.x C++20 Modules (XFAIL)" +echo "Testing: CMake 4.x C++20 Modules" echo "==========================================" echo "cmake version: $(cmake --version | head -1)" @@ -21,7 +22,7 @@ cp -r /sccache/tests/integration/cmake-modules /build/cmake-modules "$SCCACHE" --start-server || true echo "" -echo "Build 1: Capture compiler commands" +echo "Build 1: Cache miss expected" cd /build/cmake-modules cmake -B build -G Ninja \ -DCMAKE_C_COMPILER=clang \ @@ -37,6 +38,20 @@ echo "" echo "=== Full compiler commands (ninja -v) ===" cmake --build build -- -v 2>&1 | cat +echo "Checking stats after first build..." +"$SCCACHE" --show-stats + +echo "" +echo "Build 2: Cache hit expected" +cd /build/cmake-modules +rm -rf build +cmake -B build -G Ninja \ + -DCMAKE_C_COMPILER=clang \ + -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_C_COMPILER_LAUNCHER="$SCCACHE" \ + -DCMAKE_CXX_COMPILER_LAUNCHER="$SCCACHE" +cmake --build build | cat # unfold output + echo "" echo "=== sccache stats ===" "$SCCACHE" --show-stats @@ -49,16 +64,17 @@ echo "" echo "Cache hits: $CACHE_HITS" echo "Non-cacheable @: $NOT_CACHED" -if [ "$CACHE_HITS" -gt 0 ]; then - echo "XPASS: CMake 4.x C++20 modules now cacheable! Remove XFAIL status." +if [ "$NOT_CACHED" -gt 0 ]; then + echo "FAIL: cmake 4.x @ response files were rejected as non-cacheable" + echo "$STATS_JSON" | python3 -m json.tool exit 1 fi -if [ "$NOT_CACHED" -gt 0 ]; then - echo "XFAIL: cmake 4.x @ issue reproduced (expected failure)" +if [ "$CACHE_HITS" -gt 0 ]; then + echo "PASS: CMake 4.x C++20 modules test" exit 0 fi -echo "FAIL: Unexpected failure" +echo "FAIL: CMake 4.x C++20 modules test - No cache hits detected" echo "$STATS_JSON" | python3 -m json.tool exit 1