Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
993173003:
Fix the toolchain used to build the snapshots in GN. (Closed)
|
Created:
5 years, 9 months ago by Dirk Pranke Modified:
5 years, 9 months ago CC:
Nico, v8-dev, Michael Hablich, Dmitry Lomov (no reviews), vogelheim, Yang Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionFix the toolchain used to build the snapshots in GN.
We need the v8 snapshot to be compiled on the host cpu (the
machine doing the build), but using generated code that has the
same pointer size as the target_arch; i.e., for 32-bit arm builds,
we need to use a 32-bit x86 host binary, not a 64-bit host binary.
The easiest way to ensure that this happens is to just specify
a custom toolchain in GN to use to build the snapshot.
R=jochen@chromium.org, cjhopman@chromium.org
BUG=465456, 395249
LOG=Y
Committed: https://crrev.com/ad5630db87be2da6865ff17989b455791b6c152d
Cr-Commit-Position: refs/heads/master@{#27142}
Patch Set 1 #Patch Set 2 : fix snapshot_toolchain for non-android builds #
Total comments: 8
Patch Set 3 : update comments #Messages
Total messages: 21 (3 generated)
dpranke@chromium.org changed reviewers: + machenbach@chromium.org
I'm not yet sure that this is right; I haven't yet figured out a way to successfully test this, and I'm not sure if there are other cases where we need to worry about bit-depth-mismatch (some embedded 32-bit linux port being built on a 64-bit linux host, perhaps?). Possibly we still need to support a !is_clang toolchain, also ...
I don't know much about this, but !is_clang is still used by cros (which uses one toolchain for the whole system from what i understand) https://codereview.chromium.org/993173003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode22 BUILD.gn:22: # TODO(GYP): We should be just target_cpu and not need v8_target_arch. this reads grammared incorrectly
On 2015/03/10 23:54:54, Dirk Pranke wrote: > I'm not yet sure that this is right; I haven't yet figured out a way to > successfully test this, and I'm not sure if there are other cases where we need > to worry about bit-depth-mismatch (some embedded 32-bit linux port being built > on a 64-bit linux host, perhaps?). Possibly we still need to support a !is_clang > toolchain, also ... I'm not sure how, but this seems to generate the correct blob files. When looking at the set of flags passed to the compiler with your patch compared to a gyp build they are much different. None-the-less this works, so I'm happy.
lgtm with comments assuming it works :) https://codereview.chromium.org/993173003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode22 BUILD.gn:22: # TODO(GYP): We should be just target_cpu and not need v8_target_arch. On 2015/03/11 at 13:47:48, Nico (traveling) wrote: > this reads grammared incorrectly also, we want to be able to e.g. compile for linux x64 as target_cpu but use v8_target_arch arm64. This will run v8 with a simulator, allowing for better asan coverage. https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode32 BUILD.gn:32: if (target_os == "android" || target_os == "chromeos") { why this restriction?
https://codereview.chromium.org/993173003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode22 BUILD.gn:22: # TODO(GYP): We should be just target_cpu and not need v8_target_arch. On 2015/03/11 16:10:30, jochen (slow) wrote: > On 2015/03/11 at 13:47:48, Nico (traveling) wrote: > > this reads grammared incorrectly Will fix :). > also, we want to be able to e.g. compile for linux x64 as target_cpu but use > v8_target_arch arm64. This will run v8 with a simulator, allowing for better > asan coverage. Sure. https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode32 BUILD.gn:32: if (target_os == "android" || target_os == "chromeos") { On 2015/03/11 16:10:30, jochen (slow) wrote: > why this restriction? I'm not sure I understand the question. Which restriction are you referring to? The host must be linux restriction is there mostly to be sure we understand what configurations are actually being called (i.e., I don't expect trying to build android v8 from windows to work at this point, so I don't want people to try it and spend time debugging weird things). I'm happy to restructure it if you like.
https://codereview.chromium.org/993173003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode32 BUILD.gn:32: if (target_os == "android" || target_os == "chromeos") { On 2015/03/11 at 16:53:30, Dirk Pranke wrote: > On 2015/03/11 16:10:30, jochen (slow) wrote: > > why this restriction? > > I'm not sure I understand the question. Which restriction are you referring to? The host must be linux restriction is there mostly to be sure we understand what configurations are actually being called (i.e., I don't expect trying to build android v8 from windows to work at this point, so I don't want people to try it and spend time debugging weird things). > > I'm happy to restructure it if you like. ok, maybe add a comment then that this is supposed to work for all target and host os combinations that are supported by v8 (or chrome for that matter)
https://codereview.chromium.org/993173003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode32 BUILD.gn:32: if (target_os == "android" || target_os == "chromeos") { That was the intent of the comment on lines 30-31, but perhaps it's not completely clear. I will reword it a bit.
https://codereview.chromium.org/993173003/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/993173003/diff/20001/BUILD.gn#newcode32 BUILD.gn:32: if (target_os == "android" || target_os == "chromeos") { On 2015/03/11 at 18:56:10, Dirk Pranke wrote: > That was the intent of the comment on lines 30-31, but perhaps it's not completely clear. > > I will reword it a bit. ah, that's probably enough. Maybe architectures/OSs?
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/993173003/#ps40001 (title: "update comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/993173003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ad5630db87be2da6865ff17989b455791b6c152d Cr-Commit-Position: refs/heads/master@{#27142}
Message was sent while issue was closed.
This breaks/blocks the current v8 roll on linux_chromium_gn_chromeos_rel: https://codereview.chromium.org/1005483003/ Do you have a quick fix for the problem, or can you revert this CL? In both cases I can do a cherry-pick for the roll.
Message was sent while issue was closed.
On 2015/03/12 17:21:24, Michael Achenbach (travelling) wrote: > This breaks/blocks the current v8 roll on linux_chromium_gn_chromeos_rel: > https://codereview.chromium.org/1005483003/ > > Do you have a quick fix for the problem, or can you revert this CL? In both > cases I can do a cherry-pick for the roll. Ah, right, my change would break a 64-bit x86 chromeos build (which is what that bot effectively is).
Message was sent while issue was closed.
> Ah, right, my change would break a 64-bit x86 chromeos build (which is what that > bot effectively is). i.e., an x64 chromeos build.
Message was sent while issue was closed.
What action do you suggest? Fix or revert? Do you have a fix ready? Someone closed the ongoing V8 roll (I don't know with which intention).
Message was sent while issue was closed.
On 2015/03/12 20:23:25, Michael Achenbach (travelling) wrote: > What action do you suggest? Fix or revert? Do you have a fix ready? > > Someone closed the ongoing V8 roll (I don't know with which intention). I thought yangguo and jochen answered this somewhere; the fix landed in https://codereview.chromium.org/874393007 .
Message was sent while issue was closed.
On 2015/03/12 20:48:28, Dirk Pranke wrote: > On 2015/03/12 20:23:25, Michael Achenbach (travelling) wrote: > > What action do you suggest? Fix or revert? Do you have a fix ready? > > > > Someone closed the ongoing V8 roll (I don't know with which intention). > > I thought yangguo and jochen answered this somewhere; the fix landed in > https://codereview.chromium.org/874393007 . Ah, thanks. The answer somehow didn't reach me. Maybe they think as I'm in an airplane, I have no internet anyway, but I do :) Cherry-picking the fix onto the last roll attempt makes not much sense as the roll had only two commits. We can wait it out and just close the next roll attempts until the auto-roller passes the fix CL.
Message was sent while issue was closed.
On 2015/03/12 20:48:28, Dirk Pranke wrote: > On 2015/03/12 20:23:25, Michael Achenbach (travelling) wrote: > > What action do you suggest? Fix or revert? Do you have a fix ready? > > > > Someone closed the ongoing V8 roll (I don't know with which intention). > > I thought yangguo and jochen answered this somewhere; the fix landed in > https://codereview.chromium.org/874393007 . I closed the roll CL in the hope that autoroll would kick in again. However it didn't. |
