Closed
Bug 719562
Opened 12 years ago
Closed 12 years ago
IonMonkey: Compile JSOP_THROW
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
309 bytes,
application/javascript
|
Details | |
20.29 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
Once we have JSOP_NOT, the last op we need for the base64ToString function in SS string-base64 is JSOP_THROW: -- if (leftbits) throw Components.Exception('Corrupted base64 string'); -- It's never executed though. The simplest "fix" is to always bailout for now. I don't know how our exceptions work; can we compile this to a VM call, or would it require a lot of work?
Yeah, let's compile this as a vm call. It should return false to indicate error; the wrapper will take care of the rest.
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #1) > Yeah, let's compile this as a vm call. It should return false to indicate > error; the wrapper will take care of the rest. Great, going to try this tomorrow.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•12 years ago
|
||
Turned out to be a lot like JSOP_RETURN. Also moves the successor list to MAryControlInstruction so that not every instruction has to implement these methods.
Attachment #590207 -
Flags: review?(sstangl)
Assignee | ||
Comment 4•12 years ago
|
||
With test.
Attachment #590207 -
Attachment is obsolete: true
Attachment #590207 -
Flags: review?(sstangl)
Attachment #590208 -
Flags: review?(sstangl)
Comment 5•12 years ago
|
||
Inlining a function that throws trips an error; the attached test case asserts:
> [sstangl@winhill dbg32]$ IONFLAGS=aborts,inline ./js --ion-eager -n > ~/js/throw.js
> [Abort] unspecialized compare not yet supported
> [Abort] IM Compilation failed.
> [Inlining] Cannot inline due to non-object
> [Inlining] Decided not to inline
> [Inlining] Building out-of-line call
> [Inlining] Inlining good to go!
> [Inlining] Recursively building
> [Inlining] Initializing 0 arg slots
> [Inlining] Initializing 0 local slots
> [Inlining] Inline entry block MResumePoint 0x8a91ac0, 2 operands
> Assertion failure: exitBlock->lastIns()->op() == MDefinition::Op_Return, at /home/sstangl/dev/ionmonkey/js/src/ion/IonBuilder.cpp:2250
due to inlining not expecting a throw() as the final op. I would also expect there to be errors with inlining a call to h() at various positions in the for(A;B;C) loop structure, which is worth writing test cases for. The attached test uses the C position for no particular reason.
Comment 6•12 years ago
|
||
Comment on attachment 590208 [details] [diff] [review] Compile JSOP_THROW Review of attachment 590208 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.cpp @@ +1960,5 @@ > + return ControlStatus_Error; > + > + // Make sure no one tries to use this block now. > + current = NULL; > + return processControlEnd(); This needs to handle inlining, and test cases should be written to make sure that |throw| can be part of various for(A;B;C) positions. Although it is normally a syntax error to throw from there, it is legal to call functions, and we can inline functions that resolve to just |throw|. See previously attached test case. ::: js/src/ion/Lowering.cpp @@ +993,5 @@ > + > + LThrow *lir = new LThrow; > + if (!useBox(lir, LThrow::Value, value)) > + return false; > + return add(lir, ins) && assignSafepoint(lir, ins); Should LThrow be defineBox()? If we're going to handle try/catch as part of IonMonkey, we're going to need the value somehow. Not sure of the plan here. ::: js/src/ion/MIR.h @@ +791,5 @@ > return this; > } > }; > > +template <size_t Arity, size_t Successors> Good change.
Attachment #590208 -
Flags: review?(sstangl)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Sean Stangl from comment #6) > > This needs to handle inlining, and test cases should be written to make sure > that |throw| can be part of various for(A;B;C) positions. Although it is > normally a syntax error to throw from there, it is legal to call functions, > and we can inline functions that resolve to just |throw|. See previously > attached test case. Good catch (no pun intended)! There don't appear to be many exception-related tests so I'll write some today. > > Should LThrow be defineBox()? If we're going to handle try/catch as part of > IonMonkey, we're going to need the value somehow. Not sure of the plan here. JSOP_EXCEPTION is emitted at the start of the catch block and could be a stub call returning cx->getPendingException(). Not sure about the rest, compiling catch blocks may be tricky.
Let's not worry about catch blocks for the initial release. If we really have to though, we can just compile everything around the catch (but not anything inside it), and make ion::HandleException trigger invalidation or something. That's better than nothing.
Assignee | ||
Comment 9•12 years ago
|
||
The problem with inlining JSOP_THROW is that if all branches in the callee terminate with a throw-statement, there are no returns and we'd have to stop processing the current block in the caller. I don't think handling that's worth it right now, so this patch mimics JM and uses the "inlineable" flag from jsanalyze, so that we don't inline functions containing JSOP_THROW (and other complicated/uncommon ops). The patch also adds the tests you requested + some other tests like throwing inside a constructor. We don't compile JSOP_NEW yet, but when we do it should work.
Attachment #590208 -
Attachment is obsolete: true
Attachment #591078 -
Flags: review?(sstangl)
Comment 10•12 years ago
|
||
Comment on attachment 591078 [details] [diff] [review] Patch v2 Review of attachment 591078 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks for writing the test cases.
Attachment #591078 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 11•12 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/aec2f378af30
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•