diff --git a/docs/ResponseFiles.md b/docs/ResponseFiles.md index bb20a62bf..a776d58a9 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 7bd4f9689..bb7f9cc3e 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(); diff --git a/tests/integration/Makefile b/tests/integration/Makefile index ab4d13c23..c3463f7bf 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 eff43c507..2237596a3 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