Closed Bug 823482 Opened 8 years ago Closed 8 years ago

BaselineCompiler: Compile try..catch

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached patch WIP (obsolete) — Splinter Review
Works on x86 and x64, but needs some refactoring and cleanup.
Patch for inbound, adds a function to get and clear the pending exception stored in the context.
Attachment #694816 - Attachment is obsolete: true
Attachment #695460 - Flags: review?(kvijayan)
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.
Attachment #695461 - Flags: review?(kvijayan)
Attachment #695460 - Flags: review?(kvijayan) → review+
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?
Attachment #695461 - Flags: review?(kvijayan) → review-
Comment on attachment 695461 [details] [diff] [review]
Part 2: Compile try..catch

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

sorry, did r- accidentally.
Attachment #695461 - Flags: review-
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.
Attachment #695461 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.