[scopes] Skip dynamic vars in eval scopes during lookup

For variable proxies in a function inside an eval scope that point to
a dynamic variable in the eval scope, the current scope resolution will
find this variable only when the function is eagerly compiled, as the
eval scope only exists during top-level eval compilation. This causes
a mismatch between lazy- and eager- compiled functions.

With this patch, we skip these dynamic variables during lookup, so that
the lookup for the variable proxy always finds a kDynamicLocal or
kDynamicGlobal, both when compiled lazily and eagerly. This is a minor
pessimisation of performance (as we know that the lookup has to be
dynamic), but unblocks other improvements which require idempotent
bytecode generation (such as lazy source positions).

Note that the alternative, of simply not tracking dynamic variables on
the eval scope at all, is not viable due to needing this information
during conflict detection.

Bug: v8:8510
Bug: v8:9511
Change-Id: Ifa72ec05e9a97b7be418912340081b9656765fd4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1733077
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Dan Elphick <delphick@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63051}

src/ast/scopes.cc

@@ -1811,7 +1811,18 @@ Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
     // We found a variable and we are done. (Even if there is an 'eval' in this
     // scope which introduces the same variable again, the resulting variable
     // remains the same.)
-    if (var != nullptr) {
+    //
+    // For sloppy eval though, we skip dynamic variable to avoid resolving to a
+    // variable when the variable and proxy are in the same eval execution. The
+    // variable is not available on subsequent lazy executions of functions in
+    // the eval, so this avoids inner functions from looking up different
+    // variables during eager and lazy compilation.
+    //
+    // TODO(leszeks): Maybe we want to restrict this to e.g. lookups of a proxy
+    // living in a different scope to the current one, or some other
+    // optimisation.
+    if (var != nullptr &&
+        !(scope->is_eval_scope() && var->mode() == VariableMode::kDynamic)) {
       if (mode == kParsedScope && force_context_allocation &&
           !var->is_dynamic()) {
         var->ForceContextAllocation();

test/mjsunit/regress/regress-8510-2.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --enable-lazy-source-positions
+
+try {
+  (function () {
+    eval(`
+      function assertLocation() {}
+      (function foo() {
+        var x = 1;
+        assertLocation();
+        throw new Error();
+      })();
+    `);
+  })();
+} catch (e) {
+  print(e.stack);
+}
+
+try {
+  (function () {
+    var assertLocation = 2;
+    (function () {
+      eval(`
+        function assertLocation() {}
+        (function foo() {
+          var x = 1;
+          assertLocation();
+          throw new Error();
+        })();
+      `);
+    })();
+  })();
+} catch (e) {
+  print(e.stack);
+}

test/mjsunit/regress/regress-v8-9511.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+var f = function() { return 1; };
+
+(function func1() {
+  eval("var f = function canary(s) { return 2; }");
+})();
+
+assertEquals(f(), 1);