[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
LOG=Y
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.

Committed: https://crrev.com/86357d5235ceba61c151f0b6e509bcb365860454
Cr-Commit-Position: refs/heads/master@{#35183}

[slan, 2016-03-28 22:05:29]

slan@chromium.org changed reviewers:
+ alokp@chromium.org, dpranke@chromium.org, jochen@chromium.org

[slan, 2016-03-28 22:05:30]

All,

This fix was obtained via a diff of the GYP and GN builds. This define is set
for mksnapshot in GYP. Setting it in GN resolves the issue we were seeing in the
linked bug. PTAL.

[slan, 2016-03-28 22:10:41]

Description was changed from

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
Test: Formerly broken Cast apps load and run as expected.
==========

to

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.
==========

[slan, 2016-03-28 22:17:39]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-28 22:17:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839763003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839763003/1

[commit-bot: I haz the power, 2016-03-28 22:23:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-28 22:24:00]

Dry run: Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12909)

[slan, 2016-03-28 22:25:38]

Description was changed from

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.
==========

to

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
LOG=Y
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.
==========

[Dirk Pranke, 2016-03-28 22:31:56]

lgtm

[slan, 2016-03-28 23:22:37]

slan@chromium.org changed reviewers:
+ titzer@chromium.org

[slan, 2016-03-28 23:22:37]

+titzer@, as jochen@ is marked "slow"

[slan, 2016-03-28 23:22:59]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-28 23:23:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839763003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839763003/1

[commit-bot: I haz the power, 2016-03-28 23:24:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-28 23:24:19]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-03-29 09:24:34]

lgtm

[slan, 2016-03-29 17:03:23]

https://codereview.chromium.org/1839763003/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1839763003/diff/1/BUILD.gn#newcode174
BUILD.gn:174: if (arm_float_abi == "hard") {
Although this passes all the tryjobs, I've just discovered that arm_float_abi is
only declared when current_cpu == "arm". This is going to be an issue, as we
need to check this value to build a tool on the *host* (when target_cpu is "x86"
or "x64". I did not discover this until now, because Chromecast sets this value
in args.gn, giving it global visibility due to a GN KI.

In this case, we need to understand the ABI for the *target* toolchain, not the
current toolchain to set this define correctly.

Dirk: How do you feel about removing the if (current_toolchain == "arm")
condition from the delcaration in arm.gni? This would remove a developer
safeguard, but it would enable us to use the args as if their intent is to
describe the *target* (which I think is always the case).

Alternatively, we could add another arg in here, like v8_target_arm_float_abi,
but that is less preferable in my mind.

Thoughts?

[Dirk Pranke, 2016-03-29 21:03:40]

https://codereview.chromium.org/1839763003/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1839763003/diff/1/BUILD.gn#newcode174
BUILD.gn:174: if (arm_float_abi == "hard") {
On 2016/03/29 17:03:23, slan wrote:
> Although this passes all the tryjobs, I've just discovered that arm_float_abi
is
> only declared when current_cpu == "arm". This is going to be an issue, as we
> need to check this value to build a tool on the *host* (when target_cpu is
"x86"
> or "x64". I did not discover this until now, because Chromecast sets this
value
> in args.gn, giving it global visibility due to a GN KI.

I assume you mean "(when current_cpu is "x86" or "x64")", right? target_cpu is 
effectively a constant and should be == "arm" across all toolchains; I don't
know
why you would need the arm args if target_cpu != "arm" or "arm64".

As an aside, it's important to distinguish between the host toolchain (which
should
always be x64) and the snapshot toolchain (which should be x86 for arm and x64
for
arm64).

> 
> In this case, we need to understand the ABI for the *target* toolchain, not
the
> current toolchain to set this define correctly.
> 
> Dirk: How do you feel about removing the if (current_toolchain == "arm")
> condition from the delcaration in arm.gni? This would remove a developer
> safeguard, but it would enable us to use the args as if their intent is to
> describe the *target* (which I think is always the case).

I think we could probably change (current_cpu == "arm") to (target_cpu == "arm")
instead, which is probably what the intent actually is.

> 
> Alternatively, we could add another arg in here, like v8_target_arm_float_abi,
> but that is less preferable in my mind.

I think we can probably avoid that.

Let me know if my suggestions above work.

[slan, 2016-03-29 23:45:08]

https://codereview.chromium.org/1839763003/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1839763003/diff/1/BUILD.gn#newcode174
BUILD.gn:174: if (arm_float_abi == "hard") {
On 2016/03/29 21:03:40, Dirk Pranke wrote:
> On 2016/03/29 17:03:23, slan wrote:
> > Although this passes all the tryjobs, I've just discovered that
arm_float_abi
> is
> > only declared when current_cpu == "arm". This is going to be an issue, as we
> > need to check this value to build a tool on the *host* (when target_cpu is
> "x86"
> > or "x64". I did not discover this until now, because Chromecast sets this
> value
> > in args.gn, giving it global visibility due to a GN KI.
> 
> I assume you mean "(when current_cpu is "x86" or "x64")", right? target_cpu is

> effectively a constant and should be == "arm" across all toolchains; I don't
> know
> why you would need the arm args if target_cpu != "arm" or "arm64".

Yes, that is what I meant! Thanks for seeing through the typo :)
> 
> As an aside, it's important to distinguish between the host toolchain (which
> should
> always be x64) and the snapshot toolchain (which should be x86 for arm and x64
> for
> arm64).
> 
> > 
> > In this case, we need to understand the ABI for the *target* toolchain, not
> the
> > current toolchain to set this define correctly.
> > 
> > Dirk: How do you feel about removing the if (current_toolchain == "arm")
> > condition from the delcaration in arm.gni? This would remove a developer
> > safeguard, but it would enable us to use the args as if their intent is to
> > describe the *target* (which I think is always the case).
> 
> I think we could probably change (current_cpu == "arm") to (target_cpu ==
"arm")
> instead, which is probably what the intent actually is.

I like this idea:
https://codereview.chromium.org/1842833003/
> 
> > 
> > Alternatively, we could add another arg in here, like
v8_target_arm_float_abi,
> > but that is less preferable in my mind.
> 
> I think we can probably avoid that.
> 
> Let me know if my suggestions above work.

Thanks for the review!

[slan, 2016-03-31 20:30:43]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2016-03-31 20:30:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839763003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839763003/1

[commit-bot: I haz the power, 2016-04-01 00:40:36]

Description was changed from

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
LOG=Y
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.
==========

to

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
LOG=Y
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.
==========

[commit-bot: I haz the power, 2016-04-01 00:40:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-01 00:41:42]

Description was changed from

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
LOG=Y
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.
==========

to

==========
[GN] Define USE_EABI_HARDFLOAT=1 when arm_float_abi=="hard".

Add this define to the config used for mksnapshot. This fixes a bug
where certain applications would fail at runtime on Chromecast.

BUG=592660
LOG=Y
Bug: internal b/27495984

Test: Formerly broken Cast apps load and run as expected.

Committed: https://crrev.com/86357d5235ceba61c151f0b6e509bcb365860454
Cr-Commit-Position: refs/heads/master@{#35183}
==========

[commit-bot: I haz the power, 2016-04-01 00:41:43]

Patchset 1 (id:??) landed as
https://crrev.com/86357d5235ceba61c151f0b6e509bcb365860454
Cr-Commit-Position: refs/heads/master@{#35183}

[jochen (gone - plz use gerrit), 2016-04-22 10:54:27]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1906373002/ by jochen@chromium.org.

The reason for reverting is: Appears to break Android crbug.com/604422.