Repository navigation
Conversation
…s in the called function
A sloppy direct eval declares its vars in the first VarScope up the scope
chain (Interpreter::executeEval). While the parameter expressions of a
function run, the function has no VarScope there: the bytecode generator
makes the "var" environment after the parameters, as their child. So
`function f(p = eval("var x = 1")) {}` declared `x` in the code around the
function, in an enclosing function or on the global object, and stored the
value to whatever `x` meant there.
The specification gives the function an environment of its own for these
vars, outside the environment of the parameters
(FunctionDeclarationInstantiation, step 20).
- The parser records on FunctionParameters that it built a direct eval
call while it parsed the parameter list.
- For such a sloppy function, the scope that holds |this| and new.target
for arrow functions is a VarScope. It is already above the parameters.
- An arrow function has no such scope. It gets one empty VarScope before
its parameters.
- emitCallDirectEval() asserts that sloppy function code has a VarScope of
its own on the scope chain.
A function whose parameter list has no direct eval call gets the same
bytecode as before.
Collaborator
Author
|
Status
|
|
Preview build of 644dc4b: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft on purpose: for WebKit/WebKit first, not for a merge into the fork. No user report.
Problem
function f(p = eval("var q = 1")) {} f(); typeof qis"number"(V8:"undefined").const. An enclosingletgives a falseSyntaxError: Can't create duplicate variable in eval.Interpreter::executeEval(interpreter/Interpreter.cpp:1585) declares in the first VarScope up the chain. A function makes its own after its parameters (bytecompiler/BytecodeGenerator.cpp:1354).Fix
JSTests/stress/direct-eval-in-parameter-expression-*.jsfail onmain. 9 test262 expected failures pass.Background
vars.Downsides
jsctext: +1,108 B.main, wrong now: Known gaps rows 1 (delete argumentsin a top-level arrow function) and 7.Notes
Route. This PR does not ask for a merge into the fork as it stands.
main(b14e295) has the same code: the walk inInterpreter::executeEval(Interpreter.cpp:1504there) and the VarScope made after the parameters (BytecodeGenerator.cpp:1314). ItsJSTests/test262/expectations.yamlandexpectations-linux.yamllist the same 9 rows. I did not build upstream.Source/part of this patch applies to that commit withgit apply: every hunk, offsets only. Two things are fork-only, and a copy for upstream drops them: thedeclaredNamesScopesChanged()line inpushVarScopeForEvalInParameters, and therun("bytecode-optimizer", ...)mode in the first line of the four tests.main. Or it becomes a cherry-pick of the upstream commit, as [JSC] Array profiles should not turn ordinary arrays intoArrayStorage(cherry-pick WebKit/WebKit#75376) #785 was.expectations*.yamlhunks: no job of this repository runs test262, and they would be one more difference from upstream.WEBKIT_VERSIONbump and one test file,test/js/bun/jsc/direct-eval-parameter-expressions.test.ts, on the branchrobobun/17898b84/webkit-direct-eval-parameter-expressionsof oven-sh/bun. On Bun 1.4.3, 16 of its 17 tests fail. The other one is a control.--webkit-version=autobuild-preview-pr-877-644dc4b0, the preview build of this PR, 17 of 17 pass.test/bundler/bundler_bytecode_portable.test.tsgives the same hashes for the cache bytes of its whole corpus (21 MB for its libraries). One entry,libraries.js, differs from the committed snapshot on this machine, in the same way with both.repl-transform.test.ts,vm.test.ts,webkit-upgrade-33ce3bf11a.test.tsandbun-jsc.test.ts(492 tests) fail only by timeouts of the debug build: 2 with the preview build, 5 with the pinned one.How it was found. A run of the same sloppy programs in Bun and in Node. There is no user report, and a search of the oven-sh/bun issues for the symptom finds none. The reason to change it is ECMA-262 conformance and parity with Node.
What changes. Sloppy global code.
mainis the merge base 5e3554f. V8 is Node v26.3.0 with the program as a script (vm.runInThisContext).mainfunction f(p = eval("var q = 1")) {} f(); typeof qnumberundefinedundefinedvar g = (p = eval("var q = 1")) => 0; g(); typeof qnumberundefinedundefined({ m(p = eval("function q() {}")) {} }).m(); typeof qfunctionundefinedundefinedfunction f(d, p = d && eval("var q = 1")) { return typeof q } [f(true), f(false)]number,numbernumber,undefinednumber,undefined(function () { var x = "outer"; (function (p = eval("var x = 'inner'")) {})(); return x })()innerouterouterlet x = "outer"SyntaxErrorouterouterconst K = "const"; (function () { function f(p = eval("var K = 'new'")) {} f() })(); Knewconstconstvar x = 1; let y = 2; function f(p = eval("var x = 10, y = 20")) {} f(); [x, y]SyntaxError1,21,2(function (exports, require, module) { function f(p = eval("var module = 1")) {} f(); return typeof module })({}, null, {})numberobjectobject(function named(p = eval("var named = 1")) { return named })()SyntaxError11(function () { ((p = eval("var arguments = 'ab'")) => 0)(); return arguments.length })(1, 2, 3)233var w = { x: "with" }; with (w) { (function (p = eval("var x = 'inner'")) {})() } w.xinnerwithwith(function (p = eval("var arguments = 1")) {})()SyntaxErrorSyntaxErrorSyntaxErrorisCan't create duplicate variable in eval.argumentsnext to its parameters, so the specification makes it a SyntaxError. V8 does not follow it there.Why the var left the function. With a parameter list that is not simple, the generator pushes the scope of the parameters, runs the parameter expressions, and then makes the VarScope as a child of that scope (
initializeDefaultParameterValuesAndSetupFunctionScopeStack). During the parameter expressions the chain is: the scope of the parameters, the scope forthis(a lexical scope), the name scope of a function expression, then the code around the function.executeEvalandJSScope::resolveScopeForHoistingFuncDeclInEvalboth walk to the first VarScope. So they ended in an enclosing function or on the global object. The store of the value is a separate lookup by name (JSScope::abstractAccess). It reads the symbol table of each scope, and it expects a var from an eval only in a scope withusesSloppyEval. An enclosing function with no eval of its own has no such flag. So the lookup passes the new var and reaches a binding further out. That is how theconstrow gets its value.How it works now.
Parser::parseInnerreadsASTBuilder::evalCount()right after it parsed the parameter list of the function that it compiles. The builder counts every direct eval call that it builds, so the count is never zero when a parameter expression has one.SyntaxCheckerand do not count.thisandnew.targetbefore its parameters (initializeArrowFunctionContextScopeIfNeeded). With the newScopeType::FunctionEvalVarScope, the symbol table of that scope keeps the type VarScope and getsusesSloppyEval. The instructions of such a function do not change.thisfrom the code around it and has no such scope.pushVarScopeForEvalInParametersemits onecreate_lexical_environmentwith an empty symbol table of the same kind, and amov.usesSloppyEvalon the symbol table makesJSScope::abstractAccessask for var injection checks for names that resolve through the scope. A closure made in the parameters before the eval runs therefore sees the var (test262language/expressions/function/scope-param-elem-var-open.js).emitCallDirectEvalhas anASSERT: sloppy function code that calls eval has a VarScope of its own on the scope stack. It is not aRELEASE_ASSERT. If the flag ever misses an eval, a release build does whatmaindoes today, and does not abort on valid code.Interpreter::executeEval, inJSScope, in an opcode, in a JIT tier or in the bytecode cache format (cachedTypesFormatRevisionstays 13). The flag is read while the function is compiled and is not stored.Known gaps.
main, this PR and V8 as above.mainvar g = (p = eval("var arguments = 1"), d = delete arguments) => [d, typeof arguments]; g()true,undefinedfalse,numbertrue,undefined(function () { return ((p = eval("var arguments = 1"), d = delete arguments) => [d, typeof arguments])() })()false,numberfalse,numbertrue,object(function () { return (() => { eval("var arguments = 1"); return [delete arguments, typeof arguments] })() })()false,numberfalse,numbertrue,object(function (a = 1) { eval("var a = 's'"); return a })()11sfunction f(a = 1, r = () => a) { eval("var a"); a = 5; return [a, r()] } f()5,55,55,1(function () { var f = function (a = class { [eval("var q = 1; 'k'")]() {} }) { return typeof q }; return [f(), typeof q] })()number,numbernumber,undefinedundefined,undefined(function () { var x = "outer"; var f = function (a = class { [eval("var x; 'k'")]() {} }) { return String(x) }; return [f(), x] })()outer,outerundefined,outerouter,outerJSLexicalEnvironment::deleteProperty(runtime/JSLexicalEnvironment.cpp:136) refuses every property namedarguments. An eval var of that name in an arrow function is such a property. Row 1 was right onmainby accident and is wrong now: there the var was a property of the global object, which the guard does not cover.delete argumentsafter a body eval that declaresvar argumentschanges in generator and async function bodies, fromfalse,number(as in V8) totrue,object. The guard is the only thing that refuses it there until [JSC] In a generator or async function,argumentsand each parameter have one binding, for the body's declarations and for direct eval #782 gives those bodies oneargumentsbinding. The removal belongs with [JSC] In a generator or async function,argumentsand each parameter have one binding, for the body's declarations and for direct eval #782.BytecodeGenerator::variable). It is the same on both builds and no PR covers it.(function () { class A { [eval("var q = 1; 'k'")]() {} } return typeof q })()isnumberon both builds andundefinedin V8. With such a class in a parameter expression the var now stays in the function. In row 7 that moves a result away from V8: onmainthe var went to the enclosing function, which has anxalready.var argumentsin the function body is a binding of its own #715 (a bodyvar argumentswith parameter expressions) and [JSC] In a generator or async function,argumentsand each parameter have one binding, for the body's declarations and for direct eval #782 change the same files. This patch does not need either of them.Tests. Four files in
JSTests/stress.direct-eval-in-parameter-expression-var-scope.js: 18 kinds of function, 7 places for the eval in the parameter list for a function and for an arrow function, 7 things the eval declares, 3 things that are not a var of the function.direct-eval-in-parameter-expression-bindings.js: 42 checks of which binding a name is. Who sees the var, the body's own declaration, a var per call, the bindings of the code around the function (also a globalvar,letandconstwith a function between), the name of the function.direct-eval-in-parameter-expression-conflicts.js: 44 checks. The parameter names andargumentsthat stay a SyntaxError,this,new.target,superandargumentsinside the eval, an eval that throws, strict code, nested functions and classes, an eval in the body.direct-eval-in-parameter-expression-tiers.js: a loop oftestLoopCountcalls where 1 call in 16 runs the eval, and two new realms where a closure of the parameters is compiled before the first eval of the realm declares a var.--validateBytecodeOptimizerStaticScopes=1,--forceDebuggerBytecodeGeneration=true, and the type and control flow profilers. That is 20 modes for each file. The 80 runs pass on a release build and on a build with assertions (run-jsc-stress-tests). Onmainall 80 fail.es6-default-parameters.jstakes 27 to 298 ms. A single run can take longer on this machine: the longest of the 80 runs took 235 ms.var arguments.Other runs (Linux x64,
jscof the merge base 5e3554f againstjscof this PR). These are runs on one machine by hand: no job of this repository runs test262.test/language,test/annexBandtest/staging, 48,132 runs, release builds: 9 runs change, all from fail to pass (80 failures to 71). They are the 8language/eval-code/direct/arrow-fn-*-arrow-func-declare-arguments-assign*.jsfiles andstaging/sm/eval/redeclared-arguments-in-param-expression-eval.js. Their rows are removed fromexpectations.yamlandexpectations-linux.yaml.new.targetand class directories withJSC_forceDebuggerBytecodeGeneration=true. There the build of this PR has assertions.JSTests/stressthat are not new, with default options, release builds, 8 at a time: 8 files differ. They are out of memory kills and a timeout under the parallel load, and two tests that print a memory size or the order of threads. Each gives the same result on both builds when it runs alone.--dumpGeneratedBytecodes=true, JIT off) of the same files: 6,066 dumps are identical. The other 53 dumps are not stable: they differ between runs of one binary (worker threads, random constants, the order of a switch table). Every one of the 662 code blocks that both builds dumped there has the same number of instructions.--diskCachePath, JIT off) of the same files: 6,101 files write a cache, 52.4 MB in total. The caches of the two builds differ in 3 files, by 5 bytes. That is one byte for each function with a direct eval in its parameter list: 3 ines6-default-parameters.js, 1 inIIFE-es6-default-parameters.js, 1 inbytecode-optimizer-static-scopes-eval-with-global.js. Nine more files differed in the parallel run. The caches of threesampling-profiler-*.jsfiles differ between two runs of one binary as well. The other six are identical on both builds when they run alone.ASSERT:JSTests/stresswith default options has 4 aborts in 6,123 files.ensure-crash.jsandmodule-loader-promise-then-tampered.jsabort on the release builds ofmaintoo. The other two aremustCrash!tests that rely on a debugASSERT, and they abort on an assertion build of an oldermainas well. The 10,693 test262 runs of the directories above, without the debugger option, give the same results as the release build. The fivebytecode-optimizer-static-scopes-*.jstests,es6-default-parameters.jsandIIFE-es6-default-parameters.jspass in all of their 44 runs.es6.yaml,ChakraCore.yaml,LayoutTests/jsc-layout-tests.yaml,mozilla-tests.yaml,modules.yaml,typeProfiler.yaml,controlFlowProfiler.yaml,heapProfiler.yamlandcomplex.yamlwith--quick, release build of this PR: 8,234 runs pass.maindiffers in 693 of them, by the result in 22 and by a global that it leaves behind in 677. This PR differs in 4, by the result: three like the last row of the first table, and the class computed key of Known gaps.Costs, measured. Release builds (
-O2, no assertions) of the merge base and of this PR. Machine instructions are user-space instructions fromperf_event_open, with--useJIT=0, as the difference between runs of N and 2N iterations.mainsizetext ofjscsizeof(FunctionParameters)(p = eval("var q")) => 0({} = eval("var q; ({})")) => 0async (p = eval("var q")) => { await 1 }(p = (a = eval("var x = 1")) => 0) => 0(the flag is an upper bound)(p = class { static f = eval("1") }) => 0, and of the static block formfunction h(p = eval("var q")) {} h(), 848 BusesSloppyEval)(x, p = eval("1")) => p(x, {} = eval("({})")) => 1function (x, p = eval("1")) {}, and 5 shapes with no eval in the parameters(x, p = eval("1")) => p(x, {} = eval("({})")) => 1(x, p = eval("var v = 1")) => pfunction (x, p = eval("var v = 1")) { return p }function (x, p = 1) { eval("var v = 1"); return p }function (x, p = eval("1")) { return p }, median of 9 (range)function (p = 1) { eval("1"); return p }, median of 9 (range)function (a, b = 2) { return a }, median of 9function (p = 1, f = () => gv) { eval("1"); return f }function (p = eval("1"), f = () => gv) { return f }function fN(a, b = 2) { return a + b }function fN() { return eval('1') }maina parameter eval at the top level declared its var on the global object, which does not fire the realm's VarInjection watchpoint. Now the var is in a function, and the watchpoint fires as for a body eval. A checked read of a global costs 2,113 instructions after a body eval on both builds. After a parameter eval it costs 651 onmainand 2,112 with this PR.valgrind,perfandbloatyare not installed on this machine.Self-review. 15 concerns, 13 addressed.
argumentsguard ofJSLexicalEnvironment::deleteProperty. That changeddelete argumentsin generator and async function bodies. Removed from the patch (Known gaps).mainmoved under the branch (Upgrade to upstream WebKit 33ce3bf11a71 #860). Rebased on 5e3554f. Every number in this description is from that base.RELEASE_ASSERTon every sloppy eval site. Now anASSERT.drainMicrotasks().constbehind a function. Added rows for both, and Known gaps rows 6 and 7.var argumentsin the function body is a binding of its own #715 and [JSC] In a generator or async function,argumentsand each parameter have one binding, for the body's declarations and for direct eval #782 were not stated. Added to Known gaps.main, because none reads the outer binding afterwards. That is a change for tc39/test262, not for this PR.