Closed Bug 732859 Opened 12 years ago Closed 12 years ago

IonMonkey: Crash on Heap

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: h4writer)

References

Details

(Keywords: crash, testcase)

Attachments

(1 file, 1 obsolete file)

The following testcase crashes on ionmonkey revision 1fd6c40d3852 (run with --ion -n):


function TestCase(n, d, e, a)
function writeHeaderToLog( string ) {}
var SECTION = "15.1.2.4";
for ( var CHARCODE = 128; CHARCODE < 256; CHARCODE++ ) {
  new TestCase( SECTION, "%"+ToHexString(CHARCODE), escape(String.fromCharCode(CHARCODE)));
}
function ToHexString( n ) {
  var hex = new Array();
  hex[hex.length] = n % 16;
  var string ="";
  for ( var index = 0 ; index < hex.length ; index++ ) {
    switch ( hex[index] ) {
    case 10:
      string += "A";
    case 11:
    }
  }
}
Hannes, the problem here is that MTableSwitch::replaceSuccessor(i, block) can miss updating the case table. SplitCriticalEdges() can insert new, empty blocks, and if those are inserted in between a switch and a case, we'll skip over them which leads to register allocation bugs.

I'm not sure what's the best fix here...
 (1) If the case table has unique members, we could just scan it... (gross)
 (2) Could we get rid of the case list and just use the successor list?
Assignee: general → hv1989
Attached patch Patch (obsolete) — Splinter Review
Thanks for the great analysis. Made it really easy to fix this bug.

There are still two lists:
1) successors list: this is now sorted like: [default, case 1, case 2, case 3 ...]
- Question: the order of successors is always maintained? (Just to be sure)

2) blocks list: contains a list of MBasicBlocks that still needs to get processed in the IonBuilder. this is sorted on pc and only needed for IonBuilder.
- Question 1: after finishing the tableswitch opcode, should I empty that list?
- Question 2: I first looked to put this list into CFGState, but didn't succeed. Vectors can't be put into an union. But I think that should be the place for that list, actually.

(When patch is reviewed, I'll make the patch properly with possible nits addressed, with author and commit message and I'll add the testcase too ;))
Attachment #604167 - Flags: review?(dvander)
(In reply to Hannes Verschore from comment #2)
> - Question: the order of successors is always maintained? (Just to be sure)

Yes, nothing changes it.

> - Question 1: after finishing the tableswitch opcode, should I empty that
> list?

I don't think it matters.

> - Question 2: I first looked to put this list into CFGState, but didn't
> succeed. Vectors can't be put into an union. But I think that should be the
> place for that list, actually.

I agree, but I wouldn't worry about it. You could do something like:

  typedef Vector<MBasicBlock *, 0, IonAllocPolicy> BlockList;

And then add a BlockList * to CFGState.
Comment on attachment 604167 [details] [diff] [review]
Patch

Sweet, thanks for fixing this!
Attachment #604167 - Flags: review?(dvander) → review+
Added testcase (+checkin comment etc)
Carries review+ over from previous patch.
Attachment #604167 - Attachment is obsolete: true
Attachment #604533 - Flags: review+
Attachment #604533 - Flags: checkin+
Attachment #604533 - Flags: checkin+
Keywords: checkin-needed
Whiteboard: checkin on ionmonkey branch
http://hg.mozilla.org/projects/ionmonkey/rev/76e469f863ae
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: checkin on ionmonkey branch
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug732859.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.