Bug 237306 – Add a DeferTraps scope

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

<rdar://83494949>

[Saam Barati]

Created attachment 453471 [details]
patch

[Saam Barati]

Created attachment 453472 [details]
patch

[Saam Barati]

Created attachment 453474 [details]
patch

[Saam Barati]

Created attachment 453475 [details]
patch

[Mark Lam]

Comment on attachment 453475 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453475&action=review

> Source/JavaScriptCore/interpreter/InterpreterInlines.h:99
> +        {

nit: You can remove this inner { } since this is already in the DeferTraps scope and is at the tail end.

> Source/JavaScriptCore/interpreter/InterpreterInlines.h:104
> +        // Execute the code:
> +        throwScope.release();

I think it's more logical to group these 2 lines with the execute call below.  There's no reason this needs to be here.

[Saam Barati]

Comment on attachment 453475 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453475&action=review

>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:99
>> +        {
> 
> nit: You can remove this inner { } since this is already in the DeferTraps scope and is at the tail end.

I think it’s nice to keep this isolated as is. It shows exactly the region of code we’re asserting against

>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:104
>> +        throwScope.release();
> 
> I think it's more logical to group these 2 lines with the execute call below.  There's no reason this needs to be here.

If we make release() ever service traps we wouldn’t want to put it below. We want maximal things in this scope as long as we don’t recurse. Maybe we won’t ever make release() do that, but I see no downside to keeping it in here. An alternative I considered is to add a release() style function call to DeferTraps, and to use it in these types of situations so we have fewer scopes, but it seems a bit overkill

[Mark Lam]

Comment on attachment 453475 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453475&action=review

>>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:99
>>> +        {
>> 
>> nit: You can remove this inner { } since this is already in the DeferTraps scope and is at the tail end.
> 
> I think it’s nice to keep this isolated as is. It shows exactly the region of code we’re asserting against

ok, SGTM.

>>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:104
>>> +        throwScope.release();
>> 
>> I think it's more logical to group these 2 lines with the execute call below.  There's no reason this needs to be here.
> 
> If we make release() ever service traps we wouldn’t want to put it below. We want maximal things in this scope as long as we don’t recurse. Maybe we won’t ever make release() do that, but I see no downside to keeping it in here. An alternative I considered is to add a release() style function call to DeferTraps, and to use it in these types of situations so we have fewer scopes, but it seems a bit overkill

ok.

[Saam Barati]

Created attachment 453509 [details]
patch

rebased

[Mark Lam]

Comment on attachment 453509 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453509&action=review

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:955
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1020
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1311
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1407
>      // Execute the code:
>      throwScope.release();

You didn't put this ThrowScope::release() in the DeferScope unlike in `Interpreter::execute(CallFrameClosure& closure)` below.  The same is true for all Interpreter execute functions above.  Realistically, I don't think we will or should ever handle traps in ThrowScope::release() because on release builds, we rely on it being a no-op.  I still think that it is the better idiom to keep the ThrowScope::release() close to the statement that the release applies to i.e. the execute.  Also, the code reads more naturally this way.  Regardless, let's make it consistent every where.

> Source/JavaScriptCore/interpreter/InterpreterInlines.h:91
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?

[Saam Barati]

Created attachment 453557 [details]
patch for landing

[EWS]

Committed r290717 (247964@main): <https://commits.webkit.org/247964@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453557 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 237409

[Saam Barati]

Created attachment 453759 [details]
patch

[Mark Lam]

Comment on attachment 453759 [details]
patch

r=me

[EWS]

Committed r290788 (248030@main): <https://commits.webkit.org/248030@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453759 [details].