Non-SFI mode: Add --use-nacl-helper-nonsfi flag.

Non-SFI mode: Add --use-nacl-helper-nonsfi flag.

This CL introduces a flag to switch nacl_helper binary for NaCl in Non-SFI
mode. When the flag, nacl_helper_nonsfi binary is used, otherwise nacl_helper
binary in Non-SFI mode is used.
nacl_helper_nonsfi binary is under development, and this flag is for
its experiment. Also, the flag will be used to run browser_tests with
nacl_helper_nonsfi binary (similar to NaClBrowserTestNonSfiMode test suites).

BUG=358465
TEST=Ran trybot. Ran browser tests locally with local modification on
NaClBrowserTestNonSfiMode and made sure the binary is actually switched.

Committed: https://crrev.com/826a0bcea4beff6c6feca3f29d2779bd4f258c48
Cr-Commit-Position: refs/heads/master@{#302217}

[hidehiko, 2014-10-27 12:54:27]

hidehiko@chromium.org changed reviewers:
+ hamaji@chromium.org, mazda@chromium.org, mseaborn@chromium.org,
uekawa@chromium.org

[hidehiko, 2014-10-27 12:54:28]

Thank you for your review in advance,
- hidehiko

[hamaji, 2014-10-28 07:13:17]

lgtm

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_paths.cc (right):

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_paths.cc:46: // in Non-SFI mode instead.
TODO: remove this code path?

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_switches.cc (right):

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_switches.cc:53: // binary in Non-SFI mode.
TODO: make this default and remove this flag?

[hidehiko, 2014-10-28 08:47:02]

Thank you for review. PTAL.

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_paths.cc (right):

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_paths.cc:46: // in Non-SFI mode instead.
On 2014/10/28 07:13:17, hamaji wrote:
> TODO: remove this code path?

Done.

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_switches.cc (right):

https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_switches.cc:53: // binary in Non-SFI mode.
On 2014/10/28 07:13:17, hamaji wrote:
> TODO: make this default and remove this flag?

Done.

[hidehiko, 2014-10-29 16:57:37]

On 2014/10/28 08:47:02, hidehiko wrote:
> Thank you for review. PTAL.
> 
>
https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
> File components/nacl/common/nacl_paths.cc (right):
> 
>
https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
> components/nacl/common/nacl_paths.cc:46: // in Non-SFI mode instead.
> On 2014/10/28 07:13:17, hamaji wrote:
> > TODO: remove this code path?
> 
> Done.
> 
>
https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
> File components/nacl/common/nacl_switches.cc (right):
> 
>
https://codereview.chromium.org/676323002/diff/1/components/nacl/common/nacl_...
> components/nacl/common/nacl_switches.cc:53: // binary in Non-SFI mode.
> On 2014/10/28 07:13:17, hamaji wrote:
> > TODO: make this default and remove this flag?
> 
> Done.

Friendly ping?

[Junichi Uekawa, 2014-10-30 11:04:19]

lgtm

minor comment.

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
File components/nacl/common/nacl_paths.cc (right):

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
components/nacl/common/nacl_paths.cc:27: *output = output->Append(filename);
This blew my mind, I see operator= is overloaded so this probably works but I
felt really uncomfortable.

[hidehiko, 2014-10-30 11:08:40]

Thank you for review. PTAL.

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
File components/nacl/common/nacl_paths.cc (right):

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
components/nacl/common/nacl_paths.cc:27: *output = output->Append(filename);
On 2014/10/30 11:04:19, Junichi Uekawa wrote:
> This blew my mind, I see operator= is overloaded so this probably works but I
> felt really uncomfortable.

FilePath::Append() is const method, i.e. does not change the object contents,
but returns a new file path which is the {original_path}/{filename}.
So, this is the way to append |filename| to |output|.

[Mark Seaborn, 2014-10-30 19:36:13]

LGTM, but: This change is not testable on its own.  I think it's better to do
changes that do something that can be tested (whether automatically or
manually).

What remains do be done to link a usable nacl_helper_nonsfi?  It might be better
to combine those into a single CL.  (But you can still leave enabling tests in
browser_tests to a separate CL.)

For example, a reasonable manual test might be:
 1) Build Chromium
 2) Copy nacl_helper_nonsfi onto nacl_helper
 3) Run a subset of browser_tests (the Non-SFI tests)

What remains for a manual test like that to work?

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
File components/nacl/common/nacl_switches.cc (right):

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
components/nacl/common/nacl_switches.cc:52: // Use nacl_helper_nonsfi binary to
run Non-SFI nexe, instead of nacl_helper
Can you add a bit of context about newlib vs. glibc?  e.g. "Use
nacl_helper_nonsfi executable (the new, newlib-based version of the Non-SFI
runtime) to run Non-SFI nexe, instead of nacl_helper (which uses the older,
glibc-based runtime for Non-SFI mode)..."

And also link to the issue for the newlib switch.

[hidehiko, 2014-10-31 03:37:37]

Thank you for review. Submitting.

As for testing of this CL itself, we can, manually. Actually running Chrome with
--use-nacl-helper-nonsfi changes its behavior. Without the flag, we can run
Non-SFI nexe, but with it we cannot for now as nacl_helper_nonsfi is currently
almost empty executable.
If necessary, we can add some small change in nacl_helper_linux.cc (like adding
printf("hello world\n")) and make sure its behavior. (Actually I did :-).

As for remaining tasks, I'm planning to send a CL to implement
nacl_helper_nonsfi actually, i.e. the main CL, after crrev.com/686723003 and
crrev.com/687363004 are done, followed by CLs to enable browser_tests, sandbox,
and nacl_loader_unittests.

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
File components/nacl/common/nacl_switches.cc (right):

https://codereview.chromium.org/676323002/diff/50001/components/nacl/common/n...
components/nacl/common/nacl_switches.cc:52: // Use nacl_helper_nonsfi binary to
run Non-SFI nexe, instead of nacl_helper
On 2014/10/30 19:36:13, Mark Seaborn wrote:
> Can you add a bit of context about newlib vs. glibc?  e.g. "Use
> nacl_helper_nonsfi executable (the new, newlib-based version of the Non-SFI
> runtime) to run Non-SFI nexe, instead of nacl_helper (which uses the older,
> glibc-based runtime for Non-SFI mode)..."
> 
> And also link to the issue for the newlib switch.

Done.

[hidehiko, 2014-10-31 03:38:10]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-10-31 03:39:21]

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

[commit-bot: I haz the power, 2014-10-31 04:15:34]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2014-10-31 04:16:24]

Patchset 6 (id:??) landed as
https://crrev.com/826a0bcea4beff6c6feca3f29d2779bd4f258c48
Cr-Commit-Position: refs/heads/master@{#302217}