700620 - jit/interpreter disagree about pre-increment on :int at int.MAX_VALUE

Status: RESOLVED FIXED

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

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

(Uncovered while trying to isolate components of Bug 700613; but I actually think this is unrelated.)
  var u:uint;
  var i:int;
  
  u = uint.MAX_VALUE;
  i = int.MAX_VALUE;
  
  trace([++u, ++i]);

Interpeter for both 32- and 64-bit DebugDebugger avmshell says:

  4294967296,2147483648

JIT for both 32-bit and 64-bit DebugDebugger avmshell says:

  4294967296,-2147483648

Comment 1

[William Maddox]

Attachment: Increment/decrement wraparound test

The attached test case not only demonstrates the problem above, but illustrates another error that manifests in both the interpreter and the JIT:

var u:uint;
u = uint.MAX_VALUE;  /* 4294967295 */
++u == 4294967296 /*  expected 0; FAIL */
u = uint.MIN_VALUE;  /* 0 */
--u == -1 /*  expected 4294967295; FAIL */

It appears that this is an ASC problem.  The expression '++i', for an 'int' variable 'i' is compiled like this:

  37        getlocal2
  38        increment_i
  39        dup
  40        convert_i   	
  41        setlocal2   	
  /* incremented value on stack, but properly wrapped.	convert_i is redundant.	*/

The increment_i opcode always returns an 'int' result, as if toInt32() had been called on a result computed as Number.

In contrast, the expression 'u++' with a 'uint' variable 'u' is compiled like this:

  143       getlocal1
  144       increment
  145       dup
  146       convert_u
  147       setlocal1
  /* incremented value on the stack here */

Here, the incremented value is computed as a Number, but converted to 'uint' prior to assignment to 'u'.  Unfortunately, the value of the expression is not converted.

I believe that the 'int' case is correct, but an AS3 spec guru should confirm.

Comment 2

[William Maddox]

Attachment: Handle integer wraparound correctly in increment_i and decrement_i

The cause of the original error is the use of the FAST_INC_MAYBE and FAST_DEC_MAYBE fastpaths for the increment_i and decrement_i opcodes as well as for the unspecialized increment and decrement opcodes.  If the argument is a (boxed) double, the fastpath will perform the increment/decrement without wrapping, which is not correct for the int-specialized forms.

Other issues: 1) The overflow check for the intptr case can be streamlined a bit.  2) The logic in AvmCore::increment_d() is incorrect, wrapping at 32-bits for some values representable as intptr on 64-bit platforms.

I'm not submitting this patch for review yet, as I think the code can be tightened up a bit further, and a more comprehensive test is needed to make sure that types other than int and uint haven't been inadvertently broken.

It is not at all inconceivable that code in the wild is depending on the broken wrapping behavior, both due to ASC and VM issues.  On the other hand, increment and decrement are very basic operations, and attempting to version them is scary.

Comment 3

[Edwin Smith]

Comment on attachment 597733 [details] [diff] [review]
Handle integer wraparound correctly in increment_i and decrement_i

In AvmCore::increment_i(), the expression int32_t(delta) is the same as (delta) since delta is int.  (int is always int32, even though theoretically it isn't).

I'm not opposed to the extra casting for clarity tho.  Swapping delta to the RHS is also just for clarity?  ok.

All I can really say about the interpreter.cpp changes is that if there *is* a bug, theres almost no chance I would find it.  So lets focus on test cases.  One important edge case is on 64-bit cpus when you have a kIntptr atom in the 32-52bit range.  (you can get such a thing by starting with int.MAX_VALUE and adding those atoms via the fast-paths of OP_add, subtract, etc).

I also agree it would be best not to version any of this if we can.  forgiveness not permission.

Comment 4

[William Maddox]

Attachment: Verify increment/decrement expressions agree with normal arithmetic

Test pays particular attention to inflection points.  It's a bit of a shotgun, testing many cases at values that aren't particularly interesting, in order to keep the code uniform and systematic.

Comment 5

[William Maddox]

Attachment: V2: Handle integer wraparound correctly in increment_i and decrement_i

Also cleans up the code and tightens it up a bit for performance and clarity.

Comment 6

[William Maddox]

Attachment: V2: Verify increment/decrement expressions agree with normal arithmetic

I now realized the significance of Edwin's remark regarding the "important edge case" in comment 3.  Large integers like int53_max will be represented in the constant pool as doubles, and will be loaded as double values.  The test is predicated on the assumption, however, that each of the test values is represented as an atom in canonical form, represented as kIntptr if possible.  The revised patch should assure this if the result of addition is always properly canonicalized, though I'm not sure yet that this is the case for all execution paths, or for the specific patch taken by the test case.  See also bug 601426.

Comment 7

[William Maddox]

Attachment: Add methods to System class to support tests that distinguish integer representations

Allows testing whether an atom is represented as kIntptrType or not, and to force numeric atoms to be so represented if possible.

Comment 8

[William Maddox]

Attachment: V3: Verify increment/decrement expressions agree with normal arithmetic

Use System.canonicalizeNumber to reliably generate kIntptrType atoms.

Increment/Decrement appear to be OK with the revised test, but I'm seeing test failures on x86_64.  Curiously, the increment/decrement values look to be correct -- it is the reference additions/subtractions that are wrong.  Looks related to signedness -- large positive values are wrapping to small negative values.  Investigating...

Comment 9

[William Maddox]

The SIGN_EXTEND macro was defined incorrectly, resulting in some results wrapping at 53 bits where 54-bit precision was expected.  The faulty definition was also mimicked in inline-generated code.  See bug 601426 for the fix.

Comment 10

[Tamarin Bot]

changeset: 7216:a59cc6992efa
user:      William Maddox <[e-mail]>
summary:   Bug 700620: Handle integer wraparound correctly in increment_i and decrement_i (r=edwsmith)

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

Comment 11

[William Maddox]

Attachment 600226 [details] [diff] landed previously as part of another fix, see:
http://hg.mozilla.org/tamarin-redux/rev/1fb053b49a9f