(abandoned) Validator tests: convert hexes to TFs and run on bots (for prod. validator only)

Validator tests: convert hexes to TFs and run on bots (for prod. validator only)

Add scripts to convert existing hex files (together with validator golden files) to TF format, as well as to run TF tests.
Add converted TFs themselves (they were obtained by running
  ./hex2tf.py --bits 32 "../../../../validator_x86/testdata/32/*.hex" converted/32
  ./hex2tf.py --bits 64 "../../../../validator_x86/testdata/64/*.hex" converted/64
  rm converted/32/mem_overflow.tf  # because it's very specific to prod. validator
)

TF tests were added to buildbot_standard.


BUG= http://code.google.com/p/nativeclient/issues/detail?id=2996

[Vlad Shcherbina, 2012-09-07 08:10:15]

Please take a look. Note step 'validator_tf_test' on bot.

[Vlad Shcherbina, 2012-09-10 16:56:08]

Minor changes to run on windows.

https://chromiumcodereview.appspot.com/10908137/diff/14001/src/trusted/valida...
File src/trusted/validator/x86/testing/tf/hex2tf.py (right):

https://chromiumcodereview.appspot.com/10908137/diff/14001/src/trusted/valida...
src/trusted/validator/x86/testing/tf/hex2tf.py:83: 
In anticipation of possible questions: there are two ways to achieve this
result: either complicate val_runner to support golden files (in this case
val_runner, annotate_tf and hex2tf will be slightly more indirect), or keep
common usage simple and use somewhat ugly monkeypatching only in hex2tf. Given
that hex2tf is a throw-away, I chose second approach.

[pasko-google - do not use, 2012-09-10 18:08:09]

Guys, is it still a part of the plan to eliminate hex files in the nearest
future (i.e. before R-DFA release)?

without removing the old format another historical layer is added making the
system overly complex and hard to sync between formats

[Mark Seaborn, 2012-09-10 19:38:06]

Some initial feedback:

This is still a large amount of Python code.  I am not yet convinced we will not
regress on test coverage by switching over to this.  I'm still reading through
it...

http://codereview.chromium.org/10908137/diff/14001/buildbot/buildbot_standard.py
File buildbot/buildbot_standard.py (right):

http://codereview.chromium.org/10908137/diff/14001/buildbot/buildbot_standard...
buildbot/buildbot_standard.py:371: with Step('validator_tf_test', status,
halt_on_fail=False):
Don't add a new Buildbot step, please.  Scons is slow to start up, so each Scons
invocation on the bots adds to the slowness.  We usually only have different
steps where we need to pass difference options to Scons.

Please just add the new tests to small_tests, which is where the validator tests
are now.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/annotate_tf.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/annotate_tf.py:54: parser =
optparse.OptionParser('prog% [options] <tf file or wildcard>')
You mean "%prog" not "prog%", I think

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/annotate_tf.py:59: help=("don't change the
file, just print the difference between what is "
I would suggest that not changing the file should be the default, since that is
safer.  Change this to "--update" and invert the meaning?

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/build.scons (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/build.scons:23: (annotate,) = env.Command(
Please use AutoDepsCommand() instead.  Then it will track dependencies for you
automatically, and you can write something like:

AutoDepsCommand(..., ['${PYTHON}', env.File('annotate_ty.py'), '--ncval', ncval,
...)

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/build.scons:31: '--check "%s"' % (bits,
tf_wildcard)
Passing a wildcard as a command line argument is strange, especially as you're
setting "tfs" (expanded) as "source".

Please either:
 1) pass the full list here (but this might hit command line length limits)
 2) pass a directory name and have the script search the directory

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/build.scons:35:
env.AlwaysBuild(env.Alias('validator_tf_test', [annotate]))
You should be using AddNodeToTestSuite() to add this to small_tests.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/converted/32/addr16.tf (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/converted/32/addr16.tf:1: BITS: 32
"converted" isn't a good name for the directory if this is going to be the final
place for the test files.  And I wouldn't like to have to move them all in a
later change.

So I'd suggest putting them in the existing
src/trusted/validator_x86/testdata/{32,64} directories as *.tf files.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/hex2tf.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/hex2tf.py:66: tf_file = hex_base+'.tf'
Nit: put spaces around '+'

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/tf.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:52: assert ':' in line, "can't parse
line '%s" % line
Nit: missing closing '

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:216: nops = '\x90'*i.size
Nit: put spaces around '*'.  Also comment that you are padding with NOPs and 90
is NOP.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/utils.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/utils.py:62: def __exit__(self, exc_type,
ext_value, traceback):
Nit: 'ext_value' -> 'exc_value'

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/utils.py:64: if
os.path.exists(self.file.name):
Nit: can you put a comment in to say which platforms the exists() check is
necessary on?

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/val_runner.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:17: ncval32 =
os.path.join(scons_out, 'opt-linux-x86-32/staging/ncval')
You shouldn't hard-code Scons output filenames like this...

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:29: global ncval32
It would be cleaner if you didn't modify global variables like this.  Can you
pass around the options object instead?

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:91: if re.match(r'.+ > .+
\(read overflow of .+ bytes\)', line):
Why are you dropping this line of output?

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:93: if line ==
'ErrorSegmentation':
Ditto: why drop this?

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:110: #print line
Please remove commented-out code

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:149: assert False, "can't
parse line '%s'" % line
Better style would be:
  raise AssertionError("can't parse line '%s'" % line)

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:179: def ParseDfa(lines):
Please can you take out all of the DFA-validator related stuff from this file,
and save it for a later change?

[Vlad Shcherbina, 2012-09-11 14:25:59]

Mostly done, although some remarks are appealed against.

http://codereview.chromium.org/10908137/diff/14001/buildbot/buildbot_standard.py
File buildbot/buildbot_standard.py (right):

http://codereview.chromium.org/10908137/diff/14001/buildbot/buildbot_standard...
buildbot/buildbot_standard.py:371: with Step('validator_tf_test', status,
halt_on_fail=False):
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Don't add a new Buildbot step, please.  Scons is slow to start up, so each
Scons
> invocation on the bots adds to the slowness.  We usually only have different
> steps where we need to pass difference options to Scons.
> 
> Please just add the new tests to small_tests, which is where the validator
tests
> are now.

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/annotate_tf.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/annotate_tf.py:54: parser =
optparse.OptionParser('prog% [options] <tf file or wildcard>')
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> You mean "%prog" not "prog%", I think

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/annotate_tf.py:59: help=("don't change the
file, just print the difference between what is "
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> I would suggest that not changing the file should be the default, since that
is
> safer.  Change this to "--update" and invert the meaning?

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/build.scons (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/build.scons:23: (annotate,) = env.Command(
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Please use AutoDepsCommand() instead.  Then it will track dependencies for you
> automatically, and you can write something like:
> 
> AutoDepsCommand(..., ['${PYTHON}', env.File('annotate_ty.py'), '--ncval',
ncval,
> ...)

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/build.scons:31: '--check "%s"' % (bits,
tf_wildcard)
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Passing a wildcard as a command line argument is strange, especially as you're
> setting "tfs" (expanded) as "source".
> 
> Please either:
>  1) pass the full list here (but this might hit command line length limits)
>  2) pass a directory name and have the script search the directory
Explicit list of files perhaps has some advantages, but you mentioned limits
yourself (iirc, it's 2k for older versions of windows).
And why passing 'whatever_dir/32/' is better than 'whatever_dir/32/*.tf'? On the
other hand, wildcards offer additional flexibility (for instance, script can be
run on a single file).

It would make sense if scons was able to track directory dependencies, but
alas...

I'll leave it unless you object.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/build.scons:35:
env.AlwaysBuild(env.Alias('validator_tf_test', [annotate]))
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> You should be using AddNodeToTestSuite() to add this to small_tests.

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/converted/32/addr16.tf (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/converted/32/addr16.tf:1: BITS: 32
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> "converted" isn't a good name for the directory if this is going to be the
final
> place for the test files.  And I wouldn't like to have to move them all in a
> later change.
> 
> So I'd suggest putting them in the existing
> src/trusted/validator_x86/testdata/{32,64} directories as *.tf files.

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/hex2tf.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/hex2tf.py:66: tf_file = hex_base+'.tf'
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Nit: put spaces around '+'

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/tf.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:52: assert ':' in line, "can't parse
line '%s" % line
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Nit: missing closing '

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:216: nops = '\x90'*i.size
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Nit: put spaces around '*'.  Also comment that you are padding with NOPs and
90
> is NOP.

I think the name of the variable suggests it by itself, comment here would be
redundant. But I realized that the whole process requires some explanation, so I
added a more voluminous comment few lines above.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/utils.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/utils.py:62: def __exit__(self, exc_type,
ext_value, traceback):
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Nit: 'ext_value' -> 'exc_value'

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/utils.py:64: if
os.path.exists(self.file.name):
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Nit: can you put a comment in to say which platforms the exists() check is
> necessary on?

Done. It's not the platform, but the fact that GNU AS removes output file if
there are compilation errors.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/val_runner.py (right):

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:17: ncval32 =
os.path.join(scons_out, 'opt-linux-x86-32/staging/ncval')
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> You shouldn't hard-code Scons output filenames like this...
I was using it to run locally (on bots paths are always passed explicitly), but
right, it's time to get rid of it now.
Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:29: global ncval32
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> It would be cleaner if you didn't modify global variables like this.  Can you
> pass around the options object instead?
Well, I don't like it because it would mean passing mostly irrelevant argument
through chains of calls. I think this case does not qualify as mutable global
variables, because assignment is supposed to happen only once, so usual perils
of globals are more or less avoided. I prefer to leave it as it is unless you
really insist.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:91: if re.match(r'.+ > .+
\(read overflow of .+ bytes\)', line):
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Why are you dropping this line of output?

Right, this should not happen anyway. It's something very specific to prod.
validator, and it only arises in mem_overflow.hex. Removed the check.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:93: if line ==
'ErrorSegmentation':
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Ditto: why drop this?
Same.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:110: #print line
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Please remove commented-out code

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:149: assert False, "can't
parse line '%s'" % line
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Better style would be:
>   raise AssertionError("can't parse line '%s'" % line)

Done.

http://codereview.chromium.org/10908137/diff/14001/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/val_runner.py:179: def ParseDfa(lines):
On 2012/09/10 19:38:06, Mark Seaborn wrote:
> Please can you take out all of the DFA-validator related stuff from this file,
> and save it for a later change?

Done.

[Vlad Shcherbina, 2012-09-20 16:27:11]

This CL is abandoned. 

Work goes on in http://code.google.com/p/nativeclient/issues/detail?id=3037 and
related CLs.

[Mark Seaborn, 2012-09-27 02:25:42]

Below are some pending comments that I didn't get around to sending.

I'm posting them for the record, but you can ignore them, since this change has
been superseded.

http://codereview.chromium.org/10908137/diff/12181/src/trusted/validator/x86/...
File src/trusted/validator/x86/testing/tf/tf.py (right):

http://codereview.chromium.org/10908137/diff/12181/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:36: def CheckAsm(self, bits):
This method doesn't seem to be called.

http://codereview.chromium.org/10908137/diff/12181/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:65: else:
Hmm, having a fallback here means that in normal testing mode (on the bots),
you're not enforcing that every section contains a "hex" field.  Ideally you
would be strict and enforce that.

http://codereview.chromium.org/10908137/diff/12181/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:88: 'bits',
Some comments on what these fields mean would be helpful.

http://codereview.chromium.org/10908137/diff/12181/src/trusted/validator/x86/...
src/trusted/validator/x86/testing/tf/tf.py:223: # will likely be spotted as
discrepancies compared to prod. validator.
This means you're relying on having the prod validator around for testing
purposes.  But when it goes away, doesn't it leave this NOP-patching strategy on
shakier ground?