Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IonMonkey: (ARM) Assertion failure: (imm & ~(0xff)) == 0, at ion/arm/Assembler-arm.h:498

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

(1 attachment)

(Reporter)

Description

5 years ago
The following testcase asserts on ionmonkey-arm (private branch) revision  (run with --ion -n -m --ion-eager):


test();
function test() {
  enterFunc ('test');
  var whitespace = [
    {s : '\u0009', t : 'HORIZONTAL TAB'},
    {s : '\u2001', t : 'EM QUAD'},
    {s : '\u2002', t : 'EN SPACE'},
    {s : '\u2004', t : 'THREE-PER-EM SPACE'},
    {s : (.1  ), t : 'FOUR-PER-EM SPACE'},
    {s : '\u2008', t : 'PUNCTUATION SPACE'},
    {s : '\u2009', t : 'THIN SPACE'},
    {s : '\u200A', t : 'HAIR SPACE'},
    {s : '\u2029', t : 'PARAGRAPH SEPARATOR'},
    {s : '\u200B', t : 'ZERO WIDTH SPACE (category Cf)'}
    ];
}
This looks like a known bug that I have been dreading needing to fix.
(Assuming this is the issue I think it is, and making up numbers since I haven't actually stepped through this case, nor implemented spewing pool actions)
0x0: load 32 bit value from pool
0x10: unconditional branch that gives us a place to dump the pool
0xff0: load 64 bit floating point value from pool

If in the entire range of 0x10 through 0x1000, there are no unconditional branches,
then 0x10 will be the only perforation point, and get selected.
the offset from 0x0 will be made into a forward reference, like it should
and any other 32 bit loads will correctly get shifted into a backwards pool.
The problem comes in when the instruction at 0xff0 needs to be handled.  It gets shifted into the backwards half of the pool, but its offset will be at least 0xfe0, which is well outside the valid range for vectored loads.  The problem stems from the assumption that the shift-to-backwards pool will create a valid entry.
The fix will likely be to shift these entries a second time into a new forward pool.  IIRC, there are one or two minor complications with this method.
I also remember filing this bug before, but I cannot seem to find it in Bugzilla at the moment.
(Reporter)

Updated

5 years ago
Summary: IonMonkey: Assertion failure: (imm & ~(0xff)) == 0, at ion/arm/Assembler-arm.h:498 → IonMonkey: (ARM) Assertion failure: (imm & ~(0xff)) == 0, at ion/arm/Assembler-arm.h:498
Created attachment 629086 [details] [diff] [review]
/home/mrosenberg/patches/poolOverflowHack-r0.patch

A much more hackey solution than I wanted, but It'll work.  Notably I have completely broken cross-platform compatibility.  Well, mostly.  As far as I can tell, all of the assumptions that have been hard-coded into here are true on ARM, MIPS and PPC, but I don't know those architectures very well.  I'm planning on stripping them out after everything has stabilized.
Attachment #629086 - Flags: review?(Jacob.Bramley)
Comment on attachment 629086 [details] [diff] [review]
/home/mrosenberg/patches/poolOverflowHack-r0.patch

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

It's Ok, I think, but somewhat complicated.

This constant pool design still worries me, and every fix makes them more complicated. The relatively simple pools we used to use were probably the most common source of bugs (and the source of the nastiest bugs).

::: js/src/ion/arm/Assembler-arm.cpp
@@ +1496,5 @@
>      PoolHintPun php;
>      php.raw = *load;
>      php.phd.setIndex(token);
>      *load = php.raw;
> +    fprintf(stderr, "Inserted token %d into address %p\n", token, load_);

This looks like debug code.

@@ +1527,5 @@
>                            data.getCond(), instAddr);
>          }
>          break;
>        case PoolHintData::poolVDTR:
> +        if (offset+8*data.getIndex() - 8 < -1023 ||

Spaces around the operators. Also, parentheses would make this clearer:

if ((offset + 8 * data.getIndex() - 8) < -1023) ||
    [...]

@@ +1529,5 @@
>          break;
>        case PoolHintData::poolVDTR:
> +        if (offset+8*data.getIndex() - 8 < -1023 ||
> +            offset+8*data.getIndex() - 8 > 1023)
> +        {

Put the open curly bracket on the previous line.

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +671,5 @@
>              JS_ASSERT(p != NULL);
> +            int idx = p->numEntries-1;
> +            // Allocate space for tracking information that needs to be propagated to the next pool
> +            // as well as space for quickly updating the pool entries in the current pool to remove
> +            // the entries that don't actually fit.  I probably should change this over to a vector

The last bit should be flagged with a TODO (or similar).

@@ +673,5 @@
> +            // Allocate space for tracking information that needs to be propagated to the next pool
> +            // as well as space for quickly updating the pool entries in the current pool to remove
> +            // the entries that don't actually fit.  I probably should change this over to a vector
> +            outcastEntries[poolIdx] = new uint8[p->getPoolSize()];
> +            bool *preservedEntries = new bool[p->numEntries];

Did you mean to use "new" here? You don't seem to free the memory.

@@ +707,5 @@
>                  IonSpew(IonSpew_Pools, "Fixing offset to %d", codeOffset - magicAlign);
> +                if (!Asm::patchConstantPoolLoad(inst, (uint8*)inst + codeOffset - magicAlign)) {
> +                    // NOTE: if removing this entry happens to change the alignment of the next
> +                    // block, chances are you will have a bad time.
> +                    // ADDENDUM: this CANNOT happen on ARM

Why not? (Say so in the comment.)

@@ +718,5 @@
> +                }
> +            }
> +            // remove the elements of the pool that should not be there (YAY, MEMCPY)
> +            int idxDest;
> +            // If no elements were skipped, no expensive copy is necessary.

Is it really that expensive? Each entry will be pretty small.

@@ +781,5 @@
> +        }
> +        // this (*2) is not technically kosher, but I want to get this bug fixed.
> +        // It should actually be guardSize + the size of the instruction that we're attempting
> +        // to insert. Unfortunately that vaue is never passed in.  On ARM, these instructions
> +        // are always 4 bytes, so guardSize is legit to use.

Add a TODO. This needs to be fixed at some point. (Pass the value in if you need to.)

@@ +789,5 @@
> +            // There can still be an awkward situation where the element that triggered the
> +            // initial dump didn't fit into the pool backwards, and now, still does not fit into
> +            // this pool.  Now it is necessary to go and dump this pool (note: this is almost
> +            // certainly being called from dumpPool()).
> +            //

Delete the blank comment line.
Attachment #629086 - Flags: review?(Jacob.Bramley) → review+
Landed, and I remember fixing all of these nits.
http://hg.mozilla.org/projects/ionmonkey/rev/b82223048d7b
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.