Closed
Bug 759317
Opened 12 years ago
Closed 12 years ago
IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: decoder, Unassigned)
References
Details
(Keywords: crash, testcase)
Attachments
(1 file, 1 obsolete file)
5.26 KB,
patch
|
jbramley
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Crash info: Program received signal SIGILL, Illegal instruction. 0x40172b40 in ?? () (gdb) x /i $pc => 0x40172b40: ; <UNDEFINED> instruction: 0xffff0004
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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
Updated•12 years ago
|
Summary: IonMonkey: Crash due to Illegal instruction 0xffff0004 (opt-only) → IonMonkey: (ARM) Crash due to Illegal instruction 0xffff0004 (opt-only)
Comment 4•12 years ago
|
||
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
Comment 5•12 years ago
|
||
Attachment #628059 -
Flags: review?
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Comment 9•12 years ago
|
||
(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 11•12 years ago
|
||
landed + fixed: http://hg.mozilla.org/projects/ionmonkey/rev/cdfe0f8e0dd4
You need to log in
before you can comment on or make changes to this bug.
Description
•