Closed Bug 719562 Opened 12 years ago Closed 12 years ago

IonMonkey: Compile JSOP_THROW

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
(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
Attached patch Patch (obsolete) — Splinter Review
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)
Attached patch Compile JSOP_THROW (obsolete) — Splinter Review
With test.
Attachment #590207 - Attachment is obsolete: true
Attachment #590207 - Flags: review?(sstangl)
Attachment #590208 - Flags: review?(sstangl)
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 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)
(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.
Attached patch Patch v2Splinter Review
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 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+
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.