...
<rdar://83494949>
Created attachment 453471 [details] patch
Created attachment 453472 [details] patch
Created attachment 453474 [details] patch
Created attachment 453475 [details] patch
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.
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
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.
Created attachment 453509 [details] patch rebased
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?
Created attachment 453557 [details] patch for landing
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].
Re-opened since this is blocked by bug 237409
Created attachment 453759 [details] patch
Comment on attachment 453759 [details] patch r=me
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].