577449 - Fix the expression evaluation for "[un]signed right/left shift" operators

Status: RESOLVED DUPLICATE of bug 602788

(Core Graveyard :: Nanojit, defect, P2)

Description

[Cédric VINCENT]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.6) Gecko/20100627 Firefox/3.6.6
Build Identifier: nanojit rev. ccfc1e56c6f7

The section "11.7.2 The Signed Right Shift Operator (>>)" of the
ECMAScript standard specifies:

    The production ShiftExpression : ShiftExpression >>
    AdditiveExpression is evaluated as follows:

        1. Evaluate ShiftExpression.
        2. Call GetValue(Result(1)).
        3. Evaluate AdditiveExpression.
        4. Call GetValue(Result(3)).
        5. Call ToInt32(Result(2)).
        6. Call ToUint32(Result(4)).
        7. Mask out all but the least significant 5 bits of Result(6), that
           is, compute Result(6) & 0x1F.
        8. Perform sign-extending right shift of Result(5) by Result(7)
           bits. The most significant bit is propagated. The result is
           a signed 32 bit integer.
        9. Return Result(8).

The steps 6 and 7 are missing in ExprFilter::ins2(), that means
the current implementation relies on an undefined C++ behaviour,
as specified in the section "5.8 Shift operators" of the C++
standard:

    The behavior is undefined if the right operand is [...]
    greater than or equal to the length in bits of the promoted
    left operand.

Note it is the same problem for unsigned and left shift
operators.

As far as I know, this bug appears only on SH4 when the right
operand is 32 since the ISA specifies for "SH{A,L}D Rm, Rn":
    
    If Rm is less than zero, this is an arithmetic right shift
    and the shift amount is given by the least significant 5 bits
    of Rm subtracted from 32. In the case where Rm indicates an
    arithmetic right shift by 32, the result (Rn) is filled with
    copies of the sign-bit of the original Rn.


Reproducible: Always

Steps to Reproduce:
1. build NanoJIT/SH4 (see bug 568486) without the change in LIR.cpp
2. run the new test case in the lirasm test-suite

Actual Results:  
The new test case in the lirasm test-suite fails on SH4 

Expected Results:

Comment 1

[Cédric VINCENT]

Attachment: Add a new testcase for bug 577449

Comment 2

[Cédric VINCENT]

Attachment: Fix the expression evaluation for "[un]signed right/left shift" operators

Comment 3

[Cédric VINCENT]

I will rebase these patches on top of NanoJIT soon.

Regards,
Cédric.

Comment 4

[Cédric VINCENT]

Attachment: Fix the expression evaluation for "[un]signed right/left shift" operators

Comment 5

[Cédric VINCENT]

Attachment: Add a new testcase for bug 577449

Comment 6

[Cédric VINCENT]

Attachment: Fix the expression evaluation for "[un]signed right/left shift" operators

Comment 7

[Nicholas Nethercote [inactive]]

The fix, the test case and the .args file addition all seem ok to me.

Comment 8

[Rick Reitmaier]

Agreed;  Cédric did you want to mark the patches for review?

Comment 9

[Nicholas Nethercote [inactive]]

Cédric, in general there's no need to get patches re-reviewed if there have been very small changes due to rebasing or similar.

Comment 10

[Cédric VINCENT]

Is there any news about this entry?

Comment 11

[Edwin Smith]

Are you simply looking for someone to push this to nanojit-central?

Comment 12

[Cédric VINCENT]

Well, this fix was integrated in Bug 602788 but this entry was never updated.  I'm not sure to understand how patches land into Tamarin's sources:

   1. create a new entry with the patch

   2. take reviewers' comments into account (iteration)

   3. once the review is done, ... what else ?

Comment 13

[Edwin Smith]

3. push the patch -- the developer does it, if possible, and if not its best to ask the person who reviewed the patch to do it.

Have the test changes from this bug been pushed yet?  if not, that seems like all there is left to do.  I agree the code changes are duplicate at this point (done in bug 602788).

There's more info here:

https://developer.mozilla.org/en/NanojitMerge

Once a patch lands in nanojit-central, it will be peridocally merged into TR and TM, separately, and the bug corresponding to the patch will be annotated.  It looks like that has happened with bug 602788.

If there is anything left to push, please confirm what remains.  If not, we can close the bug as resolved.

Comment 14

[Cédric VINCENT]

(In reply to comment #13)
> 
> Have the test changes from this bug been pushed yet?  if not, that seems like
> all there is left to do.  I agree the code changes are duplicate at this point
> (done in bug 602788).

Once again this patch was duplicated by Nicholas: Bug 595728

Comment 15

[Cédric VINCENT]

(In reply to comment #13)
>
> If there is anything left to push, please confirm what remains.  If not, we can
> close the bug as resolved.

You can close it, thanks.