Delete old x86 validator.

Delete old x86 validator.

The old x86 validator was only being kept around for comparison testing with the new validator. Given our new testing strategy, it is time to remove it. 

TODO(shyamsundarr): Remove stubout mode completely given that only mips validator supports it.

validator/x86/ and sub directories are completely removed.
validator_x86/ now only contains the nccopycode library.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3454
TEST=trybots

[shyamsundarr, 2014-10-03 22:44:39]

Patchset #1 (id:1) has been deleted

[shyamsundarr, 2014-10-04 01:03:23]

shyamsundarr@chromium.org changed reviewers:
+ kschimpf@google.com, ncbray@chromium.org

[Mark Seaborn, 2014-10-06 17:40:00]

mseaborn@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2014-10-06 17:40:04]

This change is really big.  Could possibly you do this as smaller changes?  e.g.
Remove the "validator_ragel" bit from Scons and the validator_ragel=0 case from
the buildbot scripts first.

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
File src/trusted/validator_x86/testdata/64/mov-lea-rbp-bad-4.test (left):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
src/trusted/validator_x86/testdata/64/mov-lea-rbp-bad-4.test:46: @rdfa_output:
It looks like you're removing all of the targeted tests.  We don't want to do
that...

[Mark Seaborn, 2014-10-06 17:47:55]

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/bu...
File src/trusted/validator/build.scons (right):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/bu...
src/trusted/validator/build.scons:14: val_lib_env = env.Clone()
Nit: the Clone() is not needed if the Append() is removed.  s/val_lib_env/env/
below.

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/na...
File src/trusted/validator/nacl.scons (right):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/na...
src/trusted/validator/nacl.scons:23:
env['TRUSTED_ENV'].File('${STAGING_DIR}/ncval_new${PROGSUFFIX}'),]
How about doing a cleanup first to rename "ncval_new" to "ncval"?

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
File src/trusted/validator_x86/build.scons (left):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
src/trusted/validator_x86/build.scons:266:
env.File('testscripts/run_rdfa_validator_tests.py'),
You're removing tests for the Ragel validator here...

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

https://codereview.chromium.org/625923004/diff/10001/tests/stubout_mode/nacl....
tests/stubout_mode/nacl.scons:50: if env.Bit('build_x86'):
This disables the rest of the file on both x86 and ARM.  There's not much point
keeping the code below only for MIPS (which isn't tested on the bots).  How
about doing a separate cleanup change to remove this code from this point
downwards?

[Vlad Shcherbina, 2014-10-06 18:01:45]

shcherbina@google.com changed reviewers:
+ shcherbina@google.com

[Vlad Shcherbina, 2014-10-06 18:01:50]

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
File src/trusted/validator_x86/testdata/64/mov-lea-rbp-bad-4.test (left):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
src/trusted/validator_x86/testdata/64/mov-lea-rbp-bad-4.test:46: @rdfa_output:
On 2014/10/06 17:40:04, Mark Seaborn wrote:
> It looks like you're removing all of the targeted tests.  We don't want to do
> that...

No, real targeted tests are in src/trusted/validator_ragel/testdata/. Originally
they were copied from here, but since then diverged: old validator and
disassembler outputs were removed from them, spec (regex-based) validator
expectations were added. In addition, I did a lot to improve quality of those
tests (partitioned them into smaller units, deduplicateed, improved comments,
fixed defects in tests, added tests for other interesting cases; overall number
of testcases went from ~300 to ~530; issue 3487). 

All these changes were not reflected in validator_x86/testdata tests.

It is awesome that Shyamsundar is deleting this piece of legacy.

[shyamsundarr, 2014-10-06 22:59:00]

Patchset #2 (id:20001) has been deleted

[shyamsundarr, 2014-10-06 22:59:26]

Patchset #2 (id:20001) has been deleted

[shyamsundarr, 2014-10-06 23:06:57]

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/bu...
File src/trusted/validator/build.scons (right):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/bu...
src/trusted/validator/build.scons:14: val_lib_env = env.Clone()
On 2014/10/06 17:47:55, Mark Seaborn wrote:
> Nit: the Clone() is not needed if the Append() is removed.  s/val_lib_env/env/
> below.

Done.

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/na...
File src/trusted/validator/nacl.scons (right):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator/na...
src/trusted/validator/nacl.scons:23:
env['TRUSTED_ENV'].File('${STAGING_DIR}/ncval_new${PROGSUFFIX}'),]
On 2014/10/06 17:47:55, Mark Seaborn wrote:
> How about doing a cleanup first to rename "ncval_new" to "ncval"?

i have left a TODO in SConstruct. i think this needs to be a separate and
careful cleanup because ncval_new has references outside of our src repo like
the native_client_sdk build?

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
File src/trusted/validator_x86/build.scons (left):

https://codereview.chromium.org/625923004/diff/10001/src/trusted/validator_x8...
src/trusted/validator_x86/build.scons:266:
env.File('testscripts/run_rdfa_validator_tests.py'),
On 2014/10/06 17:47:55, Mark Seaborn wrote:
> You're removing tests for the Ragel validator here...

As vlad replied to the similar comment, there is a rdfa_validator tests in the
validator_ragel/ directory that run similar tests. these are just legacy
duplicate tests.

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

https://codereview.chromium.org/625923004/diff/10001/tests/stubout_mode/nacl....
tests/stubout_mode/nacl.scons:50: if env.Bit('build_x86'):
On 2014/10/06 17:47:55, Mark Seaborn wrote:
> This disables the rest of the file on both x86 and ARM.  There's not much
point
> keeping the code below only for MIPS (which isn't tested on the bots).  How
> about doing a separate cleanup change to remove this code from this point
> downwards?

Done here for now but i will break it out if i split this into multiple changes.

[shyamsundarr, 2014-10-06 23:08:54]

On 2014/10/06 17:40:04, Mark Seaborn wrote:
> This change is really big.  Could possibly you do this as smaller changes? 
e.g.
> Remove the "validator_ragel" bit from Scons and the validator_ragel=0 case
from
> the buildbot scripts first.

sure, i can definitely play with that if it will be easier to review. let me
keep this change open and try to rebase after getting some smaller changes in.

[shyamsundarr, 2014-10-09 21:53:21]

On 2014/10/06 23:08:54, shyamsundarr wrote:
> On 2014/10/06 17:40:04, Mark Seaborn wrote:
> > This change is really big.  Could possibly you do this as smaller changes? 
> e.g.
> > Remove the "validator_ragel" bit from Scons and the validator_ragel=0 case
> from
> > the buildbot scripts first.
> 
> sure, i can definitely play with that if it will be easier to review. let me
> keep this change open and try to rebase after getting some smaller changes in.

Thanks! that was a good suggestion. ptal. this change is now purely deleting
files in validator/x86/ and in validator_x86/

[Mark Seaborn, 2014-10-09 22:02:04]

LGTM, assuming that this only deletes files.  (Sometimes Rietveld shows "D" for
files being modified.)  One could check with "git diff --diff-filter=M
origin/master" -- the output should be empty.

Also remove this bit from the commit message: "Remove validator_ragel==0/1 build
option (it is always true)."

Does this remove all of validator_x86/ and validator/x86/?  If not, can you
explain what remains in the commit message?

[shyamsundarr, 2014-10-09 22:07:03]

On 2014/10/09 22:02:04, Mark Seaborn wrote:
> LGTM, assuming that this only deletes files.  (Sometimes Rietveld shows "D"
for
> files being modified.)  One could check with "git diff --diff-filter=M
> origin/master" -- the output should be empty.
> 
> Also remove this bit from the commit message: "Remove validator_ragel==0/1
build
> option (it is always true)."
> 
> Does this remove all of validator_x86/ and validator/x86/?  If not, can you
> explain what remains in the commit message?

yep the command returns with empty output. updated the change description.

[shyamsundarr, 2014-10-13 17:40:01]

The CQ bit was checked by shyamsundarr@chromium.org

[commit-bot: I haz the power, 2014-10-13 17:41:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625923004/2870001

[commit-bot: I haz the power, 2014-10-13 18:09:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 18:09:35]

Try jobs failed on following builders:
  nacl-win64_newlib_opt on tryserver.nacl
(http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_newlib_opt/bui...)