Eliminate bundle_size parameter from validator interface.

Eliminate bundle_size parameter from validator interface.

Some superinstructions in src/trusted/validator_x86/ncval_tests.c were changed
to work with 32 byte bundle sizes instead of 16 bytes.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2737
TEST= none

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8307

[Nick Bray (chromium), 2012-04-17 21:46:36]

Apologies about the size of the CL, the roots went deeper than I anticipated. 
If it's too much, I can try and figure out a way to split it up.

[Karl, 2012-04-17 22:20:11]

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator/...
File src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator/...
src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c:356: vstate->alignmask =
alignment - 1;
Is this safe? alignment was uint8_t and now we are using an int.

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
File src/trusted/validator_x86/build.scons (left):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
src/trusted/validator_x86/build.scons:368: options.append('--alignment=32')
What are you going to do with the tests in

src/trusted/validator_x86/testdata/32
and
src/trusted/validator_x86/testdata/64

Both directories have 16-bit tests. Are you going to remove them?

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
File src/trusted/validator_x86/ncval_tests.c (left):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
src/trusted/validator_x86/ncval_tests.c:77: "test 4",
I'm a little concerned about "just" deleting this test. What type of but is it
testing? Is there a purpose?

[Nick Bray (chromium), 2012-04-17 22:42:02]

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator/...
File src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator/...
src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c:356: vstate->alignmask =
alignment - 1;
On 2012/04/17 22:20:12, Karl wrote:
> Is this safe? alignment was uint8_t and now we are using an int.

0 < alignment and alignmask < 128, cspec says signed/unsigned casts will
preserve values when possible.

I am not seeing what could break.  What worries you?

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
File src/trusted/validator_x86/build.scons (left):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
src/trusted/validator_x86/build.scons:368: options.append('--alignment=32')
On 2012/04/17 22:20:12, Karl wrote:
> What are you going to do with the tests in
> 
> src/trusted/validator_x86/testdata/32
> and
> src/trusted/validator_x86/testdata/64
> 
> Both directories have 16-bit tests. Are you going to remove them?

Good idea.  I'll check with Robert first, however, because this is a big review
to begin with.

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
File src/trusted/validator_x86/ncval_tests.c (left):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
src/trusted/validator_x86/ncval_tests.c:77: "test 4",
On 2012/04/17 22:20:12, Karl wrote:
> I'm a little concerned about "just" deleting this test. What type of but is it
> testing? Is there a purpose?

That's a good question.  My take was that a test no one understands in not a
test - and a big chunk of code with no documentation seems to indicate that the
test isn't testing anything in particular, rather its throwing things against
the wall and seeing what sticks.  I see no point in maintaining a test that
offers no obvious additional coverage.

[Karl, 2012-04-17 22:48:01]

I'm willing to let Robert give the the thumbs up.

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator/...
File src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator/...
src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c:356: vstate->alignmask =
alignment - 1;
On 2012/04/17 22:42:02, Nick Bray (chromium) wrote:
> On 2012/04/17 22:20:12, Karl wrote:
> > Is this safe? alignment was uint8_t and now we are using an int.
> 
> 0 < alignment and alignmask < 128, cspec says signed/unsigned casts will
> preserve values when possible.
> 
> I am not seeing what could break.  What worries you?

Just nervous in general when using implicit casting. Been burned too many times.

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
File src/trusted/validator_x86/ncval_tests.c (left):

https://chromiumcodereview.appspot.com/10116004/diff/1/src/trusted/validator_...
src/trusted/validator_x86/ncval_tests.c:77: "test 4",
On 2012/04/17 22:42:02, Nick Bray (chromium) wrote:
> On 2012/04/17 22:20:12, Karl wrote:
> > I'm a little concerned about "just" deleting this test. What type of but is
it
> > testing? Is there a purpose?
> 
> That's a good question.  My take was that a test no one understands in not a
> test - and a big chunk of code with no documentation seems to indicate that
the
> test isn't testing anything in particular, rather its throwing things against
> the wall and seeing what sticks.  I see no point in maintaining a test that
> offers no obvious additional coverage.

The only thing i see this test doing is verifying that we can parse the
sequence. This code probably didn't decode at one point. However, I'm not
opposed to getting rid of this.

[Nick Bray (chromium), 2012-04-17 23:15:19]

PTAL

90 insertions(+), 725 deletions(-)

Craaaaazy.

[Robert Muth (chromium), 2012-04-18 20:22:35]

LGTM

[Mark Seaborn, 2012-04-18 22:58:20]

https://chromiumcodereview.appspot.com/10116004/diff/4070/src/trusted/validat...
File src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c (right):

https://chromiumcodereview.appspot.com/10116004/diff/4070/src/trusted/validat...
src/trusted/validator/x86/ncval_seg_sfi/ncvalidate.c:332: const int alignment =
32;
Should this be in a "#define NACL_BUNDLE_SIZE" somewhere?  Maybe shared between
the two x86 validators?

https://chromiumcodereview.appspot.com/10116004/diff/4070/src/trusted/validat...
File src/trusted/validator_x86/ncval_tests.c (right):

https://chromiumcodereview.appspot.com/10116004/diff/4070/src/trusted/validat...
src/trusted/validator_x86/ncval_tests.c:721: "83 e2 e0 \n"                      
/* and */
I'd say this sort of obscure change requires some explanation in the commit
message.

https://chromiumcodereview.appspot.com/10116004/diff/4070/src/trusted/validat...
src/trusted/validator_x86/ncval_tests.c:733: "83 e2 e0 \n"                      
/* and */
Ditto

https://chromiumcodereview.appspot.com/10116004/diff/4070/src/trusted/validat...
src/trusted/validator_x86/ncval_tests.c:996: exit(-1);
Can you use CHECK() instead?

https://chromiumcodereview.appspot.com/10116004/diff/4070/src/trusted/validat...
src/trusted/validator_x86/ncval_tests.c:1003: exit(-1);
Ditto