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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Unassigned)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Other Branch
ARM
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 627801 [details]
Testcase for shell

The attached testcase asserts on ionmonkey-arm (private branch) revision  (run with --ion -n -m --ion-eager).
(Reporter)

Updated

5 years ago
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.
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.
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.