Skip to content
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.

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

[WIP] Content hash support. (See also cargo changes) #75594

Closed
wants to merge 24 commits into from

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Aug 16, 2020

This PR adds a makefile comment (on the following line as can't be the same line in makefiles) with the file size and a hash of the file in the deps/*.d files.

The main PR is on cargo's repo: rust-lang/cargo#8623

This is to enable cargo to be able to use file sizes and content hashes as well as mtimes to both detect rapid changes that are currently missed and also to prevent compile cascades where mtimes have been altered but no actual change to the content occured (E.g. docker layers typically don't store precise times or copying a target dir from one place to another).

It seems that the hashes are already being calculated in the compiler so this is almost zero cost to rustc - the changes are:

  • Inside .rlib, lib.rmeta is renamed to libcrate_name-svh.rmeta
  • For .rmeta files we add the svh to a known location in the uncompressed header.
  • For dylibs we add one additional symbol containing the svh.

(For outanding points please see the cargo PR. For binary file tracking we build on #63012 )

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2020
@gilescope
Copy link
Contributor Author

That looks a legit test failure. Let me look into that.

@gilescope
Copy link
Contributor Author

Hmm, so that test failure wasn't legit and went away. Meanwhile spotted we already use a SourceFileHashAlgorithm for hashing source files for putting in debug info.... maybe there's no need for an additional hashing of the source - that would be v. cute.

@bjorn3
Copy link
Member

bjorn3 commented Aug 18, 2020

Compiled dependencies and probably the output files also need to be hashed.

@gilescope
Copy link
Contributor Author

gilescope commented Aug 20, 2020

Updated it so that we use the precomputed hashes - this makes this change effectively zero cost now as all the source was being hashed elsewhere. That's much better than I was exepcting (and given that maybe there's no need to put it behind a -Z rustc flag?).

That's a really weird compiler error - I don't have a mental model how that could happen. Locally it's an Rc but the CI thinks it's an Arc. It's like the CI isn't actually compiling the branch of code I'm on, but is merging the patch onto master and building that? (Maybe the process has changed a bit? - I've taken a fair few months off from submissions)

@gilescope
Copy link
Contributor Author

How should the hashes [u8] should be turned into utf8? Seems there's pleanty of options to turn bin to text - for now I've dodge that and just used the debug formatting of [u8] to turn it to a string but suggestions welcome!

@bjorn3
Copy link
Member

bjorn3 commented Aug 20, 2020

Locally it's an Rc but the CI thinks it's an Arc.

There is a type in rustc_data_structures that is an Rc when parallel rustc is disabled and an Arc when enabled.

@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@gilescope gilescope changed the title WIP: Content hash support. (See also cargo changes) [WIP] Content hash support. (See also cargo changes) Sep 20, 2020
@crlf0710 crlf0710 marked this pull request as draft October 8, 2020 11:46
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2020
@gilescope
Copy link
Contributor Author

@ehuss do my changes to base.rs look vaguely in the right direction? Once I can get one symbol appearing in the output that I can find I'll be unstuck. But I'm really clueless about this codegen side at the moment - I've not touched this part of the compiler at all before. Any hints are greatfully appreciated and may shave months of headbanging off this PR...

@gilescope
Copy link
Contributor Author

gilescope commented Oct 25, 2020

Well this have been a voyage of discovery, I made a hi.rs that printed 'Hi'. I can see the section coming out in the llvm IR code and I can see it coming out in the generated assembly code if I do:

rustc --emit=llvm-bc,llvm-ir

I can see it in hi.s machine assembler that's produced by llc ./hi.bc and if I do as hi.s then I get an a.out and when I vi inside that I can see the section (but the a.out doesn't run so I can't be incanting everything in the same way that cargo/rustc/llvm is).

So I guess it's all working fine but some optimisation layer is removing it (linker maybe?). I will try and see if I can hint to the linker not to remove it...

@bjorn3
Copy link
Member

bjorn3 commented Oct 25, 2020

So I guess it's all working fine but some optimisation layer is removing it (linker maybe?). I will try and see if I can hint to the linker not to remove it...

This for executables? You may need to do the same as for the gdb debug scripts section. (reference it from the main shim using a volatile load)

bx.insert_reference_to_gdb_debug_scripts_section_global();

@gilescope
Copy link
Contributor Author

Seem to get the symbol coming out for rmeta and executables now. Just trying to add the symbol to the dylib (it's not coming out in the .ll so I've got a way to go there...).

@bjorn3
Copy link
Member

bjorn3 commented Oct 28, 2020

The cdylib doesn't contain the svh symbol, as the metadata module is omitted for cdylibs.

compiler/rustc_metadata/src/rmeta/mod.rs Show resolved Hide resolved
compiler/rustc_metadata/src/rmeta/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/write.rs Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/write.rs Outdated Show resolved Hide resolved
.collect();

if let Some(ref backend) = sess.opts.debugging_opts.codegen_backend {
files.push(backend.to_string());
files.push((backend.to_string(), None)); // TO DO: is this something we need to hash?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It is a dylib, so it contains an SVH. You could add a parameter with the result of codegen_backend.metadata_loader() to this function to read it, but that looks like a bit of a hack.

compiler/rustc_metadata/src/rmeta/encoder.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/rmeta/encoder.rs Outdated Show resolved Hide resolved
compiler/rustc_interface/src/passes.rs Outdated Show resolved Hide resolved
compiler/rustc_span/src/lib.rs Outdated Show resolved Hide resolved
gilescope and others added 3 commits November 1, 2020 18:09
denser encoding format

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
@bjorn3
Copy link
Member

bjorn3 commented Nov 14, 2020

Did you also add the corresponding decode call?

@gilescope
Copy link
Contributor Author

I see where I was being tripped up - the encode was converting an array to a slice before writing it so there's a length in there as well messing up my offsets. I should have guessed that might happen sooner...

@bors
Copy link
Contributor

bors commented Nov 15, 2020

☔ The latest upstream changes (presumably #79070) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors
Copy link
Contributor

bors commented Dec 16, 2020

☔ The latest upstream changes (presumably #77117) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@Dylan-DPC-zz
Copy link

@gilescope any updates on this?

@gilescope
Copy link
Contributor Author

This PR is good, but blocked waiting on the cargo changes. The cargo PR needs another round of work to nail the build.rs interactions.

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 26, 2021
@ecstatic-morse ecstatic-morse removed their assignment Feb 25, 2022
@JohnCSimon
Copy link
Member

@gilescope
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Nov 27, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants