BaselineCompiler: Compile try..catch

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

7 years ago
No description provided.
Assignee

Comment 1

7 years ago
Posted patch WIP (obsolete) — Splinter Review
Works on x86 and x64, but needs some refactoring and cleanup.
Assignee

Comment 2

7 years ago
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)
Assignee

Comment 3

7 years ago
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+
Attachment #695460 - Flags: checkin+
Assignee

Comment 9

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Assignee

Updated

7 years ago
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.