stop building/testing old x86 validator.

stop building/testing old x86 validator.

We will remove the source code in the followup change.
https://codereview.chromium.org/625923004/

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3454
R=mseaborn@chromium.org

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

[shyamsundarr, 2014-10-07 17:28:21]

Patchset #1 (id:1) has been deleted

[shyamsundarr, 2014-10-07 17:53:00]

shyamsundarr@chromium.org changed reviewers:
+ mseaborn@chromium.org, ncbray@chromium.org

[shyamsundarr, 2014-10-07 17:53:01]

I broke this out from the larger CL (https://codereview.chromium.org/625923004/
) to try to make it a bit easier to review. this doesn't delete any of the old
x86 validator source code/tests. Please review this one first and i will rebase
the other larger CL once this gets in. Thanks!

[Mark Seaborn, 2014-10-07 21:53:46]

https://codereview.chromium.org/636933004/diff/20001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/636933004/diff/20001/SConstruct#newcode1192
SConstruct:1192: # TODO(shyamsundarr): consider renaming ncval_new to ncval.
Nit: "consider renaming" -> "rename" :-)

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
File src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp (right):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:61: ],
Nit: fix indentation here

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
File src/trusted/service_runtime/build.scons (left):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
src/trusted/service_runtime/build.scons:787: stderr_golden =
env.File('testdata/non_dfa_validator_hello.stderr'),
Also remove the file 'testdata/non_dfa_validator_hello.stderr'

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
File src/trusted/validator_ragel/build.scons (left):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
src/trusted/validator_ragel/build.scons:349:
env.File('verify_regular_instructions_old.py'),
This would be an appropriate change in which to also remove
verify_regular_instructions_old.py, since this file lives in validator_ragel (so
we might otherwise easily not notice that it has become dead code).

Also remove the mention of verify_regular_instructions_old.py (typo'd as
"instruction_old") from src/trusted/validator_ragel/docs/testing.html.

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
File src/trusted/validator_ragel/build.scons (right):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
src/trusted/validator_ragel/build.scons:340: 
Style nit: remove 1 empty line here (so that there's not two consecutive empty
lines inside a function)

https://codereview.chromium.org/636933004/diff/20001/tests/stubout_mode/nacl....
File tests/stubout_mode/nacl.scons (left):

https://codereview.chromium.org/636933004/diff/20001/tests/stubout_mode/nacl....
tests/stubout_mode/nacl.scons:69: # Without any debug flags, the executable
fails validation.
Can we keep this test case?

I have a strong suspicion that there is no other test that checks that
standalone sel_ldr actually rejects non-validating executables.  (You could
check this out by making sel_ldr ignore the result of main-nexe validation and
seeing if any test fails.)

[shyamsundarr, 2014-10-08 18:51:45]

Patchset #3 (id:60001) has been deleted

[shyamsundarr, 2014-10-08 18:51:52]

Patchset #2 (id:40001) has been deleted

[shyamsundarr, 2014-10-08 19:39:25]

Patchset #2 (id:80001) has been deleted

[shyamsundarr, 2014-10-08 19:39:58]

Patchset #2 (id:100001) has been deleted

[shyamsundarr, 2014-10-08 19:59:04]

Thanks! ptal.

https://codereview.chromium.org/636933004/diff/20001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/636933004/diff/20001/SConstruct#newcode1192
SConstruct:1192: # TODO(shyamsundarr): consider renaming ncval_new to ncval.
On 2014/10/07 21:53:46, Mark Seaborn wrote:
> Nit: "consider renaming" -> "rename" :-)

Done.

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
File src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp (right):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:61: ],
On 2014/10/07 21:53:46, Mark Seaborn wrote:
> Nit: fix indentation here

Done.

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
File src/trusted/service_runtime/build.scons (left):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/service_runt...
src/trusted/service_runtime/build.scons:787: stderr_golden =
env.File('testdata/non_dfa_validator_hello.stderr'),
On 2014/10/07 21:53:46, Mark Seaborn wrote:
> Also remove the file 'testdata/non_dfa_validator_hello.stderr'

Done.

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
File src/trusted/validator_ragel/build.scons (left):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
src/trusted/validator_ragel/build.scons:349:
env.File('verify_regular_instructions_old.py'),
On 2014/10/07 21:53:46, Mark Seaborn wrote:
> This would be an appropriate change in which to also remove
> verify_regular_instructions_old.py, since this file lives in validator_ragel
(so
> we might otherwise easily not notice that it has become dead code).
> 
> Also remove the mention of verify_regular_instructions_old.py (typo'd as
> "instruction_old") from src/trusted/validator_ragel/docs/testing.html.

Done.

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
File src/trusted/validator_ragel/build.scons (right):

https://codereview.chromium.org/636933004/diff/20001/src/trusted/validator_ra...
src/trusted/validator_ragel/build.scons:340: 
On 2014/10/07 21:53:46, Mark Seaborn wrote:
> Style nit: remove 1 empty line here (so that there's not two consecutive empty
> lines inside a function)

Done.

https://codereview.chromium.org/636933004/diff/20001/tests/stubout_mode/nacl....
File tests/stubout_mode/nacl.scons (left):

https://codereview.chromium.org/636933004/diff/20001/tests/stubout_mode/nacl....
tests/stubout_mode/nacl.scons:69: # Without any debug flags, the executable
fails validation.
On 2014/10/07 21:53:46, Mark Seaborn wrote:
> Can we keep this test case?
> 
> I have a strong suspicion that there is no other test that checks that
> standalone sel_ldr actually rejects non-validating executables.  (You could
> check this out by making sel_ldr ignore the result of main-nexe validation and
> seeing if any test fails.)

Done.

[Mark Seaborn, 2014-10-08 22:32:58]

LGTM

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
tests/stubout_mode/nacl.scons:53: test_suites = ['small_tests']
Shouldn't nonpexe_tests be kept?  This is still compilable and compiled with
pnacl_generate_pexe=0.

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
tests/stubout_mode/nacl.scons:74: 
Nit: remove trailing empty lines

[shyamsundarr, 2014-10-09 16:19:26]

Thanks!

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
tests/stubout_mode/nacl.scons:53: test_suites = ['small_tests']
On 2014/10/08 22:32:58, Mark Seaborn wrote:
> Shouldn't nonpexe_tests be kept?  This is still compilable and compiled with
> pnacl_generate_pexe=0.

non_pexe tests never included tests in this directory because  because non_pexe
tests were never run with validator_ragel == 0 from the top level buildbot. as
such it would always break out at line 48 (of the previous revision / TOT). it
actually fails. i have left a todo for myself to investigate/see if it's
fixable.

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
tests/stubout_mode/nacl.scons:74: 
On 2014/10/08 22:32:58, Mark Seaborn wrote:
> Nit: remove trailing empty lines

Done.

[Mark Seaborn, 2014-10-09 18:17:48]

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/636933004/diff/120001/tests/stubout_mode/nacl...
tests/stubout_mode/nacl.scons:53: test_suites = ['small_tests']
On 2014/10/09 16:19:26, shyamsundarr wrote:
> On 2014/10/08 22:32:58, Mark Seaborn wrote:
> > Shouldn't nonpexe_tests be kept?  This is still compilable and compiled with
> > pnacl_generate_pexe=0.
> 
> non_pexe tests never included tests in this directory because  because
non_pexe
> tests were never run with validator_ragel == 0 from the top level buildbot. as
> such it would always break out at line 48 (of the previous revision / TOT). it
> actually fails. i have left a todo for myself to investigate/see if it's
> fixable.

I see.  That makes sense.

[shyamsundarr, 2014-10-09 20:29:33]

Committed patchset #4 (id:200001) manually as 13876 (presubmit successful).