Last Comment Bug 692215 - IM: Assertion failure: backing.firstCopy == stackPosition_, at ion/MIRGraph.cpp:362
: IM: Assertion failure: backing.firstCopy == stackPosition_, at ion/MIRGraph.c...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2011-10-05 12:20 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 07:54 PST (History)
4 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (3.01 KB, patch)
2011-10-11 16:02 PDT, David Anderson [:dvander]
cdleary: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2011-10-05 12:20:52 PDT
The following testcase asserts on ionmonkey revision acf3c1fb7c94 (run with --ion-eager), tested on 64 bit:


function test1() {
    var src = "switch(x) {\n";
    for (var i=-1; i<4; i++) {
        src += (i >= 0) ? src : "default:\n";
    }
}
test1();
Comment 1 David Anderson [:dvander] 2011-10-11 16:02:45 PDT
Created attachment 566384 [details] [diff] [review]
fix

We were missing code to handle the case where we set a stack slot, but it's in the middle of a copy list. The copy list would become corrupted. This patch rewrites the nodes around the entry that is about to be overwritten.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-10-26 12:47:19 PDT
Comment on attachment 566384 [details] [diff] [review]
fix

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

::: js/src/ion/MIRGraph.cpp
@@ +226,5 @@
>          slots_[lowest].copyOf = NotACopy;
>          slots_[lowest].firstCopy = prev;
> +    } else if (var.isCopy()) {
> +        uint32 prev = var.copyOf;
> +        if (slots_[prev].firstCopy != slot) {

I think we would hit this case less if we iterated over the stack positions backwards in the addPredecessor. Probably doesn't matter though.

@@ +234,5 @@
> +                prev = slots_[prev].nextCopy;
> +                JS_ASSERT(prev != NotACopy);
> +            }
> +        }
> +        slots_[prev].firstCopy = var.nextCopy;

I kind of dislike this because we're abusing the firstCopy/nextCopy union, which is confusing to read. When you take the conditional you're actually clobbering a nextCopy field, and when you don't you're really setting the firstCopy field. Can we do a positive test? Like,

  uint32 backing = var.copyOf;
  if (slots_[backing].firstCopy == slot) {
    slots_[backing].firstCopy = var.nextCopy;
  } else {
    // ... iterative thing to set nextCopy on a list member to var.nextCopy...
  }

That way if they were made into non-union members it would still work alright.
Comment 3 David Anderson [:dvander] 2011-10-28 15:46:57 PDT
> That way if they were made into non-union members it would still work alright.

Thanks, I think this is clearer too.

http://hg.mozilla.org/projects/ionmonkey/rev/c7c0d34dc0d9
Comment 4 Christian Holler (:decoder) 2013-01-14 07:54:52 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug692215.js.

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