Closed
Bug 663575
Opened 13 years ago
Closed 13 years ago
IonMonkey: Assertion failure: found, at IonBuilder.cpp:853
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: adrake, Assigned: h4writer)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
4.91 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Test case: function f() { do { continue; } while (0); } f();
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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
Assignee | ||
Comment 3•13 years ago
|
||
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
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
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)
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)
Assignee | ||
Comment 7•13 years ago
|
||
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)
Assignee | ||
Comment 8•13 years ago
|
||
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)
Updated•13 years ago
|
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+
Assignee | ||
Comment 10•13 years ago
|
||
(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.
Comment 12•13 years ago
|
||
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 → ---
Assignee | ||
Comment 14•13 years ago
|
||
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)
Assignee | ||
Comment 15•13 years ago
|
||
http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/8e63cea7e912
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•