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

src/IceBrowserCompileServer.cpp

Index: src/IceBrowserCompileServer.cpp
diff --git a/src/IceBrowserCompileServer.cpp b/src/IceBrowserCompileServer.cpp
index 3b9dc3fa993e5a91691b6945184adf78d703e356..423822f3a359da4fe5751ae5003d374e702d4b42 100644
--- a/src/IceBrowserCompileServer.cpp
+++ b/src/IceBrowserCompileServer.cpp
@@ -15,6 +15,7 @@
 // Can only compile this with the NaCl compiler (needs irt.h, and the
 // unsandboxed LLVM build using the trusted compiler does not have irt.h).
 #include "IceBrowserCompileServer.h"
+#include "IceRangeSpec.h"
 
 #if PNACL_BROWSER_TRANSLATOR
 
@@ -27,6 +28,7 @@
 #include "llvm/Support/QueueStreamer.h"
 
 #include <cstring>
+#include <fstream>
 #include <irt.h>
 #include <irt_dev.h>
 #include <pthread.h>
@@ -48,6 +50,58 @@ void getIRTInterfaces() {
     llvm::report_fatal_error("Failed to get translator compile IRT interface");
 }
 
+// Allow pnacl-sz arguments to be supplied externally, instead of coming from
+// the browser.  This is meant to be used for debugging.
+//
+// If the SZARGFILE environment variable is set to a file name, arguments are
+// read from that file, one argument per line.  This requires setting 3
+// environment variables before starting the browser:
+//
+// NACL_ENV_PASSTHROUGH=NACL_DANGEROUS_ENABLE_FILE_ACCESS,NACLENV_SZARGFILE
+// NACL_DANGEROUS_ENABLE_FILE_ACCESS=1
+// NACLENV_SZARGFILE=/path/to/myargs.txt
+//
+// In addition, Chrome needs to be launched with the "--no-sandbox" argument.
+//
+// If the SZARGLIST environment variable is set, arguments are extracted from
+// that variable's value, separated by the '|' character (being careful to
+// escape/quote special shell characters).  This requires setting 2 environment
+// variables before starting the browser:
+//
+// NACL_ENV_PASSTHROUGH=NACLENV_SZARGLIST
+// NACLENV_SZARGLIST=arg
+//
+// This does not require the "--no-sandbox" argument, and is therefore much
+// safer, but does require restarting the browser to change the arguments.
+//
+// If external arguments are supplied, the browser's NumThreads specification is
+// ignored, to allow -threads to be specified as an external argument.  Note
+// that the browser normally supplies the "-O2" argument, so externally supplied
+// arguments might want to provide an explicit -O argument.
+std::vector<std::string> getExternalArgs() {
+  std::vector<std::string> ExternalArgs;

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

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

[Jim Stichnoth, 2016/04/20 16:53:54]

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.

+  char ArgsFileVar[] = "SZARGFILE";

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

const

[Jim Stichnoth, 2016/04/20 16:53:54]

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

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

+  char ArgsListVar[] = "SZARGLIST";
+  if (const char *ArgsFilename = getenv(ArgsFileVar)) {
+    std::ifstream ArgsStream(ArgsFilename);
+    std::string Arg;
+    while (ArgsStream >> std::ws, std::getline(ArgsStream, Arg)) {
+      if (!Arg.empty() && Arg[0] == '#')
+        continue;
+      ExternalArgs.emplace_back(Arg);
+    }
+    if (ExternalArgs.empty()) {
+      llvm::report_fatal_error("Failed to read arguments from file '" +
+                               std::string(ArgsFilename) + "'");
+    }
+  } else if (const char *ArgsList = getenv(ArgsListVar)) {
+    // Leverage the RangeSpec tokenizer.
+    auto Args = RangeSpec::tokenize(ArgsList, '|');
+    ExternalArgs.insert(ExternalArgs.end(), Args.begin(), Args.end());
+  }
+  return ExternalArgs;
+}
+
 char *onInitCallback(uint32_t NumThreads, int *ObjFileFDs,
                      size_t ObjFileFDCount, char **CLArgs, size_t CLArgsLen) {
   if (ObjFileFDCount < 1) {
@@ -65,17 +119,35 @@ char *onInitCallback(uint32_t NumThreads, int *ObjFileFDs,
     return strdup(StrBuf.str().c_str());
   }
   // CLArgs is almost an "argv", but is missing the argv[0] program name.
-  std::vector<char *> Argv;
-  char ProgramName[] = "pnacl-sz.nexe";
+  std::vector<const char *> Argv;
+  constexpr static char ProgramName[] = "pnacl-sz.nexe";
   Argv.reserve(CLArgsLen + 1);
   Argv.push_back(ProgramName);
-  for (size_t i = 0; i < CLArgsLen; ++i) {
-    Argv.push_back(CLArgs[i]);
+
+  bool UseNumThreadsFromBrowser = true;
+  auto ExternalArgs = getExternalArgs();
+  if (ExternalArgs.empty()) {
+    for (size_t i = 0; i < CLArgsLen; ++i) {
+      Argv.push_back(CLArgs[i]);
+    }
+  } else {
+    for (auto &Arg : ExternalArgs) {
+      Argv.emplace_back(Arg.c_str());
+    }
+    UseNumThreadsFromBrowser = false;
   }
   // NOTE: strings pointed to by argv are owned by the caller, but we parse
   // here before returning and don't store them.
-  gCompileServer->getParsedFlags(NumThreads, Argv.size(), Argv.data());
-  gCompileServer->startCompileThread(ObjFileFD);
+  gCompileServer->getParsedFlags(UseNumThreadsFromBrowser, NumThreads,
+                                 Argv.size(), Argv.data());
+  int LogFD = STDOUT_FILENO;
+  if (getFlags().getLogFilename() == "/dev/stderr") {
+    LogFD = STDERR_FILENO;
+  } else if (getFlags().getLogFilename() != "-") {
+    llvm::report_fatal_error(
+        "Log file name must be either '-' or '/dev/stderr'");

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

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:54]

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

+  }
+  gCompileServer->startCompileThread(ObjFileFD, LogFD);
   return nullptr;
 }
 
@@ -162,12 +234,14 @@ void BrowserCompileServer::run() {
   gIRTFuncs.serve_translate_request(&SubzeroCallbacks);
 }
 
-void BrowserCompileServer::getParsedFlags(uint32_t NumThreads, int argc,
-                                          char **argv) {
+void BrowserCompileServer::getParsedFlags(bool UseNumThreadsFromBrowser,
+                                          uint32_t NumThreads, int argc,
+                                          const char *const *argv) {
   ClFlags::parseFlags(argc, argv);
   ClFlags::getParsedClFlags(ClFlags::Flags);
   // Set some defaults which aren't specified via the argv string.
-  ClFlags::Flags.setNumTranslationThreads(NumThreads);
+  if (UseNumThreadsFromBrowser)
+    ClFlags::Flags.setNumTranslationThreads(NumThreads);
   ClFlags::Flags.setUseSandboxing(true);
   ClFlags::Flags.setOutFileType(FT_Elf);
   ClFlags::Flags.setTargetArch(getTargetArch());
@@ -204,9 +278,9 @@ ErrorCode &BrowserCompileServer::getErrorCode() {
 
 void BrowserCompileServer::endInputStream() { InputStream->SetDone(); }
 
-void BrowserCompileServer::startCompileThread(int ObjFD) {
+void BrowserCompileServer::startCompileThread(int ObjFD, int LogFD) {
   InputStream = new llvm::QueueStreamer();
-  LogStream = getOutputStream(STDOUT_FILENO);
+  LogStream = getOutputStream(LogFD);
   LogStream->SetUnbuffered();
   EmitStream = getOutputStream(ObjFD);
   EmitStream->SetBufferSize(1 << 14);