Skip to content

Fix the combination preprocessor cache mode + distributed compilation #2392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Fix the combination preprocessor cache mode + distributed compilation #2392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2eb95b3
Remove a few IMO pointless indirections / aliases
ahartmetz May 5, 2025
0533341
Change self of generate_hash_key() from Box<Self> to &mut Self
ahartmetz May 5, 2025
55a1a6f
Fix preprocessor cache mode with distributed builds
ahartmetz May 5, 2025
005763e
Partially revert "Don't cache dep file (#2322)"
ahartmetz May 18, 2025
43505bb
WIP: implement the fix described in the previous commit message
ahartmetz May 18, 2025
b813cb8
WIP: Do not disable preprocessor cache mode anymore for -MD and friends
ahartmetz May 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 64 additions & 59 deletions src/compiler/c.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ where
I: CCompilerImpl,
{
async fn generate_hash_key(
self: Box<Self>,
&mut self,
creator: &T,
cwd: PathBuf,
env_vars: Vec<(OsString, OsString)>,
Expand All @@ -376,42 +376,38 @@ where
cache_control: CacheControl,
) -> Result<HashResult<T>> {
let start_of_compilation = std::time::SystemTime::now();
let CCompilerHasher {
parsed_args,
executable,
executable_digest,
compiler,
} = *self;

let extra_hashes = hash_all(&parsed_args.extra_hash_files, &pool.clone()).await?;
let extra_hashes = hash_all(&self.parsed_args.extra_hash_files, &pool.clone()).await?;
// Create an argument vector containing both preprocessor and arch args, to
// use in creating a hash key
let mut preprocessor_and_arch_args = parsed_args.preprocessor_args.clone();
preprocessor_and_arch_args.extend(parsed_args.arch_args.to_vec());
let mut preprocessor_and_arch_args = self.parsed_args.preprocessor_args.clone();
preprocessor_and_arch_args.extend(self.parsed_args.arch_args.to_vec());
// common_args is used in preprocessing too
preprocessor_and_arch_args.extend(parsed_args.common_args.to_vec());
preprocessor_and_arch_args.extend(self.parsed_args.common_args.to_vec());

let absolute_input_path: Cow<'_, _> = if parsed_args.input.is_absolute() {
Cow::Borrowed(&parsed_args.input)
let absolute_input_path: Cow<'_, _> = if self.parsed_args.input.is_absolute() {
Cow::Borrowed(&self.parsed_args.input)
} else {
Cow::Owned(cwd.join(&parsed_args.input))
Cow::Owned(cwd.join(&self.parsed_args.input))
};

// Try to look for a cached preprocessing step for this compilation
// request.
let preprocessor_cache_mode_config = storage.preprocessor_cache_mode_config();
let too_hard_for_preprocessor_cache_mode =
parsed_args.too_hard_for_preprocessor_cache_mode.is_some();
if let Some(arg) = &parsed_args.too_hard_for_preprocessor_cache_mode {
let too_hard_for_preprocessor_cache_mode = self
.parsed_args
.too_hard_for_preprocessor_cache_mode
.is_some();
if let Some(arg) = &self.parsed_args.too_hard_for_preprocessor_cache_mode {
debug!(
"parse_arguments: Cannot use preprocessor cache because of {:?}",
arg
);
}

let use_preprocessor_cache_mode = {
let can_use_preprocessor_cache_mode = !may_dist
&& preprocessor_cache_mode_config.use_preprocessor_cache_mode
let can_use_preprocessor_cache_mode = preprocessor_cache_mode_config
.use_preprocessor_cache_mode
&& !too_hard_for_preprocessor_cache_mode;

let mut use_preprocessor_cache_mode = can_use_preprocessor_cache_mode;
Expand All @@ -438,16 +434,15 @@ where
use_preprocessor_cache_mode
};

// Disable preprocessor cache when doing distributed compilation
let mut preprocessor_key = if use_preprocessor_cache_mode {
preprocessor_cache_entry_hash_key(
&executable_digest,
parsed_args.language,
&self.executable_digest,
self.parsed_args.language,
&preprocessor_and_arch_args,
&extra_hashes,
&env_vars,
&absolute_input_path,
compiler.plusplus(),
self.compiler.plusplus(),
preprocessor_cache_mode_config,
)?
} else {
Expand Down Expand Up @@ -494,17 +489,20 @@ where
// the toolchain will not contain the correct path
// to invoke the compiler! Add the compiler
// executable path to try and prevent this
let weak_toolchain_key =
format!("{}-{}", executable.to_string_lossy(), executable_digest);
let weak_toolchain_key = format!(
"{}-{}",
self.executable.to_string_lossy(),
self.executable_digest
);
return Ok(HashResult {
key,
compilation: Box::new(CCompilation {
parsed_args: parsed_args.to_owned(),
parsed_args: self.parsed_args.to_owned(),
#[cfg(feature = "dist-client")]
// TODO or is it never relevant since dist?
preprocessed_input: vec![],
executable: executable.to_owned(),
compiler: compiler.to_owned(),
preprocessed_input: PREPROCESSING_SKIPPED_COMPILE_POISON
.to_vec(),
executable: self.executable.to_owned(),
compiler: self.compiler.to_owned(),
cwd: cwd.to_owned(),
env_vars: env_vars.to_owned(),
}),
Expand All @@ -518,25 +516,26 @@ where
}
}

let result = compiler
let result = self
.compiler
.preprocess(
creator,
&executable,
&parsed_args,
&self.executable,
&self.parsed_args,
&cwd,
&env_vars,
may_dist,
rewrite_includes_only,
use_preprocessor_cache_mode,
)
.await;
let out_pretty = parsed_args.output_pretty().into_owned();
let out_pretty = self.parsed_args.output_pretty().into_owned();
let result = result.map_err(|e| {
debug!("[{}]: preprocessor failed: {:?}", out_pretty, e);
e
});

let outputs = parsed_args.outputs.clone();
let outputs = self.parsed_args.outputs.clone();
let args_cwd = cwd.clone();

let mut preprocessor_result = result.or_else(move |err| {
Expand Down Expand Up @@ -595,24 +594,24 @@ where

trace!(
"[{}]: Preprocessor output is {} bytes",
parsed_args.output_pretty(),
self.parsed_args.output_pretty(),
preprocessor_result.stdout.len()
);

// Create an argument vector containing both common and arch args, to
// use in creating a hash key
let mut common_and_arch_args = parsed_args.common_args.clone();
common_and_arch_args.extend(parsed_args.arch_args.to_vec());
let mut common_and_arch_args = self.parsed_args.common_args.clone();
common_and_arch_args.extend(self.parsed_args.arch_args.to_vec());

let key = {
hash_key(
&executable_digest,
parsed_args.language,
&self.executable_digest,
self.parsed_args.language,
&common_and_arch_args,
&extra_hashes,
&env_vars,
&preprocessor_result.stdout,
compiler.plusplus(),
self.compiler.plusplus(),
)
};

Expand All @@ -639,15 +638,19 @@ where
// A compiler binary may be a symlink to another and so has the same digest, but that means
// the toolchain will not contain the correct path to invoke the compiler! Add the compiler
// executable path to try and prevent this
let weak_toolchain_key = format!("{}-{}", executable.to_string_lossy(), executable_digest);
let weak_toolchain_key = format!(
"{}-{}",
self.executable.to_string_lossy(),
self.executable_digest
);
Ok(HashResult {
key,
compilation: Box::new(CCompilation {
parsed_args,
parsed_args: self.parsed_args.clone(),
#[cfg(feature = "dist-client")]
preprocessed_input: preprocessor_result.stdout,
executable,
compiler,
executable: self.executable.clone(),
compiler: self.compiler.clone(),
cwd,
env_vars,
}),
Expand Down Expand Up @@ -1152,6 +1155,12 @@ fn include_is_too_new(
false
}

// Used as "preprocessed code" when no preprocessing took place to immediately produce an error (which
// should never happen / be caught earlier though!). Previously, an empty u8 vector was used, which is
// unfortunately a valid C/C++ program and caused errors that only surfaced when linking.
#[cfg(feature = "dist-client")]
const PREPROCESSING_SKIPPED_COMPILE_POISON: &[u8] = b"([{SCCACHE -*-* INVALID_C_CPP_CODE([{\"";

impl<T: CommandCreatorSync, I: CCompilerImpl> Compilation<T> for CCompilation<I> {
fn generate_compile_commands(
&self,
Expand All @@ -1162,21 +1171,12 @@ impl<T: CommandCreatorSync, I: CCompilerImpl> Compilation<T> for CCompilation<I>
Option<dist::CompileCommand>,
Cacheable,
)> {
let CCompilation {
ref parsed_args,
ref executable,
ref compiler,
ref cwd,
ref env_vars,
..
} = *self;

compiler.generate_compile_commands(
self.compiler.generate_compile_commands(
path_transformer,
executable,
parsed_args,
cwd,
env_vars,
&self.executable,
&self.parsed_args,
&self.cwd,
&self.env_vars,
rewrite_includes_only,
)
}
Expand Down Expand Up @@ -1212,6 +1212,11 @@ impl<T: CommandCreatorSync, I: CCompilerImpl> Compilation<T> for CCompilation<I>
Ok((inputs_packager, toolchain_packager, outputs_rewriter))
}

#[cfg(feature = "dist-client")]
fn is_preprocessed_for_distribution(&self) -> bool {
self.preprocessed_input != PREPROCESSING_SKIPPED_COMPILE_POISON
}

fn outputs<'a>(&'a self) -> Box<dyn Iterator<Item = FileObjectSource> + 'a> {
Box::new(
self.parsed_args
Expand Down
Loading
Loading