fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes #14830
+124
−93
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
rustbot
added
A-layout
epage
commented
Nov 16, 2024
…e hashes For now, they should result in the same data.
|
epage
force-pushed
the
rustflags2
branch
2 times, most recently
from
f709c73
to
2af218d
Compare
|
FWIW, I think this should resolve an issue in the Hubris build system. Currently we bypass Cargo's dirtiness checks for building deps in certain circumstances (to get maximum reuse of objects despite small controlled changes to Haven't tested it yet though. |
epage
force-pushed
the
rustflags2
branch
2 times, most recently
from
26ced7e
to
536ae6e
Compare
weihanglo
reviewed
Nov 25, 2024
| dep_c_extra_filename_hashes.hash(&mut c_extra_filename_hasher); | ||
| // Avoid trashing the caches on RUSTFLAGS changing via `c_extra_filename` | ||
| // | ||
| // Limited to `c_extra_filename` to help with reproducible build / PGO issues. |
There was a problem hiding this comment.
This should (probably) avoid the PGO problems, since the symbols won't change. However, this could still cause problems with reproducible builds if the rlib filename somehow leaks into the resulting binary (I don't have a reason to think it does, but it seems risky).
I believe this does because the filenames of object files show up as the codegen unit names in dwarf debug info (and probably PDB?). I suspect that everything else in the binary will be the same, but builds differing only in extra-filename are likely to still produce different binaries.
Have we checked if it doesn't affect PGO?
We would like to check if anything is leaking rlib filename in debug info files.
There was a problem hiding this comment.
Any idea how to go about that? I have no experience with PGO and we have no tests focusing on it.
There was a problem hiding this comment.
I'll do some experiments this week and see if we can have tests written down!
There was a problem hiding this comment.
The test passes with this PR for me
There was a problem hiding this comment.
Confirmed that this breaks reproducible builds with --remap-path-prefix flag. rlib names are leaked to .dwp and object files on Linux.
Step to reproduce
cargo new foo && cd foocargo add unicode-indent-
// src/main.rs fn main() { unicode_ident::is_xid_start('-'); } cargo b --config 'profile.dev.split-debuginfo="packed"'readelf -wi target/debug/deps/foo-2fa7d8fb1eafd428 | rg DW_AT_GNU_dwo_nameand observed both-C extra-filenameforfooandunicode_identwere leaked. They were leaked to.dwpfile as well.
Although rustc is not truly reproducible under certain circumstances (for example rust-lang/rust#117652), this change could hurt reproducibility for any use of remap-path-related flags. The -Ztrim-paths flag might improve the situation a bit, as it reduces the need for users to pass --remap-path-prefix. However, it does not change the fact that reproducibility is broken I feel like. See also #6966.
There was a problem hiding this comment.
For most rustflags, changing them changes the binary anyway. Remap is special in that it takes a build-specific value and removes it, making builds more reproducible.
epage
force-pushed
the
rustflags2
branch
2 times, most recently
from
16ce931
to
c762927
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 26, 2024
### What does this PR try to resolve? This is a regression test to prevent issues like #7416. The test only run on Linux, as other platforms have different requirements for PGO, or emit different PGO function missing warnings. ### How should we test and review this PR? Not sure how brittle it is. We can optionally run it only on Cargo's CI? cc #14830
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR try to resolve?
Fixes #8716
This splits
-C metdataand-C extra-filenameand addsRUSTFLAGSto-C extra-filenamein the hope that we can still make PGO and reproducible builds work while caching artifacts per RUSTFLAGS used.How should we test and review this PR?
Additional information