620403 - AvmCore::toUint32(), integer(), and number() should not be pure functions.

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

Description

[Edwin Smith]

When they get an object parameter, they can call valueOf(), which has observable side effects.  Common subexpression elimination and dead code elimination are incorrect on such a call.

This shows up as a JIT/interpreter difference.

It appears its always been that way -- even in Flash 9, with MIR, 
the call was marked MIR_cmop ('op' meaning CSE-able).

Comment 1

[Edwin Smith]

testcase:

{
  function test_toint(x) {
    10>>x
    10>>x
  }
  function test_touint(x) {
    10>>>x
    10>>>x
  }
  function test_tonumber(x) {
    x-1
    1-x
    1-x
  }
  function test_tostring(x) {
    x+""
    x+""
  }
  function test(f) {
    var x = {
      valueof_count:0,
      tostring_count:0,
      valueOf: function() { ++this.valueof_count },
      toString: function() { ++this.tostring_count }
    }
    f(x)
    print('valueof, tostring ', x.valueof_count, x.tostring_count)
  }
  test(test_toint)
  test(test_touint)
  test(test_tonumber)
  test(test_tostring)
}

interpreter:

valueof, tostring  2 0
valueof, tostring  2 0
valueof, tostring  3 0
valueof, tostring  0 2

jit:

valueof, tostring  0 0 <-- mismatch
valueof, tostring  0 0 <-- mismatch
valueof, tostring  3 0 <-- match, but unversioned (*)
valueof, tostring  0 2

(*)  The case for AvmCore::number() passes, because we inline
a control-flow diamond for the numerical operators.  (I tried
divide, subtract, modulo).  When inlined, the pure call
to AvmCore::number() doesn't appear dead, and so gets generated
anyway.  This means that even in Spicy without OSR, we have accidentally
changed JIT behavior by suppressing CSE and DCE of AvmCore::number().

I haven't tried, but I think if we turn off that inlining, the
number() case will behave like the int/uint cases.

Comment 2

[William Maddox]

Attachment: Straightfoward fix, chane PUREFUNCTION to FUNCTION appropriately

This patch is against TR.  CSE is simply disabled for the toUint32(), integer(), and number().  Similarly, for coerce(), which can also produce a side-effect, though it was not mentioned in the bug description.

Comment 3

[William Maddox]

Attachment: Versioned fix

This patch is against tr-spicy, for reasons of history and expedience.  Both wasabi and TR need a fix.

Here, we attempt to leave the old CSE behavior unchanged for older versions.
Note that it is a bit dicy to assume that merely versioning an an optimization is going to result in compatible behaavior.  For example, we have already seen that changes elsewhere in the code generator (e.g., inline fastpaths) can change visible behavior.

We inhibit CSE only for non-builtin code at SWF11 or greater (should be SWF12 for Wasabi).  It is not possible to determine statically the versioned behavior expected of builtins, so we conservatively stick to the old behavior.  It would perhaps be better to compile builtins according to the latest version, but this would require verifying that no unintended changes in behavior show through to the builtin's API.

It has been claimed (e.g., by Steven) that the version associated with a non-builtin method is constant, and comes from the ABC block in which the method appears.  It would make sense, then, for each PoolObject to have a BugCompatibility member, valid for a non-builtin pool.  This is not the case, however.  Instead, the BugCompatibility object for a method appears in the CodeContext an AbcEnv object, of which multiple instances may share a common pool.  There are no apparent structural constraints in API that would prevent CodeContexts with differing bug compatibility objects from belonging to two different AbcEnv objects that share a common PoolObject.

In this patch, I hack around the issue by storing a bug compatibility object in a PoolObject whenever the pool is used to construct an AbcEnv, and asserting that that the pool is either builtin, or that a common bug compatibility object is used.  The assertion has survived testing in Flash Player with the ATS10 automated certification tests, but I have no idea how well this actually exercises the relevant code paths in the player.


This may simply be part of a way to save space -- by packaging the BugCompatibility object together with a security context in a CodeContext object, they may both be referenced via a single slot in the MethodFrame. The API, however, allows us to package up an arbitrary BugCompatibility object in a CodeContext, however, 

I would really like to see a definitive statement on this aspect of bug compatibility semantics backed up by an API change that forces correctness by construction.

Whether it we want to actually version this fix (it would be best if we can avoid it), the issues in this patch will arise again, and in fact have done so already in versioning the OSR feature.

Comment 4

[William Maddox]

(In reply to comment #3)
> Created attachment 509961 [details] [diff] [review]
> Versioned fix

> This may simply be part of a way to save space -- by packaging the
> BugCompatibility object together with a security context in a CodeContext
> object, they may both be referenced via a single slot in the MethodFrame. The
> API, however, allows us to package up an arbitrary BugCompatibility object in a
> CodeContext, however, 

Ignore the paragraph above -- it was an incomplete thought that I failed to delete.

Comment 5

[Trevor Baker]

OSR for all platforms isn't happening until Nigel, so this moves out too.

Comment 6

[Edwin Smith]

Comment on attachment 509961 [details] [diff] [review]
Versioned fix

Nothing looks wrong.

for OP_returnvoid - since we know the value is undefined, we can constant fold it to whatever the target type is.  will that happen with the call to coerce_pure(), later in the LIR pipeline?

I assert we don't need the versioned (old) behavior for builtins.  In particular, if there is some builtin function that would have called toString() or valueOf() on a user object once (wrongly), it should certianly be fixed for new client code.  if this means old client code on new runtimes changes behavior, so be it.  if it's a problem, the affected builtin functions themselves can be changed to do versioning.

IOW, we should always JIT builtins with all bugs fixed, and any versioning that must go on, should be in the builtin AS3 itslef, not in the JIT compiler.  Ditto for the interpreter -- when interpreting builtin code, always use latest behavior.

Any C++ or AS3 builtin code that changes behavior based on who the innermost client-code caller is, should do that explicitly in C++ or AS3 - this isn't something we should mess with in the Interpreter or JIT.

I assert that createRestHelper/createArgumentsHelper are pure unless you have a smoking gun proving otherwise.  CSE on them would be wrong, but we only emit one call per function.  The value of making them "pure" is that if they're dead, we won't emit the call.  (translate this into a code comment, please).

Comment 7

[William Maddox]

Attachment: Test case (improved)

Test results:

$ ./deb32/shell/avmshell -Dinterp purefunctions.abc
valueof: 2 [2], tostring: 0 [0]
valueof: 2 [2], tostring: 0 [0]
valueof: 2 [2], tostring: 0 [0]
valueof: 2 [2], tostring: 0 [0]
valueof: 3 [3], tostring: 0 [0]
valueof: 3 [3], tostring: 0 [0]
valueof: 0 [0], tostring: 2 [2]
valueof: 0 [0], tostring: 2 [2]

$ ./deb32/shell/avmshell -Ojit purefunctions.abc
valueof: 0 [2], tostring: 0 [0]
FAIL!
valueof: 1 [2], tostring: 0 [0]
FAIL!
valueof: 0 [2], tostring: 0 [0]
FAIL!
valueof: 1 [2], tostring: 0 [0]
FAIL!
valueof: 3 [3], tostring: 0 [0]
valueof: 3 [3], tostring: 0 [0]
valueof: 0 [0], tostring: 2 [2]
valueof: 0 [0], tostring: 2 [2]

$ ./deb64/shell/avmshell -Dinterp purefunctions.abc
valueof: 2 [2], tostring: 0 [0]
valueof: 2 [2], tostring: 0 [0]
valueof: 2 [2], tostring: 0 [0]
valueof: 2 [2], tostring: 0 [0]
valueof: 3 [3], tostring: 0 [0]
valueof: 3 [3], tostring: 0 [0]
valueof: 0 [0], tostring: 2 [2]
valueof: 0 [0], tostring: 2 [2]

$ ./deb64/shell/avmshell -Ojit purefunctions.abc
valueof: 0 [2], tostring: 0 [0]
FAIL!
valueof: 1 [2], tostring: 0 [0]
FAIL!
valueof: 0 [2], tostring: 0 [0]
FAIL!
valueof: 1 [2], tostring: 0 [0]
FAIL!
valueof: 0 [3], tostring: 0 [0]
FAIL!
valueof: 1 [3], tostring: 0 [0]
FAIL!
valueof: 0 [0], tostring: 2 [2]
valueof: 0 [0], tostring: 2 [2]

Not only does compiled behavior differ from the interpreter, but it also differs between 32-bit and 64-bit platforms.  This is not suprising, since CSE operates at the LIR level, and is affected by the labels inserted during ABC lowering.

Comment 8

[William Maddox]

Attachment: Remove erroneous pure attribute from side-effecting helper functions

1) Fix cases reported in the bug, as well as several other issues found by inspection, and add clarifying comments regarding the pureness of some functions.

2) Make 'makeatom' impure in all cases, not just with VMCFG_FLOAT, following the argument made in the commentary, which should apply in all cases.  Review note: I have not thought about whether these arguments should simply be passed by value (which would require that they always be passed as a quadword).  Are we losing dead code elimination opportunities here?

3) Reorganize the switch statement in AvmCore::boolean() to make it clearer that atomToDouble() is invoked only on kDoubleType atoms, and is not part of a default handler for otherwise unhandled atom types.

No versioning is attempted here.  Observing that the behavior already differs on 32-bit and 64-bit platforms underscores the futility of attempting to get this right -- the effects of CSE have likely been a moving target for some time now.

Comment 9

[William Maddox]

(In reply to William Maddox from comment #7)
> Created attachment 601844 [details]

> Not only does compiled behavior differ from the interpreter, but it also
> differs between 32-bit and 64-bit platforms.  This is not suprising, since
> CSE operates at the LIR level, and is affected by the labels inserted during
> ABC lowering.

Hmm.  suspendCSE/resumeCSE should be hiding this control flow, if used consistently as intended.  On the other hand, there are gross differences in the translation for which this is not applicable, as the mechanism was never intended to somehow canonicalize CSE.

Comment 10

[Edwin Smith]

Comment on attachment 601846 [details] [diff] [review]
Remove erroneous pure attribute from side-effecting helper functions

My only worry about the makeatom change from pure to nonpure is that if the result is not used, we won't remove the call.  But since we only emit makeatom when a value is loaded, the odds seem low that we'd remove many of these anyway.

what happened to allocFloat? (gone from jit-calls.h)

Comment 11

[William Maddox]

(In reply to Edwin Smith from comment #10)
> Comment on attachment 601846 [details] [diff] [review]
 
> what happened to allocFloat? (gone from jit-calls.h)

It is not invoked directly from JIT'd code, so it doesn't need a CallInfo structure.
I simply noticed this while auditing the pure functions.

Comment 12

[Tamarin Bot]

changeset: 7256:74ab7a8d7b7d
user:      William Maddox <[e-mail]>
summary:   Bug 620403: Remove 'pure' attribute from helper functions that may invoke coercions with side-effects (r=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/74ab7a8d7b7d

Comment 13

[Tamarin Bot]

changeset: 7257:b0f5a7a8801b
user:      William Maddox <[e-mail]>
summary:   Bug 620403: Correction to previous patch

http://hg.mozilla.org/tamarin-redux/rev/b0f5a7a8801b