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

src/IceBrowserCompileServer.cpp

 //===- subzero/src/IceBrowserCompileServer.cpp - Browser compile server ---===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// \brief Defines the browser-based compile server.
 ///
 //===----------------------------------------------------------------------===//
 
 // 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
 
 // Headers which are not properly part of the SDK are included by their path in
 // the NaCl tree.
 #ifdef __pnacl__
 #include "native_client/src/untrusted/nacl/pnacl.h"
 #endif // __pnacl__
 
 #include "llvm/Support/QueueStreamer.h"
 
 #include <cstring>
+#include <fstream>
 #include <irt.h>
 #include <irt_dev.h>
 #include <pthread.h>
 #include <thread>
 
 namespace Ice {
 
 // Create C wrappers around callback handlers for the IRT interface.
 namespace {
 
 BrowserCompileServer *gCompileServer;
 struct nacl_irt_private_pnacl_translator_compile gIRTFuncs;
 
 void getIRTInterfaces() {
   size_t QueryResult =
       nacl_interface_query(NACL_IRT_PRIVATE_PNACL_TRANSLATOR_COMPILE_v0_1,
                            &gIRTFuncs, sizeof(gIRTFuncs));
   if (QueryResult != sizeof(gIRTFuncs))
     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) {
     std::string Buffer;
     llvm::raw_string_ostream StrBuf(Buffer);
     StrBuf << "Invalid number of FDs for onInitCallback " << ObjFileFDCount
            << "\n";
     return strdup(StrBuf.str().c_str());
   }
   int ObjFileFD = ObjFileFDs[0];
   if (ObjFileFD < 0) {
     std::string Buffer;
     llvm::raw_string_ostream StrBuf(Buffer);
     StrBuf << "Invalid FD given for onInitCallback " << ObjFileFD << "\n";
     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";
   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;
 }
 
 int onDataCallback(const void *Data, size_t NumBytes) {
   return gCompileServer->pushInputBytes(Data, NumBytes) ? 1 : 0;
 }
 
 char *onEndCallback() {
   gCompileServer->endInputStream();
@@ skipping 67 matching lines @@
 } // end of anonymous namespace
 
 BrowserCompileServer::~BrowserCompileServer() = default;
 
 void BrowserCompileServer::run() {
   gCompileServer = this;
   getIRTInterfaces();
   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());
   ClFlags::Flags.setInputFileFormat(llvm::PNaClFormat);
 }
 
 bool BrowserCompileServer::pushInputBytes(const void *Data, size_t NumBytes) {
   // If there was an earlier error, do not attempt to push bytes to the
   // QueueStreamer. Otherwise the thread could become blocked.
   if (HadError.load())
@@ skipping 58 matching lines @@
   llvm::report_fatal_error("no browser hookups");
 }
 
 ErrorCode &BrowserCompileServer::getErrorCode() {
   llvm::report_fatal_error("no browser hookups");
 }
 
 } // end of namespace Ice
 
 #endif // PNACL_BROWSER_TRANSLATOR