Add TRACKED_NO_CRATE_HASH and use it for --remap-path-prefix

[jyn514]

In #48162, --remap-path-prefix was changed to be untracked by incremental compilation so that .rlibs built with and without it would be bitwise identical. Since --remap-path-prefix is used for reproducible builds, it's important that it doesn't change the output rlib.

Unfortunately, that broke incremental recompilation because we no longer rebuilt the rlib when --remap-path-prefix changed. To avoid that, but still make sure the output is bitwise identical, this PR adds a new TRACKED_NO_CRATE_HASH mode for CLI options. This invalidates the incremental cache, without adding the option to the crate hash embedded in the rlib.

• Add TRACKED_NO_CRATE_HASH and cbeuw's SUBSTRUCT idea
• Refactor the unit tests to ensure non_default_option isn't the default, which found a couple bugs.
• Refactor the tests to have less boilerplate
• Remove real_rust_source_base_dir from Session so it's only hashed once

I verified locally that this fixes #66955.

r? @Aaron1011 (feel free to reassign)

[Aaron1011]

@jyn514: Can you add a test to compiler/rustc_interface/src/tests.rs to ensure that the option stays tracked?

[jyn514]

@jyn514: Can you add a test to compiler/rustc_interface/src/tests.rs to ensure that the option stays tracked?

Done, and I also tested that non_default_option isn't the default, which found a couple bugs.

[jyn514]

Hmm, the test in reproducible-builds seems wrong? Presumably the .rlib is different because it's now hashing the --remap-path-prefix argument. Is there a reason it tests the .rlib specifically instead of some binary compiled using it?

[jyn514]

Actually I wonder if the issue is that tracked options make it all the way to .rlibs? I would expect them to only be stored in the incremental cache, not in any outputs.

[jyn514]

Hmm, it looks like this was intentionally changed in #48162 ? @michaelwoerister do you have suggestions for invalidating the cache when this changes without also changing the output .rlib?

[michaelwoerister]

This is on my todo list. It does not look like a trivial problem.

[michaelwoerister]

It looks like we can't make the option [TRACKED] in its current form (because that would break reproducible builds) but #83813 will probably introduce a new form of tracking that should solve this problem.

[jyn514]

This is waiting on me to implement #83813 (comment).

[jyn514]

If you pull the body of the [TRACKED] branch in hash_option!() out into a function, it should be straightforward to add a [TRACKED_NO_CRATE_HASH] branch. You could then add a include_no_crate_hash_options parameter to dep_tracking_hash(). That way we won't have to change how things work in the dep-graph -- just pass true there and false in hir::map::index_hir().

@michaelwoerister hmm, there's also a impl dep_tracking::DepTrackingHash for Options - should that include the hash or not? Or should I remove that impl so everyone calling this has to chose explicitly?

[jyn514]

should I remove that impl so everyone calling this has to chose explicitly?

I tried this and it doesn't work because sub_hashes.insert uses a trait object:

error[E0277]: the trait bound `options::DebuggingOptions: DepTrackingHash` is not satisfied
   --> compiler/rustc_session/src/options.rs:65:34
    |
49  | / macro_rules! top_level_options {
50  | |     (pub struct Options { $(
51  | |         $opt:ident : $t:ty [$dep_tracking_marker:ident $($warn_val:expr, $warn_text:expr)*],
52  | |     )* } ) => (
...   |
65  | |                                  &self.$opt,
    | |                                  ^^^^^^^^^^ the trait `DepTrackingHash` is not implemented for `options::DebuggingOptions`
...   |
79  | |     );
80  | | }
    | |_- in this expansion of `top_level_options!`
...
99  | / top_level_options!(
100 | |     pub struct Options {
101 | |         // The crate config requested for the session, which may be combined
102 | |         // with additional crate configurations during the compile process.
...   |
171 | |     }
172 | | );
    | |__- in this macro invocation
    |
    = note: required for the cast to the object type `dyn DepTrackingHash`

Maybe I should add include_no_crate_hash: bool to the trait itself?

[cbeuw]

For reference, this is what I did when I tried to implement it: #83813 (comment) 39ebceb

There probably is a better way though

[jyn514]

Adding this argument to the trait's method doesn't make sense because it'll also affect all individual types implementing it.

Hmm, why do you say that? What if you want to add a TRACKED_NO_CRATE_HASH option to a substruct?

[michaelwoerister]

@bors rollup=never

[jyn514]

@bors r=michaelwoerister (I just changed the formatting of some of the docs, nothing related to incremental itself)

[bors]

📌 Commit 5a692a7 has been approved by michaelwoerister

[bors]

⌛ Testing commit 5a692a7 with merge 814a560...

[bors]

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 814a560 to master...

[RalfJung]

Computing the sysroot here is a problem as it means when the config driver callback changes the sysroot, that does not properly get taken into account.

This PR moved real_rust_source_base_dir from after the config callback to before, which is not good. Config should not contain "derived" values, only the input as provided by the user.

I'm trying to fix this in #122993 but I have no idea about all this "tracked" stuff, so I don't know if what I do makes any sense.