491934 - AVM -Dverifyonly switch

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Tom Harwood]

-Dverifyonly would verify eagerly, as does -Dverifyall, but not execute client code.  Useful for testing verifier and JIT against e.g. code collected by the headless player without trying to execute the SWF.

Early experimentation indicates that the verifier/compiler subsystems depend on constructs initialized by executing the builtin AS code.

Comment 1

[Edwin Smith]

Attachment: prototype of -Dverifyonly

this is a big-ish patch that adds -Dverifyonly, and also adds a more concise listing of the verifier state for each method.  this listing and associated verify errors is printed to stderr.

The patch also relaxes parsing of abc's with native methods; since we are only verifying, we allow the abc to be verified even though the native methods are missing.

invocation:
avmshell -Dverifyonly [your-host-builtins.abc] files...

Comment 2

[Steven Johnson]

Attachment: Handle stagDoABC

Note, we currently ignore stagDoABC and process only stagDoABC2. This fixes that.

Comment 3

[Steven Johnson]

Comment on attachment 425544 [details] [diff] [review]
Handle stagDoABC

http://hg.mozilla.org/tamarin-redux/rev/2ea67797b635

Comment 4

[Edwin Smith]

Attachment: (v2) prototype of -Dverifyonly

Rebased, reverted short abc opcode names and increased indentation in TypeLister.

Comment 5

[Edwin Smith]

Attachment: (v3) prototype of -Dverifyonly

changes from previous patch:

* print to stdout and use core->console, instead of stderr
* Consolidate error handling in Toplevel::throwVerifyError().  
* make sure Assert messages have \n at the end

Putting all the error handling in throwVerifyError is cleaner, but is now exposing what seem to be latent bugs; in some cases it is possible to hit an assert instead of throwing a VerifyError, for cases when the Verifier causes Traits::_buildTraitsBindings() to be called with a NULL toplevel. 

I don't know if this is a behavior unique to -Dverifyonly, or a real latent bug.

Comment 6

[Edwin Smith]

Attachment: (v4) prototype of -Dverifyonly

changes since v3:
- rebased to Argo tip without cleanup patches
- stripped out TypeLister stuff
- added more ifdefs around verifyonly logic.

Comment 7

[Edwin Smith]

Comment on attachment 428915 [details] [diff] [review]
(v4) prototype of -Dverifyonly

This -Dverifyonly mode isn't critical in general but is essential for testing verifier changes.

squishy parts, feedback wanted:
- added fflush to ensure output is flushed before asserts happen
- added \n to assert message, i'm not sure this is the best place
- exit(1) ? alternative would be to throw some value (can't constuct any AS3 objects in this mode) and exit gracefully
-Dverifyonly is enabled whenever -Dverifyall is.  alternatively we could make a dedicated feature switch.

Comment 8

[Steven Johnson]

(In reply to comment #7)
> squishy parts, feedback wanted:
> - added fflush to ensure output is flushed before asserts happen
> - added \n to assert message, i'm not sure this is the best place
> - exit(1) ? alternative would be to throw some value (can't constuct any AS3
> objects in this mode) and exit gracefully
> -Dverifyonly is enabled whenever -Dverifyall is.  alternatively we could make a
> dedicated feature switch.

All these sound like reasonable decisions for this feature.

Comment 9

[Steven Johnson]

Comment on attachment 428915 [details] [diff] [review]
(v4) prototype of -Dverifyonly

approved with two caveats:

-- I like the idea of doing fflush() in VMPI_debugBreak, but not so much in VMPI_debugLog; that will dramatically slow down debug builds. Are those absolutely necessary? If not, remove.

-- IMHO, all uses of config.verifyonly should be ifdef VMCFG_VERIFYALL (eg, Toplevel::getBuiltinClass, Traits::resolveSignaturesSelf)

Comment 10

[Krzysztof Palacz]

AFAIK, if stdout is connected to a terminal, the output is line-buffered by default,  so only logging non newline-terminated strings would be affected performance-wise. In any case, I'd move fflush() until after the message is printed. 
I believe stderr is unbuffered by default, so it wouldn't need fflush(), and the man page says that abort() will flush all the streams (arguably, someone might have changed the defaults, so fflushing doesn't hurt).  
Would we want this behavior on other platforms as well?
Do the changes in CodegenLIR belong in this patch?

Comment 11

[Krzysztof Palacz]

Come think of it, the default buffering behavior for stdout is what we'd want (line buffering if connected to a terminal, otherwise let it rip), is there a case where not fflushing is an issue?

Comment 12

[Edwin Smith]

I'm testing like this, typically:

   avmshell .... >file.log 2>&1

And, without the flushes, when we hit an assert, the assert will end up near the end of the log file, but i want it on the last line, and i want the assert message on a line by itself with a newline. I have to admit i just sprinkled it in the fflush's, I will go back and find the minimal set.  but i can say that at least one was required, to get my error.

> Do the changes in CodegenLIR belong in this patch?

I factored them into bug 548901.  once i sort out the fflush stuff i'll repost with that part removed.

> -- IMHO, all uses of config.verifyonly should be ifdef VMCFG_VERIFYALL (eg,
Toplevel::getBuiltinClass, Traits::resolveSignaturesSelf)

agreed, any missing ifdefs is an oversight.  i'll review and update the patch.

Comment 13

[Edwin Smith]

Attachment: (v5) add -Dverifyonly to verify+jit but not execute any code

Changes since v4

* reverted fflush's
* rebased
* added missing #ifdef VMCFG_VERIFYALL in a couple of places.

Comment 14

[Edwin Smith]

pushed
http://hg.mozilla.org/tamarin-redux/rev/487b68a20cec

Comment 15

[Dan Schaffer]

marking verified.  Ran -Dverifyonly on brightspot and several other .  We should add a verifier test harness to allow running -Dverifyonly on ats swfs, brightspot swfs, ...  for regression testing the verifier.  If we have a way of marking expected failures we could mark test cases where rsls are used (class not found) and the other failures occurring.

Comment 16

[Edwin Smith]

Please, pretty please, can we get -Dverifyonly running some automated tests?  A single test of hello.abc would have caught the injection from bug 582296.

Comment 17

[Brent Baker]

What is the difference between -Dverifyall and -Dverifyonly. There are -Dverifyall runs in the deep phase on windows64 on the release-debugger and debug-debugger builds

Comment 18

[Edwin Smith]

-Dverifyonly doesn't run any code, and also supports verifying real swfs (in conjunction with avmglue.abc).  from usage:

          [-Dverifyall] verify greedily instead of lazily
          [-Dverifyonly] verify greedily and don't execute anything

Comment #15 outlines some ideas, but even the simple from comment #16 would be helpful.

Comment 19

[Brent Baker]

Right, one just verifies and one verifies and executes... so wouldn't a complete acceptance run have caught this bug using the -Dverifyall switch? Just wondering why the build system was able to run with -Dverifyall and NOT hit this assert (it tested this against builds that _should_ have hit the assert). I'll take a close look at what is being run

Comment 20

[Dan Schaffer]

we are running -Dverifyonly against all brightspot tests in mac-deep.  It had not been running correctly for a while but did successfully run against 4975 on Aug-4. We could add running acceptance tests fairly easily using the mac-deep/scripts/run-brightspot.sh script.

Comment 21

[Brent Baker]

Bug# 584758 was added to have the ability for runtests to handle running a -Dverifyonly pass, which once fixed we will be able to add a simple test into the smoke phase and then an additional pass in the deep phase

Comment 22

[Edwin Smith]

couple observations
1. if a test fails in deep tests, how long before we find out about it?
2. if deep tests aren't running, how long before we find out?
3. a smoketest for one or two -Dverifyonly tests would be great and would have caught the injection.
(debug-debugger on any platform would be ideal)

Comment 23

[Dan Schaffer]

I agree we should have simple acceptance tests to verify -Dverifyonly.  deep test failures can take several builds to find and they are not treated as urgently as test phase failures.

Comment 24

[Chris Peyer]

Functionality added to runtests and buildbot: see 584758