460872 - Legal AS3 code fails verification iff jit is disabled

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P1)

Description

[Steven Johnson]

This code snippet:

   class Foo
   {
       public static function failsVerification(o:Object):void
       {
           if (o && (o is Array || o.hasOwnProperty("length")) && o.length)
           {
                 // whatever
           }
       }
   }
   Foo.failsVerification({});

It will load and run correctly with JIT (either MIR or LIR) enabled, but with -Dinterp, it will fail with

   VerifyError: Error #1068: Boolean and * cannot be reconciled

(This causes many Flex apps to fail in -Dinterp mode)

Comment 1

[Edwin Smith]

yep, verification must not depend on execution mode.  type modeling must be the same either way.

Comment 2

[Steven Johnson]

I looked at it very briefly but my Verifier-fu isn't yet where it should be.... but it looks like places we update the State to track the known type information on the stack isn't always updated in non-jit mode (eg emitCoerce(), which is called almost exclusively in jit mode).

This bug is critical to fix in order to get Flex-based code working in interp-only mode, as the code snippet above happens frequently.

Comment 3

[Lars T Hansen]

Steven says this is in redux.  I can't reproduce this in my current, clean tamarin-redux checkout.

Comment 4

[Lars T Hansen]

May have to compile with -strict -optimize -AS3; if compiling with -debug there's no failure.

Comment 5

[Lars T Hansen]

Actually it's only -AS3, and -debug may be present without removing the problem.

Comment 6

[Lars T Hansen]

(In reply to comment #1)
> yep, verification must not depend on execution mode.  type modeling must be the
> same either way.

The problem is that type modeling should not be embedded in the translation.  But in some cases it is - and it is embedded in the JIT code.  Effectively, the attempt at simplifying by splitting code paths that I introduced won't work because there is a once-only common code path that needs to be taken, with possible swings through code that emits the one or other kind of code.

Comment 7

[Lars T Hansen]

There are two separate bugs here.

The first bug is that state changes is sometimes not modeled in interp-only mode, as explained above.

The second bug is that with the recent change that runs the jit and the translator in parallel in normal mode (neither -Dinterp nor -Ojit) will model state for the jit, but this state may not be correct for the translator because they do not implement the same early binding operations.  The consequences of this problem are not clear.  It may be a benign problem because the translator always operates on atoms; more likely it will lead to subtle bugs if type-specific byte codes are applied to values not of that type.  IMO we need to back out this change and clean up the verifier so that it can be used to translate code in case the JIT fails, but it should not run both in parallel.

Comment 8

[Lars T Hansen]

(In reply to comment #2)
> I looked at it very briefly but my Verifier-fu isn't yet where it should be....
> but it looks like places we update the State to track the known type
> information on the stack isn't always updated in non-jit mode (eg emitCoerce(),
> which is called almost exclusively in jit mode).

emitCoerce() has a jit-mode-only side effect, namely, to emit code to effect the coerce.  Then it updates the state.  In interpreted mode no code is emitted, nor should it be, and it would be wrong to update the state.

Comment 9

[Lars T Hansen]

Attachment: Preliminary patch

This patch fixes the problem and passes all acceptance tests in R and DD modes.  I also believe it is correct.  Two reasons to call it preliminary.  One, I need to go over some more code to look for more problems.  Two, while cleaning up this I found another problem, which effectively disables early binding for calls through interfaces, so that needs to be fixed.  (It may best be handled as a separate bug, though.)

Comment 10

[Steven Johnson]

Sounds like Verifier probably needs some rearchitecting at some point.

Comment 11

[Steven Johnson]

Comment on attachment 344283 [details] [diff] [review]
Preliminary patch

I think there is a minor typo in Verifier::emitCallpropertySlot: int sets the type to UINT_TYPE and vice versa.

Comment 12

[Steven Johnson]

yeah, as-is we fail 

  as3/Types/Int/intConstructor.abc : int(-1) = 4294967295 FAILED! expected: -1

when running in jit mode (but not interp mode); switch the state->setType() calls in the int/uint cases fixes that. otherwise, looks good.

Comment 13

[Lars T Hansen]

Attachment: Patch

Fixes the problem Steven found.
Fixes the problem with non-early binding of calls to interfaces.
Fixes some problems in type modeling when using the ABC interpreter with -Dinterp.
Fixes a minor problem in the shell where the usage message allows -Dinterp but the options parsing barfs, in builds that do not include a JIT (I opted to allow -Dinterp in this case, it's silly to prohibit it).

Comment 14

[Steven Johnson]

Comment on attachment 344453 [details] [diff] [review]
Patch

I think there's something wrong with the handling of the "isInterface" case; adding this patch causes some Flex apps to fail with a verifier error of 

TypeError: Error #1009: Cannot access a property or method of a null object reference.
    at mx.preloaders::Preloader/initialize()
    at mx.managers::SystemManager/http://www.adobe.com/2006/flex/mx/internal::initialize()
    at mx.managers::SystemManager/initHandler()


(If I change the emitOp2 on line 2613 back to "return false" then it loads)

Comment 15

[Lars T Hansen]

Attachment: Patch

Final adjustment - an extra pop should only be inserted in word code if a callpropvoid was translated to a callmethod, not if it was left as callpropvoid because it was a call through an interface.

Comment 16

[Lars T Hansen]

Changeset 1023:b7a6676a7fe9

Comment 17

[Brent Baker]

I have verified that the code snippet no longer throws a verifier error. I confirmed the original issue using rev 1013:9e59fa049e60

Comment 18

[Brent Baker]

Attachment: testcase

Testcase that demonstrates the failure in previous avmshells when run with -Dinterp (I tested old behaviour on build 1013 with -Dinterp switch).

Comment 19

[Brent Baker]

Pushed testcase as 1620:7a6b74ed85a0