Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2060333002:
[Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions (Closed)
|
Created:
4 years, 6 months ago by pauljensen Modified:
4 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions
Some older devices may not support ARM Neon, so to ensure we run on these
devices, check that Cronet doesn't inadvertently use ARM Neon instructions.
Also, change Cronet bots to conform to this.
BUG=594316
Committed: https://crrev.com/420d1a10bcc3dde583b590fefa81884784234bc9
Cr-Commit-Position: refs/heads/master@{#413138}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : redo objdump path #
Total comments: 2
Patch Set 5 : rebase #Patch Set 6 : address comments #
Total comments: 2
Patch Set 7 : enforce -> check #Patch Set 8 : sync #Patch Set 9 : disable neon on bots #
Total comments: 6
Patch Set 10 : address jbudorick's comments #Patch Set 11 : don't generate target if preconditions not met #Patch Set 12 : sync #
Total comments: 2
Patch Set 13 : move enforcement into if #Patch Set 14 : get working #
Total comments: 11
Patch Set 15 : sync #Patch Set 16 : address comments #
Messages
Total messages: 56 (24 generated)
pauljensen@chromium.org changed reviewers: + mef@chromium.org
Misha, WDYT? Frankly, it seems a little ugly to me.
looks reasonable. https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:830: "$android_ndk_root/toolchains/arm-linux-androideabi-*/prebuilt/$host_os-x86_64/", this seems quite fragile. I wonder if there is a better way to specify current toolchain. https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:899: if (current_cpu == "armv7" || current_cpu == "arm") { does arm (v6, not armv7) ever get neon? https://codereview.chromium.org/2060333002/diff/20001/components/cronet/tools... File components/cronet/tools/enforce_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/20001/components/cronet/tools... components/cronet/tools/enforce_no_neon.py:20: sys.argv[2] + ' | grep -q "vld[1-9]\\|vst[1-9]"') ls vld and vst sprinkled all over base if neon is enabled?
https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:830: "$android_ndk_root/toolchains/arm-linux-androideabi-*/prebuilt/$host_os-x86_64/", On 2016/06/24 20:18:08, mef wrote: > this seems quite fragile. I wonder if there is a better way to specify current > toolchain. Done, I fixed it up. https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:899: if (current_cpu == "armv7" || current_cpu == "arm") { On 2016/06/24 20:18:08, mef wrote: > does arm (v6, not armv7) ever get neon? No, NEON was introduced with ARMv7: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dht0002a/BABIIFHA... https://codereview.chromium.org/2060333002/diff/20001/components/cronet/tools... File components/cronet/tools/enforce_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/20001/components/cronet/tools... components/cronet/tools/enforce_no_neon.py:20: sys.argv[2] + ' | grep -q "vld[1-9]\\|vst[1-9]"') On 2016/06/24 20:18:08, mef wrote: > ls vld and vst sprinkled all over base if neon is enabled? Yes. I think they're used for things like initializing lots of memory in less time using less code.
I think it is not that ugly any more. https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:899: if (current_cpu == "armv7" || current_cpu == "arm") { On 2016/06/27 12:58:42, pauljensen wrote: > On 2016/06/24 20:18:08, mef wrote: > > does arm (v6, not armv7) ever get neon? > > No, NEON was introduced with ARMv7: > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dht0002a/BABIIFHA... So, why do we need to do it for current_cpu == "arm"? https://codereview.chromium.org/2060333002/diff/60001/components/cronet/tools... File components/cronet/tools/enforce_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/60001/components/cronet/tools... components/cronet/tools/enforce_no_neon.py:7: enforce_no_neon.py - Enforce modules do not contain ARM Neon instructions. nit: I don't think this script really enforces no neon, it just checks for its presence.
https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:899: if (current_cpu == "armv7" || current_cpu == "arm") { On 2016/06/28 16:46:24, mef wrote: > On 2016/06/27 12:58:42, pauljensen wrote: > > On 2016/06/24 20:18:08, mef wrote: > > > does arm (v6, not armv7) ever get neon? > > > > No, NEON was introduced with ARMv7: > > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dht0002a/BABIIFHA... > > So, why do we need to do it for current_cpu == "arm"? Actually "arm" means all versions of ARM. "armv7" looks like some old relic. I've redone this line. https://codereview.chromium.org/2060333002/diff/60001/components/cronet/tools... File components/cronet/tools/enforce_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/60001/components/cronet/tools... components/cronet/tools/enforce_no_neon.py:7: enforce_no_neon.py - Enforce modules do not contain ARM Neon instructions. On 2016/06/28 16:46:24, mef wrote: > nit: I don't think this script really enforces no neon, it just checks for its > presence. Done.
lgtm https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... File components/cronet/tools/check_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... components/cronet/tools/check_no_neon.py:7: enforce_no_neon.py - Enforce modules do not contain ARM Neon instructions. nit: enforce -> check.
https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... File components/cronet/tools/check_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... components/cronet/tools/check_no_neon.py:7: enforce_no_neon.py - Enforce modules do not contain ARM Neon instructions. On 2016/06/29 15:48:06, mef wrote: > nit: enforce -> check. Done.
On 2016/07/19 10:26:44, pauljensen wrote: > https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... > File components/cronet/tools/check_no_neon.py (right): > > https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... > components/cronet/tools/check_no_neon.py:7: enforce_no_neon.py - Enforce modules > do not contain ARM Neon instructions. > On 2016/06/29 15:48:06, mef wrote: > > nit: enforce -> check. > > Done. Hrm, it seems that android_cronet_tester trybot doesn't enforce no neon?
On 2016/07/19 14:42:07, mef wrote: > On 2016/07/19 10:26:44, pauljensen wrote: > > > https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... > > File components/cronet/tools/check_no_neon.py (right): > > > > > https://codereview.chromium.org/2060333002/diff/100001/components/cronet/tool... > > components/cronet/tools/check_no_neon.py:7: enforce_no_neon.py - Enforce > modules > > do not contain ARM Neon instructions. > > On 2016/06/29 15:48:06, mef wrote: > > > nit: enforce -> check. > > > > Done. > > Hrm, it seems that android_cronet_tester trybot doesn't enforce no neon? See my other review request :)
pauljensen@chromium.org changed reviewers: + jbudorick@chromium.org
John, PTAL @ tools/mb/mb_config.pyl I know you're not an OWNER but I was trying to follow your suggestion. Instead of adding a separate mixin for disabling neon I thought I'd add a different target to prevent others from disabling Neon with ARM64 or ARMv6 accidentally. If you want me to go with your solution directly, just let me know and I'll change it.
https://codereview.chromium.org/2060333002/diff/160001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/160001/components/cronet/andr... components/cronet/android/BUILD.gn:921: deps += [ ":enforce_no_neon" ] This is an interesting way to handle this. You could also assert that arm_use_neon is false in this scenario, though you may also want to check the build artifact. https://codereview.chromium.org/2060333002/diff/160001/components/cronet/tool... File components/cronet/tools/check_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/160001/components/cronet/tool... components/cronet/tools/check_no_neon.py:14: if len(sys.argv) != 3: nit: if you keep this script, please use argparse. https://codereview.chromium.org/2060333002/diff/160001/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/2060333002/diff/160001/tools/mb/mb_config.pyl... tools/mb/mb_config.pyl:1900: 'gn_args': 'target_cpu="arm" arm_use_neon=false', I don't think this should duplicate the arm parameters. You might be able to do something like this, though: 'arm_no_neon': { 'mixins': ['arm'], 'gn_args': 'arm_use_neon=false', 'gyp_defines': 'arm_neon=0', }, That may not work, or it may not be recommended style, so you could also do 'no_neon': { 'gn_args': 'arm_use_neon=false', 'gyp_defines': 'arm_neon=0', }, 'arm_no_neon': { 'mixins': ['arm', 'no_neon'], },
Description was changed from ========== [Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions Some older devices may not support ARM Neon, so to ensure we run on these devices, check that Cronet doesn't inadvertently use ARM Neon instructions. BUG=594316 ========== to ========== [Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions Some older devices may not support ARM Neon, so to ensure we run on these devices, check that Cronet doesn't inadvertently use ARM Neon instructions. Also, change Cronet bots to conform to this. BUG=594316 ==========
https://codereview.chromium.org/2060333002/diff/160001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/160001/components/cronet/andr... components/cronet/android/BUILD.gn:921: deps += [ ":enforce_no_neon" ] On 2016/07/19 18:49:16, jbudorick wrote: > This is an interesting way to handle this. You could also assert that > arm_use_neon is false in this scenario, though you may also want to check the > build artifact. Done. https://codereview.chromium.org/2060333002/diff/160001/components/cronet/tool... File components/cronet/tools/check_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/160001/components/cronet/tool... components/cronet/tools/check_no_neon.py:14: if len(sys.argv) != 3: On 2016/07/19 18:49:16, jbudorick wrote: > nit: if you keep this script, please use argparse. Done. https://codereview.chromium.org/2060333002/diff/160001/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/2060333002/diff/160001/tools/mb/mb_config.pyl... tools/mb/mb_config.pyl:1900: 'gn_args': 'target_cpu="arm" arm_use_neon=false', On 2016/07/19 18:49:16, jbudorick wrote: > I don't think this should duplicate the arm parameters. You might be able to do > something like this, though: > > 'arm_no_neon': { > 'mixins': ['arm'], > 'gn_args': 'arm_use_neon=false', > 'gyp_defines': 'arm_neon=0', > }, > > That may not work, or it may not be recommended style, so you could also do > > 'no_neon': { > 'gn_args': 'arm_use_neon=false', > 'gyp_defines': 'arm_neon=0', > }, > > 'arm_no_neon': { > 'mixins': ['arm', 'no_neon'], > }, Done.
lgtm
pauljensen@chromium.org changed reviewers: + dpranke@chromium.org
Dirk, PTAL @ tools/mb/mb_config.pyl
Obviously I don't have the full context on this, but this seems kinda paranoid. Why isn't just setting arm_use_neon=false sufficient?
On 2016/07/20 16:09:47, Dirk Pranke wrote: > Obviously I don't have the full context on this, but this seems kinda paranoid. > Why isn't just setting arm_use_neon=false sufficient? This has bitten Cronet hard...twice... I can go into more details in a non-public forum if you like, but suffice it to say that I think we're fairly justified in our paranoia. This comes back to the fact that we don't have any bots testing on older ARMv7 devices without Neon (e.g. Samsung Galaxy Tab 10.1). I think this simple verification is easier to live with than getting bots going with these older devices. I don't think we want to use arm_use_neon=false in all configurations (e.g. not for ARM64), just in ARMv7.
ok, sounds good then. I thought for a while if there was some better way to accomplish what you're trying to do, but given that some neon symbols are okay but most aren't, you're kinda stuck doing something ad-hoc like this. lgtm.
The CQ bit was checked by pauljensen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Cronet is referenced from //src/BUILD.gn, and various bots (at least linux_android_rel_ng and cast_shell_android) generate ninja files using gn for android without setting arm_no_neon, so they hit the assertion I added in components/cronet/android/BUILD.gn. So the current solution isn't going to work as it stands now. What do reviewers think about adding a //src/build/config/cronet_build.gni file that defaults an "is_cronet" variable to false, but when "is_cronet" is set to true it enables the various other flags that are currently smattered throughout a lot of different bot config files (e.g. use_errorprone_java_compiler=true enable_websockets=false disable_file_support=true disable_ftp_support=true use_platform_icu_alternatives=true disable_brotli_filter=true). Alternativly it could at least assert that they are set. Then we could also only include the Cronet dependencies in //src/BUILD.gn if is_cronet == true, which would get the bots that currently fail with this CL passing.
On 2016/07/21 18:58:34, pauljensen wrote: > Cronet is referenced from //src/BUILD.gn, and various bots (at least > linux_android_rel_ng and cast_shell_android) generate ninja files using gn for > android without setting arm_no_neon, so they hit the assertion I added in > components/cronet/android/BUILD.gn. So the current solution isn't going to work > as it stands now. > > What do reviewers think about adding a //src/build/config/cronet_build.gni file > that defaults an "is_cronet" variable to false, but when "is_cronet" is set to > true it enables the various other flags that are currently smattered throughout > a lot of different bot config files (e.g. use_errorprone_java_compiler=true > enable_websockets=false disable_file_support=true disable_ftp_support=true > use_platform_icu_alternatives=true disable_brotli_filter=true). Alternativly it > could at least assert that they are set. Then we could also only include the > Cronet dependencies in //src/BUILD.gn if is_cronet == true, which would get the > bots that currently fail with this CL passing. We don't really want to make the default values change depending on the "product" that is being built in GN, and so we're trying to avoid things like "is_cronet" or "is_chromecast" that get included generically in //build . It would be fine to have an cronet_build.gni file that got included by the targets that need it outside of build. However, if what you really want is a bunch of flags set specifically on cronet builds, what we're encouraging people to do is to use "args files", i.e., to check in a file with a bunch of GN args, and tell the builder to use that. See, e.g., //build/args/blimp_client.gn or //build/args/headless.gn (for two similar situations). You can set a generic set of args in that file, and then set builder-specific args like is_debug if need be in a file in //build/args/bots/mastername/buildername.gn . We're going to be switching to this model more and more as we get rid of GYP. Does that make sense?
On 2016/07/21 21:01:38, Dirk Pranke wrote: > On 2016/07/21 18:58:34, pauljensen wrote: > > Cronet is referenced from //src/BUILD.gn, and various bots (at least > > linux_android_rel_ng and cast_shell_android) generate ninja files using gn for > > android without setting arm_no_neon, so they hit the assertion I added in > > components/cronet/android/BUILD.gn. So the current solution isn't going to > work > > as it stands now. > > > > What do reviewers think about adding a //src/build/config/cronet_build.gni > file > > that defaults an "is_cronet" variable to false, but when "is_cronet" is set to > > true it enables the various other flags that are currently smattered > throughout > > a lot of different bot config files (e.g. use_errorprone_java_compiler=true > > enable_websockets=false disable_file_support=true disable_ftp_support=true > > use_platform_icu_alternatives=true disable_brotli_filter=true). Alternativly > it > > could at least assert that they are set. Then we could also only include the > > Cronet dependencies in //src/BUILD.gn if is_cronet == true, which would get > the > > bots that currently fail with this CL passing. > > We don't really want to make the default values change depending on the > "product" that > is being built in GN, and so we're trying to avoid things like "is_cronet" or > "is_chromecast" > that get included generically in //build . > > It would be fine to have an cronet_build.gni file that got included by the > targets that need > it outside of build. > > However, if what you really want is a bunch of flags set specifically on cronet > builds, > what we're encouraging people to do is to use "args files", i.e., to check in a > file with > a bunch of GN args, and tell the builder to use that. > > See, e.g., //build/args/blimp_client.gn or //build/args/headless.gn (for two > similar situations). > You can set a generic set of args in that file, and then set builder-specific > args like is_debug > if need be in a file in //build/args/bots/mastername/buildername.gn . We're > going to be > switching to this model more and more as we get rid of GYP. > > Does that make sense? That makes sense but I don't think it solves the issue blocking this CL: Most bots (e.g. linux_android_rel_ng, cast_shell_android) need to *not* set arm_use_neon=false. Cronet bots (e.g. android_cronet_tester) need to set arm_use_neon=false. This makes it impossible for me to put assert(!arm_use_neon) in Cronet gn files.
On 2016/07/22 11:38:43, pauljensen wrote:
> On 2016/07/21 21:01:38, Dirk Pranke wrote:
> > On 2016/07/21 18:58:34, pauljensen wrote:
> > > Cronet is referenced from //src/BUILD.gn, and various bots (at least
> > > linux_android_rel_ng and cast_shell_android) generate ninja files using gn
> for
> > > android without setting arm_no_neon, so they hit the assertion I added in
> > > components/cronet/android/BUILD.gn. So the current solution isn't going
to
> > work
> > > as it stands now.
> > >
> > > What do reviewers think about adding a //src/build/config/cronet_build.gni
> > file
> > > that defaults an "is_cronet" variable to false, but when "is_cronet" is
set
> to
> > > true it enables the various other flags that are currently smattered
> > throughout
> > > a lot of different bot config files (e.g.
use_errorprone_java_compiler=true
> > > enable_websockets=false disable_file_support=true disable_ftp_support=true
> > > use_platform_icu_alternatives=true disable_brotli_filter=true).
> Alternativly
> > it
> > > could at least assert that they are set. Then we could also only include
> the
> > > Cronet dependencies in //src/BUILD.gn if is_cronet == true, which would
get
> > the
> > > bots that currently fail with this CL passing.
> >
> > We don't really want to make the default values change depending on the
> > "product" that
> > is being built in GN, and so we're trying to avoid things like "is_cronet"
or
> > "is_chromecast"
> > that get included generically in //build .
> >
> > It would be fine to have an cronet_build.gni file that got included by the
> > targets that need
> > it outside of build.
> >
> > However, if what you really want is a bunch of flags set specifically on
> cronet
> > builds,
> > what we're encouraging people to do is to use "args files", i.e., to check
in
> a
> > file with
> > a bunch of GN args, and tell the builder to use that.
> >
> > See, e.g., //build/args/blimp_client.gn or //build/args/headless.gn (for two
> > similar situations).
> > You can set a generic set of args in that file, and then set
builder-specific
> > args like is_debug
> > if need be in a file in //build/args/bots/mastername/buildername.gn . We're
> > going to be
> > switching to this model more and more as we get rid of GYP.
> >
> > Does that make sense?
>
> That makes sense but I don't think it solves the issue blocking this CL:
> Most bots (e.g. linux_android_rel_ng, cast_shell_android) need to *not* set
> arm_use_neon=false.
> Cronet bots (e.g. android_cronet_tester) need to set arm_use_neon=false.
> This makes it impossible for me to put assert(!arm_use_neon) in Cronet gn
files.
Would it make sense to not define the cronet targets if neon isn't properly
disabled, e.g.
if (!(target_cpu == "arm" && arm_version == 7) || !arm_use_neon) {
group("cronet_package") {
// ...
}
}
(or something similar that is more friendly for //BUILD.gn)
On 2016/07/22 14:30:36, jbudorick wrote:
> On 2016/07/22 11:38:43, pauljensen wrote:
> > On 2016/07/21 21:01:38, Dirk Pranke wrote:
> > > On 2016/07/21 18:58:34, pauljensen wrote:
> > > > Cronet is referenced from //src/BUILD.gn, and various bots (at least
> > > > linux_android_rel_ng and cast_shell_android) generate ninja files using
gn
> > for
> > > > android without setting arm_no_neon, so they hit the assertion I added
in
> > > > components/cronet/android/BUILD.gn. So the current solution isn't going
> to
> > > work
> > > > as it stands now.
> > > >
> > > > What do reviewers think about adding a
//src/build/config/cronet_build.gni
> > > file
> > > > that defaults an "is_cronet" variable to false, but when "is_cronet" is
> set
> > to
> > > > true it enables the various other flags that are currently smattered
> > > throughout
> > > > a lot of different bot config files (e.g.
> use_errorprone_java_compiler=true
> > > > enable_websockets=false disable_file_support=true
disable_ftp_support=true
> > > > use_platform_icu_alternatives=true disable_brotli_filter=true).
> > Alternativly
> > > it
> > > > could at least assert that they are set. Then we could also only
include
> > the
> > > > Cronet dependencies in //src/BUILD.gn if is_cronet == true, which would
> get
> > > the
> > > > bots that currently fail with this CL passing.
> > >
> > > We don't really want to make the default values change depending on the
> > > "product" that
> > > is being built in GN, and so we're trying to avoid things like "is_cronet"
> or
> > > "is_chromecast"
> > > that get included generically in //build .
> > >
> > > It would be fine to have an cronet_build.gni file that got included by the
> > > targets that need
> > > it outside of build.
> > >
> > > However, if what you really want is a bunch of flags set specifically on
> > cronet
> > > builds,
> > > what we're encouraging people to do is to use "args files", i.e., to check
> in
> > a
> > > file with
> > > a bunch of GN args, and tell the builder to use that.
> > >
> > > See, e.g., //build/args/blimp_client.gn or //build/args/headless.gn (for
two
> > > similar situations).
> > > You can set a generic set of args in that file, and then set
> builder-specific
> > > args like is_debug
> > > if need be in a file in //build/args/bots/mastername/buildername.gn .
We're
> > > going to be
> > > switching to this model more and more as we get rid of GYP.
> > >
> > > Does that make sense?
> >
> > That makes sense but I don't think it solves the issue blocking this CL:
> > Most bots (e.g. linux_android_rel_ng, cast_shell_android) need to *not* set
> > arm_use_neon=false.
> > Cronet bots (e.g. android_cronet_tester) need to set arm_use_neon=false.
> > This makes it impossible for me to put assert(!arm_use_neon) in Cronet gn
> files.
>
> Would it make sense to not define the cronet targets if neon isn't properly
> disabled, e.g.
>
> if (!(target_cpu == "arm" && arm_version == 7) || !arm_use_neon) {
> group("cronet_package") {
> // ...
> }
> }
>
> (or something similar that is more friendly for //BUILD.gn)
SGTM, I'll upload a patch-set
The CQ bit was checked by pauljensen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2016/07/22 14:35:52, pauljensen wrote:
> On 2016/07/22 14:30:36, jbudorick wrote:
> > On 2016/07/22 11:38:43, pauljensen wrote:
> > > On 2016/07/21 21:01:38, Dirk Pranke wrote:
> > > > On 2016/07/21 18:58:34, pauljensen wrote:
> > > > > Cronet is referenced from //src/BUILD.gn, and various bots (at least
> > > > > linux_android_rel_ng and cast_shell_android) generate ninja files
using
> gn
> > > for
> > > > > android without setting arm_no_neon, so they hit the assertion I added
> in
> > > > > components/cronet/android/BUILD.gn. So the current solution isn't
going
> > to
> > > > work
> > > > > as it stands now.
> > > > >
> > > > > What do reviewers think about adding a
> //src/build/config/cronet_build.gni
> > > > file
> > > > > that defaults an "is_cronet" variable to false, but when "is_cronet"
is
> > set
> > > to
> > > > > true it enables the various other flags that are currently smattered
> > > > throughout
> > > > > a lot of different bot config files (e.g.
> > use_errorprone_java_compiler=true
> > > > > enable_websockets=false disable_file_support=true
> disable_ftp_support=true
> > > > > use_platform_icu_alternatives=true disable_brotli_filter=true).
> > > Alternativly
> > > > it
> > > > > could at least assert that they are set. Then we could also only
> include
> > > the
> > > > > Cronet dependencies in //src/BUILD.gn if is_cronet == true, which
would
> > get
> > > > the
> > > > > bots that currently fail with this CL passing.
> > > >
> > > > We don't really want to make the default values change depending on the
> > > > "product" that
> > > > is being built in GN, and so we're trying to avoid things like
"is_cronet"
> > or
> > > > "is_chromecast"
> > > > that get included generically in //build .
> > > >
> > > > It would be fine to have an cronet_build.gni file that got included by
the
> > > > targets that need
> > > > it outside of build.
> > > >
> > > > However, if what you really want is a bunch of flags set specifically on
> > > cronet
> > > > builds,
> > > > what we're encouraging people to do is to use "args files", i.e., to
check
> > in
> > > a
> > > > file with
> > > > a bunch of GN args, and tell the builder to use that.
> > > >
> > > > See, e.g., //build/args/blimp_client.gn or //build/args/headless.gn (for
> two
> > > > similar situations).
> > > > You can set a generic set of args in that file, and then set
> > builder-specific
> > > > args like is_debug
> > > > if need be in a file in //build/args/bots/mastername/buildername.gn .
> We're
> > > > going to be
> > > > switching to this model more and more as we get rid of GYP.
> > > >
> > > > Does that make sense?
> > >
> > > That makes sense but I don't think it solves the issue blocking this CL:
> > > Most bots (e.g. linux_android_rel_ng, cast_shell_android) need to *not*
set
> > > arm_use_neon=false.
> > > Cronet bots (e.g. android_cronet_tester) need to set arm_use_neon=false.
> > > This makes it impossible for me to put assert(!arm_use_neon) in Cronet gn
> > files.
> >
> > Would it make sense to not define the cronet targets if neon isn't properly
> > disabled, e.g.
> >
> > if (!(target_cpu == "arm" && arm_version == 7) || !arm_use_neon) {
> > group("cronet_package") {
> > // ...
> > }
> > }
> >
> > (or something similar that is more friendly for //BUILD.gn)
>
> SGTM, I'll upload a patch-set
src/BUILD.gn includes cronet_package...which means ninja file generation will
fail for targets that don't set arm_use_neon. We could move the 'if
(!(target_cpu == "arm" && arm_version == 7) || !arm_use_neon) {' line into
src/BUILD.gn but that doesn't seem like the right place for Cronet-specific
logic.
https://codereview.chromium.org/2060333002/diff/220001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/220001/components/cronet/andr... components/cronet/android/BUILD.gn:935: group("cronet_package") { It seems like you generally want cronet_package to be defined and built, but you want arm_use_neon=false when (target_cpu == "arm" && arm_version == 7), right? So, the way I'd do this would be to leave the group defined unconditionally, and conditionally add the dep as you're doing on lines 947-949. You will also want :enforce_no_neon to only be defined if current_cpu == "arm" && arm_version == 7. Does that make sense (and work)?
The CQ bit was checked by pauljensen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Seems to work now, though building cronet_package in an unsupported way outputs nothing. PTAL. https://codereview.chromium.org/2060333002/diff/220001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/220001/components/cronet/andr... components/cronet/android/BUILD.gn:935: group("cronet_package") { On 2016/07/27 00:01:49, Dirk Pranke wrote: > It seems like you generally want cronet_package to be defined and built, but you > want arm_use_neon=false when (target_cpu == "arm" && arm_version == 7), right? > > So, the way I'd do this would be to leave the group defined unconditionally, and > conditionally add the dep as you're doing on lines 947-949. > > You will also want :enforce_no_neon to only be defined if current_cpu == "arm" > && arm_version == 7. > > Does that make sense (and work)? Done.
https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... components/cronet/android/BUILD.gn:915: assert(!arm_use_neon) Given the if conditions, this assert is no longer useful. https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... components/cronet/android/BUILD.gn:939: deps = [ Don't you want these deps to be set unconditionally?
https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... components/cronet/android/BUILD.gn:939: deps = [ On 2016/07/27 16:43:33, Dirk Pranke wrote: > Don't you want these deps to be set unconditionally? No, that would allow building Cronet for ARMv7 with Neon, which is what I want to disallow. I need to allow generation of ninja files for cronet_package with arm_use_neon set to true, but I need the builds to fail or produce nothing. It was recommended I add the assert(!arm_use_neon) to enforce_no_neon, which forced me to choose the "produce nothing" option.
lgtm https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... components/cronet/android/BUILD.gn:939: deps = [ On 2016/08/18 16:12:33, pauljensen wrote: > On 2016/07/27 16:43:33, Dirk Pranke wrote: > > Don't you want these deps to be set unconditionally? > > No, that would allow building Cronet for ARMv7 with Neon, which is what I want > to disallow. > > I need to allow generation of ninja files for cronet_package with arm_use_neon > set to true, but I need the builds to fail or produce nothing. It was > recommended I add the assert(!arm_use_neon) to enforce_no_neon, which forced me > to choose the "produce nothing" option. Ah, got it. https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... File components/cronet/tools/check_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... components/cronet/tools/check_no_neon.py:8: """ nit: this can probably be a single line. https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... components/cronet/tools/check_no_neon.py:14: def main(args): nit: two blank lines. https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... components/cronet/tools/check_no_neon.py:23: if __name__ == '__main__': two blank lines.
https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2060333002/diff/260001/components/cronet/andr... components/cronet/android/BUILD.gn:915: assert(!arm_use_neon) On 2016/07/27 16:43:33, Dirk Pranke wrote: > Given the if conditions, this assert is no longer useful. Done. https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... File components/cronet/tools/check_no_neon.py (right): https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... components/cronet/tools/check_no_neon.py:8: """ On 2016/08/18 17:10:38, Dirk Pranke wrote: > nit: this can probably be a single line. Done. https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... components/cronet/tools/check_no_neon.py:14: def main(args): On 2016/08/18 17:10:38, Dirk Pranke wrote: > nit: two blank lines. Done. https://codereview.chromium.org/2060333002/diff/260001/components/cronet/tool... components/cronet/tools/check_no_neon.py:23: if __name__ == '__main__': On 2016/08/18 17:10:38, Dirk Pranke wrote: > two blank lines. Done.
The CQ bit was checked by pauljensen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, jbudorick@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2060333002/#ps300001 (title: "address comments")
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 ========== [Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions Some older devices may not support ARM Neon, so to ensure we run on these devices, check that Cronet doesn't inadvertently use ARM Neon instructions. Also, change Cronet bots to conform to this. BUG=594316 ========== to ========== [Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions Some older devices may not support ARM Neon, so to ensure we run on these devices, check that Cronet doesn't inadvertently use ARM Neon instructions. Also, change Cronet bots to conform to this. BUG=594316 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions Some older devices may not support ARM Neon, so to ensure we run on these devices, check that Cronet doesn't inadvertently use ARM Neon instructions. Also, change Cronet bots to conform to this. BUG=594316 ========== to ========== [Cronet] Enforce ARMv7 Cronet doesn't inadvertently use ARM Neon instructions Some older devices may not support ARM Neon, so to ensure we run on these devices, check that Cronet doesn't inadvertently use ARM Neon instructions. Also, change Cronet bots to conform to this. BUG=594316 Committed: https://crrev.com/420d1a10bcc3dde583b590fefa81884784234bc9 Cr-Commit-Position: refs/heads/master@{#413138} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/420d1a10bcc3dde583b590fefa81884784234bc9 Cr-Commit-Position: refs/heads/master@{#413138} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
