validator_ragel: Add ncval tests from the old validator

validator_ragel: Add ncval tests from the old validator

The old validator individually analyses all instructions, and only later checks
for bundle alignment rules.  The validator_ragel fails at the first invalid
instruction it encounters.

We will introduce a feature to the validator_ragel to be able to continue with
validation after error (useful for fixing invalid code), but this would be hard
to rely on in tests since the validator would often continue to restart
from non-interesting bytes resulting in numerous false positives.

Hence, the test runner (run_ncval_tests.py) cuts each test bundle by bundle,
runs these bundles through the validator.  Instructions that cross a bundle get
additional run, on that they get placed at offset 0.

The "parse_hex.py" is used to convert the old validator output from *.rval files
into the new golden files.  It parses the contents of testdata/64 for tests,
generalizes the validation errors it sees into 2 categories: (1) crossing a
32-byte bundle, (2) all others.  For each error in (2) the offset of the first
instructions' byte is recorded into the golden file.  Some of the files cannot
yet be parsed, for example, containing the '@' syntax, this is a TODO for later
commits.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2597
TEST=make check-ncval

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

[Mark Seaborn, 2012-02-20 22:27:32]

On 20 February 2012 09:58, <pasko@google.com> wrote:

> Reviewers: khim, Mark Seaborn,
>
> Description:
> validator_ragel: Add ncval tests from the old validator
>
> The old validator individually analyses all instructions, and only later
> checks
> for bundle alignment rules.  The validator_ragel fails at the first invalid
> instruction it encounters.
>
> We will introduce a feature to the validator_ragel to be able to continue
> with
> validation after error (useful for fixing invalid code), but this would be
> hard
> to rely on in tests since the validator would often continue to restart
> from non-interesting bytes resulting in numerous false positives.
>
> Hence, the test runner (run_ncval_tests.py) cuts each test bundle by
> bundle,
> runs these bundles through the validator.  Instructions that cross a
> bundle get
> additional run, on that they get placed at offset 0.
>
> The "parse_hex.py" is used to convert the old validator output from *.rval
> files
> into the new golden files.  It parses the contents of testdata/64 for
> tests,
> generalizes the validation errors it sees into 2 categories: (1) crossing a
> 32-byte bundle, (2) all others.  For each error in (2) the offset of the
> first
> instructions' byte is recorded into the golden file.  Some of the files
> cannot
> yet be parsed, for example, containing the '@' syntax, this is a TODO for
> later
> commits.
>

To me, this seems like you're skirting around the problems with the current
validator's test suite rather than solving them.

The current test suite has three problems:

1) It checks validator-implementation-specific output via golden files.

2) It expects the validator to continue running after a validation error
and produce more validation errors.  To address this properly, we should
split test cases that contain a mixture of expected-to-be-accepted
instructions and expected-to-be-rejected instructions into separate test
cases.

3) The test cases are fragmented across a zillion files, which makes the
test suite as a whole hard to understand.  It is difficult to add
comments.  I think we need a test framework where many test cases can be
put in one file, as with validator_test.py in my validator codebase.

If you duplicate the test list from src/trusted/validator_x86/testdata/64
into another directory, it's going to get out of sync.  In fact, it seems
to be out of sync already, because you're missing "test_alias.*" from r7794.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[pasko-google - do not use, 2012-02-21 12:23:06]

Mark,
thank you for quick response.

On 2012/02/20 22:27:32, Mark Seaborn wrote:
> To me, this seems like you're skirting around the problems with the current
> validator's test suite rather than solving them.

As a general note I should say that I am not trying to solve the problems with
the existing validator testsuite, it would be too much unnecessary work.
However, I am open to discussion on how to make a testsuite better for new
validators.  The scripts I wrote should be convertible to support other formats
of tests.

> The current test suite has three problems:
> 
> 1) It checks validator-implementation-specific output via golden files.

Agreed.  It makes the testsuite non-portable across validators.  With my
approach the golden files do *not* contain validator-specific messages, they are
an ordered collection of offsets of rejected instructions, can be easily parsed
and converted to other test suites.

> 2) It expects the validator to continue running after a validation error
> and produce more validation errors.  To address this properly, we should
> split test cases that contain a mixture of expected-to-be-accepted
> instructions and expected-to-be-rejected instructions into separate test
> cases.

Yes, continuing after an error creates a lot of problems and is hard to
understand.  I address this with validating each of the rejected instructions
separately.  It differs from your suggestion in only the detail that it does not
separate invalid instructions from valid in the source input.  Which is not
essential, I think.

> 3) The test cases are fragmented across a zillion files, which makes the
> test suite as a whole hard to understand.  It is difficult to add
> comments.  I think we need a test framework where many test cases can be
> put in one file, as with validator_test.py in my validator codebase.

This is not a significant issue.  Following your logic all programs must be
written in a single file, so that they are not fragmented.  A zillion files is
not a technical problem for us since we have grep and text editors able to open
bazillion of files.  For the extreme we have 'cat'.  Separating code/data in
files is a matter of personal preference and can cause severe "religious" wars,
let's avoid that.

I have 2 points for having separate files:
* easier to sync the new golden files with the testdata/64 via parse_hex.py
* (minor) browsing a code review is easier for changes in a small number of
    tests: easy to see what tests are affected.

Later when the curent validator gets obsolete (i.e. not have to sync any more)
we can convert the script to take all inputs from a single file. It is super
easy with changing the current runner script.

The validator_test.py is an example of mixing concepts together unnecessarily
which may result in more complicated code too quickly.  Specifically, having two
programming languages in one file is unnecessarily complex: reviewing it without
syntax highlighting is a pain. Also, giving too much power to test writers (such
as "any arbitrary code in python") generally leads testing systems to
unmanageable disaster.

> If you duplicate the test list from src/trusted/validator_x86/testdata/64
> into another directory, it's going to get out of sync.  In fact, it seems
> to be out of sync already, because you're missing "test_alias.*" from r7794.

To address the out-of-sync issues I wrote a fully automatic script that converts
the files in src/trusted/validator_x86/testdata/64 into new golden files, it
whould be a policy to not edit these files manually afterwards, see
src/trusted/validator_ragel/unreviewed/golden/README in this review.

[khim, 2012-02-21 14:29:51]

LGTM

Not sure if it's good for final version but for now it's in "unreviewed "and I
REALLY want to run these tests easily when I change/fix validator.