Last Comment Bug 719562 - IonMonkey: Compile JSOP_THROW
: IonMonkey: Compile JSOP_THROW
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: 684381
  Show dependency treegraph
 
Reported: 2012-01-19 13:02 PST by Jan de Mooij [:jandem]
Modified: 2012-01-25 01:16 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (15.44 KB, patch)
2012-01-20 08:18 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Compile JSOP_THROW (15.44 KB, patch)
2012-01-20 08:20 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Test case broken with above patch with --ion-eager -n. (309 bytes, application/javascript)
2012-01-23 15:24 PST, Sean Stangl [:sstangl]
no flags Details
Patch v2 (20.29 KB, patch)
2012-01-24 06:57 PST, Jan de Mooij [:jandem]
sstangl: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-01-19 13:02:26 PST
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?
Comment 1 David Anderson [:dvander] 2012-01-19 13:05:56 PST
Yeah, let's compile this as a vm call. It should return false to indicate error; the wrapper will take care of the rest.
Comment 2 Jan de Mooij [:jandem] 2012-01-19 13:20:35 PST
(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.
Comment 3 Jan de Mooij [:jandem] 2012-01-20 08:18:10 PST
Created attachment 590207 [details] [diff] [review]
Patch

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.
Comment 4 Jan de Mooij [:jandem] 2012-01-20 08:20:01 PST
Created attachment 590208 [details] [diff] [review]
Compile JSOP_THROW

With test.
Comment 5 Sean Stangl [:sstangl] 2012-01-23 15:24:47 PST
Created attachment 590894 [details]
Test case broken with above patch with --ion-eager -n.

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 Sean Stangl [:sstangl] 2012-01-23 15:31:07 PST
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.
Comment 7 Jan de Mooij [:jandem] 2012-01-24 01:04:06 PST
(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.
Comment 8 David Anderson [:dvander] 2012-01-24 02:54:01 PST
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.
Comment 9 Jan de Mooij [:jandem] 2012-01-24 06:57:25 PST
Created attachment 591078 [details] [diff] [review]
Patch v2

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.
Comment 10 Sean Stangl [:sstangl] 2012-01-24 12:22:05 PST
Comment on attachment 591078 [details] [diff] [review]
Patch v2

Review of attachment 591078 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for writing the test cases.
Comment 11 Jan de Mooij [:jandem] 2012-01-25 01:16:59 PST
http://hg.mozilla.org/projects/ionmonkey/rev/aec2f378af30

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