fix preprocessor cache mode vs basedirs

README.md

@@ -347,6 +347,8 @@ Base directories are stripped from the preprocessed source and from the compiler
 
 In an argument only the places that are expected to spell a pathname are considered: the whole argument, the value of an option written with an `=` (either half of a prefix map), and the value glued to a short option such as `-I`. A base directory appearing anywhere else is left alone, so a definition the compiler bakes into the output verbatim, `-DROOT="/home/user/project"`, still counts. A match also has to end where a path component ends, so a sibling `/home/user/project-docs` is not one.
 
+For how base directories interact with preprocessor cache mode, see [the local doc](docs/Local.md).
+
 This is similar to ccache's `CCACHE_BASEDIR` and helps when:
 * Building the same project from different directories
 * Sharing cache between CI jobs with different checkout paths

docs/Caching.md

@@ -57,5 +57,13 @@ to the C/C++ compiler one, but with additional elements:
 * The path of the input file
 * The hash of the input file
 
+A hit on this key hands back the C/C++ compiler key that was stored with the
+entry, so the hash above is never computed on that path. What stands in for it
+is the list of include files recorded with the entry: the entry only counts as
+a hit if every one of them is still on disk with the contents it had.
+
+For how that list is checked when [`basedirs`](Configuration.md) is set, and
+what it cannot detect, see [the local doc](Local.md).
+
 Note that some compiler options can disable preprocessor cache mode. As of this
 writing, only `-Xpreprocessor` and `-Wp,*` do.

docs/Configuration.md

@@ -27,6 +27,9 @@ server_startup_timeout_ms = 10000
 # spell a pathname are considered: the whole argument, the value of
 # an =-separated option, and the value glued to a short option.
 #
+# For how basedirs interact with preprocessor cache mode, see
+# docs/Local.md.
+#
 # Example:
 #   basedir = ["/home/user/project"] results in the path prefix rewrite:
 #   "/home/user/project/src/main.c" -> "src/main.c"

docs/Local.md

@@ -34,9 +34,22 @@ The preprocessor cache may silently produce stale results in any of the followin
 
 - When a source file was compiled and its results were cached, a header file would have been included if it existed, but it did
   not exist at the time. sccache does not know about such files, so it cannot invalidate the result if the header file later exists.
+  With `basedirs` this widens: a header that exists in one checkout and not in another, at a position the compiler searches first
+  (the including file's own directory, for a quoted include, or an earlier `-I`), makes the two checkouts resolve the same
+  `#include` to different files. sccache only remembers the files the preprocessor opened, never the directories it looked in and
+  found nothing, so it cannot tell the two apart and the second checkout can be handed the first one's object. Compiler search-path
+  precedence is not modelled.
 - A macro such as `__TIME__` (etc) is used in the source code and `ignore_time_macros` is enabled
 - There are other external factors influencing the preprocessing result that sccache does not know about
 
+When [`basedirs`](Configuration.md) is set, every checkout listed shares a preprocessor cache entry, and the
+header files the entry remembers are looked for in the checkout being compiled (the base directory containing
+the input file, or else the working directory) rather than in the one that wrote the entry. Two checkouts that
+resolve an `#include` to the same path but with different contents therefore do not share a result. Two
+checkouts that resolve it to different paths still can - see the stale result note above. Headers outside
+every base directory are still remembered by absolute path, so a base directory pointing at a build tree
+separate from the source tree costs hits rather than correctness.
+
 Configuration options and their default values:
 
 - `use_preprocessor_cache_mode`: `true`. Whether to use preprocessor cache mode. This can be overridden for an sccache invocation by setting the environment variable `SCCACHE_DIRECT` to `true`/`on`/`1` or `false`/`off`/`0`.

src/compiler/c.rs

@@ -13,7 +13,9 @@
 // limitations under the License.
 
 use crate::cache::{FileObjectSource, Storage};
-use crate::compiler::preprocessor_cache::preprocessor_cache_entry_hash_key;
+use crate::compiler::preprocessor_cache::{
+    compilation_tree_root, preprocessor_cache_entry_hash_key,
+};
 use crate::compiler::{
     Cacheable, ColorMode, Compilation, CompileCommand, Compiler, CompilerArguments, CompilerHasher,
     CompilerKind, HashResult, Language,
@@ -500,13 +502,22 @@ where
                 && let Some(mut seekable) = storage
                     .get_preprocessor_cache_entry(preprocessor_key)
                     .await?
+                && let Some(mut preprocessor_cache_entry) =
+                    read_preprocessor_cache_entry(&mut seekable, preprocessor_key)
             {
-                let mut buf = vec![];
-                seekable.read_to_end(&mut buf)?;
-                let mut preprocessor_cache_entry = PreprocessorCacheEntry::read(&buf)?;
+                // The checkout this compilation belongs to. Basedirs make every
+                // checkout listed share one preprocessor cache entry, so the
+                // include files it records have to be checked against this
+                // tree's copies and not against the ones that happened to write
+                // the entry.
+                let tree_root =
+                    compilation_tree_root(&absolute_input_path, &cwd, storage.basedirs())?;
                 let mut updated = false;
-                let hit = preprocessor_cache_entry
-                    .lookup_result_digest(preprocessor_cache_mode_config, &mut updated);
+                let hit = preprocessor_cache_entry.lookup_result_digest(
+                    preprocessor_cache_mode_config,
+                    tree_root.as_deref(),
+                    &mut updated,
+                );
 
                 let mut update_failed = false;
                 if updated {
@@ -675,7 +686,12 @@ where
                 .map(|(path, digest)| (digest, path))
                 .collect();
             files.sort_unstable_by(|a, b| a.1.cmp(&b.1));
-            preprocessor_cache_entry.add_result(start_of_compilation, &key, files);
+            preprocessor_cache_entry.add_result(
+                start_of_compilation,
+                &key,
+                files,
+                storage.basedirs(),
+            );
 
             if let Err(e) = storage
                 .put_preprocessor_cache_entry(&preprocessor_key, preprocessor_cache_entry)
@@ -731,6 +747,26 @@ const HASH_31_COMMAND_LINE_NEWLINE: &[u8] = b"# 31 \"<command-line>\"\n";
 const HASH_32_COMMAND_LINE_2_NEWLINE: &[u8] = b"# 32 \"<command-line>\" 2\n";
 const INCBIN_DIRECTIVE: &[u8] = b".incbin";
 
+/// An entry that cannot be read, such as one truncated by a crash, is a miss
+/// rather than a failed compilation: the miss path overwrites it.
+fn read_preprocessor_cache_entry(
+    mut entry: impl io::Read,
+    key: &str,
+) -> Option<PreprocessorCacheEntry> {
+    let mut buf = vec![];
+    let result = entry
+        .read_to_end(&mut buf)
+        .map_err(Into::into)
+        .and_then(|_| PreprocessorCacheEntry::read(&buf));
+    match result {
+        Ok(entry) => Some(entry),
+        Err(e) => {
+            debug!("Ignoring unreadable preprocessor cache entry {key}: {e}");
+            None
+        }
+    }
+}
+
 /// Remember the include files in the preprocessor output if it can be cached.
 /// Returns `false` if preprocessor cache mode should be disabled.
 fn process_preprocessed_file(