Subzero: Allow overriding command-line args from the browser.

Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz, in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per line.  For each line, initial whitespace is ignored, and lines starting with the '#' comment character are also ignored.

2. The SZARGLIST envvar contains all the arguments, separated by the '|' character.

Chrome needs to be started with special options to allow the envvars to be passed through, and also to allow access to the local file system.

In addition, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to std::cerr, in the same way "-" gets mapped to std::cout.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
R=jpp@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=fd07ad08190cbb7c9818e79fab9059eb3ed329d7

[Jim Stichnoth, 2016-04-20 02:31:41]

Description was changed from

==========
Subzero: Allow overriding command-line args from the browser.

BUG=
==========

to

==========
Subzero: Allow overriding command-line args from the browser.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

[Jim Stichnoth, 2016-04-20 02:36:42]

Description was changed from

==========
Subzero: Allow overriding command-line args from the browser.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

to

==========
Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz,
in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per
line.

2. The SZARGLIST envvar contains all the arguments, separated by the '|'
character.

Chrome needs to be started with special options to allow the envvars to be
passed through, and also the allow access to the local file system.

Also, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to
std::cerr, in the same way "-" gets mapped to std::cout.  This makes it possible
to see the output in the terminal/shell where chrome is launched.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

[Jim Stichnoth, 2016-04-20 02:58:25]

stichnot@chromium.org changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com, sehr@chromium.org

[John, 2016-04-20 03:22:02]

lgtm

https://codereview.chromium.org/1903553004/diff/40001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1903553004/diff/40001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:81: std::vector<std::string> getExternalArgs() {
Maybe add a

if (BrowserBuild) return {};

https://codereview.chromium.org/1903553004/diff/40001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:133: Argv.emplace_back(const_cast<char
*>(Arg.c_str()));
This is, in general, bad: you're holding pointers to internal vector storage. It
shouldn't matter with the current use, but consider changing Args to be a
vector<string>.

[Jim Stichnoth, 2016-04-20 04:17:24]

https://codereview.chromium.org/1903553004/diff/40001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1903553004/diff/40001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:81: std::vector<std::string> getExternalArgs() {
On 2016/04/20 03:22:02, John wrote:
> Maybe add a
> 
> if (BrowserBuild) return {};

This whole file is already wrapped inside "#if PNACL_BROWSER_TRANSLATOR".

https://codereview.chromium.org/1903553004/diff/40001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:133: Argv.emplace_back(const_cast<char
*>(Arg.c_str()));
On 2016/04/20 03:22:02, John wrote:
> This is, in general, bad: you're holding pointers to internal vector storage.
It
> shouldn't matter with the current use, but consider changing Args to be a
> vector<string>.

Yeah, I dislike this, also the const_cast.  (Fortunately, looks like I found a
better set of declarations that now avoids the const_cast.)

The problem is that ultimately we have to call
llvm::cl::ParseCommandLineOptions() which needs an array of char* values.  I
guess I'd rather have those values managed by the std::string objects, than to
manage the memory more explicitly.

[Jim Stichnoth, 2016-04-20 13:53:27]

Description was changed from

==========
Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz,
in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per
line.

2. The SZARGLIST envvar contains all the arguments, separated by the '|'
character.

Chrome needs to be started with special options to allow the envvars to be
passed through, and also the allow access to the local file system.

Also, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to
std::cerr, in the same way "-" gets mapped to std::cout.  This makes it possible
to see the output in the terminal/shell where chrome is launched.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

to

==========
Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz,
in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per
line.  For each line, initial whitespace is ignored, and lines starting with the
'#' comment character are also ignored.

2. The SZARGLIST envvar contains all the arguments, separated by the '|'
character.

Chrome needs to be started with special options to allow the envvars to be
passed through, and also the allow access to the local file system.

Also, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to
std::cerr, in the same way "-" gets mapped to std::cout.  This makes it possible
to see the output in the terminal/shell where chrome is launched.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

[Jim Stichnoth, 2016-04-20 13:55:38]

PTAL.

The latest patchset applies the /dev/stderr processing to the browser build (the
original patch mistakenly only applied it to the command-line build).

Also, the argfile processing has been changed to use getline(), and also allows
'#' commented out lines.

[Jim Stichnoth, 2016-04-20 13:58:56]

Description was changed from

==========
Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz,
in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per
line.  For each line, initial whitespace is ignored, and lines starting with the
'#' comment character are also ignored.

2. The SZARGLIST envvar contains all the arguments, separated by the '|'
character.

Chrome needs to be started with special options to allow the envvars to be
passed through, and also the allow access to the local file system.

Also, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to
std::cerr, in the same way "-" gets mapped to std::cout.  This makes it possible
to see the output in the terminal/shell where chrome is launched.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

to

==========
Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz,
in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per
line.  For each line, initial whitespace is ignored, and lines starting with the
'#' comment character are also ignored.

2. The SZARGLIST envvar contains all the arguments, separated by the '|'
character.

Chrome needs to be started with special options to allow the envvars to be
passed through, and also to allow access to the local file system.

In addition, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to
std::cerr, in the same way "-" gets mapped to std::cout.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

[John, 2016-04-20 14:17:24]

Lgtm, with a grain of salt

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:82: std::vector<std::string> ExternalArgs;
I still think this should be disallowed in a release browser build. I'd be
scared to release a version of sz that accepts arguments not coming from the
browser

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:83: char ArgsFileVar[] = "SZARGFILE";
const

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:148: "Log file name must be either '-' or
'/dev/stderr'");
Can the user change this option?? It seems like an invalid argument should lead
to a warning, not crash sz

[Jim Stichnoth, 2016-04-20 16:53:55]

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:82: std::vector<std::string> ExternalArgs;
On 2016/04/20 14:17:24, John wrote:
> I still think this should be disallowed in a release browser build. I'd be
> scared to release a version of sz that accepts arguments not coming from the
> browser

I originally felt this way.

Then I changed my mind - wouldn't it be great if we could enable remote
debugging/diagnosis by having the user run with some SZARGLIST?

But now I realize that the MINIMAL build is basically entirely lacking in any
diagnostic output, and we don't really want to change that without good reason,
and besides we can almost always act as the end user for the purpose of
diagnosis, and we'll just drop in a non-MINIMAL translator for that purpose.

TL;DR: Done.

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:83: char ArgsFileVar[] = "SZARGFILE";
On 2016/04/20 14:17:24, John wrote:
> const

Sadly, archaic getenv() was apparently invented before "const" was discovered.

I.e., I must pass a non-const char* argument.  :(

https://codereview.chromium.org/1903553004/diff/80001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:148: "Log file name must be either '-' or
'/dev/stderr'");
On 2016/04/20 14:17:24, John wrote:
> Can the user change this option?? It seems like an invalid argument should
lead
> to a warning, not crash sz

Done.  Pushed the logic down into startCompileThread() so that we can log a
warning to the log stream after the log stream is created.

[Jim Stichnoth, 2016-04-20 17:12:49]

Description was changed from

==========
Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz,
in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per
line.  For each line, initial whitespace is ignored, and lines starting with the
'#' comment character are also ignored.

2. The SZARGLIST envvar contains all the arguments, separated by the '|'
character.

Chrome needs to be started with special options to allow the envvars to be
passed through, and also to allow access to the local file system.

In addition, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to
std::cerr, in the same way "-" gets mapped to std::cout.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
==========

to

==========
Subzero: Allow overriding command-line args from the browser.

In the browser build only, allows arguments to be explicitly passed to pnacl-sz,
in two ways:

1. The SZARGFILE envvar contains the name of a file with arguments, one per
line.  For each line, initial whitespace is ignored, and lines starting with the
'#' comment character are also ignored.

2. The SZARGLIST envvar contains all the arguments, separated by the '|'
character.

Chrome needs to be started with special options to allow the envvars to be
passed through, and also to allow access to the local file system.

In addition, specifying "-log=/dev/stderr" or "-o /dev/stderr" gets mapped to
std::cerr, in the same way "-" gets mapped to std::cout.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4370
R=jpp@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Jim Stichnoth, 2016-04-20 17:12:51]

Committed patchset #6 (id:100001) manually as
fd07ad08190cbb7c9818e79fab9059eb3ed329d7 (presubmit successful).