Closed Bug 863045 Opened 8 years ago Closed 8 years ago

[OdinMonkey] sequence expressions evaluated twice under specific conditions


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox22 --- fixed
firefox23 --- fixed


(Reporter: jlong, Assigned: luke)



(2 files)

I'm improving the way the LLJS compiler generates type annotations, and I discovered this bug. I'm not blocked on this because I actually discovered this by accident -- LLJS will not generate code like this because I accidentally used the wrong operator. I thought I'd file this bug anyway.

I tried to produce a Minimally Viable Test Case (MVTC) which is attached. There are several conditions which are needed for this to happen. I know it's a bug because the same code acts differently when asm.js is turned off.

Steps to reproduce (as seen in the test case):

* Create a function return a sequence expression like `return (0, foo(), 1)`
* Use the double->signed conversion operator on the above sequence expression
* Add another return to the function like `return 0`


The the sequence expression is executed twice each time, instead of just being executed once. If use the `|` operator to force it to an int, it works normally. If you only have 1 return statement in the whole function, it works normally. 

Code example:

    function run() {
        return ~~(0, print(2), 0);
        return 1;

    // wrap the above fun inside an asm module, etc etc

    // and run it;

Expected results:

2 should be printed once

Actual results:

2 is printed twice

You can run the attached code, try it with and without "use asm" and you'll see that it acts differently.
Attached file test case
Assignee: general → luke
I should note that I'm using the latest Nightly on OS X.
Attached patch fix and testSplinter Review
Great find, thanks for the reduced test case!

Simple bug, introduced during a particularly sleep-deprived trip...
Attachment #738769 - Flags: review?(sstangl)
Comment on attachment 738769 [details] [diff] [review]
fix and test

Review of attachment 738769 [details] [diff] [review]:

::: js/src/ion/AsmJS.cpp
@@ +3512,5 @@
> +        return true;
> +    }
> +
> +    if (!operandType.isIntish())
> +        return"Operand to ~ must be intish", operand);

This error message seems inaccurate: maybe "must be a double or intish"?

For that matter, are doublish operands intended to be supported, or is there an intentional limitation to only strict doubles?
Attachment #738769 - Flags: review?(sstangl) → review+
(In reply to Sean Stangl [:sstangl] from comment #4)
Good points, agreed on both.
Thanks for the quick fix!

I just found out that the test case can be reduced even further. It's not specific to sequence expressions, it looks like it's *any* expression that uses the ~~ operator. So this still produces the bug:

    function run() {
        return ~~print(2);
        return 1;

I'm sure you've figured that out by now but it looks like you've added some tests which use the sequence form, and you could simplify that if you want. I verified also with something like `return ~~(x = (x + 1) | 0)`.
Luke, correct me if I'm wrong, but I just realized that I think there's another bug in here.

Return statements should only be valid if they use the double or signed operators (`+` and `|`). The `~~` double operator should never validate. And it doesn't:

    function foo() {
        var x = 0;
        x = 3;
        return ~~x;

"TypeError: asm.js type error: in coercion expression, the expression must be of the form +x or x|0"

However, if you add another return, it validates fine:

    function foo() {
        var x = 0;
        x = 3;
        return ~~x;
        return x | 0;

The validator should check all return statements and fail if it's not explicitly using the double or signed operator.
Only the last return statement in a function requires the explicit + or |0.  This is the asm.js "return type declaration".  All the other return statements in the function need simply return a type that is a subtype of the declared return type.
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 738769 [details] [diff] [review]
fix and test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 840282
User impact if declined: incorrect JS evaluation
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low
Attachment #738769 - Flags: approval-mozilla-aurora?
Comment on attachment 738769 [details] [diff] [review]
fix and test

Matter of correctness in support of asm.js work. If any of these bugs are actually user critical, please nominate for tracking instead of just approval (so that we keep an eye on them).
Attachment #738769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.