A mechanism to identify/forbid/"rewrite" non-temporal instructions (and other)

A mechanism to identify/forbid/"rewrite" non-temporal instructions (and other)

For security reasons, the validator needs to identify certain instructions, then forbid or rewrite them based on further information. In this CL, we target non-temporal instructions, including non-temporal reads, writes, and prefetches.

Specifically:
1) We mark non-temporal instructions in def files with an additional attribute "nacl-unsupported";
2) The validator identifies instructions with "nacl-unsupported" attribute
3) Based on the flags passed in, the validator either forbids or "rewrites" the instruction. Note:
-- the rewriting is to be implemented in a separate CL, and the current behavior is to leave the instruction as it is (i.e., as if it passes validation), which is consistent with the old behavior.
-- the flags are currently derived from the "pnacl_mode" argument, which makes the validator disable non-temporal instructions under PNaCl.

BUG=https://code.google.com/p/chromium/issues/detail?id=500026
R=jfb@chromium.org, mseaborn@chromium.org, phosek@chromium.org

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

[ruiq, 2015-07-19 19:15:39]

ruiq@google.com changed reviewers:
+ bradnelson@chromium.org, gdeepti@chromium.org, mseaborn@chromium.org,
phosek@chromium.org

[ruiq, 2015-07-19 19:20:34]

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/def_format.py (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/def_format.py:294: 'nacl-forbid-or-rewrite',
I feel this name is more explicit than something like nacl-forbidden-optional.
We could change it if something better is proposed.

[Petr Hosek, 2015-07-20 23:58:57]

Could you please generate a new trie and compare it with the one which was
pushed to validator_snapshots to see if there are any differences?

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/b...
File src/trusted/validator/build.scons (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/b...
src/trusted/validator/build.scons:64: 'run_pnacl_mode_test.cc')
Nit: Could you please align this with the `node` as in the rest of this file?

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/v...
File src/trusted/validator/validation_pnacl_mode_test.cc (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/v...
src/trusted/validator/validation_pnacl_mode_test.cc:5: 5  */
Please remove the line numbers.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/v...
src/trusted/validator/validation_pnacl_mode_test.cc:17: class
ValidationPnaclModeTests : public ::testing::Test {
Nit: Please use PNaCl, not Pnacl.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/def_format.py (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/def_format.py:294: 'nacl-forbid-or-rewrite',
On 2015/07/19 19:20:33, ruiq wrote:
> I feel this name is more explicit than something like nacl-forbidden-optional.
> We could change it if something better is proposed.

I'd prefer `nacl-unsupported`; `nacl-forbid-or-rewrite` implies what the
implementation of the callback does which is IMHO too restrictive.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:40: struct
ForbidRewriteCallbackData *data = callback_data;
Shouldn't you doing a cast here? Is this actually compiling?

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:50: /* TODO: rewrite
instruction. For now, we keep the original instruction
Nit: Please include your LDAP in the TODO, i.e. "TODO(ruiq)".

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.h:34: struct
ForbidRewriteCallbackData {
Nit: The name should be prefixed with NaCl.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.h:39: Bool
NaClDfaForbidOrRewriteInstruction(const uint8_t *begin,
There is no need to rename this function if we use the `nacl-unsupported`
attribute (we could possibly drop the CPU part).

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/validator.h (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/validator.h:99: FORBID_OR_REWRITE_INSTRUCTION =
0x04000000,
To be consistent with the annotation, I'd call this `UNSUPPORTED_INSTRUCTION`.

[ruiq, 2015-07-21 00:39:44]

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:40: struct
ForbidRewriteCallbackData *data = callback_data;
On 2015/07/20 23:58:57, Petr Hosek wrote:
> Shouldn't you doing a cast here? Is this actually compiling?

This is valid in C, while not in C++. In C++, an explicit static_cast is needed.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.h:34: struct
ForbidRewriteCallbackData {
On 2015/07/20 23:58:57, Petr Hosek wrote:
> Nit: The name should be prefixed with NaCl.

I followed the naming of 'CodeCopyCallbackData' on line 44. Shall I rename both
with 'NaCl' prefix?

[Petr Hosek, 2015-07-21 00:46:58]

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:40: struct
ForbidRewriteCallbackData *data = callback_data;
On 2015/07/21 00:39:44, ruiq wrote:
> On 2015/07/20 23:58:57, Petr Hosek wrote:
> > Shouldn't you doing a cast here? Is this actually compiling?
> 
> This is valid in C, while not in C++. In C++, an explicit static_cast is
needed.

I'd still include an explicit cast, even if this passes through compiler.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.h:34: struct
ForbidRewriteCallbackData {
On 2015/07/21 00:39:44, ruiq wrote:
> On 2015/07/20 23:58:57, Petr Hosek wrote:
> > Nit: The name should be prefixed with NaCl.
> 
> I followed the naming of 'CodeCopyCallbackData' on line 44. Shall I rename
both
> with 'NaCl' prefix?

In that case no, we should err on the side of consistency. We could rename both
in a separate cleanup CL if needed.

[ruiq, 2015-07-21 02:52:25]

I will generate the new trie in a follow up patch set.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/b...
File src/trusted/validator/build.scons (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/b...
src/trusted/validator/build.scons:64: 'run_pnacl_mode_test.cc')
On 2015/07/20 23:58:57, Petr Hosek wrote:
> Nit: Could you please align this with the `node` as in the rest of this file?

Done.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/v...
File src/trusted/validator/validation_pnacl_mode_test.cc (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/v...
src/trusted/validator/validation_pnacl_mode_test.cc:5: 5  */
On 2015/07/20 23:58:57, Petr Hosek wrote:
> Please remove the line numbers.

Done.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator/v...
src/trusted/validator/validation_pnacl_mode_test.cc:17: class
ValidationPnaclModeTests : public ::testing::Test {
On 2015/07/20 23:58:57, Petr Hosek wrote:
> Nit: Please use PNaCl, not Pnacl.

Done.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/def_format.py (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/def_format.py:294: 'nacl-forbid-or-rewrite',
On 2015/07/20 23:58:57, Petr Hosek wrote:
> On 2015/07/19 19:20:33, ruiq wrote:
> > I feel this name is more explicit than something like
nacl-forbidden-optional.
> > We could change it if something better is proposed.
> 
> I'd prefer `nacl-unsupported`; `nacl-forbid-or-rewrite` implies what the
> implementation of the callback does which is IMHO too restrictive.

Done.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:40: struct
ForbidRewriteCallbackData *data = callback_data;
On 2015/07/21 00:46:58, Petr Hosek wrote:
> On 2015/07/21 00:39:44, ruiq wrote:
> > On 2015/07/20 23:58:57, Petr Hosek wrote:
> > > Shouldn't you doing a cast here? Is this actually compiling?
> > 
> > This is valid in C, while not in C++. In C++, an explicit static_cast is
> needed.
> 
> I'd still include an explicit cast, even if this passes through compiler.

I left this as it is: 1) It is not ambiguous; 2) it is cleaner and easier to
read, and not requiring a new line; 3) it is consistent with line 65.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:50: /* TODO: rewrite
instruction. For now, we keep the original instruction
On 2015/07/20 23:58:57, Petr Hosek wrote:
> Nit: Please include your LDAP in the TODO, i.e. "TODO(ruiq)".

Done.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.h:39: Bool
NaClDfaForbidOrRewriteInstruction(const uint8_t *begin,
On 2015/07/20 23:58:57, Petr Hosek wrote:
> There is no need to rename this function if we use the `nacl-unsupported`
> attribute (we could possibly drop the CPU part).

Done.

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
File src/trusted/validator_ragel/validator.h (right):

https://codereview.chromium.org/1234393005/diff/30001/src/trusted/validator_r...
src/trusted/validator_ragel/validator.h:99: FORBID_OR_REWRITE_INSTRUCTION =
0x04000000,
On 2015/07/20 23:58:57, Petr Hosek wrote:
> To be consistent with the annotation, I'd call this `UNSUPPORTED_INSTRUCTION`.

Done.

[gdeepti, 2015-07-28 03:08:58]

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/driver/ncval.cc (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:216: Bool result = (validation_info &
(VALIDATION_ERRORS_MASK | BAD_JUMP_TARGET))
Why BAD_JUMP_TARGET here?

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/validation_pnacl_mode_test.cc (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/validation_pnacl_mode_test.cc:57: // non-temporal writes.
In future, we will do rewriting.
Add a todo for rewriting.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/validation_pnacl_mode_test.cc:60: EXPECT_EQ(0,
memcmp(code_buffer, movnti_code, MOVNTI_CODE_SIZE));
As we are not rewriting yet, seems redundant to check it here. Especially since
we should be rewriting in non PNaCl mode. Perhaps remove this and add details to
the TODO for the rewriting check that would be needed.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/validator.h (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/validator.h:99: UNSUPPORTED_INSTRUCTION =
0x04000000,
Fix alignment to be consistent with the rest of the mask values.

[ruiq, 2015-07-28 04:58:55]

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/driver/ncval.cc (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:216: Bool result = (validation_info &
(VALIDATION_ERRORS_MASK | BAD_JUMP_TARGET))
On 2015/07/28 03:08:58, gdeepti wrote:
> Why BAD_JUMP_TARGET here? 

This is to be consistent with the handling in ProcessInstruction(), line 105.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/validation_pnacl_mode_test.cc (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/validation_pnacl_mode_test.cc:57: // non-temporal writes.
In future, we will do rewriting.
On 2015/07/28 03:08:57, gdeepti wrote:
> Add a todo for rewriting. 

There is a TODO in the place where we should do rewriting
(src/trusted/validator_ragel/dfa_validate_common.c). My thought was that the
tests are for current code, would it be a bit misleading if we add a TODO which
is essentially "TOTEST"?

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/validation_pnacl_mode_test.cc:60: EXPECT_EQ(0,
memcmp(code_buffer, movnti_code, MOVNTI_CODE_SIZE));
On 2015/07/28 03:08:58, gdeepti wrote:
> As we are not rewriting yet, seems redundant to check it here. Especially
since
> we should be rewriting in non PNaCl mode. Perhaps remove this and add details
to
> the TODO for the rewriting check that would be needed.  

This is a sanity check. It was added because the code is modifiable
(readonly_test = FALSE), and we want to make sure it's not modified in the user
callback (current validation does rewrite some CPU unsupported instructions to
HALT). Would it be better to keep this to capture possible mistakes, as it is
not that expensive?

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/validator.h (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/validator.h:99: UNSUPPORTED_INSTRUCTION =
0x04000000,
On 2015/07/28 03:08:58, gdeepti wrote:
> Fix alignment to be consistent with the rest of the mask values.

Done.

[Petr Hosek, 2015-07-28 19:05:47]

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
File src/trusted/service_runtime/elf_util.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/elf_util.c:651: nap->pnacl_mode,
ditto

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/sel_validate_image.c:76: nap->pnacl_mode,
This will become `nap->pnacl_mode ? DISABLE_UNSUPPORTED : 0`.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/sel_validate_image.c:87: nap->pnacl_mode,
ditto

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
File src/trusted/service_runtime/sys_memory.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/sys_memory.c:732: nap->pnacl_mode,
ditto

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/driver/ncval.cc (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:353: bool pnacl_mode;
Rename to `flags`.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:359: while ((opt = getopt(argc, argv,
"v")) != -1) {
Could you please use `getopt_long` variant and rename the 'p' option to
`--disable-unsupported`?

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:365: options->pnacl_mode = true;
The 'p' option seems to be missing from the optstring.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/ncvalidate.h (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/ncvalidate.h:66: int pnacl_mode,
We've discussed this with bradnelson and gdeepti and agreed that we should avoid
introducing the `pnacl_mode` into validator. Could you please rename this to
`flags` with the type uint32_t and introduce a new enum ValidatorFlags with a
value `DISABLE_UNSUPPORTED` which is going to serve the same purpose as
`pnacl_mode` does throughout the code?

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_32.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_32.c:42: struct StubOutCallbackData
callback_data;
You can use the shorthand form `struct StubOutCallbackData callback_data = {0};`
here and drop the following two lines.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:42: struct StubOutCallbackData
callback_data;
ditto

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:73: NaClDfaProcessValidationError,
Could you please align this line to start on the same column as `data`?

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:78:
NaClDfaStubOutUnsupportedInstruction,
ditto

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.h:35: int pnacl_mode;
Change to `uint32_t flags` please.

[Mark Seaborn, 2015-07-28 20:44:03]

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/driver/ncval.cc (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:359: while ((opt = getopt(argc, argv,
"v")) != -1) {
On 2015/07/28 19:05:46, Petr Hosek wrote:
> Could you please use `getopt_long` variant and rename the 'p' option to
> `--disable-unsupported`?

I think you'll find that getopt_long() is not available on Windows, which
ncval.cc is supposed to work on.  getopt() works because NaCl includes an
implementation of it.

I mention this in the hope that it saves time.  I think you'll need to stick
with getopt().

[ruiq, 2015-07-28 21:34:56]

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
File src/trusted/service_runtime/elf_util.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/elf_util.c:651: nap->pnacl_mode,
On 2015/07/28 19:05:46, Petr Hosek wrote:
> ditto

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/sel_validate_image.c:76: nap->pnacl_mode,
On 2015/07/28 19:05:46, Petr Hosek wrote:
> This will become `nap->pnacl_mode ? DISABLE_UNSUPPORTED : 0`.

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/sel_validate_image.c:87: nap->pnacl_mode,
On 2015/07/28 19:05:46, Petr Hosek wrote:
> ditto

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
File src/trusted/service_runtime/sys_memory.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/service_ru...
src/trusted/service_runtime/sys_memory.c:732: nap->pnacl_mode,
On 2015/07/28 19:05:46, Petr Hosek wrote:
> ditto

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/driver/ncval.cc (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:353: bool pnacl_mode;
On 2015/07/28 19:05:46, Petr Hosek wrote:
> Rename to `flags`.

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/driver/ncval.cc:365: options->pnacl_mode = true;
On 2015/07/28 19:05:46, Petr Hosek wrote:
> The 'p' option seems to be missing from the optstring.

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
File src/trusted/validator/ncvalidate.h (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator/...
src/trusted/validator/ncvalidate.h:66: int pnacl_mode,
On 2015/07/28 19:05:46, Petr Hosek wrote:
> We've discussed this with bradnelson and gdeepti and agreed that we should
avoid
> introducing the `pnacl_mode` into validator. Could you please rename this to
> `flags` with the type uint32_t and introduce a new enum ValidatorFlags with a
> value `DISABLE_UNSUPPORTED` which is going to serve the same purpose as
> `pnacl_mode` does throughout the code?

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_32.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_32.c:42: struct StubOutCallbackData
callback_data;
On 2015/07/28 19:05:46, Petr Hosek wrote:
> You can use the shorthand form `struct StubOutCallbackData callback_data =
{0};`
> here and drop the following two lines.

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:42: struct StubOutCallbackData
callback_data;
On 2015/07/28 19:05:46, Petr Hosek wrote:
> ditto

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.h:35: int pnacl_mode;
On 2015/07/28 19:05:46, Petr Hosek wrote:
> Change to `uint32_t flags` please.

Done.

[ruiq, 2015-07-28 22:00:31]

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:73: NaClDfaProcessValidationError,
On 2015/07/28 19:05:46, Petr Hosek wrote:
> Could you please align this line to start on the same column as `data`?

Done.

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:78:
NaClDfaStubOutUnsupportedInstruction,
On 2015/07/28 19:05:46, Petr Hosek wrote:
> ditto

Done.

[ruiq, 2015-07-28 22:15:59]

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_32.c (right):

https://codereview.chromium.org/1234393005/diff/150001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_32.c:42: struct StubOutCallbackData
callback_data;
On 2015/07/28 19:05:46, Petr Hosek wrote:
> You can use the shorthand form `struct StubOutCallbackData callback_data =
{0};`
> here and drop the following two lines.

The gcc on the trybots seemed to be pedantic about not initializing all fields
and produced errors. I think the original code would be more readable than
something like: `struct StubOutCallbackData callback_data = {NORMAL_VALIDATION,
0}; callback_data.flags = flags;`. So I changed back.

[Petr Hosek, 2015-07-28 22:24:38]

Except for the two changes mentioned, I'm happy with the change. It's fine to
stick to the short option and explicit initialization to make the compilers
happy on all platforms.

https://codereview.chromium.org/1234393005/diff/190001/src/trusted/validator/...
File src/trusted/validator/ncvalidate.h (right):

https://codereview.chromium.org/1234393005/diff/190001/src/trusted/validator/...
src/trusted/validator/ncvalidate.h:25: NORMAL_VALIDATION,
I'd drop this and just use `0` as a default value for `flags`.

https://codereview.chromium.org/1234393005/diff/190001/src/trusted/validator/...
src/trusted/validator/ncvalidate.h:26: DISABLE_UNSUPPORTED
Could you please set the value for this as `0x1`?

[ruiq, 2015-07-28 22:45:36]

https://codereview.chromium.org/1234393005/diff/190001/src/trusted/validator/...
File src/trusted/validator/ncvalidate.h (right):

https://codereview.chromium.org/1234393005/diff/190001/src/trusted/validator/...
src/trusted/validator/ncvalidate.h:25: NORMAL_VALIDATION,
On 2015/07/28 22:24:38, Petr Hosek wrote:
> I'd drop this and just use `0` as a default value for `flags`.

Done.

https://codereview.chromium.org/1234393005/diff/190001/src/trusted/validator/...
src/trusted/validator/ncvalidate.h:26: DISABLE_UNSUPPORTED
On 2015/07/28 22:24:38, Petr Hosek wrote:
> Could you please set the value for this as `0x1`?

Done.

[Petr Hosek, 2015-07-28 22:52:45]

lgtm

[ruiq, 2015-07-28 23:13:19]

ruiq@google.com changed reviewers:
+ jfb@chromium.org

[ruiq, 2015-07-28 23:13:20]

Adding JF as reviewer for ARM change.

[JF, 2015-07-29 00:08:02]

https://codereview.chromium.org/1234393005/diff/200001/src/trusted/validator_...
File src/trusted/validator_arm/ncvalidate.cc (right):

https://codereview.chromium.org/1234393005/diff/200001/src/trusted/validator_...
src/trusted/validator_arm/ncvalidate.cc:153: UNREFERENCED_PARAMETER(flags);
I'd rather CHECK(flags == 0);

[ruiq, 2015-07-29 00:14:17]

https://codereview.chromium.org/1234393005/diff/200001/src/trusted/validator_...
File src/trusted/validator_arm/ncvalidate.cc (right):

https://codereview.chromium.org/1234393005/diff/200001/src/trusted/validator_...
src/trusted/validator_arm/ncvalidate.cc:153: UNREFERENCED_PARAMETER(flags);
On 2015/07/29 00:08:02, JF wrote:
> I'd rather CHECK(flags == 0);

Done.

[JF, 2015-07-29 00:25:42]

lgtm

[Mark Seaborn, 2015-07-29 00:31:39]

https://codereview.chromium.org/1234393005/diff/200001/src/trusted/validator_...
File src/trusted/validator_arm/ncvalidate.cc (right):

https://codereview.chromium.org/1234393005/diff/200001/src/trusted/validator_...
src/trusted/validator_arm/ncvalidate.cc:153: UNREFERENCED_PARAMETER(flags);
On 2015/07/29 00:08:02, JF wrote:
> I'd rather CHECK(flags == 0);

Won't that fail when running PNaCl apps inside Chrome on ARM?  The code is
currently passing non-zero flags for all architectures when running PNaCl.

[Petr Hosek, 2015-07-29 05:21:36]

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator/...
File src/trusted/validator/validation_disable_nontemporals_test.cc (right):

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator/...
src/trusted/validator/validation_disable_nontemporals_test.cc:2: * Copyright (c)
2015 The Native Client Authors. All rights reserved.
Nit: drop the "(c)".

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:47: if (data->flags ==
DISABLE_NONTEMPORALS)
Nit: Could you put {} around the block?

[ruiq, 2015-07-29 05:44:17]

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator/...
File src/trusted/validator/validation_disable_nontemporals_test.cc (right):

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator/...
src/trusted/validator/validation_disable_nontemporals_test.cc:2: * Copyright (c)
2015 The Native Client Authors. All rights reserved.
On 2015/07/29 05:21:35, Petr Hosek wrote:
> Nit: drop the "(c)".

Done.

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1234393005/diff/230001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:47: if (data->flags ==
DISABLE_NONTEMPORALS)
On 2015/07/29 05:21:36, Petr Hosek wrote:
> Nit: Could you put {} around the block?

Done.

[JF, 2015-07-29 15:57:18]

lgtm

[Mark Seaborn, 2015-07-29 16:09:23]

These comments could be addressed in a follow-on change (to avoid holding up
this change):

https://codereview.chromium.org/1234393005/diff/240001/src/trusted/validator/...
File src/trusted/validator/ncvalidate.h (right):

https://codereview.chromium.org/1234393005/diff/240001/src/trusted/validator/...
src/trusted/validator/ncvalidate.h:26: X86_VALIDATION_FLAGS_MASK = 0x1,
Nit: these should really have a "NACL_" prefix to namespace them

You might also consider reversing the order, i.e.
NACL_VALIDATION_FLAGS_MASK_X86.

https://codereview.chromium.org/1234393005/diff/240001/src/trusted/validator_...
File src/trusted/validator_arm/ncvalidate.cc (right):

https://codereview.chromium.org/1234393005/diff/240001/src/trusted/validator_...
src/trusted/validator_arm/ncvalidate.cc:153: CHECK((flags &
ARM_VALIDATION_FLAGS_MASK) == 0);
What's the purpose of this, given that ARM_VALIDATION_FLAGS_MASK is 0?

Are you using X86_VALIDATION_FLAGS_MASK?  I didn't see a use of it (though it's
hard to search because Rietveld won't let me download the patch).

https://codereview.chromium.org/1234393005/diff/240001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1234393005/diff/240001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:47: if (data->flags ==
DISABLE_NONTEMPORALS) {
Shouldn't this be "(data->flags & DISABLE_NONTEMPORALS) != 0"?

[Mark Seaborn, 2015-07-29 16:24:44]

Rubber-stamp LGTM for src/trusted/ (in order to cover
src/trusted/validator_mips/)

[ruiq, 2015-07-29 16:52:46]

Committed patchset #25 (id:240001) manually as
f14a2194a8dd85de9304c94d8563e24b7cd45bff (presubmit successful).