Non-SFI mode: Enable PnaclTransitional Non-SFI browser tests on x86-64 bots.

Non-SFI mode: Enable PnaclTransitional Non-SFI browser tests on x86-64 bots.

Now, NaCl supports to build x86-32 nonsfi binary even with target_arch=x64,
so this CL enables browser_tests for PPAPI.

BUG=358465
TEST=browser_tests --gtest_filter=*Transitional* locally. Run trybots.
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32,linux_arm_compile,linux_arm

Committed: https://crrev.com/d3427850741e6d68f265943c25e998b51344d97c
Cr-Commit-Position: refs/heads/master@{#314148}

[hidehiko, 2015-01-16 13:55:16]

Patchset #2 (id:20001) has been deleted

[hidehiko, 2015-01-16 13:55:23]

Patchset #1 (id:1) has been deleted

[hidehiko, 2015-01-16 13:56:09]

hidehiko@chromium.org changed reviewers:
+ mseaborn@chromium.org

[hidehiko, 2015-01-16 13:56:09]

PTAL.

[hidehiko, 2015-01-16 13:57:33]

hidehiko@chromium.org changed reviewers:
+ teravest@chromium.org

[hidehiko, 2015-01-16 13:57:34]

Oops, I mistakenly sent. Justin, also PTAL, as a ppapi OWNER.

Thank you for review in advance,
- hidehiko

[teravest, 2015-01-16 19:11:51]

lgtm

Thanks for doing this!

[Mark Seaborn, 2015-01-17 00:50:47]

https://codereview.chromium.org/786273008/diff/40001/chrome/test/nacl/nacl_br...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/786273008/diff/40001/chrome/test/nacl/nacl_br...
chrome/test/nacl/nacl_browsertest_util.h:225: (defined(ARCH_CPU_X86) ||
defined(ARCH_CPU_X86_64) || \
Nit: You could use "defined(ARCH_CPU_X86_FAMILY)" here for brevity

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp
File ppapi/ppapi_nacl.gyp (right):

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp#new...
ppapi/ppapi_nacl.gyp:273: ['enable_x86_32_nonsfi==1 and
"<(target_arch)"=="ia32"', {
Can this be
  target_arch=="ia32"
rather than
  "<(target_arch)"=="ia32"
?

Or is there some Gyp oddity here?

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp#new...
ppapi/ppapi_nacl.gyp:284: '--arch=x86-64',
Shouldn't this be x86-32?

We shouldn't need to have an "x86-64(-nonsfi)" entry in the NMF that points to
"x86-32-nonsfi" binaries. :-)

I guess the right thing to do here is to tweak src/trusted/plugin/ to use
"x86-32-nonsfi" in x86-64 builds?  Or is that fiddly enough that we should leave
it to a later cleanup?

[hidehiko, 2015-01-28 15:04:16]

ptal

https://codereview.chromium.org/786273008/diff/40001/chrome/test/nacl/nacl_br...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/786273008/diff/40001/chrome/test/nacl/nacl_br...
chrome/test/nacl/nacl_browsertest_util.h:225: (defined(ARCH_CPU_X86) ||
defined(ARCH_CPU_X86_64) || \
On 2015/01/17 00:50:47, Mark Seaborn wrote:
> Nit: You could use "defined(ARCH_CPU_X86_FAMILY)" here for brevity

Done.

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp
File ppapi/ppapi_nacl.gyp (right):

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp#new...
ppapi/ppapi_nacl.gyp:273: ['enable_x86_32_nonsfi==1 and
"<(target_arch)"=="ia32"', {
On 2015/01/17 00:50:47, Mark Seaborn wrote:
> Can this be
>   target_arch=="ia32"
> rather than
>   "<(target_arch)"=="ia32"
> ?
> 
> Or is there some Gyp oddity here?

target_arch is not defined at the moment. Pls see other similar examples in
Chrome's gyp.

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp#new...
ppapi/ppapi_nacl.gyp:284: '--arch=x86-64',
On 2015/01/17 00:50:47, Mark Seaborn wrote:
> Shouldn't this be x86-32?
> 
> We shouldn't need to have an "x86-64(-nonsfi)" entry in the NMF that points to
> "x86-32-nonsfi" binaries. :-)
> 
> I guess the right thing to do here is to tweak src/trusted/plugin/ to use
> "x86-32-nonsfi" in x86-64 builds?  Or is that fiddly enough that we should
leave
> it to a later cleanup?

I thought, the arch in the program section of nmf is the chrome's (i.e. target)
arch, isn't it? Running x86-32-nonsfi on 64bit chrome sounds like an surprising
behavior, as we allows x86-64?

[Mark Seaborn, 2015-01-28 21:51:04]

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp
File ppapi/ppapi_nacl.gyp (right):

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp#new...
ppapi/ppapi_nacl.gyp:284: '--arch=x86-64',
On 2015/01/28 15:04:16, hidehiko wrote:
> On 2015/01/17 00:50:47, Mark Seaborn wrote:
> > Shouldn't this be x86-32?
> > 
> > We shouldn't need to have an "x86-64(-nonsfi)" entry in the NMF that points
to
> > "x86-32-nonsfi" binaries. :-)
> > 
> > I guess the right thing to do here is to tweak src/trusted/plugin/ to use
> > "x86-32-nonsfi" in x86-64 builds?  Or is that fiddly enough that we should
> > leave
> > it to a later cleanup?
> 
> I thought, the arch in the program section of nmf is the chrome's (i.e.
target)
> arch, isn't it?

The arch key of an NMF entry is supposed to correspond to the architecture of
the files that the NMF entry points to.

The developer shouldn't need to care about whether Chrome's renderer & browser
processes run as x86-32 or x86-64.  What matters is whether the nexe that Chrome
chooses is x86-32 or x86-64.


> Running x86-32-nonsfi on 64bit chrome sounds like an surprising
> behavior, as we allows x86-64?

Why is that surprising?  Running x86-32-nonsfi on 64-bit Chrome is the plan.  As
your design doc says, we want to "support x86-32 BMM even when the rest of
Chromium is built as x86-64"
(https://docs.google.com/a/chromium.org/document/d/1Dn7lBloLlKq8JEIZYg7xzZhrXt...).

The only tricky thing here is that, in x86-64 Chromium builds, there is a
difference between:

 1) nacl_helper (old): uses "x86-64 non-sfi mode" (which the PNaCl toolchain
can't target)
 2) nacl_helper_nonsfi (new): uses "x86-32 non-sfi mode"

[hidehiko, 2015-01-29 14:43:12]

PTAL.

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp
File ppapi/ppapi_nacl.gyp (right):

https://codereview.chromium.org/786273008/diff/40001/ppapi/ppapi_nacl.gyp#new...
ppapi/ppapi_nacl.gyp:284: '--arch=x86-64',
On 2015/01/28 21:51:04, Mark Seaborn wrote:
> On 2015/01/28 15:04:16, hidehiko wrote:
> > On 2015/01/17 00:50:47, Mark Seaborn wrote:
> > > Shouldn't this be x86-32?
> > > 
> > > We shouldn't need to have an "x86-64(-nonsfi)" entry in the NMF that
points
> to
> > > "x86-32-nonsfi" binaries. :-)
> > > 
> > > I guess the right thing to do here is to tweak src/trusted/plugin/ to use
> > > "x86-32-nonsfi" in x86-64 builds?  Or is that fiddly enough that we should
> > > leave
> > > it to a later cleanup?
> > 
> > I thought, the arch in the program section of nmf is the chrome's (i.e.
> target)
> > arch, isn't it?
> 
> The arch key of an NMF entry is supposed to correspond to the architecture of
> the files that the NMF entry points to.
> 
> The developer shouldn't need to care about whether Chrome's renderer & browser
> processes run as x86-32 or x86-64.  What matters is whether the nexe that
Chrome
> chooses is x86-32 or x86-64.
> 
> 
> > Running x86-32-nonsfi on 64bit chrome sounds like an surprising
> > behavior, as we allows x86-64?
> 
> Why is that surprising?  Running x86-32-nonsfi on 64-bit Chrome is the plan. 
As
> your design doc says, we want to "support x86-32 BMM even when the rest of
> Chromium is built as x86-64"
>
(https://docs.google.com/a/chromium.org/document/d/1Dn7lBloLlKq8JEIZYg7xzZhrXt...).
> 
> The only tricky thing here is that, in x86-64 Chromium builds, there is a
> difference between:
> 
>  1) nacl_helper (old): uses "x86-64 non-sfi mode" (which the PNaCl toolchain
> can't target)
>  2) nacl_helper_nonsfi (new): uses "x86-32 non-sfi mode"

I see.

I look into it, but fallback logic applying only for nacl_helper_nonsfi x86-64
looks to need a bit complicated code.
So, how about keep the current "hack" for now? Considering;
1) Anyway, we need to clean up browser_tests on x64 Chrome (e.g. libcfree)
2) Implementing fallback logic unconditionally looks much simpler than applying
only nacl_helper_nonsfi.
3) We are sure that nonsfi on x64 is anyway testing only.

WDYT? Added TODO, instead.

[Mark Seaborn, 2015-01-29 21:45:59]

LGTM.  Thanks for adding the TODO.

[hidehiko, 2015-01-30 02:27:40]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2015-01-30 02:28:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/786273008/100001

[commit-bot: I haz the power, 2015-01-30 02:34:25]

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

[commit-bot: I haz the power, 2015-01-30 02:34:26]

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

[hidehiko, 2015-01-30 03:31:01]

hidehiko@chromium.org changed reviewers:
+ csharp@chromium.org

[hidehiko, 2015-01-30 03:31:02]

R+=csharp@.
Could you kindly take a look at .isolate file as an OWNER?

Thanks!
- hidehiko

[hidehiko, 2015-02-02 05:02:30]

On 2015/01/30 03:31:02, hidehiko wrote:
> R+=csharp@.
> Could you kindly take a look at .isolate file as an OWNER?
> 
> Thanks!
> - hidehiko

Friendly, ping?

[csharp, 2015-02-02 14:27:16]

lgtm

[hidehiko, 2015-02-02 14:36:34]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2015-02-02 14:37:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/786273008/100001

[commit-bot: I haz the power, 2015-02-02 15:26:35]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-02-02 15:27:44]

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