543440 - TM: "Assertion failed: (((op>>24)&255)>>6) == 2 (../nanojit/NativeX64.cpp" (64-bit)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: crash stack

No reproducible testcase for this assertion that was hit by jsfunfuzz on a 64-bit debug shell on 10.6.2:

Assertion failed: (((op>>24)&255)>>6) == 2 (../nanojit/NativeX64.cpp:176)

http://hg.mozilla.org/tracemonkey/file/3c3b005de959/js/src/nanojit/NativeX64.cpp#l176

Stack is provided as an attachment. Tracing seems to appear on the stack so I'm assuming this is related to TM.

Comment 1

[Nicholas Nethercote [inactive]]

Looks like a Nanojit problem rather than a TM problem.  CC'ing some relevant people.

Comment 2

[Andreas Gal :gal]

Yeah this looks like a backend issue. RIP out of range.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Assertion failed: (((op>>24)&255)>>6) == 2 (../nanojit/NativeX64.cpp

I'm getting this a lot on TM 64-bit Linux. Sadly, no testcase yet, either.

Comment 4

[Edwin Smith]

It looks to be a legitimate bug, i think the assert is valid and blame goes to the LEARIP function (and to me, i most likely wrote the original buggy code).

LEARIP calls emitrm(X64_learip, ...), and the contract for emitrm is to take opcodes that normally are disp32 addressing modes, and if possible optimize them to use 8-bit modes (and change the mod bit).  hence the assert - the input opcode must be a disp32 opcode.

however, RIP-relative addressing is a special mode, using 00 as the mode bits.  this makes that opcode incompatible with emitrm(). 

The only call site is from emit_quad(), here:

        if (isS32(int64_t(v)-int64_t(_nIns))) {
            // value is with +/- 2GB from RIP, can use LEA with RIP-relative disp32
            int32_t d = int32_t(int64_t(v)-int64_t(_nIns));
            emitrm(X64_learip, r, d, (Register)0);
            return;
        }

this is an optimization -- if we are generating a 64bit literal value that happens to be within a 32bit offset of RIP, then it tries to use an LEA instruction with RIP addressing to generate the value.

the fallback case below that block is simply to use a 64-bit literal, 

a quick workaround is to disable that whole code block.  A proper fix would be to inline and specialize emitrm for this case, since the 32-bit to 8-bit addressing mode optimization does not apply to RIP-relative addressing.

In fact I dont see how that "optimization" could have ever worked... probably a case of premature optimization.

Comment 5

[Edwin Smith]

(In reply to comment #4)

> In fact I dont see how that "optimization" could have ever worked... probably a
> case of premature optimization.

actually it works fine most of the time, when the constant is more than +/- 8 bits from RIP.  then, you dont get down the 8-bit arm and the RIP-relative LEA comes out fine.

fix coming.

Comment 6

[Edwin Smith]

Attachment: Loosen mod==2 assert and do the right thing for other addressing modes

RIP-relative LEA uses mod 00 "disp32" encoding, but mod_disp32() can still do the right thing in the other arm of its branch.  removed the assert and tightened the 8-bit optimization case in mod_disp32().

I was able to reproduce the assert in TR by moving it up, and tested the fix on TR as well.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 439027 [details] [diff] [review]
Loosen mod==2 assert and do the right thing for other addressing modes

Yeah I think it's fine to not take the isS8() path, since it seems like LEARIP would have always triggered that assert, and the assert was very rare.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Errr... would have always triggered it the displacement fell into isS8().

Comment 9

[Edwin Smith]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/b534b9289ca8

Comment 10

[Nicholas Nethercote [inactive]]

TM: http://hg.mozilla.org/tracemonkey/rev/6afcd8e48456

Comment 11

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/f96d5b967029

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This assertion has seemed to be fixed on TM. Thanks, folks! :)

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6afcd8e48456

Comment 14

[Christian Holler (:decoder)]

Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.