Assertion failure: !suspended (../core/../nanojit/LIR.h:2251)

VERIFIED FIXED

Status

Core Graveyard
Nanojit
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Brent Baker, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Bug Flags:
in-testsuite +
flashplayer-qrb +
flashplayer-bug -
flashplayer-triage +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Running regress/bug_599357.abc with a debug shell is causing the assert to happen.

See bug# 599357 for more information on the original issue.

Looks like an issue with numerical access to object properties, possibly tied to work around bug# 696070?
Flags: in-testsuite+
Flags: flashplayer-triage+
Flags: flashplayer-qrb+
Flags: flashplayer-bug-
No longer blocks: 78094
(Reporter)

Comment 1

6 years ago
2  0x0000000100009da8 in avmplus::AvmDebugMsg (p=0x100228d40 "", debugBreak=true) at ../AVMPI/AvmAssert.cpp:69
#3  0x000000010000980e in avmplus::AvmAssertFail (message=0x100228d40 "") at AvmAssert.h:66
#4  0x00000001000cdb9d in nanojit::CseFilter::suspend (this=0x10127d0e0) at LIR.h:2251
#5  0x00000001000af266 in avmplus::CodegenLIR::suspendCSE (this=0x7fff5fbfdcb0) at ../core/CodegenLIR.cpp:772
#6  0x00000001000b5319 in avmplus::CodegenLIR::emitInlineVectorRead (this=0x7fff5fbfdcb0, objIndexOnStack=9, index=0x10128aaf8, arrayDataOffset=48, lenOffset=0, entriesOffset=16, scale=2, load_item=nanojit::LIR_ldi, helper=0x100306220) at ../core/CodegenLIR.cpp:5006
#7  0x00000001000b859e in avmplus::CodegenLIR::emitGetIndexedProperty (this=0x7fff5fbfdcb0, objIndexOnStack=9, index=0x10128aaf8, result=0x1010385a0, idxKind=avmplus::VI_UINT) at ../core/CodegenLIR.cpp:5050
#8  0x00000001000c0e7c in avmplus::CodegenLIR::emit (this=0x7fff5fbfdcb0, opcode=avmplus::ActionBlockConstants::OP_getproperty, op1=9, op2=0, result=0x1010385a0) at ../core/CodegenLIR.cpp:6253
#9  0x00000001000c3224 in avmplus::CodegenLIR::writeOp2 (this=0x7fff5fbfdcb0, state=0x1010405f0, pc=0x10121c30f "f\tH\020!", opcode=avmplus::ActionBlockConstants::OP_getproperty, opd1=9, opd2=2, type=0x1010385a0) at ../core/CodegenLIR.cpp:3707
#10 0x00000001000d1561 in avmplus::NullWriter::writeOp2 (this=0x7fff5fbfd9f0, state=0x1010405f0, pc=0x10121c30f "f\tH\020!", opcode=avmplus::ActionBlockConstants::OP_getproperty, opd1=9, opd2=2, type=0x1010385a0) at ../core/Coder.cpp:152
#11 0x000000010013f145 in avmplus::Verifier::emitGetProperty (this=0x7fff5fbfdaa0, multiname=@0x7fff5fbfd460, n=2, imm30=9, pc=0x10121c30f "f\tH\020!") at ../core/Verifier.cpp:2952
#12 0x0000000100142b31 in avmplus::Verifier::verifyBlock (this=0x7fff5fbfdaa0, start_pos=0x10121c2ee "W*c\0040e") at ../core/Verifier.cpp:1519
Assignee: nobody → virgilp
QA Contact: nanojit → brbaker
(Reporter)

Updated

6 years ago
Blocks: 613140

Comment 2

6 years ago
This would have been injected by me in bug #695328.

Comment 3

6 years ago
The reason is that the VMCFG_FASTPATH_ADD_INLINE code already suspends CSE several frames up on the stack.  I'll take this bug and add an additional condition if possible.
Assignee: virgilp → lhansen

Comment 4

6 years ago
Not able to repro in TR, will try the float queue...

Comment 5

6 years ago
Created attachment 568621 [details] [diff] [review]
Tentative fix

What I will try to use once I'm able to repro the problem.

Comment 6

6 years ago
Repro'd in float branch with base=TR6675.  Proposed test fixes it.

Comment 7

6 years ago
Generally speaking, uses of suspendCSE/resumeCSE should be bracketed in regions by macros a la these:

#define SUSPENDCSE_BEGIN \
    bool cse_flag__ = false; \
    if (cseFilter) cse_flag__ = cseFilter->suspendMaybe();

#define SUSPENDCSE_END \
    if (cseFilter) cseFilter->resumeMaybe(cse_flag__);

but that's a bigger change.

Updated

6 years ago
Attachment #568621 - Flags: review?(edwsmith)

Comment 8

6 years ago
Comment on attachment 568621 [details] [diff] [review]
Tentative fix

ouch.

fix looks correct for one level of nesting, but would need fixing again for more than that (counter, ++ for each suspend, -- for each resume).
Attachment #568621 - Flags: review?(edwsmith)
Attachment #568621 - Flags: review+
Attachment #568621 - Flags: feedback?(wmaddox)

Comment 9

6 years ago
Comment on attachment 568621 [details] [diff] [review]
Tentative fix

This is clunky, and only works for one level of nesting. It would be better to change suspendCSE()/resumeCSE() to maintain a count of open suspend requests, rather than a boolean flag, and inhibit CSE whenever the count is non-zero.  I think this is what Edwin suggested, but I'd vote for changing the basic semantics of suspendCSE()/resumeCSE() rather than wrapping them with another layer of crud.
Attachment #568621 - Flags: feedback?(wmaddox) → feedback-

Comment 10

6 years ago
OK, I'll try to find time to add counters and post another patch, but that's as far as I'm prepared to take it.  If that's not good enough then the bug will go unfixed.

I don't know what you mean by "I'd vote for changing the basic semantics of suspendCSE()/resumeCSE() rather than wrapping them with another layer of crud."  What is the "crud" here?  Boolean flag?  Counter?  Something else?

Comment 11

6 years ago
Created attachment 570210 [details] [diff] [review]
Better fix

Allows arbitrary nesting of regions, needs more testing but is straightforward.

Comment 12

6 years ago
Comment on attachment 570210 [details] [diff] [review]
Better fix

This passes various levels of testing and seems ready for review.
Attachment #570210 - Flags: review?(wmaddox)

Comment 13

6 years ago
I'd also assert that suspended==0 in CseFilter's destructor :)

Comment 14

6 years ago
(In reply to Virgil Palanciuc from comment #13)
> I'd also assert that suspended==0 in CseFilter's destructor :)

OK, good point.  I will add that before landing.

Comment 15

6 years ago
Comment on attachment 570210 [details] [diff] [review]
Better fix

Review of attachment 570210 [details] [diff] [review]:
-----------------------------------------------------------------

R+.  This is exactly what I had in mind.
Attachment #570210 - Flags: review?(wmaddox) → review+

Comment 16

6 years ago
(In reply to Lars T Hansen from comment #14)
> (In reply to Virgil Palanciuc from comment #13)
> > I'd also assert that suspended==0 in CseFilter's destructor :)
> 
> OK, good point.  I will add that before landing.

Actually it's never destructed, it comes out of a mark/release pool.

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 17

6 years ago
changeset: 6687:0a0700177c14
user:      Lars T Hansen <lhansen@adobe.com>
summary:   Fix 696287 - Assertion failure: !suspended (../core/../nanojit/LIR.h:2251) (r=wmaddox)

http://hg.mozilla.org/tamarin-redux/rev/0a0700177c14
(Reporter)

Comment 18

6 years ago
Verified fix in the float patch queue using regress/bug_599357.as

tamarin-redux: 6694:288961a14c43
float patches:      324:ba70538d70db
Status: RESOLVED → VERIFIED

Comment 19

6 years ago
changeset: 6738:a11ef92fadd0
user:      Lars T Hansen <lhansen@adobe.com>
summary:   Fix 696287 - Assertion failure: !suspended (../core/../nanojit/LIR.h:2251) (r=wmaddox)

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

Updated

6 years ago
Assignee: lhansen → nobody
Component: Baseline JIT (CodegenLIR) → Nanojit
OS: Mac OS X → All
Product: Tamarin → Core
QA Contact: brbaker → nanojit
Hardware: x86 → All
Whiteboard: fixed-in-nanojit, fixed-in-tamarin
(Assignee)

Updated

3 years ago
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.