Closed Bug 666818 Opened 13 years ago Closed 13 years ago

IonMonkey: Segmentation fault when using continue in a while

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110622 Firefox/7.0a1
Build Identifier: 

When looking to bug #663575, I tried the same with a normal while (instead of do-while)

Testcase:

function f(){
   while(1) {
      continue;
   }
}
f();

Error:
Program received signal SIGSEGV, Segmentation fault.
0x08369947 in js::ion::MInstruction::isPhi (this=0xdadadada)
    at /home/haytjes/Build/ionmonkey/js/src/ion/MIR.h:380
380	    MIR_OPCODE_LIST(OPCODE_CASTS)

As you can see the pointer to this is 0xdadadada. So doesn't exist.
When looking into the code in the function IonBuilder::processDeferredContinues there is a call to IonBuilder::newBlock(); 
Only a pc is given to that function, so no predecessor.
That's why the slots_ (StackSlots) aren't copied, but they are needed in MBasicBlock::addPredecessor, hence the 0xdadadada pointer.

Reproducible: Always
Attached patch patch (obsolete) — Splinter Review
I could solve this issue by giving state.loop.entry as predecessor to the function. Not sure if it is the right solution.
As a notice. 
The same happens with a "break" statement instead of "continue" and code looks pretty much the same. So it should get a similar patch.
After further diving into the code and better understanding how the IonBuilder works, I must admit this patch is just plain wrong.

Problem is as following:
1) In the first block (upon start), Callee_slot and this_slot is added.
2) Every block copies that data through inheritance  // newBlock(pred, pc)
3) When creating a new block without inheritance, it doesn't has that data. But is initiated with the default stackPosition (so 2 in this case).
4) before iterating over the slots addPredecessor has an assert to check if length of stackPositions are the same (it is) and then goes over the slots. But it fails hard when checking if the first slot a phi is. (=> it isn't initialized = 0xdadadada pointer).

point (3) should get solved.
I think your patch is actually on the right track - we should really avoid creating blocks with NULL predecessors. I think finalizeLoop's break handling is broken too.

How about this instead? call newBlock(edge->block, pc), this way the new block inherits its first incoming edge.
Attached patch patchSplinter Review
Dvander you are right. This would solve the issue. I've added a new patch implementing this for continues and for breaks.
Attachment #541565 - Attachment is obsolete: true
Attachment #542286 - Flags: review?(dvander)
Comment on attachment 542286 [details] [diff] [review]
patch

Nice, thanks!
Attachment #542286 - Flags: review?(dvander) → review+
http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/aec23ed9c15a
Assignee: general → hv1989
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.