Add an IRT interface for subzero/compiler to serve_translate_requests.

Add an IRT interface for subzero/compiler to serve_translate_requests.

Will be used by:
https://codereview.chromium.org/997773002/

These are basic handlers for the current SRPC methods that
we use for pnacl-llc, and can be used as a starting point
for pnacl-sz.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4091
R=mseaborn@chromium.org

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/f5a25bad1b78709c223b0940c3ab2bd567beff6f

[jvoung (off chromium), 2015-03-23 17:44:05]

jvoung@chromium.org changed reviewers:
+ dschuff@chromium.org, mseaborn@chromium.org, stichnot@chromium.org

[jvoung (off chromium), 2015-03-23 17:44:28]

jvoung@chromium.org changed reviewers:
+ jfb@chromium.org

[Mark Seaborn, 2015-03-23 18:09:39]

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_dev.h
File src/untrusted/irt/irt_dev.h (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:165: struct PNaClSubzeroFunctions {
Nit: Use lower_case_with_underscores, following the style of the IRT interfaces

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:177: "nacl-irt-private-pnacl-trnalsator-subzero-0.1"
"translator" typo

Can you avoid naming this after Subzero, please?  Is there any reason this
wouldn't be usable with llc?

e.g. Name this as "compile" rather than "subzero".

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
File src/untrusted/irt/irt_pnacl_translator_subzero.c (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
src/untrusted/irt/irt_pnacl_translator_subzero.c:93: static void log_fatal(const
char *msg) {
I don't understand the reason for having this.

You said:
"So instead, terminate, and log to
the JS console as a compromise. Errors will still show up in
the JS console, but it won't show up in the lastError attribute."

It's only LOG_FATAL in trusted code that does that.  We'd need extra plumbing to
do that for LOG_FATAL in untrusted code.

[Jim Stichnoth, 2015-03-23 18:13:41]

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_dev.h
File src/untrusted/irt/irt_dev.h (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:142: * This is an internal interface, for use by
PNaCl's subzero translator for
Sorry for the pedantry - can we capitalize Subzero in descriptive text? :)

Or, use the pnacl-sz tool name?

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:149: * (*) init_callback is used to initialze the
translation process
initialize

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:152: *     to use (there may be other worker
threads).
Mention that num_threads=0 means completely sequential?

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:177: "nacl-irt-private-pnacl-trnalsator-subzero-0.1"
translator

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
File src/untrusted/irt/irt_pnacl_translator_subzero.c (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
src/untrusted/irt/irt_pnacl_translator_subzero.c:26: * Only one object file is
needed for subzero.
Subzero

[Mark Seaborn, 2015-03-23 18:15:56]

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_dev.h
File src/untrusted/irt/irt_dev.h (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:167: int obj_file_fd,
So I guess this is specific to Subzero?  llc would want multiple FDs here.  (As
an aside, how come Subzero doesn't?)

Can you make this applicable to llc too, please?  Otherwise we'd need separate
interfaces for llc and Subzero, or a cross-repo change to change this interface.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_in...
File src/untrusted/irt/irt_interfaces.c (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_in...
src/untrusted/irt/irt_interfaces.c:145: 
Nit: remove empty line

[Mark Seaborn, 2015-03-23 18:18:36]

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
File src/untrusted/irt/irt_pnacl_translator_subzero.c (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
src/untrusted/irt/irt_pnacl_translator_subzero.c:67: const struct
NaClSrpcHandlerDesc srpc_methods[] = {
Nit: add "static"

[Jim Stichnoth, 2015-03-23 18:21:16]

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_dev.h
File src/untrusted/irt/irt_dev.h (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:167: int obj_file_fd,
On 2015/03/23 18:15:56, Mark Seaborn wrote:
> So I guess this is specific to Subzero?  llc would want multiple FDs here. 
(As
> an aside, how come Subzero doesn't?)

Subzero doesn't need to resort to module splitting for multithreaded
translation:
 - A parser thread creates initial CFGs and adds them to a work queue
 - Multiple translation threads draw from the work queue and insert assembler
buffers into an emit queue
 - An emitter thread draws from the emit queue and writes to the ELF output
file, taking care to emit in the same (deterministic) order that things were
parsed.

[JF, 2015-03-23 18:33:54]

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
File src/untrusted/irt/irt_pnacl_translator_subzero.c (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
src/untrusted/irt/irt_pnacl_translator_subzero.c:26: * Only one object file is
needed for subzero.
On 2015/03/23 18:13:40, stichnot wrote:
> Subzero

SubZero.

/TroLol

[jvoung (off chromium), 2015-03-23 21:18:55]

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_dev.h
File src/untrusted/irt/irt_dev.h (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:142: * This is an internal interface, for use by
PNaCl's subzero translator for
On 2015/03/23 18:13:40, stichnot wrote:
> Sorry for the pedantry - can we capitalize Subzero in descriptive text? :)
> 
> Or, use the pnacl-sz tool name?

Alright, well if we are going with a shared pnacl-sz/pnacl-llc
interface I'll remove all mentions of Subzero, subzero and SubZero.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:149: * (*) init_callback is used to initialze the
translation process
On 2015/03/23 18:13:40, stichnot wrote:
> initialize

Done.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:152: *     to use (there may be other worker
threads).
On 2015/03/23 18:13:40, stichnot wrote:
> Mention that num_threads=0 means completely sequential?

Hmm, alright added that conditional on pnacl-sz. In practice, the browser won't
really pick 0 unless you make a custom build. On the other hand, it could be
easy to pick 0 with the commandline pnacl-translate tool.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:165: struct PNaClSubzeroFunctions {
On 2015/03/23 18:09:38, Mark Seaborn wrote:
> Nit: Use lower_case_with_underscores, following the style of the IRT
interfaces

Done.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:167: int obj_file_fd,
On 2015/03/23 18:21:16, stichnot wrote:
> On 2015/03/23 18:15:56, Mark Seaborn wrote:
> > So I guess this is specific to Subzero?  llc would want multiple FDs here. 
> (As
> > an aside, how come Subzero doesn't?)
> 
> Subzero doesn't need to resort to module splitting for multithreaded
> translation:
>  - A parser thread creates initial CFGs and adds them to a work queue
>  - Multiple translation threads draw from the work queue and insert assembler
> buffers into an emit queue
>  - An emitter thread draws from the emit queue and writes to the ELF output
> file, taking care to emit in the same (deterministic) order that things were
> parsed.

Right.

Alright, I guess Petr is close enough to using the IRT for pnacl-llc, and they
are similar enough for now that we could share this layer. The tools will just
use what they need.

On the other hand, the browser will need to be aware if it's linking for
pnacl-llc or pnacl-sz. It turns out that the linker won't "tolerate empty
files".

If we later want to do something special for subzero like a combined
compile+link, we could add a new interface version with a new method.

Sharing would also argue for having separate methods for anything specific to
subzero. E.g., not return internal UMA info in the "end_callback" but have a
separate method.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_de...
src/untrusted/irt/irt_dev.h:177: "nacl-irt-private-pnacl-trnalsator-subzero-0.1"
On 2015/03/23 18:09:38, Mark Seaborn wrote:
> "translator" typo
> 
> Can you avoid naming this after Subzero, please?  Is there any reason this
> wouldn't be usable with llc?
> 
> e.g. Name this as "compile" rather than "subzero".

Done.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_in...
File src/untrusted/irt/irt_interfaces.c (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_in...
src/untrusted/irt/irt_interfaces.c:145: 
On 2015/03/23 18:15:56, Mark Seaborn wrote:
> Nit: remove empty line

Done.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
File src/untrusted/irt/irt_pnacl_translator_subzero.c (right):

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
src/untrusted/irt/irt_pnacl_translator_subzero.c:26: * Only one object file is
needed for subzero.
On 2015/03/23 18:33:54, JF wrote:
> On 2015/03/23 18:13:40, stichnot wrote:
> > Subzero
> 
> SubZero.
> 
> /TroLol

Acknowledged.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
src/untrusted/irt/irt_pnacl_translator_subzero.c:67: const struct
NaClSrpcHandlerDesc srpc_methods[] = {
On 2015/03/23 18:18:36, Mark Seaborn wrote:
> Nit: add "static"

Done.

https://codereview.chromium.org/984713003/diff/60001/src/untrusted/irt/irt_pn...
src/untrusted/irt/irt_pnacl_translator_subzero.c:93: static void log_fatal(const
char *msg) {
On 2015/03/23 18:09:38, Mark Seaborn wrote:
> I don't understand the reason for having this.
> 
> You said:
> "So instead, terminate, and log to
> the JS console as a compromise. Errors will still show up in
> the JS console, but it won't show up in the lastError attribute."
> 
> It's only LOG_FATAL in trusted code that does that.  We'd need extra plumbing
to
> do that for LOG_FATAL in untrusted code.

Ah... didn't think that the hook was only for trusted code, but that makes sense
that it wouldn't have been exposed to untrusted code. I hadn't yet tested this
on 32-bit chrome... still figuring out the cross build since every previous
method (chroot / install-build-deps libs32) seems deprecated.

Alright, I'll figure something else out.

[Derek Schuff, 2015-03-23 21:30:35]

> Ah... didn't think that the hook was only for trusted code, but that makes
sense
> that it wouldn't have been exposed to untrusted code. I hadn't yet tested this
> on 32-bit chrome... still figuring out the cross build since every previous
> method (chroot / install-build-deps libs32) seems deprecated.

As a side note, let me know if you find something, because I've not been able to
successfully build 32-bit chrome (other than by just commandeering a trybot)
since they switched that :(

[jvoung (off chromium), 2015-03-24 01:39:01]

On 2015/03/23 21:30:35, Derek Schuff wrote:
> > Ah... didn't think that the hook was only for trusted code, but that makes
> sense
> > that it wouldn't have been exposed to untrusted code. I hadn't yet tested
this
> > on 32-bit chrome... still figuring out the cross build since every previous
> > method (chroot / install-build-deps libs32) seems deprecated.
> 
> As a side note, let me know if you find something, because I've not been able
to
> successfully build 32-bit chrome (other than by just commandeering a trybot)
> since they switched that :(

I ended up with this:

(a) create an 32-bit lxr container.
(b) fetch chromium (possibly could skip if you mount the chromium source into
the container or something)
(c) run build/install-build-deps.sh (grabs 32-bit libs). Hack the NaCl
requirements to spell the packages differently (take out the "32" from
""lib32ncurses5-dev"... similar for lib32z1-dev).
(d) sudo dpkg --add-architecture amd64 && sudo apt-get update && sudo apt-get
install libc6-dev:amd64 libstdc++-4.8-dev:amd64
(e) run install-build-deps.sh again (somehow the apt-get clobbered a bunch of
32-bit libs).
(e.2) perhaps an alternative would have been to install the amd64 libc /
libstdc++ first... apt-get clobbering my build-deps may also have been a
fluke...
(f) gclient runhooks w/ the appropriate GYP_DEFINES="target_arch=ia32
component=shared_library"
(g) ninja -C out/Release chrome
(h) outside of the container, cp -r /var/lib/lxc/my32bitbox/...
(i) outside of the container, run the copied-out chrome

I'm an lxc noob, so I don't know of a better way than (h) and (i), since my
container isn't set up to run windowed apps. I guess if you only rely on printf
and "browser_tests" then you could possibly just xvfb-run the tests inside the
container.

[jvoung (off chromium), 2015-03-24 01:41:05]

On 2015/03/24 01:39:01, jvoung wrote:
> On 2015/03/23 21:30:35, Derek Schuff wrote:
> > > Ah... didn't think that the hook was only for trusted code, but that makes
> > sense
> > > that it wouldn't have been exposed to untrusted code. I hadn't yet tested
> this
> > > on 32-bit chrome... still figuring out the cross build since every
previous
> > > method (chroot / install-build-deps libs32) seems deprecated.
> > 
> > As a side note, let me know if you find something, because I've not been
able
> to
> > successfully build 32-bit chrome (other than by just commandeering a trybot)
> > since they switched that :(
> 
> I ended up with this:
> 
> (a) create an 32-bit lxr container.

I mean lxc...

> (b) fetch chromium (possibly could skip if you mount the chromium source into
> the container or something)
> (c) run build/install-build-deps.sh (grabs 32-bit libs). Hack the NaCl
> requirements to spell the packages differently (take out the "32" from
> ""lib32ncurses5-dev"... similar for lib32z1-dev).
> (d) sudo dpkg --add-architecture amd64 && sudo apt-get update && sudo apt-get
> install libc6-dev:amd64 libstdc++-4.8-dev:amd64
> (e) run install-build-deps.sh again (somehow the apt-get clobbered a bunch of
> 32-bit libs).
> (e.2) perhaps an alternative would have been to install the amd64 libc /
> libstdc++ first... apt-get clobbering my build-deps may also have been a
> fluke...
> (f) gclient runhooks w/ the appropriate GYP_DEFINES="target_arch=ia32
> component=shared_library"
> (g) ninja -C out/Release chrome
> (h) outside of the container, cp -r /var/lib/lxc/my32bitbox/...
> (i) outside of the container, run the copied-out chrome
> 
> I'm an lxc noob, so I don't know of a better way than (h) and (i), since my
> container isn't set up to run windowed apps. I guess if you only rely on
printf
> and "browser_tests" then you could possibly just xvfb-run the tests inside the
> container.

[jvoung (off chromium), 2015-03-25 23:10:45]

jvoung@chromium.org changed reviewers:
+ mcgrathr@chromium.org

[jvoung (off chromium), 2015-03-25 23:19:31]

+Roland since Mark is on vacation and Roland is the only other IRT owner.

Mark, if you'd like to look through this yourself, let me know.

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

LGTM

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
File src/untrusted/irt/irt_dev.h (right):

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:151: * (*) init_callback is used to initialize the
translation process
Nit: how about putting the per-function comments before each function declared
inside the struct?

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:158: *     Not all fds are valid.
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?

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:159: *     |obj_file_fd_count| number of valid fds
in the beginning of the
"fds" -> "FDs" for consistency.  Same above.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:161: *     |cmd_flags| is \0 delimited set of
command-line flags.
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*.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:162: *     |cmd_flags_len| is the length of the
|cmd_flags| array.
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().

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:172: struct irt_pnacl_compile_funcs {
Use "nacl_irt_" prefix for consistency?

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:176: int (*data_callback)(unsigned char *data,
size_t num_bytes);
Nit: maybe "const void *" instead, to match read/write()?

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
File src/untrusted/irt/irt_pnacl_translator_compile.c (right):

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:32: ++num_valid_fds;
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.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:41: /* TODO(jvoung): figure out
how to get an error message out. */
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.)

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:52: (unsigned char
*)in_args[0]->arrays.carr,
Nit: NaCl cast style is "(type) expr", with a space

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:80: * StreamEnd() ->
(is_shared_lib, soname,dependencies, error_str)
Nit: add space after ','

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:97: NaClSrpcModuleFini();
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.

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

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
File src/untrusted/irt/irt_pnacl_translator_compile.c (right):

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:41: /* TODO(jvoung): figure out
how to get an error message out. */
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> 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.)

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]

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
File src/untrusted/irt/irt_dev.h (right):

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:151: * (*) init_callback is used to initialize the
translation process
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> Nit: how about putting the per-function comments before each function declared
> inside the struct?

That sounds good. Done.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:158: *     Not all fds are valid.
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> 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?

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.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:159: *     |obj_file_fd_count| number of valid fds
in the beginning of the
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> "fds" -> "FDs" for consistency.  Same above.

Done.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:161: *     |cmd_flags| is \0 delimited set of
command-line flags.
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> 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*.

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).

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:162: *     |cmd_flags_len| is the length of the
|cmd_flags| array.
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> 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().

Changed this to mean the same as "argc".

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:172: struct irt_pnacl_compile_funcs {
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> Use "nacl_irt_" prefix for consistency?

Done.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_d...
src/untrusted/irt/irt_dev.h:176: int (*data_callback)(unsigned char *data,
size_t num_bytes);
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> Nit: maybe "const void *" instead, to match read/write()?

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...

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
File src/untrusted/irt/irt_pnacl_translator_compile.c (right):

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:32: ++num_valid_fds;
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> 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.

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

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:41: /* TODO(jvoung): figure out
how to get an error message out. */
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> 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...
> 

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

Done =)


> (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.)

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.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:52: (unsigned char
*)in_args[0]->arrays.carr,
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> Nit: NaCl cast style is "(type) expr", with a space

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

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:80: * StreamEnd() ->
(is_shared_lib, soname,dependencies, error_str)
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> Nit: add space after ','

Done.

https://codereview.chromium.org/984713003/diff/180001/src/untrusted/irt/irt_p...
src/untrusted/irt/irt_pnacl_translator_compile.c:97: NaClSrpcModuleFini();
On 2015/03/26 20:39:00, Mark Seaborn wrote:
> 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.

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

[Derek Schuff, 2015-03-26 22:30:35]

On 2015/03/26 22:29:35, Derek Schuff wrote:
> 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?

oh nevermind, i just reread your TODO in the commit message.

[jvoung (off chromium), 2015-03-26 22:30:42]

(and thanks Mark!)

[jvoung (off chromium), 2015-03-27 22:08:09]

Committed patchset #13 (id:240001) manually as
f5a25bad1b78709c223b0940c3ab2bd567beff6f (presubmit successful).