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

Allow for opting out of ThinLTO and clean up LTO related cli flag handling. #53950

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

michaelwoerister
Copy link
Member

It turns out that there currently is no way to explicitly disable ThinLTO (except for the nightly-only -Zthinlto flag). This PR extends -C lto to take yes and no in addition to thin and fat. It should be backwards compatible.

It also cleans up how LTO mode selection is handled.

Note that merging the PR in the current state would make the new values for -C lto available on the stable channel. I think that would be fine but maybe some team should vote on it.

r? @alexcrichton

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2018
@alexcrichton
Copy link
Member

Looks good to me! Perhaps someone from @rust-lang/compiler can kick off an FCP to merge?

@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 4, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:49:35] ....................................................................................................
[00:49:39] ....................................................................................................
[00:49:41] ...........................i........................................................................
[00:49:44] ....................................................................................................
[00:49:47] ............................................................................iiiiiiiii...............
[00:49:53] ....................................................................................................
[00:49:57] ....................................................................................................
[00:49:59] .........................................................i..........................................
[00:50:02] ....................................................................................................
---
[01:11:44]    Compiling rustc v0.0.0 (file:///checkout/src/librustc)
[01:12:05] error[E0308]: mismatched types
[01:12:05]     --> librustc/session/config.rs:2968:23
[01:12:05]      |
[01:12:05] 2968 |         opts.cg.lto = Lto::Fat;
[01:12:05]      |                       ^^^^^^^^ expected enum `session::config::LtoCli`, found enum `session::config::Lto`
[01:12:05]      |
[01:12:05]      = note: expected type `session::config::LtoCli`
[01:12:05]                 found type `session::config::Lto`
[01:12:14] error: aborting due to previous error
[01:12:14] 
[01:12:14] For more information about this error, try `rustc --explain E0308`.
[01:12:14] error: Could not compile `rustc`.
[01:12:14] error: Could not compile `rustc`.
[01:12:14] 
[01:12:14] To learn more, run the command again with --verbose.
[01:12:14] 
[01:12:14] 
[01:12:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "-p" "rustc" "--" "--quiet"
[01:12:14] 
[01:12:14] 
[01:12:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:12:14] Build completed unsuccessfully in 0:26:47
[01:12:14] Build completed unsuccessfully in 0:26:47
[01:12:14] make: *** [check] Error 1
[01:12:14] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:036f1db4
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:27a0b62c:start=1536083822240887881,finish=1536083822365883357,duration=124995476
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2649fcd9
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0075a638
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@cramertj
Copy link
Member

cramertj commented Sep 5, 2018

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Sep 5, 2018

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 5, 2018
@michaelwoerister
Copy link
Member Author

Ping @rust-lang/compiler: It would be great if we could get this merged before the beta cut-off (if it's not too late already). Please tick off your box if you haven't already! ❤️

@Mark-Simulacrum
Copy link
Member

We have approximately 2 days left before beta cutoff, though we might delay slightly to make sure edition-related components make it in. So far though looks like we're on schedule for Tuesday.

@rfcbot
Copy link

rfcbot commented Sep 10, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 10, 2018
@michaelwoerister
Copy link
Member Author

OK, everybody signed off but since the FCP will run for another week, I guess we'll have to do a beta backport?

@alexcrichton
Copy link
Member

@bors: r+

I'm pretty confident in this, but if a concern is raised during FCP I volunteer to be on the hook to revert it from beta

@bors
Copy link
Contributor

bors commented Sep 11, 2018

📌 Commit 24093a6 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2018
@bors
Copy link
Contributor

bors commented Sep 12, 2018

⌛ Testing commit 24093a6 with merge 502e750e7f02fe048dcf9ef51e726d24c1f74c24...

@bors
Copy link
Contributor

bors commented Sep 12, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 12, 2018
@rust-highfive
Copy link
Collaborator

The job dist-mipsel-linux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_fold:end:step_upload_script
travis_fold:start:worker_info
Worker information
hostname: 39512833-309a-4965-8dc3-e06e399cbc43@1.production-4-worker-org-a-1-gce
version: v4.1.0-17-g600200d https://github.com/travis-ci/worker/tree/600200deddbb9114004580ec8d6dd7b4722840d9
startup: 6.720447968s
travis_fold:end:worker_info
travis_fold:start:system_info
Build system information
---
[00:48:44]    Compiling rustc-demangle v0.1.9
[00:48:45] [RUSTC-TIMING] libc test:false 2.317
[00:48:45]    Compiling memmap v0.6.2
[00:48:46] [RUSTC-TIMING] memmap test:false 0.807
The job exceeded the maximum time limit for jobs, and has been terminated.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@alexcrichton
Copy link
Member

@bors: retry

we should have longer than 50m to do a build...

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Sep 13, 2018
…excrichton

Allow for opting out of ThinLTO and clean up LTO related cli flag handling.

It turns out that there currently is no way to explicitly disable ThinLTO (except for the nightly-only `-Zthinlto` flag). This PR extends `-C lto` to take `yes` and `no` in addition to `thin` and `fat`. It should be backwards compatible.

It also cleans up how LTO mode selection is handled.

Note that merging the PR in the current state would make the new values for `-C lto` available on the stable channel. I think that would be fine but maybe some team should vote on it.
bors added a commit that referenced this pull request Sep 13, 2018
Rollup of 11 pull requests

Successful merges:

 - #53371 (Do not emit E0277 on incorrect tuple destructured binding)
 - #53829 (Add rustc SHA to released DWARF debuginfo)
 - #53950 (Allow for opting out of ThinLTO and clean up LTO related cli flag handling.)
 - #53976 (Replace unwrap calls in example by expect)
 - #54070 (Add Error::description soft-deprecation to RELEASES)
 - #54076 (miri loop detector hashing)
 - #54119 (Add some unit tests for find_best_match_for_name)
 - #54147 (Add a test that tries to modify static memory at compile-time)
 - #54150 (Updated 1.29 release notes with --document-private-items flag)
 - #54163 (Update stage 0 to latest beta)
 - #54170 (COMPILER_TESTS.md has been moved)
@bors bors merged commit 24093a6 into rust-lang:master Sep 14, 2018
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants