Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

ahartmetz · 2025-05-06T12:54:01Z

No description provided.

codecov-commenter · 2025-05-06T12:58:20Z

Codecov Report

Attention: Patch coverage is 77.00000% with 46 lines in your changes missing coverage. Please review.

Project coverage is 71.51%. Comparing base (5b1e93e) to head (bf29f51).

Files with missing lines	Patch %	Lines
src/compiler/gcc.rs	37.50%	20 Missing ⚠️
src/compiler/compiler.rs	68.18%	14 Missing ⚠️
src/compiler/nvcc.rs	0.00%	4 Missing ⚠️
src/compiler/nvhpc.rs	0.00%	4 Missing ⚠️
src/compiler/rust.rs	94.82%	3 Missing ⚠️
src/compiler/c.rs	98.24%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2392      +/-   ##
==========================================
- Coverage   71.58%   71.51%   -0.08%     
==========================================
  Files          65       65              
  Lines       36214    36273      +59     
==========================================
+ Hits        25923    25939      +16     
- Misses      10291    10334      +43

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sylvestre · 2025-05-13T07:27:23Z

nice
a few jobs are failing btw

ahartmetz · 2025-05-13T14:13:30Z

This seems really annoying to auto-test, and to be honest, I haven't even tested it manually yet ;)
I will obviously need to do at least that.

ahartmetz · 2025-05-18T20:08:00Z

So I did do some practical testing and ran into another blocker for preprocessor cache mode, this time that it didn't work with -MD (which is used by at least Make and Ninja backends of CMake, so very widely I'd say).
@glandium, do the last three commits look reasonable to you?

It is curious that
#2194
is older than
#2322
so it seems like there is another, yet unfixed, bug that breaks caching and restoring .d files in preprocessor cache mode?

ahartmetz · 2025-06-16T12:50:38Z

Well, that cherry-pick didn't fix the tests.

Copilot

Pull Request Overview

This PR refactors compiler hasher APIs to support mutable state, enhances preprocessor cache behavior to preserve dependency files (.d), and updates tests and language-specific compilers to emit and handle dependency artifacts.

Change generate_hash_key and get_cached_or_compile to take &mut self and propagate mutability to callers
Add logic to filter out or restore the .d dependency file based on is_locally_preprocessed
Extend NVHPC, NVCC, and GCC compiler backends and tests to include .d artifacts

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/server.rs	Marked `hasher` argument as `mut` to match new signature
src/compiler/rust.rs	Refactored `generate_hash_key` to use `&mut self` and replaced destructuring with `self` references
src/compiler/nvhpc.rs	Added `.d` dependency descriptor in test outputs
src/compiler/nvcc.rs	Added `.d` dependency descriptor in test outputs
src/compiler/gcc.rs	Tracked `dep_path`, inserted `"d"` artifact, updated tests
src/compiler/compiler.rs	Switched to `&mut self`, filtered `.d` in cached outputs, added `is_locally_preprocessed`
src/compiler/c.rs	Added `is_locally_preprocessed` field, updated `generate_hash_key`, introduced poison constant

Comments suppressed due to low confidence (3)

src/compiler/compiler.rs:515

Fix spelling: change "informaiton" to "information."

                    outputs

src/compiler/gcc.rs:293

[nitpick] The variable dep_path is somewhat ambiguous; consider renaming it to dep_file_path or dependency_file_path for clarity.

    let mut dep_path = None;

src/compiler/compiler.rs:3386

Reusing the same hasher instance across multiple get_cached_or_compile calls may carry over mutable state between tests. Consider cloning the hasher for each invocation to ensure isolation.

                Some((

Copilot · 2025-06-20T16:07:51Z

src/compiler/rust.rs

+        trace!("[{}]: generate_hash_key", self.parsed_args.crate_name);
        // TODO: this doesn't produce correct arguments if they should be concatenated - should use iter_os_strings
-        let os_string_arguments: Vec<(OsString, Option<OsString>)> = arguments
+        let os_string_arguments: Vec<(OsString, Option<OsString>)> = self
+            .parsed_args


[nitpick] Consider aliasing &self.parsed_args to a local variable (e.g., let pa = &self.parsed_args;) to reduce repetition and improve readability.

Is that actually considered a problem and worth solving?
From how I know the similar "self.variable" usage in Python, the self. part is more considered part of the name and self-documentation rather than useless boilerplate.

Anything else that needs doing?

nope, just ignore it :)

sylvestre · 2025-06-22T08:19:58Z

nice, green builds :)

Could you please add tests ?
Especially tests that would fail without your changes and would pass now
thanks

It will need to call itself (in the next commit), and the "consumes and moves values out of the instance" model doesn't work for that.

...and disable the workaround that disabled the former in case of the latter. Under the following conditions: - Build with preprocessor cache mode enabled - Distributed build - Preprocessor cache hit - Main cache miss the compile that was run as "fallback" after the cache miss did not have preprocessed sources passed to it since skipping the preprocessing step is the whole point of preprocessor cache mode. But local preprocessing is what enables distributed compilation. To fix this, when the problem occurs, recursively re-invoke get_cached_or_compile() with ForceRecache(*) so that the preprocessor cache hit is (mostly) ignored, which makes get_cached_or_compile() invoke the local preprocessing codepath, which yields code suitable for distributed compilation. (*) which also prevents further recursion, notably by malicious code that mimics the "marker" code to signify skipped preprocessing

This reverts the "main" code changes of commit c0a0b6e but keeps the added test. A fix that is better than disabling preprocessor cache mode whenever the compiler generates a dep file seems possible.

It works as follows: - Always store the dep file when committing entries to cache - Cache hit in non-preprocessor cache mode: This is based on preprocessed file contents, so it can confuse cache entries with same preprocessed contents but different dependency file names. Do not restore the dep file. It could be for another source file, and restoring it is not necessary because local preprocessing (to get the preprocessed source file contents!) will produce the dep file. - Cache hit in preprocessor cache mode: This cache key is made up of input file names and hashes and preprocessing is skipped. In this case, restoring the dep file from cache is both safe and necessary, so do it. CMake's dependency tracking is based on dep files, so this enables preprocessor cache mode with CMake.

The (ab)use of preprocessed_input to carry information about whether local preprocessing was done did not work without distributed compilation, where the data is not needed for its main purpose. So add an explicit data member for that and use it.

It should work now.

ahartmetz · 2025-06-24T15:23:54Z

nice, green builds :)

Could you please add tests ? Especially tests that would fail without your changes and would pass now thanks

For dependency files (-MD etc) with preprocessor cache mode, I have it easy: when preprocessor cache mode was disabled in case of -MD etc detected: c0a0b6e, there were tests added which I kept, and they failed in builds without the "dist" feature, which was required to make the new logic work before 60b826b.
(Edit: nope, that's wrong - different failure mode: these tests were for non-preprocessor cache mode.)

For distributed compilation with preprocessor cache mode, I will actually need to write the tests.

ahartmetz marked this pull request as draft May 6, 2025 12:54

ahartmetz force-pushed the fix_direct_mode_distributed branch from 100c577 to 422cf17 Compare May 13, 2025 14:12

ahartmetz force-pushed the fix_direct_mode_distributed branch from 422cf17 to d97d906 Compare May 18, 2025 19:53

ahartmetz force-pushed the fix_direct_mode_distributed branch 3 times, most recently from 6004164 to c30cfb7 Compare May 24, 2025 21:55

ahartmetz marked this pull request as ready for review May 31, 2025 13:21

ahartmetz changed the title ~~Fix the combination preprocessor cache mode + distributed compilation~~ Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) May 31, 2025

ahartmetz force-pushed the fix_direct_mode_distributed branch from c30cfb7 to 3576a2b Compare June 1, 2025 19:10

ahartmetz force-pushed the fix_direct_mode_distributed branch 2 times, most recently from ded3235 to 5296730 Compare June 12, 2025 00:40

ahartmetz force-pushed the fix_direct_mode_distributed branch from b44b158 to 5296730 Compare June 16, 2025 12:51

sylvestre requested a review from Copilot June 20, 2025 16:05

Copilot AI reviewed Jun 20, 2025

View reviewed changes

ahartmetz force-pushed the fix_direct_mode_distributed branch from d1a8e71 to 73f9766 Compare June 21, 2025 13:46

ahartmetz added 7 commits June 23, 2025 09:06

Remove a few IMO pointless indirections / aliases

de7180a

Change self of generate_hash_key() from Box<Self> to &mut Self

d6589db

It will need to call itself (in the next commit), and the "consumes and moves values out of the instance" model doesn't work for that.

Partially revert "Don't cache dep file (mozilla#2322)"

4ce4ac1

This reverts the "main" code changes of commit c0a0b6e but keeps the added test. A fix that is better than disabling preprocessor cache mode whenever the compiler generates a dep file seems possible.

Do not disable preprocessor cache mode anymore for -MD and similar

bf29f51

It should work now.

sylvestre force-pushed the fix_direct_mode_distributed branch from 73f9766 to bf29f51 Compare June 23, 2025 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Uh oh!

ahartmetz commented May 6, 2025

Uh oh!

codecov-commenter commented May 6, 2025 •

edited

Loading

Uh oh!

sylvestre commented May 13, 2025

Uh oh!

ahartmetz commented May 13, 2025

Uh oh!

ahartmetz commented May 18, 2025 •

edited

Loading

Uh oh!

ahartmetz commented Jun 16, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 20, 2025

Uh oh!

ahartmetz Jun 23, 2025

Uh oh!

sylvestre Jun 24, 2025

Uh oh!

sylvestre commented Jun 22, 2025

Uh oh!

ahartmetz commented Jun 24, 2025 •

edited

Loading

Uh oh!

Uh oh!

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Are you sure you want to change the base?

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Uh oh!

Conversation

ahartmetz commented May 6, 2025

Uh oh!

codecov-commenter commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sylvestre commented May 13, 2025

Uh oh!

ahartmetz commented May 13, 2025

Uh oh!

ahartmetz commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahartmetz commented Jun 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

ahartmetz Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

sylvestre Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

sylvestre commented Jun 22, 2025

Uh oh!

ahartmetz commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 6, 2025 •

edited

Loading

ahartmetz commented May 18, 2025 •

edited

Loading

ahartmetz commented Jun 24, 2025 •

edited

Loading