Last Comment Bug 696287 - Assertion failure: !suspended (../core/../nanojit/LIR.h:2251)
: Assertion failure: !suspended (../core/../nanojit/LIR.h:2251)
Status: VERIFIED FIXED
fixed-in-nanojit, fixed-in-tamarin
:
Product: Core Graveyard
Classification: Graveyard
Component: Nanojit (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: float/float4
  Show dependency treegraph
 
Reported: 2011-10-20 18:23 PDT by Brent Baker
Modified: 2014-03-17 08:00 PDT (History)
3 users (show)
brbaker: in‑testsuite+
brbaker: flashplayer‑qrb+
brbaker: flashplayer‑bug-
brbaker: flashplayer‑triage+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Tentative fix (4.27 KB, patch)
2011-10-21 00:37 PDT, Lars T Hansen
edwsmith: review+
wmaddox: feedback-
Details | Diff | Splinter Review
Better fix (3.15 KB, patch)
2011-10-28 02:21 PDT, Lars T Hansen
wmaddox: review+
Details | Diff | Splinter Review

Description Brent Baker 2011-10-20 18:23:05 PDT
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 2011-10-20 18:26:57 PDT
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 2011-10-21 00:16:35 PDT
This would have been injected by me in bug #695328.
Comment 3 Lars T Hansen 2011-10-21 00:18:21 PDT
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 2011-10-21 00:30:20 PDT
Not able to repro in TR, will try the float queue...
Comment 5 Lars T Hansen 2011-10-21 00:37:00 PDT
Created attachment 568621 [details] [diff] [review]
Tentative fix

What I will try to use once I'm able to repro the problem.
Comment 6 Lars T Hansen 2011-10-21 01:20:15 PDT
Repro'd in float branch with base=TR6675.  Proposed test fixes it.
Comment 7 Lars T Hansen 2011-10-21 01:28:10 PDT
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 2011-10-21 07:49:02 PDT
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 2011-10-21 13:30:14 PDT
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 2011-10-23 03:31:31 PDT
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 2011-10-28 02:21:27 PDT
Created attachment 570210 [details] [diff] [review]
Better fix

Allows arbitrary nesting of regions, needs more testing but is straightforward.
Comment 12 Lars T Hansen 2011-10-28 02:41:14 PDT
Comment on attachment 570210 [details] [diff] [review]
Better fix

This passes various levels of testing and seems ready for review.
Comment 13 Virgil Palanciuc 2011-10-28 03:12:55 PDT
I'd also assert that suspended==0 in CseFilter's destructor :)
Comment 14 Lars T Hansen 2011-10-28 05:08:23 PDT
(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 2011-10-28 12:02:19 PDT
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 2011-10-29 00:04:53 PDT
(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 2011-10-29 00:43:28 PDT
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
Comment 18 Brent Baker 2011-11-02 04:55:29 PDT
Verified fix in the float patch queue using regress/bug_599357.as

tamarin-redux: 6694:288961a14c43
float patches:      324:ba70538d70db
Comment 19 Tamarin Bot 2011-11-17 13:12:08 PST
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

Note You need to log in before you can comment on or make changes to this bug.