Skip to content

Commit

Permalink
CallVarargs should identify that it can read inline call frame argume…
Browse files Browse the repository at this point in the history
…nts.

rdar://112936988

Reviewed by Yusuke Suzuki.

Call already does this, but CallVarargs has a special case that forgot.

We should not be allowed to push PutStacks below a call of any kind, since
it might access our call frame's arguments via foo.arguments, unless
we are strict.

The only exception is TailCall (but not TailCallForwardVarargsInlinedCaller),
because it will destroy the entire frame.

We do not un-pessimize TailCall yet to reduce risk, but it could be made
to match TailCallForwardVarargs in the future.

* JSTests/stress/putstacksinking-callvarargs.js: Added.
(main.opt.x):
(main.opt.y):
(main.opt.z):
(main.opt):
(main):
* JSTests/stress/putstacksinking-tailcallvarargs.js: Added.
(main.opt.x):
(main.opt.y):
(main.opt.z):
(main.opt):
(main):
* Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:
(JSC::DFG::PreciseLocalClobberizeAdaptor::readTop):

Originally-landed-as: 259548.856@safari-7615-branch (c3d2e36). rdar://111361499
Canonical link: https://commits.webkit.org/266393@main
  • Loading branch information
Justin Michaud authored and robert-jenner committed Jul 28, 2023
1 parent 9b88dbf commit e5652c9
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 2 deletions.
33 changes: 33 additions & 0 deletions JSTests/stress/putstacksinking-callvarargs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//@ runDefault("--useConcurrentJIT=0")
var iter;
function main() {
function opt(i) {
function x() {
return y(...[1]);
}

function y() {
return z.apply(this, arguments);
}

function z() {
if (iter > 10136) {
if (x.arguments[2] != 1.3)
throw "Wrong value"
return x.arguments;
}
}
noInline(z);

x(1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 1.21, 1.22);
}

noInline(opt);
for (let i = 0; i < 20000; ++i) {
iter = i;
opt(i);
}
}
noDFG(main);

main();
34 changes: 34 additions & 0 deletions JSTests/stress/putstacksinking-tailcallvarargs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//@ runDefault("--useConcurrentJIT=0")
var iter;
function main() {
function opt(i) {
function x() {
return y(...[1]);
}

function y() {
"use strict"
return z.apply(this, arguments);
}

function z() {
if (iter > 10136) {
if (x.arguments[2] != 1.3)
throw "Wrong value"
return x.arguments;
}
}
noInline(z);

x(1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 1.21, 1.22);
}

noInline(opt);
for (let i = 0; i < 20000; ++i) {
iter = i;
opt(i);
}
}
noDFG(main);

main();
13 changes: 11 additions & 2 deletions Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h
Original file line number Diff line number Diff line change
Expand Up @@ -190,13 +190,19 @@ class PreciseLocalClobberizeAdaptor {
case CreateRest: {
bool isForwardingNode = false;
bool isPhantomNode = false;
bool mayReadArguments = false;
switch (m_node->op()) {
case ForwardVarargs:
// This is used iff allInlineFramesAreTailCalls, so we will
// actually do a real tail call and destroy our frame.
case TailCallForwardVarargs:
isForwardingNode = true;
break;
case CallForwardVarargs:
case ConstructForwardVarargs:
case TailCallForwardVarargs:
case TailCallForwardVarargsInlinedCaller:
isForwardingNode = true;
mayReadArguments = true;
break;
case PhantomDirectArguments:
case PhantomClonedArguments:
Expand All @@ -208,7 +214,10 @@ class PreciseLocalClobberizeAdaptor {

if (isPhantomNode && m_graph.m_plan.isFTL())
break;


if (mayReadArguments)
readWorld(m_node);

if (isForwardingNode && m_node->hasArgumentsChild() && m_node->argumentsChild()
&& (m_node->argumentsChild()->op() == PhantomNewArrayWithSpread || m_node->argumentsChild()->op() == PhantomSpread)) {
if (m_node->argumentsChild()->op() == PhantomNewArrayWithSpread)
Expand Down

0 comments on commit e5652c9

Please sign in to comment.