Enable vdiff test on validator bots

Modify vdiff to accept a "--easydiff" option.

On 64-bits, this option causes vdiff to run a test with a single REX prefix and with no REX prefix. The default is to use 16 REX prefixes, resulting in a test that requires about two hours to run. With this change it runs in about 15-minutes, better suited for the bots.

Also, update SConscript and buildbot scripts to run vdiff on validator bots.

BUG=nativeclient:2597
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=9596

[Brad Chen, 2012-08-28 21:03:08]

Brad Nelson for bot changes.
Karl for vdiff changes.
Trybot runs are in progress...

[bradn, 2012-08-28 21:14:10]

http://codereview.chromium.org/10871078/diff/23001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/10871078/diff/23001/SConstruct#newcode340
SConstruct:340: BitFromArgument(env, 'validator_diff_tests', default=False,
So I think you should be able to drop this.
This is adding a boolean argument like:
validator_diff_tests=0 etc.
Whereas your actual target is just a normal test target (which should work fine
as you've added it to a new test suite).

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/enuminsts/build.scons (right):

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/build.scons:20: if
env.AllBits('windows', 'debug'):
So this crt stuff is obsolete. I should do a pass thru the code base and remove.
Leave it for this change though.

[Karl, 2012-08-28 21:23:52]

I did not check the bot building code, and will leave that to brad. My comments
focussed on the enuminst code.

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/enuminsts/build.scons (right):

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/build.scons:58:
env.AddNodeToTestSuite(node, ['validator_diff_tests'], 'vdiff_test')
Should there be a blank line here for readability?

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/enuminsts/vdiff.c (right):

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/vdiff.c:278: static char*
StrPrefix(const unsigned int prefix, char* ps, char* str) {
It is not clear to me what the arguments here are. A comment would be helpful.

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/vdiff.c:331: const uint32_t kREXMax
= 0x50;  /* kREXBase + kREXRange */
Add a blank line here?

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/vdiff.c:368: rprefix = (prefix << 8
| irex);
Could this operation be converted to a MACRO. It appears several times in the
code, and isn't obvious that you are appending a value to the prefix.

[Brad Chen, 2012-08-28 22:09:42]

Thanks for the feedback. PTAL.

http://codereview.chromium.org/10871078/diff/23001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/10871078/diff/23001/SConstruct#newcode340
SConstruct:340: BitFromArgument(env, 'validator_diff_tests', default=False,
On 2012/08/28 21:14:10, bradn wrote:
> So I think you should be able to drop this.
> This is adding a boolean argument like:
> validator_diff_tests=0 etc.
> Whereas your actual target is just a normal test target (which should work
fine
> as you've added it to a new test suite).

Done.

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/enuminsts/build.scons (right):

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/build.scons:58:
env.AddNodeToTestSuite(node, ['validator_diff_tests'], 'vdiff_test')
On 2012/08/28 21:23:52, Karl wrote:
> Should there be a blank line here for readability?

Done.

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/enuminsts/vdiff.c (right):

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/vdiff.c:278: static char*
StrPrefix(const unsigned int prefix, char* ps, char* str) {
On 2012/08/28 21:23:52, Karl wrote:
> It is not clear to me what the arguments here are. A comment would be helpful.

Done.

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/vdiff.c:331: const uint32_t kREXMax
= 0x50;  /* kREXBase + kREXRange */
On 2012/08/28 21:23:52, Karl wrote:
> Add a blank line here?

Done.

http://codereview.chromium.org/10871078/diff/23001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/enuminsts/vdiff.c:368: rprefix = (prefix << 8
| irex);
On 2012/08/28 21:23:52, Karl wrote:
> Could this operation be converted to a MACRO. It appears several times in the
> code, and isn't obvious that you are appending a value to the prefix.

Done.

[bradn, 2012-08-28 22:11:48]

LGTM on the build.

[Karl, 2012-08-29 15:51:01]

lgtm