Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2158753003:
Clang toolchain: use lld to compile LLVM Gold plugin and lld with LTO. (Closed)
|
Created:
4 years, 5 months ago by krasin Modified:
4 years, 4 months ago CC:
chromium-reviews, eugenis+clang_chromium.org, vmpstr+watch_chromium.org, yunlian, glider+clang_chromium.org, Nico, ukai+watch_chromium.org, Reid Kleckner, dmikurube+clang_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClang toolchain: ship lld; build the whole toolchain with LTO.
This only affects the Linux build.
BUG=607968
Committed: https://crrev.com/36bb2be2523c83aa6b7601e92753dd49ae1924d3
Cr-Commit-Position: refs/heads/master@{#407702}
Patch Set 1 #Patch Set 2 : build whole Clang toolchain with LTO. #Patch Set 3 : remove debug trick #Patch Set 4 : Fix cxxflags #Patch Set 5 : Windows workaround #Patch Set 6 : Copy lld into the Linux archive. #
Total comments: 6
Patch Set 7 : Always checkout lld #Patch Set 8 : sync #Patch Set 9 : Also set cxx flags #Patch Set 10 : sync #Messages
Total messages: 22 (6 generated)
krasin@chromium.org changed reviewers: + hans@chromium.org, krasin@chromium.org, pcc@chromium.org, thakis@chromium.org
Hello there, I think I have got it working. There's some slowdown to the Linux build, but we'll be able to cut the time back after retiring LLVM Gold plugin. Three changes here (all on Linux): 1. We build lld and copy it to the Clang tooclhain 2. We build LTO LLVM Gold plugin with lld instead of LLVM Gold plugin 3. We build Clang toolchain with LTO (and lld) Please, take a look.
Description was changed from ========== Clang toolchain: use lld to compile LLVM Gold plugin and lld with LTO. BUG=607968 ========== to ========== Clang toolchain: ship lld; build the whole toolchain with LTO. This only affects the Linux build. BUG=607968 ==========
Very sorry for the slow reply. Traveling hasn't been great for my code reviews. https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... tools/clang/scripts/update.py:442: Checkout('LLD', LLVM_REPO_URL + '/lld/trunk', LLD_DIR) I wonder if we should just always build it, to simplify the logic here. package.py still decides if it's included in the tarball. https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... tools/clang/scripts/update.py:605: llvm_build_env = os.environ.copy() Pretty scary. But now we'll have an empty llvm_build_env on Windows. Does that work? https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... tools/clang/scripts/update.py:823: help='build Clang toolchain with LTO') The flag also controls whether to build the Gold plugin. Maybe a little bit confusing..
Hello Hans, thank you for taking the first look. Please, take another one. :) https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... tools/clang/scripts/update.py:442: Checkout('LLD', LLVM_REPO_URL + '/lld/trunk', LLD_DIR) On 2016/07/22 15:33:05, hans wrote: > I wonder if we should just always build it, to simplify the logic here. > package.py still decides if it's included in the tarball. This is a good idea. Done. https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... tools/clang/scripts/update.py:605: llvm_build_env = os.environ.copy() On 2016/07/22 15:33:05, hans wrote: > Pretty scary. But now we'll have an empty llvm_build_env on Windows. Does that > work? This is a non-functional change for windows, as passing None as env into subprocess.call means "use the system env". This is also exactly what we passed before my change, as deployment_env was initialized to None as well. https://codereview.chromium.org/2158753003/diff/100001/tools/clang/scripts/up... tools/clang/scripts/update.py:823: help='build Clang toolchain with LTO') On 2016/07/22 15:33:05, hans wrote: > The flag also controls whether to build the Gold plugin. Maybe a little bit > confusing.. Yes, but I hope we can remove Gold plugin altogether very soon. If not, we can refactor it later.
lgtm How big is the size increase of the Clang package?
On 2016/07/25 20:56:46, hans wrote: > lgtm > > How big is the size increase of the Clang package? Before: 32.49 MiB 2016-07-01T23:04:21Z gs://chromium-browser-clang/Linux_x64/clang-274369-1.tgz After: 48.47 MiB 2016-07-20T23:13:36Z gs://chromium-browser-clang/Linux_x64/clang-274369-5.tgz Again, this can be mitigated by using xz for compression, based on my earlier findings. In Python, this package potentially can be used: https://docs.python.org/3/library/lzma.html Also, note that I have got some unexplainable issues with the build on bots. Sometimes it succeeds: https://codereview.chromium.org/2166823003/ And sometimes it doesn't: https://codereview.chromium.org/2180543002/ At this time, I have no explanation, and I also could not reproduce this locally (but was able to reproduce in the interactive mode on the bot). I will figure out that prior submitting this CL.
On Mon, Jul 25, 2016 at 2:08 PM, <krasin@google.com> wrote: > Reviewers: Nico (ooo until Aug 1), hans, pcc1, krasin1 > CL: https://codereview.chromium.org/2158753003/ > > Message: > On 2016/07/25 20:56:46, hans wrote: >> lgtm >> >> How big is the size increase of the Clang package? > > Before: 32.49 MiB 2016-07-01T23:04:21Z > gs://chromium-browser-clang/Linux_x64/clang-274369-1.tgz > After: 48.47 MiB 2016-07-20T23:13:36Z > gs://chromium-browser-clang/Linux_x64/clang-274369-5.tgz Size increase sucks, but I think at this point it makes sense to include it on Linux. Also, given that lld is 55 MB or so, it could be worse :-) > Again, this can be mitigated by using xz for compression, based on my > earlier > findings. In Python, this package potentially can be used: > https://docs.python.org/3/library/lzma.html We'd need Python 3 though, which realistically I wonder if we'll ever get to.. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes, we'll need Python 3. What what specifically actually prevents us from having it?
On 2016/07/25 21:20:28, krasin wrote: > Yes, we'll need Python 3. What what specifically actually prevents us from > having it? All of Chromium's infrastructure would have to be ported to it. I imagine it would be a yak shave of gigantic proportions.
Honestly, I don't see the global cleanup as a prerequisite for running a single python script. What prevents us from having both versions of Python installed, have Python 2 by default, and use Python 3, when explicitly requested in shebang (or whatever)? Sorry, if the question is silly, is the a dedicated person / team that maintains the infrastructure (other than the bots)? I know that we have a couple of people working on GYP->GN transition. Do we have anyone else?
On 2016/07/25 21:56:05, krasin wrote: > Honestly, I don't see the global cleanup as a prerequisite for running a single > python script. > What prevents us from having both versions of Python installed, have Python 2 by > default, and use Python 3, when explicitly requested in shebang (or whatever)? > > Sorry, if the question is silly, is the a dedicated person / team that maintains > the infrastructure (other than the bots)? I know that we have a couple of people > working on GYP->GN transition. Do we have anyone else? g/chrome-infra are the folks running it.
On 2016/07/25 22:44:06, hans wrote: > On 2016/07/25 21:56:05, krasin wrote: > > Honestly, I don't see the global cleanup as a prerequisite for running a > single > > python script. > > What prevents us from having both versions of Python installed, have Python 2 > by > > default, and use Python 3, when explicitly requested in shebang (or whatever)? > > > > Sorry, if the question is silly, is the a dedicated person / team that > maintains > > the infrastructure (other than the bots)? I know that we have a couple of > people > > working on GYP->GN transition. Do we have anyone else? > > g/chrome-infra are the folks running it. thx. I will probably start a discussion with creating a bug to make Python 3 available in the Chromium checkout, and then add some people from go/chrome-infra there.
So, I have built the Linux toolchain successfully: https://codereview.chromium.org/2181883002/ The Windows bot is red there, but it's irrelevant to the current CL. I will submit it, but there's a chance that the build is not deterministic anymore. While it's bad and a sign of a bigger problem in LLVM, we are better learn that from package-Clang-on-bots CLs rather than while building desktop Chrome: https://crbug.com/630335
The CQ bit was checked by krasin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hans@chromium.org Link to the patchset: https://codereview.chromium.org/2158753003/#ps180001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Clang toolchain: ship lld; build the whole toolchain with LTO. This only affects the Linux build. BUG=607968 ========== to ========== Clang toolchain: ship lld; build the whole toolchain with LTO. This only affects the Linux build. BUG=607968 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Clang toolchain: ship lld; build the whole toolchain with LTO. This only affects the Linux build. BUG=607968 ========== to ========== Clang toolchain: ship lld; build the whole toolchain with LTO. This only affects the Linux build. BUG=607968 Committed: https://crrev.com/36bb2be2523c83aa6b7601e92753dd49ae1924d3 Cr-Commit-Position: refs/heads/master@{#407702} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/36bb2be2523c83aa6b7601e92753dd49ae1924d3 Cr-Commit-Position: refs/heads/master@{#407702}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2217113003/ by thakis@chromium.org. The reason for reverting is: Somewhat speculative; it's possible this breaks building clang packages. See crbug.com/629966 comment 36 / 37.. |
