Mojo + NaCl prototype.

Mojo + NaCl prototype.

Create a standalone executable that plumbs public Mojo API calls into a NaCl
sandbox.  This allows the public Mojo unit tests to run inside NaCl.

BUG=401761
Committed: https://crrev.com/95d2d4025c767fbc5669377aae457a4bfb5ca46b
Cr-Commit-Position: refs/heads/master@{#294484}

[Nick Bray (chromium), 2014-07-18 22:48:18]

Initial RFC.  All but two of the mojo/public tests work.  About 60% of the line
count is generated code.  Yak shave CLs are needed before this CL works:

https://codereview.chromium.org/405713003/
https://codereview.appspot.com/114010044/
https://codereview.chromium.org/401453002/

Known issues:

1) It's linking against internal NaCl interfaces.  This needs to be cleaned up
before the test is built or run on the bots.

2) Code generator needs to be cleaned up.

But... it would be nice to get this CL committed before it gets any larger. 
Fixing 2) before committing is probably a good idea, but letting 1) slide seems
reasonable.

[viettrungluu, 2014-07-18 23:00:50]

On 2014/07/18 22:48:18, Nick Bray (chromium) wrote:
> Initial RFC.  All but two of the mojo/public tests work.  About 60% of the
line
> count is generated code.

Do you think you could take a stab at having the generated code generated at
build time rather than checking them in?


> Yak shave CLs are needed before this CL works:
> 
> https://codereview.chromium.org/405713003/
> https://codereview.appspot.com/114010044/
> https://codereview.chromium.org/401453002/
> 
> Known issues:
> 
> 1) It's linking against internal NaCl interfaces.  This needs to be cleaned up
> before the test is built or run on the bots.
> 
> 2) Code generator needs to be cleaned up.
> 
> But... it would be nice to get this CL committed before it gets any larger. 
> Fixing 2) before committing is probably a good idea, but letting 1) slide
seems
> reasonable.

[Mark Seaborn, 2014-07-21 16:31:39]

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/DEPS
File mojo/monacl/DEPS (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/DEPS#newcode2
mojo/monacl/DEPS:2: "+native_client",
Don't use a blanket "all of native_client/" rule, please.  Instead list specific
directories, such as "native_client/public/" and
"native_client/src/trusted/desc".

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/README
File mojo/monacl/README (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/README#newcode5
mojo/monacl/README:5: gen/interface.py contains a programatic description of the
stable Mojo
Nit: "programmatic"

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/generate...
File mojo/monacl/gen/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/generate...
mojo/monacl/gen/generate_monacl_bindings.py:167: code << "%s_ptr =
reinterpret_cast<%s>(%s);" % (param.name, paramPtr(param), src)
Lines >80 chars (and elsewhere too)

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/libmojo....
File mojo/monacl/gen/libmojo.cc.template (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/libmojo....
mojo/monacl/gen/libmojo.cc.template:14: int imc_sendmsg(int desc, struct
NaClAbiNaClImcMsgHdr const *nmhp, int flags) {
Please use the existing wrapper from src/untrusted/nacl/imc_sendmsg.c rather
than using NACL_SYSCALL here.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/mojo_sys...
File mojo/monacl/gen/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/mojo_sys...
mojo/monacl/gen/mojo_syscall.cc.template:67: NaClLog(LOG_FATAL,
"MojoDescRecvMsg: Not implemented\n");
Don't introduce more Chrome-side uses of NaClLog(), please.  Use Chromium's
base/logging.h instead.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall.cc
File mojo/monacl/mojo_syscall.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall...
mojo/monacl/mojo_syscall.cc:62: uintptr_t temp = NaClUserToSysAddrRange(nap,
params[1], sizeof(*options));
You should use NaClCopyInFromUser() and NaClCopyOutToUser() rather than the
NaClUserToSys functions.

The NaClCopy* functions will do the right security checks and use locks properly
on your behalf.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/monacl_sel_m...
File mojo/monacl/monacl_sel_main.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/monacl_sel_m...
mojo/monacl/monacl_sel_main.cc:88: NaClAppStartModule(nap, NULL, NULL);
Don't duplicate all of this NaCl-side startup code in the Chrome repo, please. 
You should be able to call into src/public/chrome_main.h instead.

You might just need to turn NaClChromeMainArgs's "imc_bootstrap_handle" into an
optional parameter so that you don't need any legacy SRPC code on the Chrome
side.

[Nick Bray (chromium), 2014-07-21 21:26:37]

> Do you think you could take a stab at having the generated code generated at
> build time rather than checking them in?

How important is this, in your opinion?  I was planning on checking code in for
two reasons.  One, it makes it reviewable.  I just caught a bug by reviewing the
generated code, and in general this code is security critical so reviewability
is good.  Two, I've been viewing codegen as a development tool more than
something permanent.  In the future, when everything is stable, it would be
possible to chuck to code generator.  (If this is desirable is a different
question.)  I don't have a strong opinion.  I can see the consistency advantages
of generating at build time, but being able to review the generated diffs is
also pretty nice.  Thoughts?

[viettrungluu, 2014-07-21 22:32:21]

On 2014/07/21 21:26:37, Nick Bray (chromium) wrote:
> > Do you think you could take a stab at having the generated code generated at
> > build time rather than checking them in?
> 
> How important is this, in your opinion?  I was planning on checking code in
for
> two reasons.  One, it makes it reviewable.  I just caught a bug by reviewing
the
> generated code, and in general this code is security critical so reviewability
> is good.  Two, I've been viewing codegen as a development tool more than
> something permanent.  In the future, when everything is stable, it would be
> possible to chuck to code generator.  (If this is desirable is a different
> question.)  I don't have a strong opinion.  I can see the consistency
advantages
> of generating at build time, but being able to review the generated diffs is
> also pretty nice.  Thoughts?

I guess it's not super-important. What qsr@ does, which I quite like, is to
include the generated code in the code review in a "not-to-be-committed"
directory. This allows it to be reviewed. If you're careful, you can also
convince git-cl/rietveld to display diffs too. Admittedly, this is all kind of
annoying. (Probably the easiest way would be to maintain a branch with the
"not-to-be-committed" directory and generated code, do your work off on that
branch, and then to land any change just cherry-pick the changes from the branch
to a branch off of origin/master and remove the generated files.)

If you think that the odds are good that we'll eventually get rid of the code
generator, then we can go with what you're currently doing.

[Nick Bray (chromium), 2014-07-21 22:38:37]

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/README
File mojo/monacl/README (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/README#newcode5
mojo/monacl/README:5: gen/interface.py contains a programatic description of the
stable Mojo
On 2014/07/21 16:31:39, Mark Seaborn wrote:
> Nit: "programmatic"

Done.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/generate...
File mojo/monacl/gen/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/generate...
mojo/monacl/gen/generate_monacl_bindings.py:167: code << "%s_ptr =
reinterpret_cast<%s>(%s);" % (param.name, paramPtr(param), src)
On 2014/07/21 16:31:39, Mark Seaborn wrote:
> Lines >80 chars (and elsewhere too)

This file needs to be cleaned up before commit, as is.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/libmojo....
File mojo/monacl/gen/libmojo.cc.template (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/libmojo....
mojo/monacl/gen/libmojo.cc.template:14: int imc_sendmsg(int desc, struct
NaClAbiNaClImcMsgHdr const *nmhp, int flags) {
On 2014/07/21 16:31:39, Mark Seaborn wrote:
> Please use the existing wrapper from src/untrusted/nacl/imc_sendmsg.c rather
> than using NACL_SYSCALL here.

Done, although it dirtys up GYP a little.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/mojo_sys...
File mojo/monacl/gen/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/mojo_sys...
mojo/monacl/gen/mojo_syscall.cc.template:67: NaClLog(LOG_FATAL,
"MojoDescRecvMsg: Not implemented\n");
On 2014/07/21 16:31:39, Mark Seaborn wrote:
> Don't introduce more Chrome-side uses of NaClLog(), please.  Use Chromium's
> base/logging.h instead.

sel_ldr.h included for range checks.  sel_ldr includes nacl_log.h.  LOG_*
constant names conflict with base.  Removed logging.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall.cc
File mojo/monacl/mojo_syscall.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall...
mojo/monacl/mojo_syscall.cc:62: uintptr_t temp = NaClUserToSysAddrRange(nap,
params[1], sizeof(*options));
On 2014/07/21 16:31:39, Mark Seaborn wrote:
> You should use NaClCopyInFromUser() and NaClCopyOutToUser() rather than the
> NaClUserToSys functions.
> 
> The NaClCopy* functions will do the right security checks and use locks
properly
> on your behalf.

Except that the logic is more complicated than NaClCopy*.  Null pointers must be
passed through, in/outs require copies in and out of the same pointer, options
structures have internal size declarations, data arrays will not be copied, but
handle arrays may be.  There are enough ways to shoot yourself in the foot here,
I don't see using NaClCopy* as having a huge benefit, all other complexities
considered.  Using NaClCopy* will also entail acquiring the copy lock ~10 times
instead of twice, which sort of sucks.

That being said, this code is the ugliest part of the CL and need to be cleaned
up.  For example, I just noticed that the "return -1" statements do not release
the copy lock.  I am pondering a combination of templated pointer conversion
logic and a scoped copy lock.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/monacl_sel_m...
File mojo/monacl/monacl_sel_main.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/monacl_sel_m...
mojo/monacl/monacl_sel_main.cc:88: NaClAppStartModule(nap, NULL, NULL);
On 2014/07/21 16:31:39, Mark Seaborn wrote:
> Don't duplicate all of this NaCl-side startup code in the Chrome repo, please.

> You should be able to call into src/public/chrome_main.h instead.
> 
> You might just need to turn NaClChromeMainArgs's "imc_bootstrap_handle" into
an
> optional parameter so that you don't need any legacy SRPC code on the Chrome
> side.

This is a discussion for elsewhere.  Getting the inter-repo interface right may
be arbitrarily messy.  I am not convinced making that a hard dependency for this
CL is the right move.  It will need to be done soon, but there's already a large
amount of yak shaving going on.

[Mark Seaborn, 2014-07-21 23:25:44]

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/DEPS
File mojo/monacl/DEPS (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/DEPS#newcode2
mojo/monacl/DEPS:2: "+native_client",
On 2014/07/21 16:31:39, Mark Seaborn wrote:
> Don't use a blanket "all of native_client/" rule, please.  Instead list
specific
> directories, such as "native_client/public/" and
> "native_client/src/trusted/desc".

Ping -- this is really easy to do.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/mojo_sys...
File mojo/monacl/gen/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/gen/mojo_sys...
mojo/monacl/gen/mojo_syscall.cc.template:67: NaClLog(LOG_FATAL,
"MojoDescRecvMsg: Not implemented\n");
On 2014/07/21 22:38:36, Nick Bray (chromium) wrote:
> On 2014/07/21 16:31:39, Mark Seaborn wrote:
> > Don't introduce more Chrome-side uses of NaClLog(), please.  Use Chromium's
> > base/logging.h instead.
> 
> sel_ldr.h included for range checks.  sel_ldr includes nacl_log.h.  LOG_*
> constant names conflict with base.

Right -- that kind of conflict is the reason why you shouldn't be #including
service_runtime headers in the Chromium repo. :-)

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall.cc
File mojo/monacl/mojo_syscall.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall...
mojo/monacl/mojo_syscall.cc:62: uintptr_t temp = NaClUserToSysAddrRange(nap,
params[1], sizeof(*options));
On 2014/07/21 22:38:36, Nick Bray (chromium) wrote:
> On 2014/07/21 16:31:39, Mark Seaborn wrote:
> > You should use NaClCopyInFromUser() and NaClCopyOutToUser() rather than the
> > NaClUserToSys functions.
> > 
> > The NaClCopy* functions will do the right security checks and use locks
> properly
> > on your behalf.
> 
> Except that the logic is more complicated than NaClCopy*.
> Null pointers must be passed through,

You could make a wrapper around CopyIn/Out to handle NULLs.

> in/outs require copies in and out of the same pointer, options
> structures have internal size declarations, data arrays will not be copied,
but
> handle arrays may be.  There are enough ways to shoot yourself in the foot
here,
> I don't see using NaClCopy* as having a huge benefit, all other complexities
> considered.  Using NaClCopy* will also entail acquiring the copy lock ~10
times
> instead of twice, which sort of sucks.

This does raise the question of why you're calling NaClUserToSysAddrRange() once
for each parameter.  Why don't you pass the parameters in a struct instead?

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/monacl_sel_m...
File mojo/monacl/monacl_sel_main.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/monacl_sel_m...
mojo/monacl/monacl_sel_main.cc:88: NaClAppStartModule(nap, NULL, NULL);
On 2014/07/21 22:38:36, Nick Bray (chromium) wrote:
> On 2014/07/21 16:31:39, Mark Seaborn wrote:
> > Don't duplicate all of this NaCl-side startup code in the Chrome repo,
please.
> 
> > You should be able to call into src/public/chrome_main.h instead.
> > 
> > You might just need to turn NaClChromeMainArgs's "imc_bootstrap_handle" into
> an
> > optional parameter so that you don't need any legacy SRPC code on the Chrome
> > side.
> 
> This is a discussion for elsewhere.  Getting the inter-repo interface right
may
> be arbitrarily messy.  I am not convinced making that a hard dependency for
this
> CL is the right move.  It will need to be done soon, but there's already a
large
> amount of yak shaving going on.

Can you compromise on at least part of the cleanup of this change?  Otherwise
this change would adding a large amount of technical debt in terms of unclean
Chromium->NaCl dependencies.

https://codereview.chromium.org/385983008/diff/60001/mojo/monacl.gypi
File mojo/monacl.gypi (right):

https://codereview.chromium.org/385983008/diff/60001/mojo/monacl.gypi#newcode58
mojo/monacl.gypi:58: '<(DEPTH)/native_client/src/untrusted/nacl/imc_sendmsg.c',
Please don't do this!  Instead, use the 'imc_syscalls_lib' that is defined in
native_client/src/untrusted/nacl/nacl.gyp.

[Nick Bray (chromium), 2014-07-25 23:51:14]

The state of things:

I believe the code is pretty much in its final form, modulo generating the code
from gyp rather than checking it in.  That'll be the next thing I do.

The service runtime startup has been refactored to be similar to chrome_main,
and this makes it clear why using chrome_main will not fly without yak shaving a
better embedding interface.

It looks like the gtest yak shave will be the long poll that is blocking this
CL.

[Nick Bray (chromium), 2014-08-14 20:14:27]

The yaks have been shaven.

The CL is in a state that I personally believe is reasonable for commit.  It is
not complete or 100% solid, but I am trying to balance that against
reviewability.  Let's discuss.

[viettrungluu, 2014-08-20 19:52:38]

(I should also point out that jamesr is busy adding .gn files --
https://codereview.chromium.org/494523004/ -- so you'll need to do something for
that. Probably, you'll want to add a suitable source_set for the public system
tests, etc.)

https://codereview.chromium.org/385983008/diff/220001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/385983008/diff/220001/build/all.gyp#newcode414
build/all.gyp:414: '../mojo/monacl.gyp:*',
This change seems a bit odd to me.

https://codereview.chromium.org/385983008/diff/220001/mojo/mojo_srcs.gypi
File mojo/mojo_srcs.gypi (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/mojo_srcs.gypi#new...
mojo/mojo_srcs.gypi:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
I wonder if, instead of adding another .gypi file, you shouldn't reuse
mojo_variables.gypi (which I believe was added after you started this change),
updating the comment at the top.

(At the very least, this file could use a comment indicating what it's for and
who uses it....)

https://codereview.chromium.org/385983008/diff/220001/mojo/public/cpp/system/...
File mojo/public/cpp/system/handle.h (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/public/cpp/system/...
mojo/public/cpp/system/handle.h:242: // Passing the operands by reference
confuses the STL implementation in NaCl's
Probably we can just make it take |Handle|s by value permanently.

We already mostly pass |Handle|s by value[*], and where we don't it's mostly out
of habit (or misguided sense of consistency, leading to even more inconsistency
of course).

(If we do that, we should probably make |Wait()|, above, just take a |Handle|
instead of a |const Handle&|.)

[*] This should never be less efficient, and sometimes potentially more
efficient.

[Mark Seaborn, 2014-08-26 16:25:54]

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl.gyp
File mojo/monacl.gyp (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl.gyp#newcode9
mojo/monacl.gyp:9: 'monacl_codegen_dir': '<(SHARED_INTERMEDIATE_DIR)/<!(python
<(DEPTH)/build/inverse_depth.py <(DEPTH))/monacl',
Nit: indent -1 space

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
File mojo/monacl/gen/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
mojo/monacl/gen/generate_monacl_bindings.py:5: 
Nit: Isn't "gen/" usually for generated code, not for the generator?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
mojo/monacl/gen/generate_monacl_bindings.py:51: def templateFile(name):
Style nit: should be TemplateFile (see
http://www.chromium.org/chromium-os/python-style-guidelines)

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
mojo/monacl/gen/generate_monacl_bindings.py:284: code << "ScopedCopyLock
copyLock(nap);"
Style nit: "copy_lock"

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
File mojo/monacl/gen/libmojo.cc.template (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:4: 
Could you name this "*template.cc" so that it gets checked by tools?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:5: // WARNING this file was generated by
$script_name
Rather confusing because this file *isn't* generated.  Can you have the script
add this comment instead?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:14: #define NACL_MOJO_DESC 8
No need to hard-code this.  Use NACL_CHROME_DESC_BASE instead.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:18: {params, numParams}
Style nit: num_params

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
File mojo/monacl/gen/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:20: class ScopedCopyLock {
How about putting all these definitions into a normal .h file instead of a
template file?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:22: explicit ScopedCopyLock(struct
NaClApp *nap) : nap_(nap) {
Use "* " spacing in Chromium code.  Same elsewhere in the change.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:34: uintptr_t uaddr,
Nit: use uint32_t (as you do below)

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:37: // TODO overflow checking
Nit: "TODO(ncbray)"

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:42: template <typename T> bool
ConvertScalarInput(
"Convert" is vague.  How about ReadScalarInput or CopyInScalarInput?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:47: uintptr_t temp =
NaClUserToSysAddrRange(nap, user_ptr, sizeof(T));
Nit: 'temp' -> 'sys_addr'

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:56: template <typename T> bool
ConvertScalarOutput(
This looks like it would be like ConvertScalarInput but operate in the reverse
direction.  But that's not the case.

Maybe rename to "ScalarOutputUserToSys" or "GetScalarOutputSysAddr"?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:67: return false;
Security bug: this leaves *sys_ptr undefined, so the caller will write to an
undefined trusted address.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
File mojo/monacl/monacl_sel_main.cc (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:23: static int MoNaClLoadApp(
Nit: Chromium style is to use an anon namespace instead of static.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:32:
NaClErrorLogHookInit(NaClBootstrapChannelErrorReporter, nap);
You don't need to reference the IMC bootstrap channel.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:36: if (skip_qualification) {
Always 0, so remove this branch.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:40: // HACK no fault injection.
Remove "HACK" comment

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:125: // TODO free nap?
Not if you're exiting.  You can remove this comment.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:127: NaClAllModulesFini();
Not safe because threads can be running.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:128: return ret_code;
Calling exit() is not safe because it does some teardown that can breaking
running threads.  (I've seen crashes happen as a result.)  Use NaClExit()
instead.

https://codereview.chromium.org/385983008/diff/220001/mojo/public/cpp/system/...
File mojo/public/cpp/system/handle.h (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/public/cpp/system/...
mojo/public/cpp/system/handle.h:243: // GCC toolchain and results in specialized
templates that break strict aliasing
I don't think we build with -fstrict-aliasing though, do we?  What's the problem
you see?  A warning or something else?

Is there a GCC/libstdc++ issue filed about this?

[Nick Bray (chromium), 2014-09-03 23:45:03]

PTAL

https://codereview.chromium.org/385983008/diff/220001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/385983008/diff/220001/build/all.gyp#newcode414
build/all.gyp:414: '../mojo/monacl.gyp:*',
On 2014/08/20 19:52:38, viettrungluu wrote:
> This change seems a bit odd to me.

Otherwise the gyp file is not part of the build.  Previously I put the gtest
file here to "compile test" my gtest port, but now the transitive dependency
makes that unnecessary.  Hence the swap.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl.gyp
File mojo/monacl.gyp (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl.gyp#newcode9
mojo/monacl.gyp:9: 'monacl_codegen_dir': '<(SHARED_INTERMEDIATE_DIR)/<!(python
<(DEPTH)/build/inverse_depth.py <(DEPTH))/monacl',
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Nit: indent -1 space

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
File mojo/monacl/gen/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
mojo/monacl/gen/generate_monacl_bindings.py:5: 
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Nit: Isn't "gen/" usually for generated code, not for the generator?

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
mojo/monacl/gen/generate_monacl_bindings.py:51: def templateFile(name):
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Style nit: should be TemplateFile (see
> http://www.chromium.org/chromium-os/python-style-guidelines)

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/generat...
mojo/monacl/gen/generate_monacl_bindings.py:284: code << "ScopedCopyLock
copyLock(nap);"
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Style nit: "copy_lock"

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
File mojo/monacl/gen/libmojo.cc.template (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:4: 
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Could you name this "*template.cc" so that it gets checked by tools?

I suspect the fact it isn't a syntactically valid .cc means this would be asking
for trouble.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:5: // WARNING this file was generated by
$script_name
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Rather confusing because this file *isn't* generated.  Can you have the script
> add this comment instead?

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:14: #define NACL_MOJO_DESC 8
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> No need to hard-code this.  Use NACL_CHROME_DESC_BASE instead.

Done, but it doesn't seem right to add a dependance on chrome_main.h.  Thoughts?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:18: {params, numParams}
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Style nit: num_params

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
File mojo/monacl/gen/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:20: class ScopedCopyLock {
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> How about putting all these definitions into a normal .h file instead of a
> template file?

Moved out templates and inline functions.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:22: explicit ScopedCopyLock(struct
NaClApp *nap) : nap_(nap) {
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Use "* " spacing in Chromium code.  Same elsewhere in the change.

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:34: uintptr_t uaddr,
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Nit: use uint32_t (as you do below)

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:37: // TODO overflow checking
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> Nit: "TODO(ncbray)"

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:42: template <typename T> bool
ConvertScalarInput(
On 2014/08/26 16:25:52, Mark Seaborn wrote:
> "Convert" is vague.  How about ReadScalarInput or CopyInScalarInput?

Although correct for this function, all the "convert" functions are similar and
not all of them read.  See the generated file.  If I am going to rename one of
them, I'd like some sort of consistent scheme.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:47: uintptr_t temp =
NaClUserToSysAddrRange(nap, user_ptr, sizeof(T));
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Nit: 'temp' -> 'sys_addr'

Breaks naming used in related functions.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:56: template <typename T> bool
ConvertScalarOutput(
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> This looks like it would be like ConvertScalarInput but operate in the reverse
> direction.  But that's not the case.
> 
> Maybe rename to "ScalarOutputUserToSys" or "GetScalarOutputSysAddr"?

Part of larger naming discussion.  (See above)

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/mojo_sy...
mojo/monacl/gen/mojo_syscall.cc.template:67: return false;
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Security bug: this leaves *sys_ptr undefined, so the caller will write to an
> undefined trusted address.

The caller will always early out.  Added paranoid null pointer assignment, just
in case.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
File mojo/monacl/monacl_sel_main.cc (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:23: static int MoNaClLoadApp(
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Nit: Chromium style is to use an anon namespace instead of static.

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:32:
NaClErrorLogHookInit(NaClBootstrapChannelErrorReporter, nap);
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> You don't need to reference the IMC bootstrap channel.

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:36: if (skip_qualification) {
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Always 0, so remove this branch.

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:40: // HACK no fault injection.
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Remove "HACK" comment

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:125: // TODO free nap?
On 2014/08/26 16:25:54, Mark Seaborn wrote:
> Not if you're exiting.  You can remove this comment.

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:127: NaClAllModulesFini();
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Not safe because threads can be running.

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:128: return ret_code;
On 2014/08/26 16:25:53, Mark Seaborn wrote:
> Calling exit() is not safe because it does some teardown that can breaking
> running threads.  (I've seen crashes happen as a result.)  Use NaClExit()
> instead.

Done.

https://codereview.chromium.org/385983008/diff/220001/mojo/public/cpp/system/...
File mojo/public/cpp/system/handle.h (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/public/cpp/system/...
mojo/public/cpp/system/handle.h:242: // Passing the operands by reference
confuses the STL implementation in NaCl's
On 2014/08/20 19:52:38, viettrungluu wrote:
> Probably we can just make it take |Handle|s by value permanently.
> 
> We already mostly pass |Handle|s by value[*], and where we don't it's mostly
out
> of habit (or misguided sense of consistency, leading to even more
inconsistency
> of course).
> 
> (If we do that, we should probably make |Wait()|, above, just take a |Handle|
> instead of a |const Handle&|.)
> 
> [*] This should never be less efficient, and sometimes potentially more
> efficient.

Done.  The only quirk would be if Handle got a destructor.

https://codereview.chromium.org/385983008/diff/220001/mojo/public/cpp/system/...
mojo/public/cpp/system/handle.h:243: // GCC toolchain and results in specialized
templates that break strict aliasing
On 2014/08/26 16:25:54, Mark Seaborn wrote:
> I don't think we build with -fstrict-aliasing though, do we?  What's the
problem
> you see?  A warning or something else?
> 
> Is there a GCC/libstdc++ issue filed about this?

No bug filed because NaCl GCC is old and awful, and the only practical fix is to
make it new and less awful.

For posterity:
../mojo/public/cpp/system/handle.h: In member function ‘virtual void
mojo::<unnamed>::CoreCppTest_Basic_Test::TestBody()’:
../mojo/public/cpp/system/handle.h:241: error: dereferencing pointer
‘<anonymous>’ does break strict-aliasing rules [-Wstrict-aliasing]
/Users/ncbray/repos/gitchrome/src/out/Debug/gen/sdk/mac_x86/nacl_x86_newlib/bin/../lib/gcc/x86_64-nacl/4.4.3/../../../../x86_64-nacl/include/c++/4.4.3/bits/stl_tree.h:175:
note: initialized from here

I get this with a vanilla build_nexe.py build.  I believe -Wall -Werror is the
reason it happens.

[Nick Bray (chromium), 2014-09-03 23:46:03]

https://codereview.chromium.org/385983008/diff/220001/mojo/mojo_srcs.gypi
File mojo/mojo_srcs.gypi (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/mojo_srcs.gypi#new...
mojo/mojo_srcs.gypi:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2014/08/20 19:52:38, viettrungluu wrote:
> I wonder if, instead of adding another .gypi file, you shouldn't reuse
> mojo_variables.gypi (which I believe was added after you started this change),
> updating the comment at the top.
> 
> (At the very least, this file could use a comment indicating what it's for and
> who uses it....)

Missed this one, looping back and doing it.

[Nick Bray (chromium), 2014-09-04 19:19:18]

OK, I think all the changes are in place, modulo:

1) Serializing through a structure.
2) Changing the "Convert*" names.

I think 1) isn't entirely a clean tradeoff, a design change, and not strictly
necessary, so I'd prefer to punt.
For 2), I agree that the Convert* names could be better, but I can't think of a
better, consistent naming scheme off the top of my head.  Suggestions for a
naming scheme welcome, but it shouldn't impact the larger review.

[Mark Seaborn, 2014-09-09 19:13:12]

It's getting there.  Can you fill out the commit message to describe the change,
and link to a tracking issue in BUG=?

Also run trybots -- I believe that would catch the missing header file below.

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall.cc
File mojo/monacl/mojo_syscall.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall...
mojo/monacl/mojo_syscall.cc:62: uintptr_t temp = NaClUserToSysAddrRange(nap,
params[1], sizeof(*options));
On 2014/07/21 23:25:44, Mark Seaborn wrote:
> This does raise the question of why you're calling NaClUserToSysAddrRange()
> once for each parameter.  Why don't you pass the parameters in a struct
> instead?

Can you add a TODO for passing the parameters in a struct instead?

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
File mojo/monacl/gen/libmojo.cc.template (right):

https://codereview.chromium.org/385983008/diff/220001/mojo/monacl/gen/libmojo...
mojo/monacl/gen/libmojo.cc.template:14: #define NACL_MOJO_DESC 8
On 2014/09/03 23:45:02, Nick Bray (chromium) wrote:
> On 2014/08/26 16:25:52, Mark Seaborn wrote:
> > No need to hard-code this.  Use NACL_CHROME_DESC_BASE instead.
> 
> Done, but it doesn't seem right to add a dependance on chrome_main.h. 
Thoughts?

It seems right to me.

https://codereview.chromium.org/385983008/diff/260001/build/all.gyp
File build/all.gyp (left):

https://codereview.chromium.org/385983008/diff/260001/build/all.gyp#oldcode417
build/all.gyp:417: '../testing/gtest_nacl.gyp:*',
Why are you removing this?  Shouldn't the new target be added alongside it?

If you really intend to do this, please comment in the commit message.

https://codereview.chromium.org/385983008/diff/260001/mojo/mojo_variables.gypi
File mojo/mojo_variables.gypi (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/mojo_variables.gyp...
mojo/mojo_variables.gypi:45: 'public/c/system/tests/core_unittest.cc',
Nit: indent by 2 from the parent level, not 4

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl.gyp
File mojo/monacl.gyp (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl.gyp#newcode87
mojo/monacl.gyp:87: '<(DEPTH)/native_client/src/untrusted/nacl/imc_sendmsg.c',
Shouldn't be needed, since you use imc_syscalls_lib below.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl.gyp#newcode100
mojo/monacl.gyp:100: '-pthread',
Don't you also need to add libpthread to 'dependencies'?

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README
File mojo/monacl/README (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:2: currently insecure, does not provide a stable ABI, and
does not
Can you say what you mean by insecure, so people know what to review/fix in
future?

Known issues:

 * No use of NaClVmIoWillStart()/NaClVmIoHasEnded() when untrusted buffers are
passed to Mojo code (e.g. for MojoWriteMessage()).

 * No separation between trusted & untrusted Mojo handles.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:3: support Mojo functions that return pointers.
...for example, MojoMapBuffer()?  (If I understand this correctly.)

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:5: gen/interface.py contains a programmatic description of
the stable
It's now generator/interface.py.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:7: changes.  Run gen/generate_monacl_bindings.py to generate
the bindings
"generator"

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/N...
File mojo/monacl/generator/NOT_FOR_COMMIT/mojo_syscall.cc (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/N...
mojo/monacl/generator/NOT_FOR_COMMIT/mojo_syscall.cc:12: #include
"mojo/monacl/mojo_syscall_internal.h"
This header seems to be missing from your changeset.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/g...
File mojo/monacl/generator/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:18: self.lines = []
Can these be made private?

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:61: # Wraps comma seperated
lists as needed.
"separated"

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
File mojo/monacl/generator/interface.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface.py:45: # TODO this output is a little weird.
Can you elaborate?  What does this mean?

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
File mojo/monacl/generator/interface_dsl.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:9: def func(self, name, returnType):
"return_type"

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:25: self.paramLUT = {}
How about "paramsByName"?

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:28: def param(self, name,
paramType=None):
"param_type"

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:35: return [param.paramType + " "+
param.name for param in self.params]
Add space before "+"

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:39: return ", ".join(self.paramList())
Nit: Google Python style is usually '' rather than "".  Your other file uses ''.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:67: def inp(self, t):
't' -> 'ty' for readability?

'inp' -> 'input', or add a comment?

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/l...
File mojo/monacl/generator/libmojo.cc.template (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/l...
mojo/monacl/generator/libmojo.cc.template:7: #include <errno.h>
I think this isn't used.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
File mojo/monacl/generator/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:20:
UNREFERENCED_PARAMETER(handle);
Not needed in Chromium-side code?  Chromium doesn't enable this warning.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:30: if (msg->iov_length != 1 ||
msg->iov[0].length < 8 || msg->ndesc_length != 0) {
Line is >80 chars

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:35: uint32_t numParams =
msg->iov[0].length / sizeof(*params);
"num_params"

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:37: uint32_t msgType = params[0];
"msg_type"

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:48: struct NaClImcTypedMsgHdr*
msg,
Fix indentation

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:67: #define NACL_MOJO_DESC
(NACL_CHROME_DESC_BASE + 2)
Nit: should this go in a header?  It's duplicated in libmojo.cc.template.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/mojo_syscall.h
File mojo/monacl/mojo_syscall.h (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/mojo_syscal...
mojo/monacl/mojo_syscall.h:8: void InjectMojo(struct NaClApp *nap);
"* " spacing

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
File mojo/monacl/monacl_sel_main.cc (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:23: perror(irt_file);
perror isn't appropriate because NaClDescIoDescOpen() doesn't set errno.

CHECK would be more appropriate.  I think NaClDescIoDescOpen() would only fail
on out-of-memory.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:47: // TODO(ncbray): graceful cleanup?
No TODO needed.  NaClExit() is the preferred way to exit.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
File mojo/monacl/monacl_sel_main.h (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.h:8: #include <stdint.h>
Nit: not used below

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.h:10: void LaunchNaCl(const char *nexe_file, const
char *irt_file,
Nit: should this be namespaced?  In a "mojo" namespace perhaps?

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.h:11: int app_argc, char *app_argv[]);
"* " spacing

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
File mojo/monacl/monacl_shell.cc (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
mojo/monacl/monacl_shell.cc:12: int main (int argc, char* argv[]) {
Nit: remove space before "("

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
mojo/monacl/monacl_shell.cc:14: std::cout << "Usage: " << argv[0] << " irt app
[args for app]" << std::endl;
Nit: maybe "irt.nexe app.nexe" to disambiguate?  Otherwise it looks like "irt
app" is 1 argument.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
mojo/monacl/monacl_shell.cc:19: const char* irt_file = argv[1];
Nit: put before nexe_file to keep the args ordered?

[viettrungluu, 2014-09-09 22:02:48]

https://codereview.chromium.org/385983008/diff/260001/mojo/mojo_variables.gypi
File mojo/mojo_variables.gypi (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/mojo_variables.gyp...
mojo/mojo_variables.gypi:6: # A set of GYP variables that are shared between
mojo.gyp and mojo_base.gyp
nit: Probably update this to "... between various mojo .gyp files." or something
like that.

[Nick Bray (chromium), 2014-09-09 23:12:34]

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall.cc
File mojo/monacl/mojo_syscall.cc (right):

https://codereview.chromium.org/385983008/diff/40001/mojo/monacl/mojo_syscall...
mojo/monacl/mojo_syscall.cc:62: uintptr_t temp = NaClUserToSysAddrRange(nap,
params[1], sizeof(*options));
On 2014/09/09 19:13:10, Mark Seaborn wrote:
> On 2014/07/21 23:25:44, Mark Seaborn wrote:
> > This does raise the question of why you're calling NaClUserToSysAddrRange()
> > once for each parameter.  Why don't you pass the parameters in a struct
> > instead?
> 
> Can you add a TODO for passing the parameters in a struct instead?

This transcends the scope of a TODO.  The issue tracker, if anything.

https://codereview.chromium.org/385983008/diff/260001/build/all.gyp
File build/all.gyp (left):

https://codereview.chromium.org/385983008/diff/260001/build/all.gyp#oldcode417
build/all.gyp:417: '../testing/gtest_nacl.gyp:*',
On 2014/09/09 19:13:10, Mark Seaborn wrote:
> Why are you removing this?  Shouldn't the new target be added alongside it?
> 
> If you really intend to do this, please comment in the commit message.

Done.  I was misinterpreting the intent of this file.  (I thought that only leaf
build targets were included, but it looks like the normal gtest gyp file is
included in here, too.)

https://codereview.chromium.org/385983008/diff/260001/mojo/mojo_variables.gypi
File mojo/mojo_variables.gypi (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/mojo_variables.gyp...
mojo/mojo_variables.gypi:6: # A set of GYP variables that are shared between
mojo.gyp and mojo_base.gyp
On 2014/09/09 22:02:48, viettrungluu wrote:
> nit: Probably update this to "... between various mojo .gyp files." or
something
> like that.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/mojo_variables.gyp...
mojo/mojo_variables.gypi:45: 'public/c/system/tests/core_unittest.cc',
On 2014/09/09 19:13:10, Mark Seaborn wrote:
> Nit: indent by 2 from the parent level, not 4

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl.gyp
File mojo/monacl.gyp (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl.gyp#newcode87
mojo/monacl.gyp:87: '<(DEPTH)/native_client/src/untrusted/nacl/imc_sendmsg.c',
On 2014/09/09 19:13:10, Mark Seaborn wrote:
> Shouldn't be needed, since you use imc_syscalls_lib below.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl.gyp#newcode100
mojo/monacl.gyp:100: '-pthread',
On 2014/09/09 19:13:10, Mark Seaborn wrote:
> Don't you also need to add libpthread to 'dependencies'?

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README
File mojo/monacl/README (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:2: currently insecure, does not provide a stable ABI, and
does not
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Can you say what you mean by insecure, so people know what to review/fix in
> future?
> 
> Known issues:
> 
>  * No use of NaClVmIoWillStart()/NaClVmIoHasEnded() when untrusted buffers are
> passed to Mojo code (e.g. for MojoWriteMessage()).
> 
>  * No separation between trusted & untrusted Mojo handles.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:3: support Mojo functions that return pointers.
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> ...for example, MojoMapBuffer()?  (If I understand this correctly.)

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:5: gen/interface.py contains a programmatic description of
the stable
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> It's now generator/interface.py.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/README#newc...
mojo/monacl/README:7: changes.  Run gen/generate_monacl_bindings.py to generate
the bindings
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> "generator"

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/N...
File mojo/monacl/generator/NOT_FOR_COMMIT/mojo_syscall.cc (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/N...
mojo/monacl/generator/NOT_FOR_COMMIT/mojo_syscall.cc:12: #include
"mojo/monacl/mojo_syscall_internal.h"
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> This header seems to be missing from your changeset.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/g...
File mojo/monacl/generator/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:18: self.lines = []
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Can these be made private?

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:61: # Wraps comma seperated
lists as needed.
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> "separated"

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
File mojo/monacl/generator/interface.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface.py:45: # TODO this output is a little weird.
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Can you elaborate?  What does this mean?

Better off removed.  It was a note for myself, but it boils down to the "returns
a pointer" case.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
File mojo/monacl/generator/interface_dsl.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:9: def func(self, name, returnType):
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> "return_type"

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:25: self.paramLUT = {}
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> How about "paramsByName"?

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:28: def param(self, name,
paramType=None):
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> "param_type"

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:35: return [param.paramType + " "+
param.name for param in self.params]
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Add space before "+"

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:39: return ", ".join(self.paramList())
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Nit: Google Python style is usually '' rather than "".  Your other file uses
''.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:67: def inp(self, t):
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> 't' -> 'ty' for readability?
> 
> 'inp' -> 'input', or add a comment?

"input" is a builtin and "in" is a keyword.  Otherwise done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/l...
File mojo/monacl/generator/libmojo.cc.template (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/l...
mojo/monacl/generator/libmojo.cc.template:7: #include <errno.h>
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> I think this isn't used.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
File mojo/monacl/generator/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:20:
UNREFERENCED_PARAMETER(handle);
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> Not needed in Chromium-side code?  Chromium doesn't enable this warning.

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:30: if (msg->iov_length != 1 ||
msg->iov[0].length < 8 || msg->ndesc_length != 0) {
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Line is >80 chars

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:35: uint32_t numParams =
msg->iov[0].length / sizeof(*params);
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> "num_params"

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:37: uint32_t msgType = params[0];
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> "msg_type"

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:48: struct NaClImcTypedMsgHdr*
msg,
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Fix indentation

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:67: #define NACL_MOJO_DESC
(NACL_CHROME_DESC_BASE + 2)
On 2014/09/09 19:13:11, Mark Seaborn wrote:
> Nit: should this go in a header?  It's duplicated in libmojo.cc.template.

Should it?  Yes.  It would be the only thing in a header shared by two files,
however, which seemed borderline.  So I was lazy.  I can add the header if you
wish, I don't feel strongly either way.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/mojo_syscall.h
File mojo/monacl/mojo_syscall.h (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/mojo_syscal...
mojo/monacl/mojo_syscall.h:8: void InjectMojo(struct NaClApp *nap);
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> "* " spacing

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
File mojo/monacl/monacl_sel_main.cc (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:23: perror(irt_file);
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> perror isn't appropriate because NaClDescIoDescOpen() doesn't set errno.
> 
> CHECK would be more appropriate.  I think NaClDescIoDescOpen() would only fail
> on out-of-memory.

It will fail if the file does not exist.  I cargoculted the code (sel_ldr?), and
then got curious about what it did.  It's actually useful.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.cc:47: // TODO(ncbray): graceful cleanup?
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> No TODO needed.  NaClExit() is the preferred way to exit.

"Graceful cleanup" == shutdown NaCl without torching the entire process.  This
is a larger issue, so removed the TODO here.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
File mojo/monacl/monacl_sel_main.h (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.h:8: #include <stdint.h>
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> Nit: not used below

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.h:10: void LaunchNaCl(const char *nexe_file, const
char *irt_file,
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> Nit: should this be namespaced?  In a "mojo" namespace perhaps?

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_sel_...
mojo/monacl/monacl_sel_main.h:11: int app_argc, char *app_argv[]);
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> "* " spacing

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
File mojo/monacl/monacl_shell.cc (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
mojo/monacl/monacl_shell.cc:12: int main (int argc, char* argv[]) {
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> Nit: remove space before "("

Done.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
mojo/monacl/monacl_shell.cc:14: std::cout << "Usage: " << argv[0] << " irt app
[args for app]" << std::endl;
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> Nit: maybe "irt.nexe app.nexe" to disambiguate?  Otherwise it looks like "irt
> app" is 1 argument.

Done, although it's unclear the best way to line wrap IO streams.

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/monacl_shel...
mojo/monacl/monacl_shell.cc:19: const char* irt_file = argv[1];
On 2014/09/09 19:13:12, Mark Seaborn wrote:
> Nit: put before nexe_file to keep the args ordered?

Done.

[Mark Seaborn, 2014-09-10 23:19:21]

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
File mojo/monacl/generator/interface_dsl.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:67: def inp(self, t):
On 2014/09/09 23:12:33, Nick Bray (chromium) wrote:
> On 2014/09/09 19:13:11, Mark Seaborn wrote:
> > 't' -> 'ty' for readability?
> > 
> > 'inp' -> 'input', or add a comment?
> 
> "input" is a builtin and "in" is a keyword.  Otherwise done.

Hmm, if you name it "Input" (for the other naming style comment), it doesn't
clash with a builtin.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/README
File mojo/monacl/README (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/README#newc...
mojo/monacl/README:5: 
On naming: "monacl" is cutesy, but it's not necessarily obvious that "monacl" is
just "mojo + nacl".

Wouldn't "mojo/nacl" be more obvious, and more consistent with other directories
like "components/nacl"?

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/README#newc...
mojo/monacl/README:23: Data races.
What do you mean by "data races"?  Does this mean cases where trusted code
accesses untrusted address space without proper atomic/volatile qualifiers?

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
File mojo/monacl/generator/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:25: def pushMargin(self):
I realised that a lot of these method names are not following the style guide
(http://www.chromium.org/developers/coding-style).

So this should be "PushMargin".  Can you do a bulk fix-up?

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:42: self.writer = writer
This field can be private

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:88: numParams = len(f.params)
+ 2
"num_params"

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:92: returnType =
f.resultParam.baseType
"return_type"

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:104: castTemplate =
'params[%d] = reinterpret_cast<uint32_t>(%s);'
"cast_template"

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:109: code <<  castTemplate %
(p.uid + 1, ptr)
Nit: remove one space after "<<"

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:308: # TODO copy out on
failure?
Can you elaborate or remove this?

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
File mojo/monacl/generator/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:27: if (msg->iov_length != 1 ||
msg->iov[0].length < 8 ||
Minimum should be 4, not 8, surely?  This could move to after num_params is
calculated...

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:32: uint32_t* params =
static_cast<uint32_t*>(msg->iov[0].base);
This pointer should be volatile too, since it is reading from untrusted address
space.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:33: size_t num_params =
msg->iov[0].length / sizeof(*params);
After:
if (num_params < 1)
  return -1;

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/mojo_syscal...
File mojo/monacl/mojo_syscall_internal.h (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/mojo_syscal...
mojo/monacl/mojo_syscall_internal.h:25: static INLINE uintptr_t
NaClUserToSysAddrArray(
"inline" (lower case), since this is C++.

[Nick Bray (chromium), 2014-09-11 00:56:41]

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
File mojo/monacl/generator/interface_dsl.py (right):

https://codereview.chromium.org/385983008/diff/260001/mojo/monacl/generator/i...
mojo/monacl/generator/interface_dsl.py:67: def inp(self, t):
On 2014/09/10 23:19:19, Mark Seaborn wrote:
> On 2014/09/09 23:12:33, Nick Bray (chromium) wrote:
> > On 2014/09/09 19:13:11, Mark Seaborn wrote:
> > > 't' -> 'ty' for readability?
> > > 
> > > 'inp' -> 'input', or add a comment?
> > 
> > "input" is a builtin and "in" is a keyword.  Otherwise done.
> 
> Hmm, if you name it "Input" (for the other naming style comment), it doesn't
> clash with a builtin.

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/README
File mojo/monacl/README (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/README#newc...
mojo/monacl/README:5: 
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> On naming: "monacl" is cutesy, but it's not necessarily obvious that "monacl"
is
> just "mojo + nacl".
> 
> Wouldn't "mojo/nacl" be more obvious, and more consistent with other
directories
> like "components/nacl"?

Bulk moved.  Some instances of "monacl" not changed due to lack of better name.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/README#newc...
mojo/monacl/README:23: Data races.
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> What do you mean by "data races"?  Does this mean cases where trusted code
> accesses untrusted address space without proper atomic/volatile qualifiers?

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
File mojo/monacl/generator/generate_monacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:25: def pushMargin(self):
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> I realised that a lot of these method names are not following the style guide
> (http://www.chromium.org/developers/coding-style).
> 
> So this should be "PushMargin".  Can you do a bulk fix-up?

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:42: self.writer = writer
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> This field can be private

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:88: numParams = len(f.params)
+ 2
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> "num_params"

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:92: returnType =
f.resultParam.baseType
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> "return_type"

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:104: castTemplate =
'params[%d] = reinterpret_cast<uint32_t>(%s);'
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> "cast_template"

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:109: code <<  castTemplate %
(p.uid + 1, ptr)
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> Nit: remove one space after "<<"

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/g...
mojo/monacl/generator/generate_monacl_bindings.py:308: # TODO copy out on
failure?
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> Can you elaborate or remove this?

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
File mojo/monacl/generator/mojo_syscall.cc.template (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:27: if (msg->iov_length != 1 ||
msg->iov[0].length < 8 ||
On 2014/09/10 23:19:21, Mark Seaborn wrote:
> Minimum should be 4, not 8, surely?  This could move to after num_params is
> calculated...

Technically correct, (always a return parameter), but simplified for
understandability.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:32: uint32_t* params =
static_cast<uint32_t*>(msg->iov[0].base);
On 2014/09/10 23:19:21, Mark Seaborn wrote:
> This pointer should be volatile too, since it is reading from untrusted
address
> space.

Unclear from the API.

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/generator/m...
mojo/monacl/generator/mojo_syscall.cc.template:33: size_t num_params =
msg->iov[0].length / sizeof(*params);
On 2014/09/10 23:19:20, Mark Seaborn wrote:
> After:
> if (num_params < 1)
>   return -1;

Done.

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/mojo_syscal...
File mojo/monacl/mojo_syscall_internal.h (right):

https://codereview.chromium.org/385983008/diff/360001/mojo/monacl/mojo_syscal...
mojo/monacl/mojo_syscall_internal.h:25: static INLINE uintptr_t
NaClUserToSysAddrArray(
On 2014/09/10 23:19:21, Mark Seaborn wrote:
> "inline" (lower case), since this is C++.

Done.

[darin (slow to review), 2014-09-11 07:32:19]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2014-09-11 07:32:20]

https://codereview.chromium.org/385983008/diff/380001/mojo/nacl/generator/gen...
File mojo/nacl/generator/generate_nacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/380001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:57: template =
string.Template(data)
not that this should hold up your CL, but in the mojo bindings generator code we
use jinja2. had you considered using it as well?

[Nick Bray (chromium), 2014-09-11 18:16:23]

https://codereview.chromium.org/385983008/diff/380001/mojo/nacl/generator/gen...
File mojo/nacl/generator/generate_nacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/380001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:57: template =
string.Template(data)
On 2014/09/11 07:32:20, darin wrote:
> not that this should hold up your CL, but in the mojo bindings generator code
we
> use jinja2. had you considered using it as well?

I was unaware jinja2 was being used.  I did a simple/superficial change to start
using it.  Down the road I'll try to move more of the codegen into the template
system, where possible.

[Mark Seaborn, 2014-09-11 19:24:02]

LGTM

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
File mojo/nacl/generator/generate_nacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:31:
sys.path.append(os.path.join(_GetDirAbove("mojo"), "third_party"))
Use '' quotes

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:94: 
Can you consistently put 2 empty lines between top-level decls, please?

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:106: for line in Wrap('%s %s(' %
(f.returnType, f.name), f.ParamList(), '){'):
Nit: add space in ') {'

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:108: num_params = len(f.params) +
2
Add a comment: Add 2 for the operation ID and return value.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:146: # Parameters passed into
trusted code are handled differently depending on
Can you make this more explicit by starting with: "ParamImpl instances are used
for generating trusted code."

This helps me know where to look for who calls methods of ParamImpl.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:148: class ParamImpl(object):
For each overridden method, can you add something like:

  # Returns C++ code for reading in and validating a parameter.
  def ConvertParam(self):
    raise NotImplemented()

This makes it clearer what's supposed to be overridden.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:224: return ('ConvertBytes(nap,
params[%d], %s, %s, &%s)' %
Why have separate ConvertBytes/ConvertArray functions?  Can you have just
ConvertArray and pass an element size of 1 when baseType == 'void'?

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:309: # Arrays cannot be validated
util the size parameter has been converted.
"until"

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
File mojo/nacl/generator/interface_dsl.py (right):

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:23: self.returnType = return_type
These don't follow the style guide still.  "return_type" etc.?

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:25: self.paramByName = {}
"param_by_name"

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:45: p.Finalize()
This is a no-op, right?

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:54: self.baseType = param_type
Ditto: "base_type" etc.?

[viettrungluu, 2014-09-11 19:39:40]

mojo/ (outside mojo/nacl/) lgtm

[Nick Bray, 2014-09-11 20:24:04]

ncbray@google.com changed reviewers:
+ ncbray@google.com

[Nick Bray, 2014-09-11 20:24:09]

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
File mojo/nacl/generator/generate_nacl_bindings.py (right):

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:31:
sys.path.append(os.path.join(_GetDirAbove("mojo"), "third_party"))
On 2014/09/11 19:24:01, Mark Seaborn wrote:
> Use '' quotes

Done.  The irony being this was originally copied code.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:94: 
On 2014/09/11 19:24:01, Mark Seaborn wrote:
> Can you consistently put 2 empty lines between top-level decls, please?

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:106: for line in Wrap('%s %s(' %
(f.returnType, f.name), f.ParamList(), '){'):
On 2014/09/11 19:24:01, Mark Seaborn wrote:
> Nit: add space in ') {'

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:108: num_params = len(f.params) +
2
On 2014/09/11 19:24:01, Mark Seaborn wrote:
> Add a comment: Add 2 for the operation ID and return value.

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:146: # Parameters passed into
trusted code are handled differently depending on
On 2014/09/11 19:24:02, Mark Seaborn wrote:
> Can you make this more explicit by starting with: "ParamImpl instances are
used
> for generating trusted code."
> 
> This helps me know where to look for who calls methods of ParamImpl.

Clarified, although these instances will likely crosscut and handle all stages
of parameter transfer.  Right now the untrusted scheme is pretty simple so
ParamImpls were not used, but that will likely change.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:148: class ParamImpl(object):
On 2014/09/11 19:24:01, Mark Seaborn wrote:
> For each overridden method, can you add something like:
> 
>   # Returns C++ code for reading in and validating a parameter.
>   def ConvertParam(self):
>     raise NotImplemented()
> 
> This makes it clearer what's supposed to be overridden.

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:224: return ('ConvertBytes(nap,
params[%d], %s, %s, &%s)' %
On 2014/09/11 19:24:01, Mark Seaborn wrote:
> Why have separate ConvertBytes/ConvertArray functions?  Can you have just
> ConvertArray and pass an element size of 1 when baseType == 'void'?

Done.  I am not convinced adding an extra parameter that can typically be
derived from type information is the best idea, but I can live with it.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/gen...
mojo/nacl/generator/generate_nacl_bindings.py:309: # Arrays cannot be validated
util the size parameter has been converted.
On 2014/09/11 19:24:01, Mark Seaborn wrote:
> "until"

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
File mojo/nacl/generator/interface_dsl.py (right):

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:23: self.returnType = return_type
On 2014/09/11 19:24:02, Mark Seaborn wrote:
> These don't follow the style guide still.  "return_type" etc.?

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:25: self.paramByName = {}
On 2014/09/11 19:24:02, Mark Seaborn wrote:
> "param_by_name"

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:45: p.Finalize()
On 2014/09/11 19:24:02, Mark Seaborn wrote:
> This is a no-op, right?

Done.

https://codereview.chromium.org/385983008/diff/420001/mojo/nacl/generator/int...
mojo/nacl/generator/interface_dsl.py:54: self.baseType = param_type
On 2014/09/11 19:24:02, Mark Seaborn wrote:
> Ditto: "base_type" etc.?

Done.

[Nick Bray (chromium), 2014-09-11 20:26:03]

The CQ bit was checked by ncbray@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:30:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/385983008/440001

[commit-bot: I haz the power, 2014-09-11 23:05:02]

Committed patchset #23 (id:440001) as 37bcbd443b9b5955d23fbe28924a548ba6d7dc6e

[commit-bot: I haz the power, 2014-09-11 23:14:25]

Patchset 23 (id:??) landed as
https://crrev.com/95d2d4025c767fbc5669377aae457a4bfb5ca46b
Cr-Commit-Position: refs/heads/master@{#294484}