823482 - BaselineCompiler: Compile try..catch

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: WIP

Works on x86 and x64, but needs some refactoring and cleanup.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1: Refactor JSOP_EXCEPTION logic

Patch for inbound, adds a function to get and clear the pending exception stored in the context.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2: Compile try..catch

Adds a bytecode offset -> native code offset mapping and compiles ops we need for try..catch. The handling of ENTERBLOCK/LEAVEBLOCK is not complete, we also have to clone the block if needed and push it on the frame's block chain. Not doing that doesn't cause any test failures so far so I will file a follow-up bug for it; this patch is already getting pretty large.

Comment 4

[Kannan Vijayan [:djvj]]

Comment on attachment 695461 [details] [diff] [review]
Part 2: Compile try..catch

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

Assuming this is a work in progress so canceling review after commenting.  Let me know if I'm wrong about that.

::: js/src/ion/BaselineCompiler.cpp
@@ +1320,5 @@
> +        frame.push(UndefinedValue());
> +
> +    // Pushed values will be accessed using GETLOCAL and SETLOCAL, so ensure
> +    // they are synced.
> +    frame.syncStack(0);

Nit: you should be able to omit the syncStack() at the beginning of the method.

::: js/src/ion/IonFrames.cpp
@@ +339,5 @@
> +    for (; tn != tnEnd; ++tn) {
> +        if (pcOffset < tn->start)
> +            continue;
> +        if (pcOffset >= tn->start + tn->length)
> +            continue;

How do try notes keep track of nested try/catches?  Do we need to worry about that?

Because if we get something like:
  try {
     blah;
     try {
       blah2;
     } catch(err2) {
        ...
     }
  } catch(err) {
    ...
  }

Won't an exception with a PC pointing to blah2 end up hitting the outer try/catch region given the scanning logic?

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 695461 [details] [diff] [review]
Part 2: Compile try..catch

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

sorry, did r- accidentally.

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 695461 [details] [diff] [review]
Part 2: Compile try..catch

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

Actually, I just re-read the patch comment and realized that while the patch was partial, it was meant for a proper review for inclusion.

Lemme know about the scanning of the TryNotes and whether or not it needs fixing.  That change should be relatively straightforward if needed, so R+-ing anyway.

Comment 7

[Jan de Mooij [:jandem]]

Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab05052b3b5

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/mozilla-central/rev/1ab05052b3b5

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/projects/ionmonkey/rev/e5b05b7abbcc

(In reply to Kannan Vijayan [:djvj] from comment #4)
> 
> Won't an exception with a PC pointing to blah2 end up hitting the outer
> try/catch region given the scanning logic?

The try notes are stored inner-to-outer, so we will see the inner catch block before the outer one.