[MIPS] Forbid $sp update at the end of the last bundle.

[MIPS] Forbid $sp update at the end of the last bundle.

Validator should not allow change of $sp at the end of the last bundle, since
someone could place a code right after it.

This change mimics fix on ARM from https://codereview.chromium.org/11361222

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3138
TEST= ./scons run_mips_validator_tests


Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=10318

[Mark Seaborn, 2012-11-16 20:08:08]

LGTM, thanks

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
File src/trusted/validator_mips/validator_tests.cc (right):

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
src/trusted/validator_mips/validator_tests.cc:370: vector<mips_inst>
code(_validator.bytes_per_bundle()/kInstrSize, kNop);
Nit: please put spaces around '/'

[JF, 2012-11-16 21:03:27]

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
File src/trusted/validator_mips/validator.cc (right):

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
src/trusted/validator_mips/validator.cc:361: DecodedInstruction
one_past_end(segment.EndAddr(), nop, initial_decoder);
Why initial_decoder here?

The ARM change does:
decode_state_.decode(nop)

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
File src/trusted/validator_mips/validator_tests.cc (right):

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
src/trusted/validator_mips/validator_tests.cc:369: TEST_F(ValidatorTests,
UnmaskedSpUpdate) {
Please also add a test for nop (as in the ARM change): otherwise the validator
might be failing because nop is disallowed and not because ending with sp update
is disallowed.

There is also a test in the ARM validator that the masked SP updates succeed.

[petarj, 2012-11-19 18:46:10]

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
File src/trusted/validator_mips/validator.cc (right):

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
src/trusted/validator_mips/validator.cc:361: DecodedInstruction
one_past_end(segment.EndAddr(), nop, initial_decoder);
On 2012/11/16 21:03:27, JF wrote:
> Why initial_decoder here?
> 
> The ARM change does:
> decode_state_.decode(nop)

Changed to nacl_mips_dec::decode(nop, decode_state_).

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
File src/trusted/validator_mips/validator_tests.cc (right):

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
src/trusted/validator_mips/validator_tests.cc:369: TEST_F(ValidatorTests,
UnmaskedSpUpdate) {
On 2012/11/16 21:03:27, JF wrote:
> Please also add a test for nop (as in the ARM change): otherwise the validator
> might be failing because nop is disallowed and not because ending with sp
update
> is disallowed.
> 
> There is also a test in the ARM validator that the masked SP updates succeed.

Done.

http://codereview.chromium.org/11415031/diff/1/src/trusted/validator_mips/val...
src/trusted/validator_mips/validator_tests.cc:370: vector<mips_inst>
code(_validator.bytes_per_bundle()/kInstrSize, kNop);
On 2012/11/16 20:08:08, Mark Seaborn wrote:
> Nit: please put spaces around '/'

Done.

[JF, 2012-11-19 18:49:46]

lgtm

[commit-bot: I haz the power, 2012-11-19 19:17:43]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11415031/6001

[commit-bot: I haz the power, 2012-11-19 19:17:47]

Presubmit check for 11415031-6001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The Native
Client Authors\. All rights reserved\.\n.*? Use of this source code is governed
by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  src/trusted/validator_mips/model.h \
  src/trusted/validator_mips/validator_tests.cc

/b/commit-queue/workdir/nacl/src/trusted/validator_mips/model.h: cpplint
  /b/commit-queue/workdir/nacl/src/trusted/validator_mips/model.h:81: 
Single-argument constructors should be marked explicit.  [runtime/explicit] [5]
Done processing /b/commit-queue/workdir/nacl/src/trusted/validator_mips/model.h
Total errors found: 1


Presubmit checks took 1.5s to calculate.

[commit-bot: I haz the power, 2012-11-21 00:21:46]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/11415031/3004

[commit-bot: I haz the power, 2012-11-21 03:19:03]

Change committed as 10318