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

Status: VERIFIED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Brent Baker]

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?

Comment 1

[Brent Baker]

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

Comment 2

[Lars T Hansen]

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

Comment 3

[Lars T Hansen]

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.

Comment 4

[Lars T Hansen]

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

Comment 5

[Lars T Hansen]

Attachment: Tentative fix

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

Comment 6

[Lars T Hansen]

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

Comment 7

[Lars T Hansen]

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.

Comment 8

[Edwin Smith]

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).

Comment 9

[William Maddox]

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.

Comment 10

[Lars T Hansen]

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

[Lars T Hansen]

Attachment: Better fix

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

Comment 12

[Lars T Hansen]

Comment on attachment 570210 [details] [diff] [review]
Better fix

This passes various levels of testing and seems ready for review.

Comment 13

[Virgil Palanciuc]

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

Comment 14

[Lars T Hansen]

(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

[William Maddox]

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.

Comment 16

[Lars T Hansen]

(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.

Comment 17

[Tamarin Bot]

changeset: 6687:0a0700177c14
user:      Lars T Hansen <[e-mail]>
summary:   Fix 696287 - Assertion failure: !suspended (../core/../nanojit/LIR.h:2251) (r=wmaddox)

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

Comment 18

[Brent Baker]

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

tamarin-redux: 6694:288961a14c43
float patches:      324:ba70538d70db

Comment 19

[Tamarin Bot]

changeset: 6738:a11ef92fadd0
user:      Lars T Hansen <[e-mail]>
summary:   Fix 696287 - Assertion failure: !suspended (../core/../nanojit/LIR.h:2251) (r=wmaddox)

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