Last Comment Bug 759317 - IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004
: IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: ARM Linux
: -- major (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-29 06:24 PDT by Christian Holler (:decoder)
Modified: 2012-07-02 14:58 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/noPoolInCall-r0.patch (3.47 KB, patch)
2012-05-29 12:28 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
slightly more complete patch (5.26 KB, patch)
2012-06-01 00:10 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-29 06:24:05 PDT
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();
Comment 1 Christian Holler (:decoder) 2012-05-29 06:24:31 PDT
Crash info:

Program received signal SIGILL, Illegal instruction.
0x40172b40 in ?? ()
(gdb) x /i $pc
=> 0x40172b40:                  ; <UNDEFINED> instruction: 0xffff0004
Comment 2 Marty Rosenberg [:mjrosenb] 2012-05-29 07:33:09 PDT
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.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-05-29 07:53:37 PDT
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
Comment 4 Marty Rosenberg [:mjrosenb] 2012-05-29 12:26:26 PDT
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
Comment 5 Marty Rosenberg [:mjrosenb] 2012-05-29 12:28:18 PDT
Created attachment 628059 [details] [diff] [review]
/home/mrosenberg/patches/noPoolInCall-r0.patch
Comment 6 Marty Rosenberg [:mjrosenb] 2012-06-01 00:10:30 PDT
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.
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2012-06-01 00:15:34 PDT
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.
Comment 8 Jacob Bramley [:jbramley] 2012-06-01 07:37:03 PDT
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.
Comment 9 Marty Rosenberg [:mjrosenb] 2012-06-01 11:52:28 PDT
(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.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-04 15:40:40 PDT
Marty sez not s-s.
Comment 11 Marty Rosenberg [:mjrosenb] 2012-07-02 14:57:17 PDT
landed + fixed: http://hg.mozilla.org/projects/ionmonkey/rev/cdfe0f8e0dd4
Comment 12 Marty Rosenberg [:mjrosenb] 2012-07-02 14:58:01 PDT
*cough*

Note You need to log in before you can comment on or make changes to this bug.