Finish separation of testing from sel_ldr validation. Also, automate

Finish separation of testing from sel_ldr validation. Also, automate
the process of generating tests from the specified decoder tables.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2673
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8337

[robertm, 2012-04-11 01:28:43]

It don't go much into detail about what I mean by templates
in the python code - ping me about it

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/address_set_test.cc (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/address_set_test.cc:12: #error("This file is not meant
for use in the TCB")
I think #error is usually used without parens

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/armv7.table (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/armv7.table:29: # For documentation and testing, an
"=InstClass" can be followed by up to 3
thanks for reverse engineering and documenting it

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/build.scons (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/build.scons:42: GEN_LIST = map(_gen_file,
['arm32_decode.cc', 'arm32_decode.h',
use list comprehension, e.g.

GEM_LIST = [_gen_file(x) for x  in ['arm32_decode.cc'.
                                    'arm32_decode.h',
                                    ...]]

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/dgen_core.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_core.py:240: for a in r.action.split():
could this be simplified with array slicing, e.g.

r.action.split()[0:num_actions]

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/dgen_input.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_input.py:102: def table_row(table):
can you add a docstring and maybe a sample line or two of what this tried to
parse

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_input.py:157: _line = re.sub(r'#.*', '',
_line).strip()
does this line sill make sense, since you are already processing comments in the
block above?

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/dgen_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_output.py:29: if named_decoders:
this is getting really hard to understand, maybe it is time
to switch to a template based approach, i.e. "%"
you probably need two templates, depending on tcb on/off.
Using the "%(name)s" placeholders will help making this readable.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_output.py:211: out.enter_block('class %s : public
NamedClassDecoder' %
this would also benefit from a template

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/gen/arm32_decode.cc (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/gen/arm32_decode.cc:25: const ClassDecoder
&Arm32DecoderState::decode_ARMv7(const Instruction insn) const {
move & to left

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/gen/arm32_decode_named.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/gen/arm32_decode_named.h:39: class NamedCoprocessorOp
: public NamedClassDecoder {
can you add a comment about the purpose of these classes.
They do not appear to do much.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/named_class_decoder.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/named_class_decoder.h:18: // Defines a wrapper class
used for testing, that adds a name to
not sure whether just getting a name is worth having these classes. Even if they
are only used for testing I am fine with leaving them in the TCB code.

[Karl, 2012-04-16 23:18:10]

Please take a look at my changes. Thanks.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/address_set_test.cc (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/address_set_test.cc:12: #error("This file is not meant
for use in the TCB")
On 2012/04/11 01:28:43, robertm wrote:
> I think #error is usually used without parens

Done.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/armv7.table (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/armv7.table:29: # For documentation and testing, an
"=InstClass" can be followed by up to 3
On 2012/04/11 01:28:43, robertm wrote:
> thanks for reverse engineering and documenting it

Done.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/build.scons (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/build.scons:42: GEN_LIST = map(_gen_file,
['arm32_decode.cc', 'arm32_decode.h',
On 2012/04/11 01:28:43, robertm wrote:
> use list comprehension, e.g.
> 
> GEM_LIST = [_gen_file(x) for x  in ['arm32_decode.cc'.
>                                     'arm32_decode.h',
>                                     ...]]

Done.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/dgen_core.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_core.py:240: for a in r.action.split():
On 2012/04/11 01:28:43, robertm wrote:
> could this be simplified with array slicing, e.g.
> 
> r.action.split()[0:num_actions]

This code simplified once I removed parsing from the represenation.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/dgen_input.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_input.py:102: def table_row(table):
On 2012/04/11 01:28:43, robertm wrote:
> can you add a docstring and maybe a sample line or two of what this tried to
> parse

Did this in general. This file has gone through major revision.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_input.py:157: _line = re.sub(r'#.*', '',
_line).strip()
On 2012/04/11 01:28:43, robertm wrote:
> does this line sill make sense, since you are already processing comments in
the
> block above?

Since the parser is now free-form, it is no longer necessary to play games
around commented and empty lines. The new code for reading lines is greatly
simplified.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/dgen_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_output.py:29: if named_decoders:
On 2012/04/11 01:28:43, robertm wrote:
> this is getting really hard to understand, maybe it is time
> to switch to a template based approach, i.e. "%"
> you probably need two templates, depending on tcb on/off.
> Using the "%(name)s" placeholders will help making this readable.

Done.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/dgen_output.py:211: out.enter_block('class %s : public
NamedClassDecoder' %
On 2012/04/11 01:28:43, robertm wrote:
> this would also benefit from a template

Done.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/gen/arm32_decode.cc (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/gen/arm32_decode.cc:25: const ClassDecoder
&Arm32DecoderState::decode_ARMv7(const Instruction insn) const {
On 2012/04/11 01:28:43, robertm wrote:
> move & to left

Done.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/gen/arm32_decode_named.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/gen/arm32_decode_named.h:39: class NamedCoprocessorOp
: public NamedClassDecoder {
On 2012/04/11 01:28:43, robertm wrote:
> can you add a comment about the purpose of these classes.
> They do not appear to do much.

Added comment in dgen_test_output.py that generates these.

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
File src/trusted/validator_arm/named_class_decoder.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/3002/src/trusted/validato...
src/trusted/validator_arm/named_class_decoder.h:18: // Defines a wrapper class
used for testing, that adds a name to
On 2012/04/11 01:28:43, robertm wrote:
> not sure whether just getting a name is worth having these classes. Even if
they
> are only used for testing I am fine with leaving them in the TCB code.

While I agree it looks like a lot, it makes debugging test errors much easier.
Since this is only used in test code, I don't see a problem with leaving it in.

[robertm, 2012-04-17 17:12:19]

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/build.scons (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/build.scons:158: ['gen/arm32_decode_tests.cc'],
is this gen/ dir different from the one above?

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/decoder_tester.cc (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/decoder_tester.cc:135: const NamedClassDecoder
&expected_decoder)
move "&" to left

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/decoder_tester.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/decoder_tester.h:61: virtual const NamedClassDecoder
&ExpectedDecoder() const = 0;
move "&" to left

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/decoder_tester.h:117: const NamedClassDecoder
&expected_decoder);
here and below as well

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_core.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:251: self.name = name
for private member fields we usually use the leading "_".
For private member functions we do not care all that much

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:271: value =
cmp(other.__class__.__name__, 'DecoderAction')
add a function comment

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:290: """An action defining a parse method
to call."""
maybe give example of a parse method

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:362: """Defines a class decoder which
consists of set of tables, with a primary
give example of a class

(it would be nice if one could just read all the class comments and get a rough
idea of what they do and how they work together - this way we do not need a
design doc)

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_decoder_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:32: # For testing purposes (see
dgen_test_output.py) different rules are
mention that the other file as a nice description

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:191: 'header_filename':
filename[:-2] + 'h',
maybe assert that file name ends with '.cc'

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:217: method_calls = filter(
try to replace this with list comprehension
(there is a special syntax aimed for filtering)

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:223: values['tests'] = ' &&
'.join(["(%s)" % p.to_c_expr('insn')
add a comment explaining the introspection magic - can it be avoided?

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_input.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_input.py:95: while self._next_token().kind not in
['|', '+', '(']:
not using a loop might be clearer

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_input.py:189: self._read_token('"')
add a comment here

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_output.py:9: Responsible for generating the
decoder based on parsed table representations.
may need updating

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_test_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_test_output.py:20: # To fix this, we allow table
row actions to have multiple identifiers
can you elaborate on the two modes, maybe give a high level example

also cross link to the other generator python script

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/gen/arm32_decode.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/gen/arm32_decode.h:31: inline const ClassDecoder&
decode_ARMv7(
can these be static?

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/gen/arm32_decode.h:124: Binary3RegisterShiftedTest
Binary3RegisterShiftedTest_instance_;
add high level comment about instance variables

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/generate_decoder.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/generate_decoder.py:56:
dgen_test_output.generate_named_classes_h(decoder,
put args on new line to avoid 80char limit

[Karl, 2012-04-17 19:28:31]

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/build.scons (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/build.scons:158: ['gen/arm32_decode_tests.cc'],
On 2012/04/17 17:12:19, robertm wrote:
> is this gen/ dir different from the one above?

It is the same as the above. However, this generated file is a test executable
(rather than a library file).

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/decoder_tester.cc (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/decoder_tester.cc:135: const NamedClassDecoder
&expected_decoder)
On 2012/04/17 17:12:19, robertm wrote:
> move "&" to left

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/decoder_tester.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/decoder_tester.h:61: virtual const NamedClassDecoder
&ExpectedDecoder() const = 0;
On 2012/04/17 17:12:19, robertm wrote:
> move "&" to left

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/decoder_tester.h:117: const NamedClassDecoder
&expected_decoder);
On 2012/04/17 17:12:19, robertm wrote:
> here and below as well

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_core.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:251: self.name = name
On 2012/04/17 17:12:19, robertm wrote:
> for private member fields we usually use the leading "_".
> For private member functions we do not care all that much

These fields are intentionally public.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:271: value =
cmp(other.__class__.__name__, 'DecoderAction')
On 2012/04/17 17:12:19, robertm wrote:
> add a function comment 

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:290: """An action defining a parse method
to call."""
On 2012/04/17 17:12:19, robertm wrote:
> maybe give example of a parse method

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:362: """Defines a class decoder which
consists of set of tables, with a primary
On 2012/04/17 17:12:19, robertm wrote:
> give example of a class
> 
> (it would be nice if one could just read all the class comments and get a
rough
> idea of what they do and how they work together - this way we do not need a
> design doc)

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_decoder_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:32: # For testing purposes (see
dgen_test_output.py) different rules are
On 2012/04/17 17:12:19, robertm wrote:
> mention that the other file as a nice description

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:191: 'header_filename':
filename[:-2] + 'h',
On 2012/04/17 17:12:19, robertm wrote:
> maybe assert that file name ends with '.cc'

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:217: method_calls = filter(
On 2012/04/17 17:12:19, robertm wrote:
> try to replace this with list comprehension
> (there is a special syntax aimed for filtering) 

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:223: values['tests'] = ' &&
'.join(["(%s)" % p.to_c_expr('insn')
On 2012/04/17 17:12:19, robertm wrote:
> add a comment explaining the introspection magic - can it be avoided?

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_output.py:9: Responsible for generating the
decoder based on parsed table representations.
On 2012/04/17 17:12:19, robertm wrote:
> may need updating

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/dgen_test_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/dgen_test_output.py:20: # To fix this, we allow table
row actions to have multiple identifiers
On 2012/04/17 17:12:19, robertm wrote:
> can you elaborate on the two modes, maybe give a high level example
> 
> also cross link to the other generator python script

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/gen/arm32_decode.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/gen/arm32_decode.h:31: inline const ClassDecoder&
decode_ARMv7(
On 2012/04/17 17:12:19, robertm wrote:
> can these be static?

The need to refer to the instance fields, which are created once (up front),
rather than each time one is returned. I don't really see how we can make these
static. I did add a comment to better describe these.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/gen/arm32_decode.h:124: Binary3RegisterShiftedTest
Binary3RegisterShiftedTest_instance_;
On 2012/04/17 17:12:19, robertm wrote:
> add high level comment about instance variables 

Done.

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
File src/trusted/validator_arm/generate_decoder.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/11001/src/trusted/validat...
src/trusted/validator_arm/generate_decoder.py:56:
dgen_test_output.generate_named_classes_h(decoder,
On 2012/04/17 17:12:19, robertm wrote:
> put args on new line to avoid 80char limit

Done.

[sehr (please use chromium), 2012-04-19 19:52:47]

Nits from my previous version.

http://codereview.chromium.org/9960043/diff/19001/src/trusted/validator_arm/d...
File src/trusted/validator_arm/dgen_core.py (right):

http://codereview.chromium.org/9960043/diff/19001/src/trusted/validator_arm/d...
src/trusted/validator_arm/dgen_core.py:222: action_string: a string indicating
the action to take; must begin
s/action_string/action/

http://codereview.chromium.org/9960043/diff/19001/src/trusted/validator_arm/d...
src/trusted/validator_arm/dgen_core.py:225: arg: a string defining the
architectures it applies to. None
s/arg/arch/

[sehr (please use chromium), 2012-04-19 21:54:46]

A number of nits.  Ignore the spacing comments. Fix the nits and LGTM.

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/d...
File src/trusted/validator_arm/dgen_decoder_output.py (right):

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/d...
src/trusted/validator_arm/dgen_decoder_output.py:18: # decoder tables.  The code
is specifically written to minize the
minimize

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/d...
File src/trusted/validator_arm/dgen_test_output.py (right):

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/d...
src/trusted/validator_arm/dgen_test_output.py:52: # its only 'name'. For
testing, it will include other fields, depending
s/its/it's/

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/d...
src/trusted/validator_arm/dgen_test_output.py:99: #  virtual
~NamedAdd_Rule_7_A1_P26Binary4RegisterShiftedOp() {}
Should this be ~NamedRuleInstClass?

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
File src/trusted/validator_arm/gen/arm32_decode.cc (right):

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
src/trusted/validator_arm/gen/arm32_decode.cc:21: ,
Binary3RegisterShiftedTest_instance_()
why is this double-spaced?

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
src/trusted/validator_arm/gen/arm32_decode.cc:108: 
why are there so many blank lines here and below?

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
File src/trusted/validator_arm/gen/arm32_decode.h (right):

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
src/trusted/validator_arm/gen/arm32_decode.h:1: 
Why are these lines here?

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
File src/trusted/validator_arm/gen/arm32_decode_named.cc (right):

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
src/trusted/validator_arm/gen/arm32_decode_named.cc:27: :
nacl_arm_dec::DecoderState()
lots of blank lines again.

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
File src/trusted/validator_arm/gen/arm32_decode_named_decoder.h (right):

http://codereview.chromium.org/9960043/diff/27001/src/trusted/validator_arm/g...
src/trusted/validator_arm/gen/arm32_decode_named_decoder.h:44: // that can be
returned by the API function "decode_name". They
decode_named?

[Karl, 2012-04-24 19:58:26]

https://chromiumcodereview.appspot.com/9960043/diff/19001/src/trusted/validat...
File src/trusted/validator_arm/dgen_core.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/19001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:222: action_string: a string indicating
the action to take; must begin
On 2012/04/19 19:52:47, sehr wrote:
> s/action_string/action/

Done.

https://chromiumcodereview.appspot.com/9960043/diff/19001/src/trusted/validat...
src/trusted/validator_arm/dgen_core.py:225: arg: a string defining the
architectures it applies to. None
On 2012/04/19 19:52:47, sehr wrote:
> s/arg/arch/

Done.

https://chromiumcodereview.appspot.com/9960043/diff/27001/src/trusted/validat...
File src/trusted/validator_arm/dgen_decoder_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/27001/src/trusted/validat...
src/trusted/validator_arm/dgen_decoder_output.py:18: # decoder tables.  The code
is specifically written to minize the
On 2012/04/19 21:54:46, sehr wrote:
> minimize

Done.

https://chromiumcodereview.appspot.com/9960043/diff/27001/src/trusted/validat...
File src/trusted/validator_arm/dgen_test_output.py (right):

https://chromiumcodereview.appspot.com/9960043/diff/27001/src/trusted/validat...
src/trusted/validator_arm/dgen_test_output.py:52: # its only 'name'. For
testing, it will include other fields, depending
On 2012/04/19 21:54:46, sehr wrote:
> s/its/it's/

Done.

https://chromiumcodereview.appspot.com/9960043/diff/27001/src/trusted/validat...
src/trusted/validator_arm/dgen_test_output.py:99: #  virtual
~NamedAdd_Rule_7_A1_P26Binary4RegisterShiftedOp() {}
On 2012/04/19 21:54:46, sehr wrote:
> Should this be ~NamedRuleInstClass?

Done.

https://chromiumcodereview.appspot.com/9960043/diff/27001/src/trusted/validat...
File src/trusted/validator_arm/gen/arm32_decode_named_decoder.h (right):

https://chromiumcodereview.appspot.com/9960043/diff/27001/src/trusted/validat...
src/trusted/validator_arm/gen/arm32_decode_named_decoder.h:44: // that can be
returned by the API function "decode_name". They
On 2012/04/19 21:54:46, sehr wrote:
> decode_named?

Done.