Skip to content

Commit

Permalink
[JSC] eval() call with ...spread syntax should be direct
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=268028
<rdar://problem/121547890>

Reviewed by Justin Michaud.

Before this change, EvalFunctionCallNode was emitting op_call_varargs for calls with ...spread syntax,
rather than op_call_direct_eval, thus always performing indirect eval. Per spec [1], that was wrong:
CoverCallExpressionAndAsyncArrowHead production matches ...spread syntax as well.

Since global eval() function takes only one parameter, and we would like to avoid introducing yet
another call bytecode just for this very rare case, this change emits op_spread to perform full iteration
and passes first argument into op_call_direct_eval, conditional on callee function to be built-in eval()
from the lexical realm.

To perform this check, we need a LinkTimeConstant and the globalObject->evalFunction() to share the same
JSFunction, which is tricky since m_linkTimeConstants stores stateful LazyProperty object directly, without
a pointer, and their initializers should be stateless. That is why initializeEvalFunction() is introduced.

[1]: https://tc39.es/ecma262/#sec-function-calls-runtime-semantics-evaluation

* JSTests/stress/direct-eval-spread.js: Added.
* JSTests/test262/expectations.yaml: Mark 6 tests as passing.
* Source/JavaScriptCore/builtins/BuiltinNames.h:
* Source/JavaScriptCore/bytecode/LinkTimeConstant.h:
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::emitJumpIfNotEvalFunction):
(JSC::BytecodeGenerator::emitCall):
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:
* Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:
(JSC::EvalFunctionCallNode::emitBytecode):
* Source/JavaScriptCore/runtime/JSGlobalObject.cpp:
(JSC::initializeEvalFunction):
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::visitChildrenImpl):
* Source/JavaScriptCore/runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::evalFunction const): Deleted.
* Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:
(JSC::JSGlobalObject::evalFunction const):

Canonical link: https://commits.webkit.org/273788@main
  • Loading branch information
Alexey Shvayka committed Jan 30, 2024
1 parent 106249c commit bb825e0
Show file tree
Hide file tree
Showing 10 changed files with 75 additions and 19 deletions.
30 changes: 30 additions & 0 deletions JSTests/stress/direct-eval-spread.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error(`Bad value: ${actual}!`);
}

var xx = 0;
var xxx = 0;

function* genFn() {
yield "x++";
yield "throw new Error()";
yield xx++;
}

var x = 0;

(function() {
var x = 0;

for (var i = 0; i < 1e5; i++) {
eval(...genFn());
eval(...genFn(), (() => xxx++)());
}

shouldBe(x, 1e5 * 2);
shouldBe(xx, 1e5 * 2);
shouldBe(xxx, 1e5);
})();

shouldBe(x, 0);
9 changes: 0 additions & 9 deletions JSTests/test262/expectations.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -941,15 +941,6 @@ test/language/expressions/assignmenttargettype/direct-callexpression-arguments.j
default: 'Test262: This statement should not be evaluated.'
test/language/expressions/assignmenttargettype/parenthesized-callexpression-arguments.js:
default: 'Test262: This statement should not be evaluated.'
test/language/expressions/call/eval-spread-empty-leading.js:
default: 'Test262Error: Expected SameValue(«local», «0») to be true'
strict mode: 'Test262Error: Expected SameValue(«local», «0») to be true'
test/language/expressions/call/eval-spread-empty-trailing.js:
default: 'Test262Error: Expected SameValue(«local», «0») to be true'
strict mode: 'Test262Error: Expected SameValue(«local», «0») to be true'
test/language/expressions/call/eval-spread.js:
default: 'Test262Error: Expected SameValue(«local», «1») to be true'
strict mode: 'Test262Error: Expected SameValue(«local», «1») to be true'
test/language/expressions/call/tco-non-eval-function-dynamic.js:
default: 'RangeError: Maximum call stack size exceeded.'
test/language/expressions/call/tco-non-eval-function.js:
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/builtins/BuiltinNames.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ namespace JSC {
macro(toLength) \
macro(importMapStatus) \
macro(importInRealm) \
macro(evalFunction) \
macro(evalInRealm) \
macro(moveFunctionToRealm) \
macro(newTargetLocal) \
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/bytecode/LinkTimeConstant.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class JSGlobalObject;
v(Map, nullptr) \
v(importMapStatus, nullptr) \
v(importInRealm, nullptr) \
v(evalFunction, nullptr) \
v(evalInRealm, nullptr) \
v(moveFunctionToRealm, nullptr) \
v(isConstructor, nullptr) \
Expand Down
6 changes: 6 additions & 0 deletions Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1538,6 +1538,11 @@ void BytecodeGenerator::emitJumpIfNotFunctionApply(RegisterID* cond, Label& targ
OpJneqPtr::emit(this, cond, moveLinkTimeConstant(nullptr, LinkTimeConstant::applyFunction), target.bind(this));
}

void BytecodeGenerator::emitJumpIfNotEvalFunction(RegisterID* cond, Label& target)
{
OpJneqPtr::emit(this, cond, moveLinkTimeConstant(nullptr, LinkTimeConstant::evalFunction), target.bind(this));
}

void BytecodeGenerator::emitJumpIfEmptyPropertyNameEnumerator(RegisterID* cond, Label& target)
{
OpJeqPtr::emit(this, cond, moveLinkTimeConstant(nullptr, LinkTimeConstant::emptyPropertyNameEnumerator), target.bind(this));
Expand Down Expand Up @@ -3593,6 +3598,7 @@ RegisterID* BytecodeGenerator::emitCall(RegisterID* dst, RegisterID* func, Expec
ArgumentListNode* n = callArguments.argumentsNode()->m_listNode;
if (n && n->m_expr->isSpreadExpression()) {
RELEASE_ASSERT(!n->m_next);
RELEASE_ASSERT(opcodeID != op_call_direct_eval);
auto expression = static_cast<SpreadExpressionNode*>(n->m_expr)->expression();
if (expression->isArrayLiteral()) {
auto* elements = static_cast<ArrayNode*>(expression)->elements();
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -873,6 +873,7 @@ namespace JSC {
void emitJumpIfFalse(RegisterID* cond, Label& target);
void emitJumpIfNotFunctionCall(RegisterID* cond, Label& target);
void emitJumpIfNotFunctionApply(RegisterID* cond, Label& target);
void emitJumpIfNotEvalFunction(RegisterID* cond, Label& target);
void emitJumpIfEmptyPropertyNameEnumerator(RegisterID* cond, Label& target);
void emitJumpIfSentinelString(RegisterID* cond, Label& target);
unsigned emitWideJumpIfNotFunctionHasOwnProperty(RegisterID* cond, Label& target);
Expand Down
26 changes: 25 additions & 1 deletion Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1229,7 +1229,31 @@ RegisterID* EvalFunctionCallNode::emitBytecode(BytecodeGenerator& generator, Reg
}

RefPtr<RegisterID> returnValue = generator.finalDestination(dst, func.get());
return generator.emitCallDirectEval(returnValue.get(), func.get(), callArguments, divot(), divotStart(), divotEnd(), DebuggableCall::No);

if (m_args->m_listNode && m_args->m_listNode->m_expr && m_args->m_listNode->m_expr->isSpreadExpression()) {
Ref<Label> notEvalFunction = generator.newLabel();
Ref<Label> done = generator.newLabel();
generator.emitJumpIfNotEvalFunction(func.get(), notEvalFunction.get());

{
SpreadExpressionNode* spread = static_cast<SpreadExpressionNode*>(m_args->m_listNode->m_expr);
RefPtr<RegisterID> spreadRegister = generator.emitNode(spread->expression());
generator.emitExpressionInfo(spread->divot(), spread->divotStart(), spread->divotEnd());

CallArguments directEvalArguments(generator, nullptr, 1);
generator.move(directEvalArguments.thisRegister(), callArguments.thisRegister());
generator.emitGetByVal(directEvalArguments.argumentRegister(0), spreadRegister.get(), generator.emitLoad(nullptr, jsNumber(0)));
generator.emitCallDirectEval(returnValue.get(), func.get(), directEvalArguments, divot(), divotStart(), divotEnd(), DebuggableCall::No);
generator.emitJump(done.get());
}

generator.emitLabel(notEvalFunction.get());
generator.emitCallInTailPosition(returnValue.get(), func.get(), NoExpectedFunction, callArguments, divot(), divotStart(), divotEnd(), DebuggableCall::Yes);
generator.emitLabel(done.get());
} else
generator.emitCallDirectEval(returnValue.get(), func.get(), callArguments, divot(), divotStart(), divotEnd(), DebuggableCall::No);

return returnValue.get();
}

// ------------------------------ FunctionCallValueNode ----------------------------------
Expand Down
16 changes: 9 additions & 7 deletions Source/JavaScriptCore/runtime/JSGlobalObject.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,11 @@ static JSC_DECLARE_HOST_FUNCTION(tracePointStop);
static JSC_DECLARE_HOST_FUNCTION(signpostStart);
static JSC_DECLARE_HOST_FUNCTION(signpostStop);

static JSValue initializeEvalFunction(VM&, JSObject* object)
{
return jsCast<JSGlobalObject*>(object)->evalFunction();
}

static JSValue createProxyProperty(VM& vm, JSObject* object)
{
JSGlobalObject* global = jsCast<JSGlobalObject*>(object);
Expand Down Expand Up @@ -621,7 +626,7 @@ const GlobalObjectMethodTable* JSGlobalObject::baseGlobalObjectMethodTable()
decodeURIComponent globalFuncDecodeURIComponent DontEnum|Function 1
encodeURI globalFuncEncodeURI DontEnum|Function 1
encodeURIComponent globalFuncEncodeURIComponent DontEnum|Function 1
eval JSGlobalObject::m_evalFunction DontEnum|CellProperty
eval initializeEvalFunction DontEnum|PropertyCallback
globalThis JSGlobalObject::m_globalThis DontEnum|CellProperty
parseInt JSGlobalObject::m_parseIntFunction DontEnum|CellProperty
parseFloat JSGlobalObject::m_parseFloatFunction DontEnum|CellProperty
Expand Down Expand Up @@ -1241,11 +1246,6 @@ capitalName ## Constructor* lowerName ## Constructor = featureFlag ? capitalName
[] (const Initializer<Structure>& init) {
init.set(createAccessorPropertyDescriptorObjectStructure(init.vm, *init.owner));
});

m_evalFunction.initLater(
[] (const Initializer<JSFunction>& init) {
init.set(JSFunction::create(init.vm, init.owner, 1, init.vm.propertyNames->eval.string(), globalFuncEval, ImplementationVisibility::Public));
});

m_collatorStructure.initLater(
[] (const Initializer<Structure>& init) {
Expand Down Expand Up @@ -1591,6 +1591,9 @@ capitalName ## Constructor* lowerName ## Constructor = featureFlag ? capitalName
m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::importInRealm)].initLater([] (const Initializer<JSCell>& init) {
init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, "importInRealm"_s, importInRealm, ImplementationVisibility::Private));
});
m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::evalFunction)].initLater([] (const Initializer<JSCell>& init) {
init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 1, init.vm.propertyNames->eval.string(), globalFuncEval, ImplementationVisibility::Public));
});
m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::evalInRealm)].initLater([] (const Initializer<JSCell>& init) {
init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, "evalInRealm"_s, evalInRealm, ImplementationVisibility::Private));
});
Expand Down Expand Up @@ -2463,7 +2466,6 @@ void JSGlobalObject::visitChildrenImpl(JSCell* cell, Visitor& visitor)
thisObject->m_objectProtoToStringFunction.visit(visitor);
thisObject->m_arrayProtoToStringFunction.visit(visitor);
thisObject->m_arrayProtoValuesFunction.visit(visitor);
thisObject->m_evalFunction.visit(visitor);
thisObject->m_promiseResolveFunction.visit(visitor);
visitor.append(thisObject->m_objectProtoValueOfFunction);
thisObject->m_numberProtoToStringFunction.visit(visitor);
Expand Down
3 changes: 1 addition & 2 deletions Source/JavaScriptCore/runtime/JSGlobalObject.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,6 @@ class JSGlobalObject : public JSSegmentedVariableObject {
LazyProperty<JSGlobalObject, JSFunction> m_objectProtoToStringFunction;
LazyProperty<JSGlobalObject, JSFunction> m_arrayProtoToStringFunction;
LazyProperty<JSGlobalObject, JSFunction> m_arrayProtoValuesFunction;
LazyProperty<JSGlobalObject, JSFunction> m_evalFunction;
LazyProperty<JSGlobalObject, JSFunction> m_promiseResolveFunction;
LazyProperty<JSGlobalObject, JSFunction> m_numberProtoToStringFunction;
WriteBarrier<JSFunction> m_objectProtoValueOfFunction;
Expand Down Expand Up @@ -664,7 +663,7 @@ class JSGlobalObject : public JSSegmentedVariableObject {
JSFunction* parseIntFunction() const { return m_parseIntFunction.get(this); }
JSFunction* parseFloatFunction() const { return m_parseFloatFunction.get(this); }

JSFunction* evalFunction() const { return m_evalFunction.get(this); }
JSFunction* evalFunction() const;
JSFunction* throwTypeErrorFunction() const;
JSFunction* objectProtoToStringFunction() const { return m_objectProtoToStringFunction.get(this); }
JSFunction* objectProtoToStringFunctionConcurrently() const { return m_objectProtoToStringFunction.getConcurrently(); }
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ ALWAYS_INLINE Structure* JSGlobalObject::arrayStructureForIndexingTypeDuringAllo
RELEASE_AND_RETURN(scope, InternalFunction::createSubclassStructure(globalObject, asObject(newTarget), functionGlobalObject->arrayStructureForIndexingTypeDuringAllocation(indexingType)));
}

inline JSFunction* JSGlobalObject::evalFunction() const { return jsCast<JSFunction*>(linkTimeConstant(LinkTimeConstant::evalFunction)); }
inline JSFunction* JSGlobalObject::throwTypeErrorFunction() const { return jsCast<JSFunction*>(linkTimeConstant(LinkTimeConstant::throwTypeErrorFunction)); }
inline JSFunction* JSGlobalObject::iteratorProtocolFunction() const { return jsCast<JSFunction*>(linkTimeConstant(LinkTimeConstant::performIteration)); }
inline JSFunction* JSGlobalObject::newPromiseCapabilityFunction() const { return jsCast<JSFunction*>(linkTimeConstant(LinkTimeConstant::newPromiseCapability)); }
Expand Down

0 comments on commit bb825e0

Please sign in to comment.