Intial Thumb2 Sandbox (naclrev 6680)

Intial Thumb2 Sandbox (naclrev 6583) by MMaurer


BUG=http://code.google.com/p/nativeclient/issues/detail?id=1713
BUG=http://code.google.com/p/nativeclient/issues/detail?id=2193

TEST=none

[jasonwkim, 2011-08-30 00:42:55]

PTAL

This is the newly updated issue: This CL supercedes ALL PRIOR EMAIL.

Thanks

[Karl, 2011-08-30 19:53:52]

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:136: if env.Bit('build_arm_thumb2') or
env.Bit('target_arm_thumb2'):
Why is this needed for the build environment? I understand the need for the
target, but do not understand the build.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_validate_image.c:31: #if
NACL_ARCH(NACL_BUILD_ARCH) == NACL_arm
Again, I think this code is confusing build with target.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_validate_image.c:47: , thumb
Why is "thumb" needed. Isn't it already communicated with the
NACL_TARGET_SUBARCH?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_validate_image.c:59: , thumb
Same question as above.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
File src/trusted/validator_arm/address_set.cc (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:16: AddressSet::AddressSet(uint32_t
base, uint32_t size)
There appears to be inconsistencies between sizes. The argument base is 32-bits,
but ADDRFACTOR is based on 16?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:17: : base_(base), size_(size),
bits_(new uint32_t[(size + 3) / ADDRFACTOR]) {
Isn't 3 based on ADDRFACTOR?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:29: bits_[word_address / 32] |= 1 <<
(word_address % 32);
Doesn't 32 conflict with size of ADDRFACTOR?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:37: return bits_[word_address / 32] &
(1 << (word_address % 32));
Again, is 32 consistent with ADDRFACTOR?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:48: return Iterator(*this, (size_ + 3)
/ 4, 0);
Is 4 supposed to be ADDRFACTOR (to match size allocated)?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:76: uint32_t word = (shift_ > 31)? 0 :
parent_.bits_[index_] >> shift_;
Does this conflict with ADDRFACTOR value?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
File src/trusted/validator_arm/inst_classes.cc (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
src/trusted/validator_arm/inst_classes.cc:501: | i.bits(5, 0)) << 11) |
i.bits(26, 16)) << 1;
My head huts trying to figure out what this statement does! Maybe some
documentation on what it does (other than the useless comment "construct the
value").

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
src/trusted/validator_arm/inst_classes.cc:503: return ((int32_t)(val << 9)) >>
9;
Again, there must be some knowledge here that I don't see. How does left/right
shifting 9 do sign extension to a 32 bit value?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
src/trusted/validator_arm/inst_classes.cc:516: | i.bits(9, 0)) << 11) |
i.bits(26, 16)) << 1;
Again, a better comment might be added here.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
src/trusted/validator_arm/inst_classes.cc:597: // TODO(mrm) Add a guard to the
parse table to make sure the mask is never 0
This scares me. What if the value is all zero?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
src/trusted/validator_arm/inst_classes.cc:670: return
immediate_addressing_defs(i) + i.reg(15 + 16, 12 + 16);
I notice that numbers like 8/16 are used a lot. It is hard to tell if they are
meant to represent the same thing, or if they have different meanings in
different contexts. Would some symbolic constants be useful here to help
readability?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
File src/trusted/validator_arm/inst_classes.h (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
src/trusted/validator_arm/inst_classes.h:873: class Def3 :  public ClassDecoderT
{
Not thrilled with the use of T suffix for thumb (why not use Thumb)?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/in...
src/trusted/validator_arm/inst_classes.h:1017: class BranchT : public
ClassDecoder {
Again, why not Thumb suffix?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/mo...
File src/trusted/validator_arm/model.h (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/mo...
src/trusted/validator_arm/model.h:142: static const RegisterList
kRegisterListEverything = RegisterList(-1);
Why are we passing a NEGATIVE number into a uint_t value?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/nc...
File src/trusted/validator_arm/ncvalidate.cc (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/nc...
src/trusted/validator_arm/ncvalidate.cc:72: NaClValidationStatus
NACL_SUBARCH_NAME(ApplyValidator, arm, 32) (
I would have two functions here, one for each subarchitecture.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/nc...
src/trusted/validator_arm/ncvalidate.cc:91: NaClValidationStatus
NACL_SUBARCH_NAME(ApplyValidatorCodeReplacement, arm, 32)
Same here, and ApplyValidatorCopy below.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
File src/trusted/validator_arm/validator.cc (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.cc:220: }
Nit. blank line before commented section?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.cc:229: else return PATTERN_UNSAFE; /*
Should never be reached */
Nit. Return should be on separate line.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.cc:233: case nacl_arm_dec::NEVER: return
PATTERN_UNSAFE;
Nit. Return on separate line.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.cc:236: return PATTERN_UNSAFE;
Not clear to me that intentional fall through expected here. If expected,
comment why.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.cc:237: case nacl_arm_dec::ALWAYS: {}  //
Fall through, we're fine.
Why use {} instead of break?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.cc:296: // The list of initial patterns to
run (during the first window)
Indent comment to match code.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.cc:299: // The list of patterns to run in
the second window.
Indent comment to match code. (apply to all comments in this method).

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
File src/trusted/validator_arm/validator.h (left):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.h:324: 
Nit. Why blank line removed?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
File src/trusted/validator_arm/validator.h (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.h:216: * 16 is a hack, it is a magic number
to make sure that the second pattern
What 16?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/va...
src/trusted/validator_arm/validator.h:261: return condition() == condition()
Why compare condition to itself?

[bsy, 2011-09-01 00:30:00]

some nits.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:138:
thumb2_env.Replace(ASFLAGS=['-mthumb'])
could env already have other ASFLAGS already set?  Environment.Replace will get
rid of these other values, so -- are previous values bad?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_validate_image.c:47: , thumb
stylistically speaking, conditionally adding an actual argument in this way is a
bit yucky.

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
File src/trusted/validator_arm/address_set.cc (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:73: uint32_t max_index = (parent_.size_
+ 3) / ADDRFACTOR;
s/3/(ADDRFACTOR-1)/?

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ar...
File src/trusted/validator_arm/armv7-thumb.table (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ar...
src/trusted/validator_arm/armv7-thumb.table:6: # Native Client may require the
explicit permission of ARM Limited.
how was this obtained?  it's great that this is data-driven.  did this come from
decoding a PDF?  directly from ARM (and unchanged)?  or manual data input?  (how
confident are we that the data is correct?)

[jasonwkim, 2011-09-14 21:14:37]

PTAL - all tests passing

[bsy, 2011-09-16 18:28:34]

please reply to previous CR comments in reitveld.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/service_runtime...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/service_runtime...
src/trusted/service_runtime/build.scons:139:
thumb2_env.Replace(ASFLAGS=['-mthumb'])
append vs replace question still stands.

[bsy, 2011-09-16 18:35:51]

more comments

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
File src/trusted/validator_arm/validator.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.cc:149: // TODO(mrm) re-enable
s/mrm/jasonwkim/  cannot assign TODO to people who aren't working on the code.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.cc:195: // TODO(mrm) The clears_bits thing
here doesn't save us, secvuln
explain/expand on this please.  if there is really a security vulnerability,
there should be an explanation for why and a bug filed, ideally w/ the details
in the issue and the issue # in here.

ditto on mrm.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.cc:303: // TODO(mrm) re-enable (see comment
on actual function)
"

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
File src/trusted/validator_arm/validator_tests.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator_tests.cc:195: 0);  // ARM mode
comment like this is a good indication that an enum with symbolic names would be
nice.

[bsy, 2011-09-16 18:42:21]

more nits.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
File src/trusted/validator_arm/validator.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.h:12: * MRM DEBUG Header, clean this out
mrm

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.h:209: uint32_t thumb_;
this is C++ code, and bool is used elsewhere.  why not make this a bool?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.h:252: // TODO(mrm) These predicates will
all be able to lie without an IT predicator
"

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.h:281: /* Passes through queries about the
it status */
what is "it"?  that's a horrible acronym/name/whatever.

it's the real thing?

iterator?

[jasonwkim, 2011-09-16 20:09:16]

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:136: if env.Bit('build_arm_thumb2') or
env.Bit('target_arm_thumb2'):
On 2011/08/30 19:53:52, Karl wrote:
> Why is this needed for the build environment? I understand the need for the
> target, but do not understand the build.

It was just to keep the prior bifurcation bet. BUILD and TARGET alive. we can
cut it out later

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:138:
thumb2_env.Replace(ASFLAGS=['-mthumb'])
On 2011/09/01 00:30:00, bsy wrote:
> could env already have other ASFLAGS already set?  Environment.Replace will
get
> rid of these other values, so -- are previous values bad?

Don't know. I'll have to check

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/service_runtime/...
src/trusted/service_runtime/sel_validate_image.c:47: , thumb
On 2011/09/01 00:30:00, bsy wrote:
> stylistically speaking, conditionally adding an actual argument in this way is
a
> bit yucky.

Fixex

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
File src/trusted/validator_arm/address_set.cc (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ad...
src/trusted/validator_arm/address_set.cc:73: uint32_t max_index = (parent_.size_
+ 3) / ADDRFACTOR;
On 2011/09/01 00:30:00, bsy wrote:
> s/3/(ADDRFACTOR-1)/?
Done

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ar...
File src/trusted/validator_arm/armv7-thumb.table (right):

http://codereview.chromium.org/7799013/diff/2001/src/trusted/validator_arm/ar...
src/trusted/validator_arm/armv7-thumb.table:6: # Native Client may require the
explicit permission of ARM Limited.
On 2011/09/01 00:30:00, bsy wrote:
> how was this obtained?  it's great that this is data-driven.  did this come
from
> decoding a PDF?  directly from ARM (and unchanged)?  or manual data input? 
(how I do't know. The original file for the ARM (in src/third_party/arm-isa/)
also has the same header. Mmmaurer just copied it over and modified it
> confident are we that the data is correct?)

For ARM mode, we are pretty assured. For thunb2, its a work in progress

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
File src/trusted/validator_arm/validator.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/v...
src/trusted/validator_arm/validator.cc:303: // TODO(mrm) re-enable (see comment
on actual function)
On 2011/09/16 18:35:52, bsy wrote:
> "

All TODO(mrm) haver been replaceds

[Karl, 2011-09-19 19:56:04]

http://codereview.chromium.org/7799013/diff/15002/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:93: 
NIT: why blank line here?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/a...
File src/trusted/validator_arm/address_set.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.h:38: * - If the address is not 4-byte
aligned, the address of the 4-byte word
Doesn't this conflict with the 2-byte alignment comment on line 20?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.h:45: * 4-byte aligned, the address of the
4-byte word containing the address is
Similar question here.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/b...
File src/trusted/validator_arm/build.scons (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/b...
src/trusted/validator_arm/build.scons:75: validator_tests_thumb = {
NIT: Should this be named thumb2 instead of thunb?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/b...
src/trusted/validator_arm/build.scons:107: [ncval, env.File('testdata-thumb/' +
test + '.nexe')],
NIT: Again, isn't this thumb2, not thumb?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
File src/trusted/validator_arm/decode.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
src/trusted/validator_arm/decode.h:21: * by using delete_state below.
Any explanation of what parameter "thumb" is? Is it boolean expecting 0/1?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
File src/trusted/validator_arm/dgen_output.py (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
src/trusted/validator_arm/dgen_output.py:15: def generate_decoder(arm_tables,
thumb_tables, out):
NIT: Again, thumb2 or thumb?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/g...
File src/trusted/validator_arm/generate_decoder.py (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/g...
src/trusted/validator_arm/generate_decoder.py:11: Usage: generate-decoder.py
<arm-table-file> <thumb-table-file> <output-cc-file>
Again, thumb or thumb2?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
File src/trusted/validator_arm/inst_classes.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.cc:423: /* Thumb Functions */
Thumb2?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
File src/trusted/validator_arm/inst_classes.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:54: }
Blank line?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:55: inline ITCond it_set(uint8_t index,
ITMode mode, ITCond cond) {
Comment describing function?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:871: /* Thumb Classes */
Thumb2?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncval.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:54: int validate(const ncfile *ncf, bool
use_zero_masks, bool thumb) {
Should this be static?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:56: if (thumb) {
Thumb2?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:82: fprintf(stderr, "CodeSegment(%x, %x,
%x)\n", (unsigned int)(ncf->data + (shdr[i].sh_addr - ncf->vbase)),
Should this have a DEBUG prefix like line 126?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncvalidate.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncvalidate.cc:50: bool thumb) {
Thumb2?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncvalidate.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncvalidate.h:30: *   thumb  whether the code should be
evaluated as thumb mode
Thumb2?

[sehr (please use chromium), 2011-09-20 18:24:15]

One nit.  More to come.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/compile_tests.sh (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/compile_tests.sh:14:
tools="$topdir/toolchain/pnacl_linux_x86_64/arm-none-linux-gnueabi/bin"
I believe this path has changed to:
$topdir/toolchain/pnacl_linux_x86_64_newlib/pkg/binutils/bin

[bsy, 2011-09-21 22:32:17]

nits and more nits

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator/ncfil...
File src/trusted/validator/ncfileutil.c (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator/ncfil...
src/trusted/validator/ncfileutil.c:196: memcpy(ncf->eheader, &h,
sizeof(Elf_Ehdr));
*ncf->eheader = h; is clearer.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator/ncfil...
src/trusted/validator/ncfileutil.c:242: if (ncf->data != NULL)    
free(ncf->data);
NULL check is not needed, since free(NULL) is defined to be a no-op.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/a...
File src/trusted/validator_arm/address_set.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.h:2: * Copyright 2009 The Native Client
Authors.  All rights reserved.
11

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
File src/trusted/validator_arm/inst_classes.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.cc:8: // TODO(mrm) remove when debug is
gone
mrm

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
File src/trusted/validator_arm/inst_classes.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:13: // TODO(mrm) Remove this with
debugging lines
mrm

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:56: return (mode << (2 * index)) |
(cond & (~(0x3 << (2 * index))));
nits:

CHECK(0 <= index); CHECK(index < 4)???

these two are fns that are const wrt their parameters, so in particular it_set
doesn't really "set" anything being purely functional.

i did a quick experiment, and gcc can fully inline a class such as:

class SmallValArray {
 public:
  typedef enum { NONE=0, THEN=1, ELSE=2} val_t;
  static size_t const max_index = 4;

  SmallValArray() { packed_ = 0; }
  val_t get(size_t ix) const {
    assert(0 <= ix);
    assert(ix < max_index);
    return static_cast<val_t>(0x3 & (packed_ >> (2*ix)));
  }
  void set(size_t ix, val_t val) {
    assert(0 <= ix);
    assert(ix < max_index);
    packed_ = (val << (2*ix)) | (packed_ & ~(0x3 << (2*ix)));
  }
 private:
  uint8_t packed_;
};

and process its singleton member in a register etc, so doing things more C-style
doesn't seem to buy much.  (uint32_t instead of uint8_t probably would be more
efficient.)

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:274: return ALWAYS;
this seems like an unsafe default.  if a subclass of ClassDecoder forgot to
implement this, we'd use this implementation and all instructions are safe for
use in it blocks.

why don't we use virtual ITSafety it_safe(Instruction i) const = 0; ?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/m...
File src/trusted/validator_arm/model.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/m...
src/trusted/validator_arm/model.h:2: * Copyright 2011 The Native Client Authors.
 All rights reserved.
(c), "be"

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/m...
src/trusted/validator_arm/model.h:5: * Copyright 2011, Google Inc.
is this one necessary?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncval.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:54: int validate(const ncfile *ncf, bool
use_zero_masks, bool thumb) {
since this is C++ code, put this in a namespace?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncvalidate.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncvalidate.h:2: * Copyright 2011 The Native Client
Authors.  All rights reserved.
(c) symbol.  "be" movement.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/it_test.S (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/it_test.S:69: // TODO(mrm) Manually add
a sequence that will fire the it_safety issue.
mrm

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_external_jumps.S (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_external_jumps.S:1: @ Copyright
2010 The Native Client Authors.  All rights reserved.
(c) 2011, be

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_forbidden_instructions.S
(right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_forbidden_instructions.S:1: @
Copyright 2011 The Native Client Authors.  All rights reserved.
(c)

"be" at the end of 2nd line.

our presubmit check doesn't understand .S and @-prefixes.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_internal_jumps.S (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_internal_jumps.S:1: @ Copyright
2010 The Native Client Authors.  All rights reserved.
(c) 2011 etc

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_stores.S (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_stores.S:1: @ Copyright 2009 The
Native Client Authors.  All rights reserved.
was this a file rename from another location?  copy?  s/2009/2011/ if it is new
test data.

[jasonwkim, 2011-09-26 21:35:52]

hopefully all nits addressed. PTAL

http://codereview.chromium.org/7799013/diff/15002/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_validate_image.c (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/service_runtime...
src/trusted/service_runtime/sel_validate_image.c:93: 
On 2011/09/19 19:56:05, Karl wrote:
> NIT: why blank line here?

Fixed

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator/ncfil...
File src/trusted/validator/ncfileutil.c (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator/ncfil...
src/trusted/validator/ncfileutil.c:196: memcpy(ncf->eheader, &h,
sizeof(Elf_Ehdr));
On 2011/09/21 22:32:17, bsy wrote:
> *ncf->eheader = h; is clearer.

done

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator/ncfil...
src/trusted/validator/ncfileutil.c:242: if (ncf->data != NULL)    
free(ncf->data);
On 2011/09/21 22:32:17, bsy wrote:
> NULL check is not needed, since free(NULL) is defined to be a no-op.

fixed

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/a...
File src/trusted/validator_arm/address_set.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.h:45: * 4-byte aligned, the address of the
4-byte word containing the address is
On 2011/09/19 19:56:05, Karl wrote:
> Similar question here.

both fixed

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/b...
File src/trusted/validator_arm/build.scons (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/b...
src/trusted/validator_arm/build.scons:75: validator_tests_thumb = {
On 2011/09/19 19:56:05, Karl wrote:
> NIT: Should this be named thumb2 instead of thunb?

not really.
we are teting thumb MODE
thumb MODE
thumb2 ISA

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/b...
src/trusted/validator_arm/build.scons:107: [ncval, env.File('testdata-thumb/' +
test + '.nexe')],
On 2011/09/19 19:56:05, Karl wrote:
> NIT: Again, isn't this thumb2, not thumb?

no this is for thumb MODE, this is not the ISA

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
File src/trusted/validator_arm/decode.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
src/trusted/validator_arm/decode.h:21: * by using delete_state below.
On 2011/09/19 19:56:05, Karl wrote:
> Any explanation of what parameter "thumb" is? Is it boolean expecting 0/1?

Done

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
File src/trusted/validator_arm/dgen_output.py (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/d...
src/trusted/validator_arm/dgen_output.py:15: def generate_decoder(arm_tables,
thumb_tables, out):
On 2011/09/19 19:56:05, Karl wrote:
> NIT: Again, thumb2 or thumb?

in this case, I am going to punt and not fix - there are other hardcoded string
dependencies. thumb == thumb2

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/g...
File src/trusted/validator_arm/generate_decoder.py (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/g...
src/trusted/validator_arm/generate_decoder.py:11: Usage: generate-decoder.py
<arm-table-file> <thumb-table-file> <output-cc-file>
On 2011/09/19 19:56:05, Karl wrote:
> Again, thumb or thumb2?

Decision on whethwe you are in thumb2 (vs old thumb) actually depends on the
architecture level.

and transition between arm and thumb mode is idential (i.e. LSB of jump target
ADDRESS set to 1 whether or not you are using thumb2 instructions or not

so the correct thing to say is

thumb MODE
thumb2 ISA

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
File src/trusted/validator_arm/inst_classes.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.cc:8: // TODO(mrm) remove when debug is
gone
On 2011/09/21 22:32:17, bsy wrote:
> mrm

all mrm fixed

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.cc:423: /* Thumb Functions */
On 2011/09/19 19:56:05, Karl wrote:
> Thumb2?

Thumb is more correct in this case

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
File src/trusted/validator_arm/inst_classes.h (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:56: return (mode << (2 * index)) |
(cond & (~(0x3 << (2 * index))));
On 2011/09/21 22:32:17, bsy wrote:
> nits:
> 
> CHECK(0 <= index); CHECK(index < 4)???
> 
> these two are fns that are const wrt their parameters, so in particular it_set
> doesn't really "set" anything being purely functional.
> 
> i did a quick experiment, and gcc can fully inline a class such as:
> 
> class SmallValArray {
>  public:
>   typedef enum { NONE=0, THEN=1, ELSE=2} val_t;
>   static size_t const max_index = 4;
> 
>   SmallValArray() { packed_ = 0; }
>   val_t get(size_t ix) const {
>     assert(0 <= ix);
>     assert(ix < max_index);
>     return static_cast<val_t>(0x3 & (packed_ >> (2*ix)));
>   }
>   void set(size_t ix, val_t val) {
>     assert(0 <= ix);
>     assert(ix < max_index);
>     packed_ = (val << (2*ix)) | (packed_ & ~(0x3 << (2*ix)));
>   }
>  private:
>   uint8_t packed_;
> };
> 
> and process its singleton member in a register etc, so doing things more
C-style
> doesn't seem to buy much.  (uint32_t instead of uint8_t probably would be more
> efficient.)

Done
Don't want to add in dependency to NACL_LOG here. just added assert() 

assert(index < 4) is sufficient (index is unsigned)

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/i...
src/trusted/validator_arm/inst_classes.h:871: /* Thumb Classes */
On 2011/09/19 19:56:05, Karl wrote:
> Thumb2?

no, thumb2 adds some 32bit insts to thumb. This works
for both (so thumb is more general, so more correct

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
File src/trusted/validator_arm/ncval.cc (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:54: int validate(const ncfile *ncf, bool
use_zero_masks, bool thumb) {
On 2011/09/21 22:32:17, bsy wrote:
> since this is C++ code, put this in a namespace?

since this code is already only called by main, probably not worth it?

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:54: int validate(const ncfile *ncf, bool
use_zero_masks, bool thumb) {
On 2011/09/19 19:56:05, Karl wrote:
> Should this be static?

only called by main - I think its fine

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:56: if (thumb) {
On 2011/09/19 19:56:05, Karl wrote:
> Thumb2?

again thumb mode

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/n...
src/trusted/validator_arm/ncval.cc:82: fprintf(stderr, "CodeSegment(%x, %x,
%x)\n", (unsigned int)(ncf->data + (shdr[i].sh_addr - ncf->vbase)),
On 2011/09/19 19:56:05, Karl wrote:
> Should this have a DEBUG prefix like line 126?

debug printfs killed

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/compile_tests.sh (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/compile_tests.sh:14:
tools="$topdir/toolchain/pnacl_linux_x86_64/arm-none-linux-gnueabi/bin"
On 2011/09/20 18:24:15, sehr wrote:
> I believe this path has changed to:
> $topdir/toolchain/pnacl_linux_x86_64_newlib/pkg/binutils/bin
\domne

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_external_jumps.S (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_external_jumps.S:1: @ Copyright
2010 The Native Client Authors.  All rights reserved.
On 2011/09/21 22:32:17, bsy wrote:
> (c) 2011, be
done

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_forbidden_instructions.S
(right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_forbidden_instructions.S:1: @
Copyright 2011 The Native Client Authors.  All rights reserved.
On 2011/09/21 22:32:17, bsy wrote:
> (c)
> 
> "be" at the end of 2nd line.
> 
> our presubmit check doesn't understand .S and @-prefixes.

Done.

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_internal_jumps.S (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_internal_jumps.S:1: @ Copyright
2010 The Native Client Authors.  All rights reserved.
On 2011/09/21 22:32:17, bsy wrote:
> (c) 2011 etc

done

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata-thumb/test_stores.S (right):

http://codereview.chromium.org/7799013/diff/15002/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata-thumb/test_stores.S:1: @ Copyright 2009 The
Native Client Authors.  All rights reserved.
On 2011/09/21 22:32:17, bsy wrote:
> was this a file rename from another location?  copy?  s/2009/2011/ if it is
new
> test data.

done

[Karl, 2011-09-27 17:05:38]

LGTM, assuming no one else has any problems.

[bsy, 2011-09-27 23:27:28]

given the 32 vs 16 address_set bug that i found, i assert that there aren't
enough tests here.

ideally, you should leave those bugs in there, add a test that detects their
manifestations, and then fix them.

i haven't looked through the arm decoder table.  big TODO to audit it should be
in place, plus an issue filed to track the audit when the table / CL is checked
in.

http://codereview.chromium.org/7799013/diff/40003/src/trusted/service_runtime...
File src/trusted/service_runtime/elf_util.c (right):

http://codereview.chromium.org/7799013/diff/40003/src/trusted/service_runtime...
src/trusted/service_runtime/elf_util.c:273: nacl_phdr_check_data[j].p_vaddr,
(int) j);
use NACL_PRIdS in the format string as in other NaClLog statement above (L250)
and omit this cast.

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
File src/trusted/validator_arm/address_set.cc (right):

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.cc:32: bits_[word_address / 32] |= 1 <<
(word_address % 32);
this should be BITSINADDRF instead of 32 in both of these.

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.cc:40: return bits_[word_address / 32] &
(1 << (word_address % 32));
ditto

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.cc:72: return parent_.base_ + ADDRFACTOR *
((index_ * 32) + shift_);
should this be 32?

[bsy, 2011-09-28 00:31:12]

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
File src/trusted/validator_arm/address_set.cc (right):

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.cc:20: bits_(new uint32_t[(size + RUPADDR)
/ ADDRFACTOR]) {
as discussed, this is a bit vector the max size of which depends on the sandbox
model: the |size| parameter is the address space size but on arm we only care
about |address| div 4 (in add, contains, etc), and on thumb2 we care about
|address| div 2.

i think this should be templated or otherwise parameterized according to the
sandbox model.  in particular, this CL would have the same AddressSet be used
for both arm and thumb2, and when used for arm the sequence

AddressSet as;
as.add(n);  // n is 0 mod 4, within size, etc.
CHECK(as.contains(n+2));

would pass on the original code but fail with the CL applied.

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.cc:32: bits_[word_address / 32] |= 1 <<
(word_address % 32);
this code was correct.  my apologies.

it would be nice if the code were more obviously correct....

http://codereview.chromium.org/7799013/diff/40003/src/trusted/validator_arm/a...
src/trusted/validator_arm/address_set.cc:40: return bits_[word_address / 32] &
(1 << (word_address % 32));
ditto