Closed Bug 663575 Opened 13 years ago Closed 13 years ago

IonMonkey: Assertion failure: found, at IonBuilder.cpp:853

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Test case:

function f() {
	do {
		continue;
	} while (0);
}
f();
Attached file patch (obsolete) —
After some debugging I narrowed it down to:

In IonBuilder::doWhileLoop a new newLoopHeader(current, pc) is created. Lateron this is given to IonBuilder::pushLoop. Silently pushLoop takes the pc from the loopHeader as the continuePc. In case of a while loop this is correct. (In a while loop the first instruction is a jump to the condition followed by the ifne). In case of a do-while loop this is incorrect. (the first instruction is a NOP, followed by TRACE and then the code. Somewhere in between there is the condition and the ifne.). So to solve this newLoopHeader should get created with the right pc. That way it can get matched in IonBuilder::processContinue with the jump pc in the continue.

When applying this patch and running the testcase you will get a segmentation fault. It is the same segmentation fault as bug #666818. But I noticed that the patch on that bug report doesn't fix it totally. So patch in that bug report isn't complete yet.
Attached patch patch2 (obsolete) — Splinter Review
Ok, let me update my last statement. 

- The given patch will solve the problem that the right loop can't be found
- doWhile still remains buggy. There are more issues. I think the main issue is that a doWhile is done as one big block. I think there should be a block containing the body and a block containing the conditions/ifne. This way the deferredContinue's can be handled in the "body-part" and the other things in the "cond-part"
=> So that's what I've done in this patch.

function f(){ do{} while(0); } didn't regress and still works
function f(){ do{continue;} while(0); } doesn't fail anymore. C1visualizer does give some errors when opening the ion.cfg. I'm not sure why, but the graph looks like intended.

Note: patch in #541565 is also applied in this patch.
Attachment #541589 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
The cause of the errors in c1visualizer was bug the faulty patch in bug #666818. Now that it is corrected, it works correct.

So this patch only contains the fix for this bug. (The fix in #66818 isn't applied. So when trying without that patch you'll get an assertion failure).
I've also updated the pc ranges of the blocks. They should be correct, except the stopAt value given to
Attachment #541691 - Attachment is obsolete: true
    if (!pushLoop(CFGState::DO_WHILE_LOOP_BODY, conditionpc, header, bodyStart, bodyEnd, exitpc, conditionpc))

is still one to high. It should contain the last instruction of the body block. Now it contains the first instruction of the condition block. I have no idea how to correct this.

(sorry, about the messed up comment. Accidentally pressed submit.
Attached patch patch (obsolete) — Splinter Review
I was wrong with my previous comment. After careful looking the stopAt pc is correct. So this patch is the same as previous, except it had bit rotten. So did it again on top of the newest revision.
Attachment #542315 - Attachment is obsolete: true
Attachment #542429 - Flags: review?(dvander)
Blocks: IonMonkey
Comment on attachment 542429 [details] [diff] [review]
patch

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

Sorry for the delay. You're absolutely right, this should be two blocks.

::: js/src/ion/IonBuilder.cpp
@@ +977,4 @@
>  
>      // do { } while() loops have the following structure:
>      //    NOP         ; SRC_WHILE (offset to IFNE)
> +    //    TRACE       ; SRC_WHILE (offset to COND)

The TRACE actually goes to the IFNE, if you run dis(f):

main:
00000:  nop
00001:  trace 0
00004:  goto 8 (+4)
00007:  nullblockchain
00008:  false
00009:  ifne 1 (-8)
00012:  stop

 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] newline 
  1:    2     0 [   0] while    offset 9
  3:    2     1 [   1] while    offset 8
  5:    2     4 [   3] newline 
  6:    3     4 [   0] continue

In jsemit.cpp, around line ~4930, both srcnotes get set to the same offset:
        if (!js_SetSrcNoteOffset(cx, cg, noteIndex2, 0, beq - top))
            return JS_FALSE;
        if (!js_SetSrcNoteOffset(cx, cg, noteIndex, 0, 1 + (beq - top)))
            return JS_FALSE;

So, the easiest thing to do is probably to change one of these src notes to point at the condition. Changing the TRACE would mess up the tracer, so that's not too appealing. Changing the NOP is probably easier. The only tricky part is in the decompiler, which is in jsopcode.cpp.

Around line ~2230 or so you can see where it sniffs JSOP_NOP and uses SRC_WHILE to infer a do-while loop. But, we're guaranteed the presence of TRACE opcodes, we can use that to read the next srcnote and get the IFNE.
Attachment #542429 - Flags: review?(dvander)
Just minor changes (to IonBuilder about do-while-loop) to reflect what has been said above.
Attachment #542429 - Attachment is obsolete: true
Attachment #544559 - Flags: review?(dvander)
The changes to jsemit/jsopcode to adjust srcnote of NOP.
I did a bit different than requested:
jsemit: I didn't move the js_SetSrcNoteOffset because of the comment that the srcnote of trace could move things.
jsopcode: I also didn't adjust the GetSrcNoteOffset for all instructions to the TRACE op. I only use the TRACE offset if it is a WHILE.

I ran jstests.py and only it gave only timeouts, no fails
Attachment #544563 - Flags: review?(dvander)
Attachment #544563 - Flags: review?(dvander) → review+
Comment on attachment 544559 [details] [diff] [review]
Ionmonkey changes

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

Looks good, r=me with one change:

::: js/src/ion/IonBuilder.cpp
@@ +996,5 @@
>          return ControlStatus_Error;
>  
> +    CFGState &state = cfgStack_.back();
> +    state.loop.updatepc = conditionpc;
> +    state.loop.updateEnd = ifne;

These two things are only needed for |for| loops.
Attachment #544559 - Flags: review?(dvander) → review+
(In reply to comment #9)

> ::: js/src/ion/IonBuilder.cpp
> @@ +996,5 @@
> >          return ControlStatus_Error;
> >  
> > +    CFGState &state = cfgStack_.back();
> > +    state.loop.updatepc = conditionpc;
> > +    state.loop.updateEnd = ifne;
> 
> These two things are only needed for |for| loops.

I think I can remove the state.loop.updatepc, but I can't remove the state.loop.updateEnd. I need the position of the ifne in "processDoWhileBodyEnd" to set the last instruction it should parse before halting. I have no idea how I should get it otherwise ...
Ah, okay, that's fine then.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/9b2e6ea86756
Assignee: general → hv1989
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Version: unspecified → Trunk
(Reopening, the first patch still has to land on the IonMonkey branch.)
Status: RESOLVED → REOPENED
Flags: in-testsuite?
Resolution: FIXED → ---
As soon as mozilla-central gets merged into ionmonkey, I will commit the second part. (This is by no means a request to merge, just a notice that I didn't forget to land the second part)
http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/8e63cea7e912
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.