474968 - verifyall doesn't

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

We want -Dverifyall to be a true early-verify mode.  Therefore MethodInfo.verifyEnter() and NativeFunction.verifyEnter() should never be called at runtime.  in builds without a jit, it should be possible for MethodInfo/Env->impl32 to be const.

Currently, verifyall is eager but not 100% eager, it is still triggered by a call to verifyEnter of a root method (a script init).

Comment 1

[Edwin Smith]

Attachment: add assert(false) to lazy-verify, eager-verify main

in verifyEnter, if config.verifyall is set, then assert(false).  Kick off eager verification by verifying the main method just before we call it.

this successfully runs simple abc's without asserting.  but:

- we want to verify earlier than just before we call the first method
- it doesn't directly handle non-entry point scripts in the abc.  if they aren't referenced by the main script they might not be early verified.

posted here for early review

Comment 2

[Edwin Smith]

Attachment: more thorough eager verification, tested on esc repl

- renamed enq functions to enqFunction and enqTraits
- renamed processVerifyQueue to verifyEarly()
- removed dead code from MethodInfo/NativeMethod.verifyEnter().  only assert when config.verifyall enabled
- enqueue activationObject traits when OP_newactivation is seen; some activation objects have generated init methods, which need early verification too.
- process script traits and init methods in prepareActionPool, before any init methods are called.

Tested this on esc compiling parse.es, as well as esc's REPL.  Should be a good test because it involves loading multiple abc's on the shell command line, processing abc's with multiple scripts (builtin & shell.abc), as well as early-verifying dynamically loaded abc's (esc compiles on the fly). 

Uncomment the print statement near the bottom of AvmCore.cpp to see the method names printed in the order they are verified, e.g.

$ shell/avmshell -Dverifyall -Dinterp <esc files> main.es.abc --
...
pre verify ESC::Flags$cinit()
pre verify compile()
pre verify global$init()
pre verify evaluateInScopeArray()
pre verify global$init()
pre verify <anonymous ../src/esc-env.es:56>()
pre verify global$init()
pre verify repl()
pre verify ?()
pre verify eval()
es> print("Hello World")
pre verify global$init()
Hello World
es>

Comment 3

[Lars T Hansen]

BTW, using this to verify the shell toplevel code there are two unverified functions, but I have not looked into it.  Otherwise this fix works very well for me.

Comment 4

[Edwin Smith]

i'll investigate, if theres a bug i'll fix it before pushing.  if those 2 methods really are unreachable, that'll be interesting too.

Comment 5

[Edwin Smith]

From what i can tell using said verbose mode and abcdump, all the builtin, shell, and user.abc methodinfo's are getting verified.  point me to a test case?  otherwise i think this is ready to go.. will update and push soon.

Comment 6

[Lars T Hansen]

You should just push it, I will investigate separately.

Comment 7

[Edwin Smith]

pushed to tamarin-redux
changeset:   1333:e3cc0ba266a2

Comment 8

[Brent Baker]

QE: Should look into adding the test that Edwin mentions in comment #2, running esc with -Dverifyall

Comment 9

[Chris Peyer]

Attachment: Add --verify switch to runtests

This patch adds --verify to runtests.

When enabled, this will compare the # of functions found with abcdump vs. the number of functions found with "-Dverifyall -Dverbose=verify" and produce an error if the # of functions differ.

Comment 10

[Chris Peyer]

Attachment: Updated patch

- Check and build abcdump.abc
- Remove need for recompiling .abcs
- Set vmargs so they are part of config

Comment 11

[Chris Peyer]

Test changes pushed redux changeset 2954	7a936553b2e9