1697223 - Support static binding across strict direct eval scopes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Jason Orendorff [:jorendorff]]

The main enhancement here:

"use strict";
{
  let x = 1;
  eval("x");
}

Currently we emit GetName to access the variable x. We should emit GetAliasedVar instead.

Secondarily:

• in "use strict"; eval("var x = 0; ++x") we might as well store x in a stack slot and access it using GetLocal and SetLocal; and
• in "use strict"; eval("2 + 2"); we don't need a VarEnvironment, so we should not be emitting a PushVarEnv instruction.

This is an optimization we've put off ever since strict eval was standardized—11+ years! But I'm not doing it for performance. I want to use static scopes to emit optimized bytecode for private methods in bug 1662559, and we can only do that if the information is accessible through all strict-mode scopes.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1697223 - Part 1: Tweak a comment on EvalKernel. r=mgaudet

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1697223 - Part 2: Statically bind more identifiers in strict direct eval. r=mgaudet

Toplevel var and function bindings in strict direct eval are now statically
bound. Direct eval code is also permitted to search enclosing scopes, the same
way lazy functions do. References to variables in enclosing scopes will now
typically use aliased ops.

Depends on D107691

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1697223 - Part 3: Allow `var` and `function` bindings in strict eval to be stored in stack locals. r=mgaudet,arai

Depends on D107692

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1697223 - Part 4: Let the frontend optimize away the var environment for strict eval. r=mgaudet

Depends on D107693

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1697223 - Part 5: Tests. r=mgaudet

Depends on D107694

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc2532c020ae
Part 1: Tweak a comment on EvalKernel. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/eaad657b60bb
Part 2: Statically bind more identifiers in strict direct eval. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/82f030dcb363
Part 3: Allow `var` and `function` bindings in strict eval to be stored in stack locals. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/17ff9a19478e
Part 4: Let the frontend optimize away the var environment for strict eval. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/5f0d6bd6b4af
Part 5: Tests. r=mgaudet

Comment 7

[Natalia Csoregi [:nataliaCs]]

Backed out for sm failures on Frame-evalWithBindings-01.js

backout: https://hg.mozilla.org/integration/autoland/rev/e01287da1374a834f4e19300e3d5a9b6f27787ce

push: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=B457DB_RQzO421I4oh647g.0&revision=5f0d6bd6b4af135a904d7c467d47f430c0292744

failure log: https://treeherder.mozilla.org/logviewer?job_id=332883804&repo=autoland&lineNumber=21727

[task 2021-03-11T21:05:28.274Z] TEST-PASS | js/src/jit-test/tests/debug/Frame-eval-stack.js | Success (code 0, args "--no-blinterp --no-baseline --no-ion --more-compartments") [0.1 s]
[task 2021-03-11T21:05:28.278Z] /builds/worker/checkouts/gecko/js/src/jit-test/tests/debug/Frame-evalWithBindings-01.js:8:13 Error: Assertion failed: got NaN, expected 5
[task 2021-03-11T21:05:28.278Z] Stack:
[task 2021-03-11T21:05:28.278Z] @/builds/worker/checkouts/gecko/js/src/jit-test/tests/debug/Frame-evalWithBindings-01.js line 26 > eval line 1 > eval:1:12
[task 2021-03-11T21:05:28.278Z] f@/builds/worker/checkouts/gecko/js/src/jit-test/tests/debug/Frame-evalWithBindings-01.js line 26 > eval:1:30
[task 2021-03-11T21:05:28.278Z] @/builds/worker/checkouts/gecko/js/src/jit-test/tests/debug/Frame-evalWithBindings-01.js:27:3
[task 2021-03-11T21:05:28.278Z] /builds/worker/checkouts/gecko/js/src/jit-test/tests/debug/Frame-evalWithBindings-01.js:35:9 Error: Assertion failed: got 5, expected 6
[task 2021-03-11T21:05:28.278Z] Stack:
[task 2021-03-11T21:05:28.278Z] @/builds/worker/checkouts/gecko/js/src/jit-test/tests/debug/Frame-evalWithBindings-01.js:35:9
[task 2021-03-11T21:05:28.278Z] Exit code: 3

Comment 8

[Jason Orendorff [:jorendorff]]

Hmm. In this build, the test fails in two test configurations: args "--no-blinterp --no-baseline --no-ion --more-compartments" and args "". The other 4 configurations pass.

For me, even with --tbpl, all the tests pass. I'm sure this is my patch's fault, since both patch and test are extremely eval-related, but can't reproduce locally yet. Will go after it tomorrow.

Comment 9

[Jason Orendorff [:jorendorff]]

OK, here's as far as I got:

• The test fails in an ordinary debug build if JS_GC_ZEAL=GenerationalGC is set in the environment at run time.

• The test does eval-in-frame, using the .eval() method of Debugger.Frame. The bytecode of the eval-in-frame uses GetGName instructions to get the variables x and y. I believe this bytecode is correct, and I think it is the same with or without the patch.

• However, the GetGName instruction in this weird eval-in-frame context sometimes fails to find y. It thinks it has succeeded, it returns undefined (should be 3), and the test then fails because the value is wrong.

• Stepping into GetGName in the interpreter, I end up in GetEnvironmentName. This function has a NoGC fast path, but we fall off of it right away in LookupNameNoGC because the environment is a With object.

• The next enclosing environment is a DebugEnvironmentProxy. This is the proxy for the scope that actually contains y. I got as far as the Proxy::has call, which calls JS_HasPropertyById on this static eval environment, and does indeed find the shape y. Then I had to stop.

I don't know what the interaction with GC is. Puzzling.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

I'm investigating this

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

I think I have the fix for the failing test.

• When the debugger statement gets executed, with zeal enabled, we end up tracing the interpreter frame while allocating the inner environment for the bindings.

• While tracing the frame we end up setting the local to undefined, so by the time we actually read the value in the evalWithBindings, it’s gone.

• Bytecode for the script that has the debugger statement:

    main:
    00000:   1  Int8 3                      # 3
    00002:   1  SetLocal 0                  # 3
    00006:   1  Pop                         # 
    00007:   1  Debugger                    # 
    00008:   1  RetRval                     # 
  

• Tweaking the test case creates a function g() with identical bytecode.

// in a function with strict
g.eval("function f() { 'use strict'; function g() {var y = 3; debugger; }; dis(g); g(); }");
g.f();

• Parallel debugging this version, and Jason’s patches and the original test I identify one divergence between the two test cases: The computation of nlivefixed produces a different value (0 in the bad case, 1 in the good case.
• Tracing that computation shows that JSScript::numAlwaysLiveFixedSlots has special case support for FunctionScope and ModuleScopes, but not eval scopes.

Adding the following diff makes the test case pass:

diff --git a/js/src/vm/JSScript.cpp b/js/src/vm/JSScript.cpp
--- a/js/src/vm/JSScript.cpp
+++ b/js/src/vm/JSScript.cpp
@@ -720,6 +720,10 @@ size_t JSScript::numAlwaysLiveFixedSlots
   if (bodyScope()->is<js::ModuleScope>()) {
     return bodyScope()->as<js::ModuleScope>().nextFrameSlot();
   }
+  if (bodyScope()->is<js::EvalScope>() &&
+      bodyScope()->kind() == ScopeKind::StrictEval) {
+    return bodyScope()->as<js::EvalScope>().nextFrameSlot();
+  }
   return 0;
 }

I do need to figure out what patch this goes in, and do a quick audit for other potential missing places.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a183e024cae
Part 1: Tweak a comment on EvalKernel. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/d6d0427c2985
Part 2: Statically bind more identifiers in strict direct eval. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/ef40f475ce65
Part 3: Allow `var` and `function` bindings in strict eval to be stored in stack locals. r=mgaudet,arai
https://hg.mozilla.org/integration/autoland/rev/5b4a59de5970
Part 4: Let the frontend optimize away the var environment for strict eval. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/5a9e649ae7dc
Part 5: Tests. r=mgaudet

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/9a183e024cae
https://hg.mozilla.org/mozilla-central/rev/d6d0427c2985
https://hg.mozilla.org/mozilla-central/rev/ef40f475ce65
https://hg.mozilla.org/mozilla-central/rev/5b4a59de5970
https://hg.mozilla.org/mozilla-central/rev/5a9e649ae7dc

Comment 14

[Jason Orendorff [:jorendorff]]

<3