Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1809643003:
Use GCC for snapshot_toolchain when is_clang=false. (Closed)
DescriptionUse GCC for snapshot_toolchain when is_clang=false.
Currently, snapshot_toolchain is hardcoded to use a clang host
toolchain. Use a GCC toolchain if is_clang is false.
Revert this when this is root-caused (see crbug.com/601486)
LOG=Y
BUG=601486
Committed: https://crrev.com/920370d1a910681bf464bd91aced316058ea2f6c
Cr-Commit-Position: refs/heads/master@{#35341}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 23 (8 generated)
slan@chromium.org changed reviewers: + alokp@chromium.org, dpranke@chromium.org, jochen@chromium.org
Reproduced from https://chromium-review.googlesource.com/#/c/332986 https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni#newc... snapshot_toolchain.gni:36: if (is_clang) { comment from alokp@ in Gerrit Review: This seems equivalent to "snapshot_toolchain = host_toolchain" https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni#newc... snapshot_toolchain.gni:36: if (is_clang) { On 2016/03/16 17:40:05, slan wrote: > comment from alokp@ in Gerrit Review: > > This seems equivalent to "snapshot_toolchain = host_toolchain" > https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD... That is what I wanted to do, but there seems to be an explicit enforcement that x86/x64 host and target toolchain architectures are aligned. jochen@, do you see any issue with just assigning snapshot_toolchain=host_toolchain?
https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni#newc... snapshot_toolchain.gni:36: if (is_clang) { On 2016/03/16 17:41:46, slan wrote: > On 2016/03/16 17:40:05, slan wrote: > > comment from alokp@ in Gerrit Review: > > > > This seems equivalent to "snapshot_toolchain = host_toolchain" > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD... > > That is what I wanted to do, but there seems to be an explicit enforcement that > x86/x64 host and target toolchain architectures are aligned. > > jochen@, do you see any issue with just assigning > snapshot_toolchain=host_toolchain? You definitely can't use snapshot_toolchain=host_toolchain; we need to use the 32-bit x86 toolchain to maintain the same bit-width as the target 32-bit arm toolchain (and host_toolchain is always 64-bit; we don't support 32-bit host toolchains), otherwise the snapshot won't even work. We did intentionally choose to always use clang for host toolchains on linux, rather than match the compiler for the target toolchain. For android, at least, that is an intentional choice because we want to use clang wherever possible for linux (desktop) builds, so that we don't have to worry about supporting both gcc and clang builds of the 64-bit tools. Why are you making this change? Are you building in an environment where you don't have clang for the host and snapshot toolchains?
On 2016/03/16 22:14:45, Dirk Pranke wrote: > https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni > File snapshot_toolchain.gni (right): > > https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni#newc... > snapshot_toolchain.gni:36: if (is_clang) { > On 2016/03/16 17:41:46, slan wrote: > > On 2016/03/16 17:40:05, slan wrote: > > > comment from alokp@ in Gerrit Review: > > > > > > This seems equivalent to "snapshot_toolchain = host_toolchain" > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD... > > > > That is what I wanted to do, but there seems to be an explicit enforcement > that > > x86/x64 host and target toolchain architectures are aligned. > > > > jochen@, do you see any issue with just assigning > > snapshot_toolchain=host_toolchain? > > You definitely can't use snapshot_toolchain=host_toolchain; we need to use the > 32-bit x86 toolchain to maintain the same bit-width as the target 32-bit arm > toolchain (and host_toolchain is always 64-bit; we don't support 32-bit host > toolchains), otherwise the snapshot won't even work. sgtm, thanks for the details! > > We did intentionally choose to always use clang for host toolchains on linux, > rather than match the compiler for the target toolchain. For android, at least, > that is an intentional choice because we want to use clang wherever possible for > linux (desktop) builds, so that we don't have to worry about supporting both gcc > and clang builds of the 64-bit tools. > > Why are you making this change? Are you building in an environment where you > don't have clang for the host and snapshot toolchains? The motivation is twofold: 1) Our GYP build uses a gcc host toolchain to do this step. We also use a gcc toolchain to build all of our targets on GN builds. Will this cause an issue? 2) Attempting to build mksnapshot 51.0.2679.0 with clang++ causes the build command to hang. Meanwhile, using gcc works just fine. I am stumped as to what is causing this strange behavior. Any thoughts? I will keep looking into it.
On 2016/03/17 22:15:07, slan wrote: > On 2016/03/16 22:14:45, Dirk Pranke wrote: > > https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni > > File snapshot_toolchain.gni (right): > > > > > https://codereview.chromium.org/1809643003/diff/1/snapshot_toolchain.gni#newc... > > snapshot_toolchain.gni:36: if (is_clang) { > > On 2016/03/16 17:41:46, slan wrote: > > > On 2016/03/16 17:40:05, slan wrote: > > > > comment from alokp@ in Gerrit Review: > > > > > > > > This seems equivalent to "snapshot_toolchain = host_toolchain" > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD... > > > > > > That is what I wanted to do, but there seems to be an explicit enforcement > > that > > > x86/x64 host and target toolchain architectures are aligned. > > > > > > jochen@, do you see any issue with just assigning > > > snapshot_toolchain=host_toolchain? > > > > You definitely can't use snapshot_toolchain=host_toolchain; we need to use the > > 32-bit x86 toolchain to maintain the same bit-width as the target 32-bit arm > > toolchain (and host_toolchain is always 64-bit; we don't support 32-bit host > > toolchains), otherwise the snapshot won't even work. > > sgtm, thanks for the details! > > > > > We did intentionally choose to always use clang for host toolchains on linux, > > rather than match the compiler for the target toolchain. For android, at > least, > > that is an intentional choice because we want to use clang wherever possible > for > > linux (desktop) builds, so that we don't have to worry about supporting both > gcc > > and clang builds of the 64-bit tools. > > > > Why are you making this change? Are you building in an environment where you > > don't have clang for the host and snapshot toolchains? > > The motivation is twofold: > 1) Our GYP build uses a gcc host toolchain to do this step. We also use a gcc > toolchain to build all of our targets on GN builds. Will this cause an issue? > > 2) Attempting to build mksnapshot 51.0.2679.0 with clang++ causes the build > command to hang. Meanwhile, using gcc works just fine. I am stumped as to what > is causing this strange behavior. Any thoughts? I will keep looking into it. Note: I stand corrected on #2. I just did a clean build, and the target built (it was slow but never hung). I think we're good here, considering your desire to standardized host toolchains. I'll close this issue out, and make sure that we're building GN builds with a clang host toolchain. Thanks, all!
On 2016/03/17 22:22:21, slan wrote: > On 2016/03/17 22:15:07, slan wrote: > > On 2016/03/16 22:14:45, Dirk Pranke wrote: > > > Why are you making this change? Are you building in an environment where you > > > don't have clang for the host and snapshot toolchains? > > > > The motivation is twofold: > > 1) Our GYP build uses a gcc host toolchain to do this step. We also use a gcc > > toolchain to build all of our targets on GN builds. Will this cause an issue? It's certainly conceivable that we could hit issues w/ host clang vs. host gcc, but we'd like to err on the side of host clang, so let's see if we can make that work.
On 2016/03/17 22:25:58, Dirk Pranke wrote: > On 2016/03/17 22:22:21, slan wrote: > > On 2016/03/17 22:15:07, slan wrote: > > > On 2016/03/16 22:14:45, Dirk Pranke wrote: > > > > Why are you making this change? Are you building in an environment where > you > > > > don't have clang for the host and snapshot toolchains? > > > > > > The motivation is twofold: > > > 1) Our GYP build uses a gcc host toolchain to do this step. We also use a > gcc > > > toolchain to build all of our targets on GN builds. Will this cause an > issue? > > It's certainly conceivable that we could hit issues w/ host clang vs. host gcc, > but > we'd like to err on the side of host clang, so let's see if we can make that > work. Hey all, we continue to be plagued by this issue on Chromecast. It seems like a clang corner-case bug, related to the optimization level used on clang. Here are the compile times for counters.cc. We currently pass -O2, and changing to -O0 (no other differences), causes a night-and-day speedup: -O0: 3.535s -O2: 8m41.966s There have been a few more findings (see internal b/28027960), all of which point to the FOR_EACH_INTRINSIC macro as the piece that exposes the clang behavior. I would like to get upstream repro steps and assign a crbug to fix. Until then, how do we feel about landing this change as a stopgap?
On 2016/04/06 23:30:26, slan wrote: > I would like to get upstream repro steps and assign a crbug to fix. Until then, > how do we feel about landing this change as a stopgap? As long as we file a bug and hunt down what's going on, I'm fine w/ landing this CL. i.e., lgtm.
lgtm
Description was changed from ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. BUG= ========== to ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. Revert this when this is root-caused (see crbug.com/601486) BUG=601486 ==========
The CQ bit was checked by slan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809643003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809643003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13372)
Description was changed from ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. Revert this when this is root-caused (see crbug.com/601486) BUG=601486 ========== to ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. Revert this when this is root-caused (see crbug.com/601486) LOG=Y BUG=601486 ==========
The CQ bit was checked by slan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809643003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809643003/1
Message was sent while issue was closed.
Description was changed from ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. Revert this when this is root-caused (see crbug.com/601486) LOG=Y BUG=601486 ========== to ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. Revert this when this is root-caused (see crbug.com/601486) LOG=Y BUG=601486 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. Revert this when this is root-caused (see crbug.com/601486) LOG=Y BUG=601486 ========== to ========== Use GCC for snapshot_toolchain when is_clang=false. Currently, snapshot_toolchain is hardcoded to use a clang host toolchain. Use a GCC toolchain if is_clang is false. Revert this when this is root-caused (see crbug.com/601486) LOG=Y BUG=601486 Committed: https://crrev.com/920370d1a910681bf464bd91aced316058ea2f6c Cr-Commit-Position: refs/heads/master@{#35341} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/920370d1a910681bf464bd91aced316058ea2f6c Cr-Commit-Position: refs/heads/master@{#35341}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1873113002/ by hablich@chromium.org. The reason for reverting is: Blocks roll: https://codereview.chromium.org/1874173002/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
