Last Comment Bug 759209 - IonMonkey: (ARM) Assertion failure: data == imm, at ion/arm/Assembler-arm.h:540
: IonMonkey: (ARM) Assertion failure: data == imm, at ion/arm/Assembler-arm.h:540
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: ARM Linux
: -- major (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-28 15:34 PDT by Christian Holler (:decoder)
Modified: 2012-06-11 13:28 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase for shell (953 bytes, text/plain)
2012-05-28 15:34 PDT, Christian Holler (:decoder)
no flags Details
/home/mrosenberg/patches/fixPoolOffByOne-r0.patch (2.73 KB, patch)
2012-05-31 19:03 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-28 15:34:33 PDT
Created attachment 627801 [details]
Testcase for shell

The attached testcase asserts on ionmonkey-arm (private branch) revision  (run with --ion -n -m --ion-eager).
Comment 1 Marty Rosenberg [:mjrosenb] 2012-05-31 16:42:47 PDT
Haven't quite finished looking into this, but it seems *quite* impressive.
Currently, this looks like a very silly off by one error that should have existed since the pool code was written.  I'd thought I had caught all of those at this point.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-05-31 18:38:59 PDT
Well, I was mostly right.  There were *two* off by one errors, in the same direction!
Guess that code path wasn't tested much at all.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-05-31 19:03:56 PDT
Created attachment 629026 [details] [diff] [review]
/home/mrosenberg/patches/fixPoolOffByOne-r0.patch

I think there are still some off by one issues, but this works, so I'm tempted to not go hunting.  If there are off by one errors, they are in opposite directions, so they cancel out.
Comment 4 Jacob Bramley [:jbramley] 2012-06-01 03:29:38 PDT
Comment on attachment 629026 [details] [diff] [review]
/home/mrosenberg/patches/fixPoolOffByOne-r0.patch

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

::: js/src/ion/arm/Assembler-arm.h
@@ +1240,5 @@
>      {
>          // Set up the backwards double region
>          new (&pools_[2]) Pool (1024, 8, 4, 8, 8, true);
>          // Set up the backwards 32 bit region
> +        new (&pools_[3]) Pool (4096, 4, 4, 8, 4, true, true);

Is the 'bias' offset the PC+8 thing? If so, this change is fine.
Comment 5 Marty Rosenberg [:mjrosenb] 2012-06-01 12:01:08 PDT
(In reply to Jacob Bramley [:jbramley] from comment #4)
> Comment on attachment 629026 [details] [diff] [review]
> /home/mrosenberg/patches/fixPoolOffByOne-r0.patch
> 
> Review of attachment 629026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/arm/Assembler-arm.h
> @@ +1240,5 @@
> >      {
> >          // Set up the backwards double region
> >          new (&pools_[2]) Pool (1024, 8, 4, 8, 8, true);
> >          // Set up the backwards 32 bit region
> > +        new (&pools_[3]) Pool (4096, 4, 4, 8, 4, true, true);
> 
> Is the 'bias' offset the PC+8 thing? If so, this change is fine.

Yeah, I'm not entirely sure what I was thinking when I wrote this code.  This is definitely one case where named arguments would make me so happy, because seeing "bais:=4" would have been a red flag from the beginning.
Comment 6 Marty Rosenberg [:mjrosenb] 2012-06-11 13:28:31 PDT
landed, seems stable: http://hg.mozilla.org/projects/ionmonkey/rev/2062cc1c4b06

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