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..39db61710d1bae88623e546c34d4006a848b6f24 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,56 @@ 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() {

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

Maybe add a

if (BrowserBuild) return {};

[Jim Stichnoth, 2016/04/20 04:17:23]

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

+  std::vector<std::string> ExternalArgs;
+  char ArgsFileVar[] = "SZARGFILE";
+  char ArgsListVar[] = "SZARGLIST";
+  if (const char *ArgsFilename = getenv(ArgsFileVar)) {
+    std::ifstream ArgsStream(ArgsFilename);
+    std::string Arg;
+    while (ArgsStream >> Arg) {
+      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) {
@@ -69,12 +121,23 @@ char *onInitCallback(uint32_t NumThreads, int *ObjFileFDs,
   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(const_cast<char *>(Arg.c_str()));

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

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

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.

+    }
+    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->getParsedFlags(UseNumThreadsFromBrowser, NumThreads,
+                                 Argv.size(), Argv.data());
   gCompileServer->startCompileThread(ObjFileFD);
   return nullptr;
 }
@@ -162,12 +225,14 @@ void BrowserCompileServer::run() {
   gIRTFuncs.serve_translate_request(&SubzeroCallbacks);
 }
 
-void BrowserCompileServer::getParsedFlags(uint32_t NumThreads, int argc,
+void BrowserCompileServer::getParsedFlags(bool UseNumThreadsFromBrowser,
+                                          uint32_t NumThreads, int argc,
                                           char **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());