Add an IRT interface for subzero/compiler to serve_translate_requests.

src/untrusted/irt/irt_pnacl_translator_compile.c

+/*
+ * Copyright (c) 2015 The Native Client Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "native_client/src/shared/platform/nacl_log.h"
+#include "native_client/src/shared/srpc/nacl_srpc.h"
+#include "native_client/src/untrusted/irt/irt_dev.h"
+#include "native_client/src/untrusted/irt/irt_interfaces.h"
+
+#include <string.h>
+
+static const int kMaxObjectFiles = 16;
+
+static const struct irt_pnacl_compile_funcs *g_funcs;
+
+static void stream_init_with_split(
+    NaClSrpcRpc *rpc, NaClSrpcArg **in_args,
+    NaClSrpcArg **out_args, NaClSrpcClosure *done) {
+  int num_threads = in_args[0]->u.ival;
+  if (num_threads < 0 || num_threads > kMaxObjectFiles) {
+    NaClLog(LOG_FATAL, "Invalid # of threads (%d)\n", num_threads);
+  }
+  int num_valid_fds = 0;
+  int fd_start = 1;
+  int obj_fds[kMaxObjectFiles];
+  for (int i = fd_start; i < kMaxObjectFiles + fd_start; i++) {
+    int fd = in_args[i]->u.hval;
+    obj_fds[i - fd_start] = fd;
+    if (fd >= 0)
+      ++num_valid_fds;

[Mark Seaborn, 2015/03/26 20:39:00]

This is odd...

If you get "-1, 100, -1, -1, ..." as input, you'll set num_valid_fds = 1.  Would
the callee be expected to skip over the first -1 it gets?

It would be cleaner to stop on the first fd<0 if you can.

Also see comment in irt_dev.h.

[jvoung (off chromium), 2015/03/26 22:30:12]

True -- simplified to stop on the first fd < 0.

+    else
+      break;
+  }
+  const char *cmd_line = in_args[kMaxObjectFiles + fd_start]->arrays.carr;
+  size_t cmd_line_len = in_args[kMaxObjectFiles + fd_start]->u.count;
+  int result = g_funcs->init_callback(num_threads, obj_fds, num_valid_fds,
+                                      cmd_line, cmd_line_len);
+  rpc->result = result == 0 ? NACL_SRPC_RESULT_OK : NACL_SRPC_RESULT_APP_ERROR;
+  /* TODO(jvoung): figure out how to get an error message out. */

[Mark Seaborn, 2015/03/26 20:39:00]

The lack of error reporting means we can't switch pnacl-llc over to this without
a regression (or a cross-repo change to this interface).  So I think it would be
worth adding this now.

How about having init_callback() return an error string (with NULL for success)?
 You can comment that the string will leak but that we're not too worried about
that. :-)

It doesn't matter if Subzero doesn't use that as long as pnacl-llc can...

(Aside: I'm not sure how mid-translation errors are handled, e.g. in
stream_chunk.  I guess that at the moment we either don't handle them, or we
defer reporting them to the end of translation somehow.)

[Derek Schuff, 2015/03/26 22:29:35]

It's deferred.
Today errors during translation cause LLVM's fatal error hook to set a flag in
SRPStreamer on the compile thread. Then stream_chunk (on the RPC thread) returns
immediately. Then the browser calls stream_end (it does this regardless of
whether there was any errors), which returns the actual error message. I guess
the problem now is that we don't have a good way to get the error message across
the IRT boundary? Did you just decide you didn't want to do that in the
end_callback with the new IRT interface?

[jvoung (off chromium), 2015/03/26 22:30:12]

Okay fair enough -- might as well add it now instead of more cross-repo changes.

Done =)
For mid-translation errors, stream_chunk will return an error code which will
tell the browser to stop streaming and skip to stream_end. However, stream_end
is still invoked and that is how it gets the error message. Added a comment in
irt-dev.h.

+  if (result)
+    out_args[0]->arrays.str = strdup("There was some sort of error");
+  else
+    out_args[0]->arrays.str = strdup("no error");
+  done->Run(done);
+}
+
+static void stream_chunk(NaClSrpcRpc *rpc, NaClSrpcArg **in_args,
+                         NaClSrpcArg **out_args, NaClSrpcClosure *done) {
+  int result = g_funcs->data_callback(
+      (unsigned char *)in_args[0]->arrays.carr,

[Mark Seaborn, 2015/03/26 20:39:00]

Nit: NaCl cast style is "(type) expr", with a space

[jvoung (off chromium), 2015/03/26 22:30:12]

Done. (not needed w/ the void * signature)

+      in_args[0]->u.count);
+  rpc->result = result == 0 ? NACL_SRPC_RESULT_OK : NACL_SRPC_RESULT_APP_ERROR;
+  done->Run(done);
+}
+
+static void stream_end(NaClSrpcRpc *rpc, NaClSrpcArg **in_args,
+                       NaClSrpcArg **out_args, NaClSrpcClosure *done) {
+  int result = g_funcs->end_callback();
+  rpc->result = result == 0 ? NACL_SRPC_RESULT_OK : NACL_SRPC_RESULT_APP_ERROR;
+  /* Fill in the deprecated return values with dummy values. */
+  out_args[0]->u.ival = 0;
+  out_args[1]->arrays.str = strdup("");
+  out_args[2]->arrays.str = strdup("");
+  /* TODO(jvoung): figure out how to get an error message out. */
+  if (result)
+    out_args[3]->arrays.str = strdup("There was some sort of error");
+  else
+    out_args[3]->arrays.str = strdup("no error");
+  done->Run(done);
+}
+
+static const struct NaClSrpcHandlerDesc srpc_methods[] = {
+  /*
+   * Protocol for streaming:
+   * StreamInitWithSplit(num_threads, obj_fd x 16, cmdline_flags) -> error_str
+   * StreamChunk(data) +
+   * TODO(jvoung): remove these is_shared_lib, etc.
+   * StreamEnd() -> (is_shared_lib, soname,dependencies, error_str)

[Mark Seaborn, 2015/03/26 20:39:00]

Nit: add space after ','

[jvoung (off chromium), 2015/03/26 22:30:12]

Done.

+   */
+  { "StreamInitWithSplit:ihhhhhhhhhhhhhhhhC:s", stream_init_with_split },
+  { "StreamChunk:C:", stream_chunk },
+  { "StreamEnd::isss", stream_end },
+  { NULL, NULL },
+};
+
+static void serve_translate_request(
+    const struct irt_pnacl_compile_funcs *funcs) {
+  g_funcs = funcs;
+  if (!NaClSrpcModuleInit()) {
+    NaClLog(LOG_FATAL, "NaClSrpcModuleInit() failed\n");
+  }
+  if (!NaClSrpcAcceptClientConnection(srpc_methods)) {
+    NaClLog(LOG_FATAL, "NaClSrpcAcceptClientConnection() failed\n");
+  }
+  NaClSrpcModuleFini();

[Mark Seaborn, 2015/03/26 20:39:00]

Nit: you might want to make _compile & _link.c consistent about this error/exit
handling.  We don't really need to call NaClSrpcModuleFini(), though handling
any error from NaClSrpcAcceptClientConnection() is nice.

[jvoung (off chromium), 2015/03/26 22:30:12]

Added check for NaClSrpcAcceptClientConnection() to _link and removed Fini from
_compile.

+}
+
+const struct nacl_irt_private_pnacl_translator_compile
+    nacl_irt_private_pnacl_translator_compile = {
+  serve_translate_request
+};