640052 - Relational operators > and >= incorrectly use AvmCore::compare()

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Edwin Smith]

ECMAScript requires left-to-right evaluation.  Side effects from calling ToPrimitive on the left and right arguments must be observed in order.

Both the Interpreter and CodegenLIR implement the symmetric relational operators by swapping arguments, then calling a helper.  Eventually, the call to AvmCore::compare() occurs, which always calls ToPrimitive in the same order.

Interpreter only ever calls AvmCore::compare(), either with the arguments in the correct order, or swapped.  These opcodes have them swapped:

  lessequals, greaterthan, ifle, ifnle, ifgt, ifngt

CodegenLIR uses two helpers:

  cmpLt(a,b) calls compare(a,b)
  cmpLt(b,a) calls compare(b,a)   <-- swapped
  cmpLe(a,b) calls compare(b,a)   <-- swapped
  cmpLe(b,a) calls compare(a,b)

So, the broken opcodes are the ones that call cmpLt(b,a) or cmpLe(a,b):

  greaterthan, ifgt, ifngt    call cmpLt(b,a)
  lessequals,  ifle, ifnle    call cmpLe(a,b)

So, while wrong, it seems the interpreter and jit agree.

Comment 1

[Edwin Smith]

We need to make sure we have good coverage of these paths, to make sure ongoing jit/interpreter work don't accedentially break when side effects are visible. (marking in-testsuite).

Comment 2

[Edwin Smith]

No matter what, this bug should stay open until the bytecode docs are clarified.

Comment 4

[Dan Smith]

Testing/doc should be required prior to Halfmoon.  Targeting to Cyril.

Comment 5

[Brent Baker]

What am I currently missing here, my understanding of the spec seems to agree with how the following code runs.

function MyObject(value)
{
    this.value = value;
    this.valueOf = function(){var v = ++this.value; return v};
}

var foo = new MyObject(0); // increments value every time valueOf is called
print(foo<foo); // (1<2), LeftFirst true (expressions 8.4.1.2)
print(foo>foo); // (4>3), LeftFirst false (expressions 8.4.2.2)
print(foo<=foo); // (6<=5), LeftFirst false (expressions 8.4.3.2)
print(foo>=foo); // (7>=8), LeftFirst true (expressions 8.4.4.2)

This code produces results that iI would expect (true, true, false, false). This is with both -Dinterp and -Ojit

Comment 6

[Lars T Hansen]

I believe the spec is "wrong" because it reflects the reality of this bug, see the comment to that effect following the definition of the relational comparison algorithm (Defn 8.4) about how leftFirst is ignored.  The "right" spec is Ecma-262.

Comment 7

[Brent Baker]

(In reply to Lars T Hansen from comment #6)
> I believe the spec is "wrong" because it reflects the reality of this bug,
> see the comment to that effect following the definition of the relational
> comparison algorithm (Defn 8.4) about how leftFirst is ignored.  The "right"
> spec is Ecma-262.

But ecma 262 changes the LeftFirst value depending on the operator as well (11.8.2 and 11.8.3 set the LeftFirst to false)

***Ah, but this is the big difference between the AS Expressions spec and the ECMA spec:
11.8.2: Greater-than (>)
11.8.2.5: Let r be the result of performing abstract relational comparison rval < lval with LeftFirst equal to false. (see 11.8.5).

So this is how it should behave:
print(foo>foo); 
--> foo_left > foo_right
    --> foo_right < foo_left with LeftFirst false
        --> 2 < 1 
            -> false