User-Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; GTB6.6; Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1) ; SLCC1; .NET CLR 2.0.50727; Media Center PC 5.0; InfoPath.2; .NET CLR 3.5.21022; .NET CLR 3.5.30729; .NET CLR 3.0.30729; .NET4.0C; .NET4.0E) Build Identifier: The implementation of LIR_divi doesn't handle unsigned operands correctly (other than for power-of-two divisors, which are lowered into shifts), and uses signed division exclusively. This gives wrong results when the signed interpretation of the operand(s) is negative. Reproducible: Always

LIR_divi is only for signed division. Maybe the bug is tamarin's frontend generating LIR_divi for unsigned values? do you have a test case?

var x : uint = 0xffffffff; x /= 3; trace(x); This code calculates -1 / 3 which is 0. Make sure to run this with -Ojit, as the interpreter gives the correct result.

(In reply to comment #1) > LIR_divi is only for signed division. Maybe the bug is tamarin's frontend > generating LIR_divi for unsigned values? do you have a test case? Yeah, this is what I meant. Sorry for the unclearity.

> Yeah, this is what I meant. Sorry for the unclearity. No problem. I can reproduce in TR, wow, talk about a hole in the test suite.

This was injected by bug 600585, in this change: http://hg.mozilla.org/tamarin-redux/rev/5512

(In reply to comment #5) > This was injected by bug 600585, in this change: > http://hg.mozilla.org/tamarin-redux/rev/5512 Correction, looks like this change did it: http://hg.mozilla.org/tamarin-redux/rev/5395 bug 570476. Support integer division (minus NJ part). (+r wmaddox)

Comment on attachment 520004 [details] [diff] [review] Avoid using LIR_divi on unsigned integers This part looks right, but do we already do the right thing when everything is a constant? Also, needs a test case.

Nanojit asserts if LIR_divi is used with constant arguments, and there is a comment to the effect that front-ends should not emit such code. (The restriction exists for the sake of the divi/modi hack.) There is no ABC divide_i operator -- conceptually, all division produces a Number result, and I've experimentally verified that expressions that attempt to divide integer constants convert to doubles and fold the double division. For example, the following abcasm program: function main() { getlocal0 pushscope findpropstrict print pushint 555 pushint 111 divide callproperty print 1 returnvoid } In the following case, the result is converted to an integer, and the entire division gets folded to the constant '5': function main():int { pushint 555 pushint 111 divide coerce_i returnvalue } This is apparently being done by folding the double result of the double division of the operands coerced to doubles. I believe that the only situation in which Tamarin actually generates a LIR_divi is the case of a non-constant integer dividend and a constant, nonzero divisor.

It would apparently be possible, however, for imm2Int to produce a constant value, so that both arguments would be immediates, which would crash Nanojit. I'm not yet convinced this cannot happen.

Tighten up the logic a bit to protect against any possibility that we give LIR_divi two constant arguments, which will crash Nanojit (assertion, by design). Rick -- this needs to land in TR ASAP, so I'm asking you to review. Edwin -- if you happen to see this first, feel free to poach the review.

Edwin and I agree that this code is less obvious than it should be, and that the lowest risk fix for Wasabi is to rip it out. The optimization is specific to i386 and X64, as no other platforms support LIR_divi in Nanojit.

Pushed to tr-spicy: http://hg.mozilla.org/users/stejohns_adobe.com/tr-spicy/rev/55ac92ea3533

I'd be comfortable r+'ing attachment 520004 [details] [diff] [review], but I'll have to examine attachment 520055 [details] [diff] [review] in more detail as there are some subtle changes in the logic.

Retargetting for Serrano, as we need to fix the broken optimization that we just removed from Wasabi.

Grumble. It looks like the optimization in Spicy landed before the Nanojit support, and NJ_DIVI_SUPPORTED is not defined for any platfom there. As a result, despite the presence of the buggy optimization, as identified and reproduced in TR, Spicy/Wasabi did not actually manifest the bug. Silly me to test the result of the fix, but not verify the original bug in Spicy/Wasabi as well as TR. I suspect that this is another example of unfortunate timing for creation of the Spicy branch. Most likely, the Nanojit support for LIR_divi was still tied up in review, and the dependent optimization was landed in advance.

Silver lining: it means the bug hasn't been released into the wild yet, and won't be now.

changeset: 6146:5ef0a32feefc user: William Maddox <wmaddox@adobe.com> summary: Bug 642535 - Avoid using LIR_divi on unsigned integers. (r=rreitmai) http://hg.mozilla.org/tamarin-redux/rev/5ef0a32feefc