[parser] Always mark shadowed vars maybe_assigned

If there was an assignment to a maybe-shadowing dynamic variable,
then the shadowing variable would be marked maybe_assigned, but the
maybe-shadowed variable would stay unchanged. This meant that in
non-shadowing cases, the not-actually-shadowed variable would have
the wrong maybe_assigned state, and e.g. would break context
specialization.

This patch pessimistically unconditionally sets maybe_assigned on
variables shadowed by a dynamic variable in a `with` scope. This
marking can cause false positives and sub-optimal optimization for
some functions with 'with' blocks, but it's also the simplest fix
for this issue which doesn't affect performance in the common case
of no 'with' blocks.

Bug: v8:9394
Change-Id: I6924bd7d48dda61232aa9d72c39df1c76c665c67
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1678365
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62407}

src/ast/scopes.cc

@@ -1887,7 +1887,7 @@ Variable* Scope::LookupWith(VariableProxy* proxy, Scope* scope,
     DCHECK(!scope->already_resolved_);
     var->set_is_used();
     var->ForceContextAllocation();
-    if (proxy->is_assigned()) var->set_maybe_assigned();
+    var->set_maybe_assigned();
   }
   if (entry_point != nullptr) entry_point->variables_.Remove(var);
   Scope* target = entry_point == nullptr ? scope : entry_point;

test/mjsunit/regress/regress-v8-9394.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: --allow-natives-syntax
+
+(function testMaybeAssignedWithShadowing() {
+
+  function foo() {
+    let a = 0;
+    let g;
+
+    with ({}) {
+      g = function g() {
+        // Increment a, should set it maybe_assigned but doesn't in the bug.
+        ++a;
+      }
+      // Shadowing the outer 'a' with a dynamic one.
+      a;
+    }
+
+    return function () {
+      // The access to a would be context specialized (to 2 since it's after the
+      // second call) if maybe_assigned were incorrectly not set.
+      g(a);
+      return a;
+    }
+  };
+
+  f = foo();
+  %PrepareFunctionForOptimization(f);
+  assertEquals(f(), 1);
+  assertEquals(f(), 2);
+  %OptimizeFunctionOnNextCall(f);
+  assertEquals(f(), 3);
+
+})();
+
+// Same test as above, just with more shadowing (including dynamic->dynamic
+// shadowing) and skipping over scopes with shadows.
+(function testMaybeAssignedWithDeeplyNestedShadowing() {
+
+  function foo() {
+    let a = 0;
+    let g;
+
+    // Increment a, should set it maybe_assigned but doesn't in the bug.
+    with ({}) {
+      with ({}) {
+        with ({}) {
+          with ({}) {
+            with ({}) {
+              g = function g() { ++a; }
+              // Shadow the second dynamic 'a'.
+              a;
+            }
+            // Shadowing the first dynamic 'a'.
+            a;
+          }
+          // Skip shadowing here
+        }
+        // Skip shadowing here
+      }
+      // Shadowing the outer 'a' with a dynamic one.
+      a;
+    }
+
+    return function () {
+      // The access to a would be context specialized (to 2 since it's after the
+      // second call) if maybe_assigned were incorrectly not set.
+      g(a);
+      return a;
+    }
+  };
+
+  f = foo();
+  %PrepareFunctionForOptimization(f);
+  assertEquals(f(), 1);
+  assertEquals(f(), 2);
+  %OptimizeFunctionOnNextCall(f);
+  assertEquals(f(), 3);
+
+})();