[MIPS] Adding NaCl/PNaCl support for mipsel to GN

[MIPS] Adding NaCl/PNaCl support for mipsel to GN

Add missing support to GN files.
Properly set nonsfi and glibc dependencies.

BUG=GN gen failure
TEST=Run GN gen with: target_cpu="mipsel" mips_arch_variant="r2" enable_nacl=true

Committed: https://crrev.com/3de038d3292b8d9b10c68013a59221577118ef49
Cr-Commit-Position: refs/heads/master@{#412778}

[lmilko, 2016-08-12 16:34:01]

milko.leporis@imgtec.com changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org, mseaborn@chromium.org

[lmilko, 2016-08-12 16:34:01]

Petar will upload Native Client part.

Please take a look.

[brettw, 2016-08-12 19:52:41]

My understanding of nonsfi is weak, but as far as I understand, I think this
patch is incorrect.

This is the definition of is_nacl_nonsfi:
  is_nacl_nonsfi = current_toolchain == nacl_nonsfi_toolchain
It tells you whether you're current compiling the magic nonsfi code. It doesn't
indicate if the nonsfi sandbox is "enabled".

Stuff in chrome and components is never compiled in nonsfi mode, to my
knowledge. These have deps on helper_nonsfi which is a bounce target that
forwards to a copy run in the right toolchain. With this patch, I believe this
will never be compiled which I believe is incorrect.

https://codereview.chromium.org/2242893002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2242893002/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:538: } else if (current_cpu == "mipsel" &&
!is_nacl) {
This nacl check doesn't make immediate sense to me. Why wouldn't the mips arch
variants apply to nacl code?

[Dirk Pranke, 2016-08-12 22:55:10]

On 2016/08/12 19:52:41, brettw (ping on IM after 24h) wrote:
> My understanding of nonsfi is weak, but as far as I understand, I think this
> patch is incorrect.
> 
> This is the definition of is_nacl_nonsfi:
>   is_nacl_nonsfi = current_toolchain == nacl_nonsfi_toolchain
> It tells you whether you're current compiling the magic nonsfi code. It
doesn't
> indicate if the nonsfi sandbox is "enabled".

Brett is right. `is_nacl_nonsfi` means "we need to use the nonsfi toolchain for
this target", not
not "we want to build the nonsfi stuff".

If nonsfi isn't yet available on mips, then we need to add a new flag for it.
You can't
reuse is_nacl_nonsfi.

[lmilko, 2016-08-15 14:56:10]

I added enable_nonsfi variable to features.gni.
Is name OK? Maybe enable_nacl_nonsfi would be better?

Please take a look.

https://codereview.chromium.org/2242893002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/2242893002/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:538: } else if (current_cpu == "mipsel" &&
!is_nacl) {
On 2016/08/12 19:52:40, brettw (ping on IM after 24h) wrote:
> This nacl check doesn't make immediate sense to me. Why wouldn't the mips arch
> variants apply to nacl code?

mipsel-nacl-clang compiler does not need any of the flags defined in this block.
Alternative would be to add !is_nacl checks for each cflags and ldflags, not to
include -fno-integrated-as, --target, -march, -mcpu and dsp flags.

[Dirk Pranke, 2016-08-15 23:46:33]

I think this is okay now, except that I do think that it should be
`enable_nacl_nonsfi` rather than just `enable_nonsfi`.

lgtm w/ that change.

[lmilko, 2016-08-16 10:50:01]

The CQ bit was checked by milko.leporis@imgtec.com

[lmilko, 2016-08-16 10:50:01]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2242893002/#ps40001
(title: "Changed variable name to enable_nacl_nonsfi")

[commit-bot: I haz the power, 2016-08-16 10:50:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 10:55:23]

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

[commit-bot: I haz the power, 2016-08-16 10:55:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lmilko, 2016-08-16 11:30:07]

milko.leporis@imgtec.com changed reviewers:
+ bbudge@chromium.org, bradnelson@chromium.org, rkc@chromium.org

[lmilko, 2016-08-16 11:30:07]

Approval needed from OWNERS:

@bbudge

ppapi/BUILD.gn
ppapi/native_client/BUILD.gn
ppapi/native_client/nacl_test_data.gni
ppapi/tests/extensions/BUILD.gn


@mseaborn or @bradnelson

chrome/test/data/nacl/BUILD.gn
components/nacl/browser/BUILD.gn
components/nacl/loader/BUILD.gn


@rkc

extensions/shell/BUILD.gn

Please take a look.

[rkc, 2016-08-16 17:16:25]

extensions/shell/BUILD.gn lgtm

[lmilko, 2016-08-17 07:53:28]

Fixing GN gen failures on non Linux builds.

[lmilko, 2016-08-17 07:53:49]

The CQ bit was checked by milko.leporis@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 07:54:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 09:01:55]

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

[commit-bot: I haz the power, 2016-08-17 09:01:56]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-08-17 17:00:05]

ppapi lgtm

[bradnelson, 2016-08-17 17:28:25]

lgtm

[lmilko, 2016-08-18 06:41:46]

The CQ bit was unchecked by milko.leporis@imgtec.com

[lmilko, 2016-08-18 06:41:54]

The CQ bit was checked by milko.leporis@imgtec.com

[lmilko, 2016-08-18 06:41:54]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org,
dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2242893002/#ps60001
(title: "Fix non Linux builds")

[commit-bot: I haz the power, 2016-08-18 06:42:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 10:00:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-18 10:02:42]

Description was changed from

==========
[MIPS] Adding NaCl/PNaCl support for mipsel to GN

Add missing support to GN files.
Properly set nonsfi and glibc dependencies.

BUG=GN gen failure
TEST=Run GN gen with: target_cpu="mipsel" mips_arch_variant="r2"
enable_nacl=true
==========

to

==========
[MIPS] Adding NaCl/PNaCl support for mipsel to GN

Add missing support to GN files.
Properly set nonsfi and glibc dependencies.

BUG=GN gen failure
TEST=Run GN gen with: target_cpu="mipsel" mips_arch_variant="r2"
enable_nacl=true

Committed: https://crrev.com/3de038d3292b8d9b10c68013a59221577118ef49
Cr-Commit-Position: refs/heads/master@{#412778}
==========

[commit-bot: I haz the power, 2016-08-18 10:02:43]

Patchset 4 (id:??) landed as
https://crrev.com/3de038d3292b8d9b10c68013a59221577118ef49
Cr-Commit-Position: refs/heads/master@{#412778}