Closed Bug 759317 Opened 10 years ago Closed 10 years ago

IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004

Categories

(Core :: JavaScript Engine, defect)

Other Branch
ARM
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Unassigned)

References

Details

(Keywords: crash, testcase)

Attachments

(1 file, 1 obsolete file)

The following testcase crashes on ionmonkey-arm (private branch) revision  (run with --ion -n -m --ion-eager):


function f() {
    var proto = {p8:8};
    var obj = {
        p0:0, p1:1, p2:2, p3:3, p4:.1 , p5:5, p6:6, 
	String: ["toSource", "quote", "bold", "italics", "fixed", "fontsize",
               "fontcolor", "link", "anchor", "strike", "small", "big", "blink",
               "sup", "sub", "substr", "trimLeft", "trimRight", "toJSON"], p8:8, p9:9, 
        p10:0, p11:1, p12:2, p13:3, p14:4, p15:5, p16:6, p17:7, p18:8, p19:9, 
        m: function() { return 42; }
    };
}
actual = f();
Crash info:

Program received signal SIGILL, Illegal instruction.
0x40172b40 in ?? ()
(gdb) x /i $pc
=> 0x40172b40:                  ; <UNDEFINED> instruction: 0xffff0004
That is almost certainly us attempting to execute a pool's guard.  They are intentionally constructed with the upper 16 bits set so they look nothing like a valid instruction.
Yup.  Normally, Ion -> Ion calls are made by the sequence
push pc;
call func;
in this case, we were inserting a pool in between the push and the call, so the sequence looked like:
push pc;
b afterPool;
poolHeader;
poolData1;
...
call func;

os when we return from the "call", we actually return to the poolHeader, which will never be a legal instruction
Summary: IonMonkey: Crash due to Illegal instruction 0xffff0004 (opt-only) → IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004 (opt-only)
The fact that it was opt only has nothing to do with the failure, just we generate more instructions in a debug build, and as a result, the pool ends up getting placed in a slightly less volatile location
Summary: IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004 (opt-only) → IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004
Evidently, I had not qrefreshed before uploading this patch.  Also leaving the requestee field as ":jbramley" does not actually ask anyone for a review.
Attachment #628059 - Attachment is obsolete: true
Attachment #628059 - Flags: review?
Attachment #629090 - Flags: review?
Comment on attachment 629090 [details] [diff] [review]
slightly more complete patch

In this case, it seems that Jacob wasn't cc'ed to the bug, and as such did not have access to the bug, so the reviewer selection didn't work as a result.
Attachment #629090 - Flags: review? → review?(Jacob.Bramley)
Comment on attachment 629090 [details] [diff] [review]
slightly more complete patch

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

::: js/src/ion/arm/Assembler-arm.cpp
@@ +2279,5 @@
>  
> +bool instIsBNop(Instruction *inst) {
> +    // In some special situations, it is necessary to insert a NOP
> +    // into the instruction stream that nobody knows about, since nobody should know about
> +    // it, make sure nobody sees it while traversing the instruction stream.

This comment doesn't explain the code. Clearly, _somebody_ needs to see the BNop else this function wouldn't exist.

You should also mention what instruction you are looking for, just to make it clear, since assembly instructions are easier to read than (even high-level) instrution decode logic.

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +833,5 @@
> +            // and there have been no valid places that a pool could be dumped thusfar.
> +            // If a pool were to fill up before this no-pool section ends, we need to go back
> +            // in the stream and enter a pool guard after the fact.  This is feasable, but
> +            // for now, it is easier to just allocate a junk instruction, default it to a nop, and
> +            // finally, if the pool *is* needed, patch the nop to  apool guard.

JaegerMonkey's equivalent to enterNoPool had a 'size' parameter so you could specify the maximum number of instructions that would be emitted. (The equivalent to leaveNoPool checked that the limit was respected.) This worked quite well, and didn't require any weirdness. Why not do the same here?

Also, the comment would be much clearer with some assembly examples of what's happening.
Attachment #629090 - Flags: review?(Jacob.Bramley) → review+
(In reply to Jacob Bramley [:jbramley] from comment #8)
> Comment on attachment 629090 [details] [diff] [review]
> slightly more complete patch
> 
> Review of attachment 629090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/arm/Assembler-arm.cpp
> @@ +2279,5 @@
> >  
> > +bool instIsBNop(Instruction *inst) {
> > +    // In some special situations, it is necessary to insert a NOP
> > +    // into the instruction stream that nobody knows about, since nobody should know about
> > +    // it, make sure nobody sees it while traversing the instruction stream.
> 
> This comment doesn't explain the code. Clearly, _somebody_ needs to see the
> BNop else this function wouldn't exist.
Yes, the comment is poorly worded.  This function exists so the next function can know to skip over it.


> 
> ::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
> @@ +833,5 @@
> > +            // and there have been no valid places that a pool could be dumped thusfar.
> > +            // If a pool were to fill up before this no-pool section ends, we need to go back
> > +            // in the stream and enter a pool guard after the fact.  This is feasable, but
> > +            // for now, it is easier to just allocate a junk instruction, default it to a nop, and
> > +            // finally, if the pool *is* needed, patch the nop to  apool guard.
> 
> JaegerMonkey's equivalent to enterNoPool had a 'size' parameter so you could
> specify the maximum number of instructions that would be emitted. (The
> equivalent to leaveNoPool checked that the limit was respected.) This worked
> quite well, and didn't require any weirdness. Why not do the same here?
I may have designed this specifically attempting to avoid doing that, since there were some places where we'd specified "no pools", but got the number wrong, and this lead to problems.  I felt that in general, knowing how many instructions were going to be generated between two points in time was difficult, and expecting people to manually compute it was hard.
Marty sez not s-s.
Group: core-security
*cough*
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.