590212 - MIPS jmp instruction corruption

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Chris Dearman]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.8) Gecko/20100723 Ubuntu/9.10 (karmic) Firefox/3.6.8
Build Identifier: 

An incorrect mask can cause invalid jmp instructions to be emitted dependent on the target address


Reproducible: Sometimes

Comment 1

[Chris Dearman]

Attachment: Use correct mask for j/jal instructions

Comment 2

[William Maddox]

Could you explain exactly what the patch is fixing?
Most of it looks like simply a stylistic cleanup (good though!).
The substantive change appears to be forcing the shift to be
logical (unsigned) in the JAL case.  Is there anything more I am
missing?

Comment 3

[Chris Dearman]

The hardwired mask was 0x3fffffff instead of 0x03ffffff so some bits of a large address were leaking into the opcode field. The mask used in JINDEX was correct and should have been used for all of the j/jal variants.

Comment 4

[William Maddox]

Ah, so indeed the mask was too wide, and you now are masking a 26-bit field as required.  In the case that you would have overflowed into the opcode previously, however, it now looks like you are just truncating the high bits (e.g., in excess of 28) of the original address 'dest'. I'm not clear on how this is supposed to work.  Or was it always guaranteed that the address was in range, and you were just picking up spurious high-order bits by doing an arithmetic rather than a logical shift?

Also, is there any reason why the JINDEX isn't simply folded into J_FORMAT?

Comment 5

[William Maddox]

(In reply to comment #4)
> Ah, so indeed the mask was too wide, and you now are masking a 26-bit field as
> required.  In the case that you would have overflowed into the opcode
> previously, however, it now looks like you are just truncating the high bits
> (e.g., in excess of 28) of the original address 'dest'. I'm not clear on how
> this is supposed to work.

Hi, Chris.  Was just looking at some MIPS docs.  I missed that the high
bits of the address were filled in from the PC.  False alarm.

If you want to make any more changes, let me know, I'll be happy to grant a review and push this.

Comment 6

[William Maddox]

Pushed to nanojit-central:

http://hg.mozilla.org/projects/nanojit-central/rev/2a6f54d0e9aa

Comment 7

[Rick Reitmaier]

http://hg.mozilla.org/users/rreitmai_adobe.com/tr-working/rev/b7970b6a8c47

Comment 8

[Rick Reitmaier]

(In reply to comment #7)
> http://hg.mozilla.org/users/rreitmai_adobe.com/tr-working/rev/b7970b6a8c47

Bad cut and paste:

http://hg.mozilla.org/tamarin-redux/rev/db439cf3a690

Comment 9

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/9af1c8cd1be6

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9af1c8cd1be6