Add an IRT interface for subzero/compiler to serve_translate_requests.

src/untrusted/irt/irt_dev.h

 /*
  * Copyright (c) 2013 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.
  */
 #ifndef NATIVE_CLIENT_SRC_UNTRUSTED_IRT_IRT_DEV_H_
 #define NATIVE_CLIENT_SRC_UNTRUSTED_IRT_IRT_DEV_H_
 
 #include <stdarg.h>
 #include <stddef.h>
@@ skipping 120 matching lines @@
  * success or a non-zero value on error.
  */
 #define NACL_IRT_PRIVATE_PNACL_TRANSLATOR_LINK_v0_1 \
     "nacl-irt-private-pnacl-translator-link-0.1"
 struct nacl_irt_private_pnacl_translator_link {
   void (*serve_link_request)(int (*func)(int nexe_fd,
                                          const int *obj_file_fds,
                                          int obj_file_fd_count));
 };
 
+/*
+ * This is an internal interface, for use by PNaCl's sandboxed compilers for
+ * communicating with the browser. This applies to pnacl-llc and pnacl-sz
+ * though how pnacl-llc and pnacl-sz use the interface can be slightly
+ * different (e.g., pnacl-sz will only use the first the obj_file_fds entry).
+ *
+ * serve_translate_request(funcs) loops waiting for sequences of requests
+ * via IPC, and then calls the appropriate callback to handle the IPC,
+ * on the same thread.
+ *
+ * (*) init_callback is used to initialize the translation process

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

Nit: how about putting the per-function comments before each function declared
inside the struct?

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

That sounds good. Done.

+ *     with configuration info:
+ *     |num_threads| is the number of recommended *translate* threads
+ *     to use (there may be other worker threads). For pnacl-sz,
+ *     num_threads == 0 is for completely sequential translation
+ *     (no worker threads).
+ *     |obj_file_fds| is an array of FDs for writing an output object file.
+ *     Not all fds are valid.

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

What does it mean for an FD to be invalid?  Does this mean you're passing -1 as
a dummy value?  Why not pass a smaller obj_file_fd_count value instead?

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

Hmm, right... I mixed up the "array length" from the SRPC interface which has a
fixed width (16 FDs) vs this which doesn't care so much about the size of the
underlying array, just the number of valid FDs. Changed comment.

+ *     |obj_file_fd_count| number of valid fds in the beginning of the

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

"fds" -> "FDs" for consistency.  Same above.

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

Done.

+ *     |obj_file_fds| array (the array may have invalid fds at the end).
+ *     |cmd_flags| is \0 delimited set of command-line flags.

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

Do you mean "terminated" rather than "delimited"?  Presumably the flags aren't
separated by nulls?  Are they separated by spaces?  (If so, can you say that
too?)

If you really mean null-delimited, that's rather unusual.  It would be better to
use a char** or a space-separated char*.

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

I really meant null-delimited. At the time we thought using argz would be a good
idea... which uses null-delimited strings.

I lifted the argz stuff to the IRT side since SRPC sends such a thing, and made
the llc/subzero side simpler (just char** argv/argc).

+ *     |cmd_flags_len| is the length of the |cmd_flags| array.

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

Isn't this redundant if cmd_flags is null-terminated?  It would be more
idiomatic to omit this if you can easily get it with strlen().

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

Changed this to mean the same as "argc".

+ * (*) data_callback is invoked to stream additional bitcode data
+ *     to the translator.
+ * (*) end_callback is invoked to signal the end of the data stream.
+ *     This must block until translation is actually complete and
+ *     the translator process can be terminated.
+ *
+ * All callbacks return zero on success or a non-zero value on error.
+ * TODO(jvoung): Add error output.
+ */
+struct irt_pnacl_compile_funcs {

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

Use "nacl_irt_" prefix for consistency?

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

Done.

+  int (*init_callback)(uint32_t num_threads,
+                       int *obj_file_fds, size_t obj_file_fd_count,
+                       const char *cmd_flags, size_t cmd_flags_len);
+  int (*data_callback)(unsigned char *data, size_t num_bytes);

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

Nit: maybe "const void *" instead, to match read/write()?

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

Done. The underlying LLVM QueueStreamer doesn't take "const" but we could change
that.

For now, left this as const and did a const_cast in subzero...

+  int (*end_callback)(void);
+  /* TODO(jvoung): have a separate method for getting internal UMA stats. */
+};
+
+#define NACL_IRT_PRIVATE_PNACL_TRANSLATOR_COMPILE_v0_1 \
+  "nacl-irt-private-pnacl-translator-compile-0.1"
+struct nacl_irt_private_pnacl_translator_compile {
+  void (*serve_translate_request)(
+      const struct irt_pnacl_compile_funcs *funcs);
+};
+
 #if defined(__cplusplus)
 }
 #endif
 
 #endif /* NATIVE_CLIENT_SRC_UNTRUSTED_IRT_IRT_DEV_H */