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

RESOLVED FIXED in flash10.1

Status

()

--
critical
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: gkw, Assigned: edwsmith)

Tracking

(Blocks: 1 bug, {assertion, regression})

Trunk
flash10.1
assertion, regression
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
Created attachment 424568 [details]
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.
Looks like a Nanojit problem rather than a TM problem.  CC'ing some relevant people.

Comment 2

9 years ago
Yeah this looks like a backend issue. RIP out of range.
(Reporter)

Comment 3

9 years ago
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.
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
(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.
(Assignee)

Comment 6

9 years ago
Created attachment 439027 [details] [diff] [review]
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.
Assignee: general → edwsmith
Attachment #439027 - Flags: review?(dvander)
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.
Attachment #439027 - Flags: review?(dvander) → review+
Errr... would have always triggered it the displacement fell into isS8().
(Assignee)

Comment 9

9 years ago
NJ: http://hg.mozilla.org/projects/nanojit-central/rev/b534b9289ca8
Whiteboard: fixed-in-nanojit
TM: http://hg.mozilla.org/tracemonkey/rev/6afcd8e48456
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
(Assignee)

Comment 11

9 years ago
TR: http://hg.mozilla.org/tamarin-redux/rev/f96d5b967029
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin

Updated

9 years ago
Target Milestone: --- → flash10.1
(Reporter)

Comment 12

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

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/6afcd8e48456
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.