OdinMonkey: unsound type for *

RESOLVED FIXED in mozilla24



6 years ago
6 years ago


(Reporter: cscott, Unassigned)



Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

https://hg.mozilla.org/mozilla-central/file/eebeea69662c/js/src/ion/AsmJS.cpp#l3863 gives the return type for:

expr:MultiplicativeExpression * n:NumericLiteral
n:NumericLiteral * expr:UnaryExpression

as "signed".  This is incorrect: the result could overflow a signed type.  The result should be "intish" (as http://asmjs.org/spec/latest/#multiplicativeexpression states).
This example arises in the lua-binarytrees.js benchmark from https://github.com/dvander/arewefastyet/blob/9de7037324e0b5b155b761225f26f94fff5c76f5/benchmarks/asmjs-apps/lua-binarytrees.js
in the following expression:

   if ((c[d + e * 4 >> 2] | 0) == 0)

This should properly be written as:

   var imul = global.Math.imul;
   if ((c[d + Math.imul(e, 4) >> 2] | 0) == 0)


   if ((c[d + (e << 2) >> 2] | 0) == 0)

But that's an emscripten bug, not an OdinMonkey bug.  The OdinMonkey bug is that this example doesn't fail to typecheck.
Standalone test case:

Prints "the answer is 0" on chrome and firefox-with-asmjs-disabled.

Prints "the answer is 1" with asm.js enabled.
One-line patch to fix.
Duplicate of this bug: 878417
Instead of enforcing the current spec, we could accept this pattern. Logically, there is no reason not to - multiplying by a small integer is not dangerous here, even if added to various other such things, given the usual constraints etc. I don't know how elegant it would be in the type system, but it would let current code still validate - and again, there is no reason for it not to, except perhaps for type system elegance.
Multiplying by even a small integer (say, two) will result in a value which can overflow the int type.  The proper return value in that case is intish.  If you wanted to accept this pattern, you'd do it by pattern matching (int + int * constant + int + ...) and have that whole mess be intish.  Basically you'd extend the current additive chain hack to allow terms which are (int*constant).  You then have to carefully reason about the maximum number of elements you allow in that chain when some of them are multiplied by constant factors.  It's doable... but certainly it's easier just to use imul or shift?
Comment on attachment 759929 [details] [diff] [review]
Change return type of * to intish.

Before landing this (http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed) could you add a test-case that fails to jit-tests/tests/asm.js/testExpressions.js?
Attachment #759929 - Flags: review+
It isn't signed of course, yeah. I was saying the pattern is potentially interpretable in a way that is fine. But yes, it would require some hacks. The hacks would enable that pattern, which is perfectly fine logically if not in the current type system.
Updated patch, adds test cases.
Attachment #759929 - Attachment is obsolete: true
Comment on attachment 759987 [details] [diff] [review]
Change return type of * to intish.

Attachment #759987 - Flags: review?(luke)
Comment on attachment 759987 [details] [diff] [review]
Change return type of * to intish.

Attachment #759987 - Flags: review?(luke) → review+
Ever confirmed: true
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.