554915 - Verifier doesn't account for interrupt checks in empty loops in wordcode interp

Status: RESOLVED FIXED

(Tamarin Graveyard :: Interpreter, defect, P1)

Description

[Edwin Smith]

There is an implicit exception edge from the loop header to each catch block, any time you have a loop label inside a try block.  That's where the exception check occurs and it might be the only thing making the catch block reachable.

Test case:

try {
	while(true)
	{
	}
} catch (err) {
	try{
		while(true)
		{
		}
	}
	catch (err2){
		result = grabError(err2, err2.toString());
	}
	finally {
		result = grabError(err2, err2.toString());
	}
} finally {
	this[fileName+"Str"][testIndex] = "A script failed to exit after 30 seconds and was terminated. ";
	this[fileName+"Ans"][testIndex] = "Error #" + code;
	this[fileName][testIndex] = result;
	testIndex++;
}

Comment 1

[Edwin Smith]

Attachment: Ensure that when a loop header occurs in a try block, the catch block is reachable.

The implicit interrupt check can throw an exception, which the verifier must model like any other exception source.

Comment 2

[Edwin Smith]

Attachment: interrupt exceptions should originate from loop edge targets, as modeled by Verifier

I redefined SET_EXPC_S24 as SET_EXPC_TARGET, and it sets expc to the pending target of the branch, instead of the current pc.  If an interrupt exception is thrown by branchCheck it will be based on the target pc.  this matches Verifier's exception modelling and JIT's codegen.

Also removed the redundant (go < 0) check in branchCheck(), and moved it to Interpreter.cpp since it's only called from there.

I double checked that branch condition logic sets expc correctly before running (say) equals().  The fast paths for int/double compare+branch could be sped up by only setting expc for the "generic_cmp" case, but the logic is tricky, because we dont detect that case until after reading the branch offset.  So this patch leaves it alone.

Comment 3

[Lars T Hansen]

Comment on attachment 434870 [details] [diff] [review]
interrupt exceptions should originate from loop edge targets, as modeled by Verifier

If I were to quibble it would be that SAVE_EXPC_TARGET should take i1 as a parameter.

Comment 4

[Edwin Smith]

I actually quibbled over that when coding, so I'll make it so before commit.

Comment 5

[Rick Reitmaier]

Comment on attachment 434868 [details] [diff] [review]
Ensure that when a loop header occurs in a try block, the catch block is reachable.

Might be nice to add comments to the "if (pc < tryTo .. " as its no longer as clear as it once was.

Comment 6

[Edwin Smith]

Comment on attachment 434870 [details] [diff] [review]
interrupt exceptions should originate from loop edge targets, as modeled by Verifier

pushed to tr and tr-argo
http://hg.mozilla.org/tamarin-redux/rev/eb34444d201a

Comment 7

[Edwin Smith]

Comment on attachment 434868 [details] [diff] [review]
Ensure that when a loop header occurs in a try block, the catch block is reachable.

pushed to tr and tr-argo
http://hg.mozilla.org/tamarin-redux/rev/d43bf6dbfe43

Comment 8

[Chris Peyer]

Since there is no timeout in the avmshell - is there an .as testcase that will test this behaviour in the shell?

Comment 9

[Brent Baker]

There is a shell timeout 
[-Dtimeout]   enforce maximum 15 seconds execution

However running with the testcase in comment #0 I am  not really sure what the expected outcome of the test is. Running with avmshells that are before and after this change in tr-argo (3868, 3871) I get the same output of the test:

Error: Error #1503: A script failed to exit after 30 seconds and was terminated.
        at global$init()

I assume that it is not possible to "catch" a script timeout so not sure what the original failure was.

Comment 10

[Edwin Smith]

See ShellCore::interrupt() in ShellCore.cpp.

It is possible to catch a script timeout.  The initial timeout is hardcoded in the shell to 15 seconds at which point it throws a kScriptTimeoutError (#1502).  the test script continues to run for 5 more seconds (also hardcoded), causing an uncatchable kScriptTerminatedError is thrown (#1503).  total runtime is ~20 seconds, if things are working right.  (the error message is wrong, its 20 not 30, based on the shell's hardcoded constants).

timeout detection is slightly different between jit and interp, so the test needs to run both ways.

Comment 11

[Chris Peyer]

Attachment: Testcase for catchable timeout

We don't have any testcases that test the catchable timeout in the shell.  This only tests an Error #1502.

Missing in this diff are modifications to testconfig to only run this in deep as it takes 15s for the test to run.

Comment 12

[Chris Peyer]

Regarding testing kScriptTerminatedError (Error #1503) is it sufficient to run the testcase in the description and to just verify that an Error #1503 is thrown?  We currently don't have a way of failing a test in the suite that exceeds a certain run time (though it could be added).

As Brent mentions in comment #9, the external behavior has not changed pre/post fix.

Comment 13

[Edwin Smith]

save test as t.as; asc t.as => t.abc

before:
avmshell -Dtimeout -Ojit t.abc
eight:~/vm/argo/objdir-d-jit-x86$ time shell/avmshell -Dtimeout -Ojit ../t.abc
Illegal instruction

real	0m15.575s
user	0m15.051s
sys	0m0.007s

after:
avmshell -Dtimeout -Ojit t.abc
Error: Error #1503

real	0m20.060s
user	0m20.048s
sys	0m0.008s

The cause of the illegal instruction is the subject of the first patch.  The initial (catchable) timeout occurs, the test case should catch it and loop for 5 more seconds, but the bug causes a crash in the catch block.  if there is any side effect code in the try-block loop, such as a call, the crash won't occur.    And, it won't occur with the interpreter.

The second patch fixes an observed problem but needs a test case, and i'm not sure if abcasm can generate one. you want code structured like this:

    looplabel:
    try {
        jmp looplabel
    } catch(e) { ... }

before the second patch, a timeout exception is thrown from the jmp (or any other branch instruction.... all should be tested, really).  but the timeout exception *should* be thrown from the label position, which is outside the try block, and therefore should not be caught.  The but manifests in the abc interpreter and wordcode interpreter, but not the jit.  (jit throws timeouts from the label position).

here's another variant:

     try {
         looplabel:
     } catch (e) { ... }
     jmp looplabel

in this case, its the other way around.  the timeout *should* be caught, since the label is in the try block.  before the patch, the abc/wordcode interpreters won't catch the timeout, but the jit will.  (the jit is correct).

Comment 14

[Edwin Smith]

Comment on attachment 437720 [details] [diff] [review]
Testcase for catchable timeout

good so far, need another one that spins after catching #1502, which should trigger #1503 next.  presumably this will get tested with -Ojit by the various buildsteps?  (the bug was specifically with -Ojit, but the test should ensure jit/interp behave the same way).

Comment 15

[Chris Peyer]

Attachment: testcase that verifies that a second timeout throws Error #1503

Testcase depends on patch from Bug 558126.

Comment 16

[Edwin Smith]

Comment on attachment 437983 [details] [diff] [review]
testcase that verifies that a second timeout throws Error #1503

Looks workable, although the finally blocks are extraneous for this test.  Suggest you remove them, as long as the test still works like we expect.

Comment 17

[Chris Peyer]

testcases for catchable timeout and double timeout pushed to tr and argo:
changeset 4375	534c78ed88a2

Testcases for comment #13 still need to be created.

Comment 18

[Chris Peyer]

Attachment: abcasm loop testcases

Testcases for comment #13

Comment 19

[Chris Peyer]

Tests pushed to argo: http://asteam/hg/tamarin-redux-argo/rev/028bf25c2ed2

Comment 20

[Chris Peyer]

avmshell-wordcode shell fails abcasm/bug_554915b (added in attachment 439133 [details] [diff] [review]).  The timeout error is not caught and therefore causes the test to throw Error #1502 and exit with exitcode of 1.

Setting test to expectedfail for wordcode. 
http://asteam/hg/tamarin-redux-argo/rev/4ed858c5f8df

Comment 21

[Dan Smith]

Chris, does this only fail for wordcode interp?  If so, I'll retarget bug to Future.

Comment 22

[Chris Peyer]

Yes - only happens with wordcode interpreter.  Bug retargeted to future.

Comment 23

[Edwin Smith]

still says P1/10.2, is that correct?

Comment 24

[Edwin Smith]

Removing OP_abs_jump (bug 576265) fixes this bug as a side effect.

Comment 25

[Edwin Smith]

spidermonkey/js1_5/Regress/regress-146596.abc

asserts because some catch blocks are dead code and WC has junk in exception_fixes at end of method.

Comment 26

[Lars T Hansen]

(In reply to comment #25)
> spidermonkey/js1_5/Regress/regress-146596.abc
> 
> asserts because some catch blocks are dead code and WC has junk in
> exception_fixes at end of method.

Also triggers assert with abcasm/bug_554915a, see bug #545254, which will land some adjustments to testconfig.txt to mask that.

Comment 27

[Edwin Smith]

Attachment: Candidate fix

This test appears to already be fixed in TR, but is still marked skip:
abcasm/bug_554915b

These two are fixed by the attached patch:
abcasm/bug_554915a
spidermonkey/js1_5/Regress/regress-146596.abc

However, bug_554915a now asserts here:
Assertion failed: "((callStackNode == __null))" ("/Users/edwsmith/vm/jitpolicy/core/Interpreter.cpp":1066)

Investigating, but any insights would be appreciated.  The test is designed to ensure that timeout exceptions are *not* caught at the point of a branch, but rather at the point of the target label.


function main() {
    getlocal0
    pushscope

loop:
    label
    .try {
        jump loop
    }
    .catch {
        getlocal0
        pushscope
        setlocal2   // save Error
        getlocal2
        .print
        pushstring "Exception thrown outside try, should not have been caught FAILED!"
    }
    pushstring "End"
    .print
    returnvoid
}

Comment 28

[Edwin Smith]

The problem is that the wordcode translator eliminates the label, so we end up with

try {
 3:jump 3
} catch {
}

And, so, the original fix is a no-op; since the jump and the jump target are at the same location and this location is inside the try block, the interrupt exception is caught by this method, incorrectly.  and since no catch handler was created, we end up off the end of the wordcode array, executing garbage.

Maybe a better fix is for the wordcode translator to emit an interrupt check instruction at the point of the label (like the JIT does), and not do an interrupt check in the branch instructions.  The code for doing the check has to remain, for the ABC interpreter.

messy.

Comment 29

[Edwin Smith]

Attachment: (v2) Candidate fix

Fixes the problem in comment #28 more simply, just by inserting a NOP at the point of a label, if the label is the target of a backedge.  Using a NOP and leaving the interrupt check in the branch instruction saved me a lot of code churn, but costs one extra WOP per loop iteration.

only marking F? for now because patch still needs comments & cleanup but now passes regressions with the offending tests now included. 

Now that I think about it, we only need the NOP if the method has exception handlers in the first place.  will fix, test, comment.

Note that the patch also removes code to sort catch_info's for exception regions because the Verifier already requires they be sorted.  (asserts added, of course).

Comment 30

[Edwin Smith]

Attachment: Fix

Ready to review path with comments added and cruft removed.  This patch restricts the NOP to just loop targets in methods with try blocks.

Comment 31

[Lars T Hansen]

Comment on attachment 464926 [details] [diff] [review]
Fix

Somewhat of a rubber stamp, I haven't managed to swap all of that machinery in.  But I'm happy with the limited scope of the fix.

Comment 32

[Edwin Smith]

http://hg.mozilla.org/tamarin-redux/rev/2746eaa2f9ab