Status

Tamarin
Virtual Machine
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Edwin Smith, Assigned: Edwin Smith)

Tracking

unspecified
Bug Flags:
in-testsuite +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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).
(Assignee)

Updated

9 years ago
Assignee: nobody → edwsmith
(Assignee)

Comment 1

9 years ago
Created attachment 358372 [details] [diff] [review]
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
Attachment #358372 - Flags: review?(lhansen)
(Assignee)

Comment 2

9 years ago
Created attachment 358397 [details] [diff] [review]
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>
Attachment #358372 - Attachment is obsolete: true
Attachment #358397 - Flags: review?(lhansen)
Attachment #358372 - Flags: review?(lhansen)

Updated

9 years ago
Attachment #358397 - Flags: review?(lhansen) → review+

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
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

9 years ago
You should just push it, I will investigate separately.
(Assignee)

Comment 7

9 years ago
pushed to tamarin-redux
changeset:   1333:e3cc0ba266a2
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 8

8 years ago
QE: Should look into adding the test that Edwin mentions in comment #2, running esc with -Dverifyall
Flags: in-testsuite?

Updated

8 years ago
Status: RESOLVED → VERIFIED

Comment 9

8 years ago
Created attachment 409114 [details] [diff] [review]
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.
Attachment #409114 - Flags: review?(brbaker)

Comment 10

8 years ago
Created attachment 409179 [details] [diff] [review]
Updated patch

- Check and build abcdump.abc
- Remove need for recompiling .abcs
- Set vmargs so they are part of config
Attachment #409114 - Attachment is obsolete: true
Attachment #409179 - Flags: review?(brbaker)
Attachment #409114 - Flags: review?(brbaker)

Updated

8 years ago
Attachment #409179 - Flags: review?(brbaker) → review+

Comment 11

8 years ago
Test changes pushed redux changeset 2954	7a936553b2e9
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.