The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

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

People

(Reporter: decoder, Unassigned)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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();
(Reporter)

Comment 1

5 years ago
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
Created attachment 628059 [details] [diff] [review]
/home/mrosenberg/patches/noPoolInCall-r0.patch
Attachment #628059 - Flags: review?
Created attachment 629090 [details] [diff] [review]
slightly more complete patch

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
landed + fixed: http://hg.mozilla.org/projects/ionmonkey/rev/cdfe0f8e0dd4
*cough*
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.