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

Sort the fat LTO modules to produce deterministic output. #63352

Merged
merged 6 commits into from Aug 10, 2019

Conversation

@jgalenson
Copy link

commented Aug 7, 2019

Some projects that use LTO for their release builds are not reproducible. We can fix this by sorting the fat LTO modules before using them.

It might also be useful to do this for thin LTO, but I couldn't get that to work to test it so I didn't do it.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@estebank

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Thanks for the PR, @jgalenson! This seems like a reasonable thing to do. Would you mind moving the sorting into generate_lto_work?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I'd actually also prefer if this matches ThinLTO as much as possible as well. It looks like we don't do any sorting there, and I think that's because ThinLTO is inherently more deterministic in its passes.

Could the sort be pushed even further into the fat_lto method in the backend somewhere around here? I believe the only piece which should affect determinisim is the order in which we link the modules together through LLVM, and the serialized_modules list is what's going to get linked in there.

(there's also already a helpful name to sort everything by at that point)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Oh also, mind adding a test which exercises this?

@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

That's actually exactly where I initially put the sorting code, but I moved it earlier in case that helped any other uses. Since it seems there aren't, I'll be happy to move it back. :)

I don't know the rustc test framework well. Where would you recommend I add the test?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I think we have a few determinism tests in src/test/run-make-fulldeps/* already, and they can probably be extended with "fat lto" versions as well

@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

I made the requested change and added a test in a second commit.

Note, however, that the test is not very good, as it passes even without the fix. I'm not sure how to trigger the nondeterminism in a small testcase like this.

@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

And of course I got the test working right after I pushed it. :) It seems to work now.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Nice, thanks! Could you also throw in a comment about why the sorting is done? Other than that looks great to me!

@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

Done.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

📌 Commit 3e6a927 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Aug 8, 2019
Rollup merge of rust-lang#63352 - jgalenson:reproducible-lto, r=alexc…
…richton

Sort the fat LTO modules to produce deterministic output.

Some projects that use LTO for their release builds are not reproducible.  We can fix this by sorting the fat LTO modules before using them.

It might also be useful to do this for thin LTO, but I couldn't get that to work to test it so I didn't do it.
@Centril Centril referenced this pull request Aug 8, 2019
bors added a commit that referenced this pull request Aug 8, 2019
Auto merge of #63389 - Centril:rollup-8oz5uu6, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #63162 (Miri tests: use xargo to build separate libstd)
 - #63352 (Sort the fat LTO modules to produce deterministic output.)
 - #63373 (gitignore: add comment explaining policy)
 - #63374 (move of packed fields might or might not occur when they actually are sufficiently aligned)
 - #63381 (reduce visibility)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Failed in #63389 (comment), @bors r-

@bors bors removed the S-waiting-on-bors label Aug 8, 2019

Joel Galenson
@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

It looks like the new test is no deterministic on Windows (it is on Linux, as the other bots passed). I tried reverting the opt-level=1 to see if that fixes it. If it does not, should we just remove the test altogether?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Hm so the nondeterminism may be coming from the linker rather than rustc, could this perhaps produce an LTO'd staticlib to rule that out?

@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 9, 2019

Unfortunately, doing that seems to make the test reproducible even without this patch, whatever opt-level I use. I could still add it, of course, but I'm not sure how useful it would be like that.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Hm ok in that case it's probably best to execute this test just on Linux for now where we have a high level of confidence that the linker used is deterministic.

Joel Galenson
@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 9, 2019

Done. I couldn't tell how to stop only a single part of the test from running on Windows, so I instead copied the code into a new non-Windows test. Is there a way to start a Windows buildbot manually to ensure it works there?

As an aside, should we keep all these commits separate or squash them to avoid intermediate states with the broken test?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

📌 Commit b6767b3 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Aug 9, 2019
Rollup merge of rust-lang#63352 - jgalenson:reproducible-lto, r=alexc…
…richton

Sort the fat LTO modules to produce deterministic output.

Some projects that use LTO for their release builds are not reproducible.  We can fix this by sorting the fat LTO modules before using them.

It might also be useful to do this for thin LTO, but I couldn't get that to work to test it so I didn't do it.
@Centril Centril referenced this pull request Aug 9, 2019
bors added a commit that referenced this pull request Aug 9, 2019
Auto merge of #63424 - Centril:rollup-2k5gn6b, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #62756 (Stabilize duration_float)
 - #62860 (Stabilize checked_duration_since for 1.38.0)
 - #63337 (Tweak mismatched types error)
 - #63350 (Use associated_type_bounds where applicable - closes #61738)
 - #63352 (Sort the fat LTO modules to produce deterministic output.)
 - #63394 (Add test for issue 36804)
 - #63399 (More explicit diagnostic when using a `vec![]` in a pattern)
 - #63419 (check against more collisions for TypeId of fn pointer)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

⌛️ Testing commit b6767b3 with merge 6f70adc...

bors added a commit that referenced this pull request Aug 10, 2019
Auto merge of #63352 - jgalenson:reproducible-lto, r=alexcrichton
Sort the fat LTO modules to produce deterministic output.

Some projects that use LTO for their release builds are not reproducible.  We can fix this by sorting the fat LTO modules before using them.

It might also be useful to do this for thin LTO, but I couldn't get that to work to test it so I didn't do it.
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 6f70adc to master...

@bors bors added the merged-by-bors label Aug 10, 2019

@bors bors merged commit b6767b3 into rust-lang:master Aug 10, 2019

5 checks passed

homu Test successful
Details
pr Build #20190809.30 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

@jgalenson jgalenson deleted the jgalenson:reproducible-lto branch Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.