Closed Bug 759209 Opened 12 years ago Closed 12 years ago

IonMonkey: (ARM) Assertion failure: data == imm, at ion/arm/Assembler-arm.h:540

Categories

(Core :: JavaScript Engine, defect)

Other Branch
ARM
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Unassigned)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file Testcase for shell
The attached testcase asserts on ionmonkey-arm (private branch) revision  (run with --ion -n -m --ion-eager).
Summary: IonMonkey: Assertion failure: data == imm, at ion/arm/Assembler-arm.h:540 → IonMonkey: (ARM) Assertion failure: data == imm, at ion/arm/Assembler-arm.h:540
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.
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.
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.
Attachment #629026 - Flags: review?(Jacob.Bramley)
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.
Attachment #629026 - Flags: review?(Jacob.Bramley) → review+
(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.
landed, seems stable: http://hg.mozilla.org/projects/ionmonkey/rev/2062cc1c4b06
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.