[ignition] Fix return value of delete on global lexical variables

BytecodeGenerator previously assumed that any UNALLOCATED variable
must be a global object property, but that's incorrect for global
lexical variables declared in a different script.

This patch fixes the behavior by always falling back to the runtime
to deal with deleting UNALLOCATED variables. This is sub-optimal,
but should be correct, and it's unclear if speed is important for
this case.

Bug: v8:6733
Change-Id: I83c2a0b6e30e5e5f4c79bfe14ebf196529816c71
Reviewed-on: https://chromium-review.googlesource.com/627636Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47554}

src/interpreter/bytecode-generator.cc

@@ -3476,45 +3476,36 @@ void BytecodeGenerator::VisitDelete(UnaryOperation* expr) {
     VariableProxy* proxy = expr->expression()->AsVariableProxy();
     Variable* variable = proxy->var();
     DCHECK(is_sloppy(language_mode()) || variable->is_this());
-    switch (variable->location()) {
-      case VariableLocation::UNALLOCATED: {
-        // Global var, let, const or variables not explicitly declared.
-        Register native_context = register_allocator()->NewRegister();
-        Register global_object = register_allocator()->NewRegister();
-        builder()
-            ->LoadContextSlot(execution_context()->reg(),
-                              Context::NATIVE_CONTEXT_INDEX, 0,
-                              BytecodeArrayBuilder::kImmutableSlot)
-            .StoreAccumulatorInRegister(native_context)
-            .LoadContextSlot(native_context, Context::EXTENSION_INDEX, 0,
-                             BytecodeArrayBuilder::kImmutableSlot)
-            .StoreAccumulatorInRegister(global_object)
-            .LoadLiteral(variable->raw_name())
-            .Delete(global_object, language_mode());
-        break;
-      }
-      case VariableLocation::PARAMETER:
-      case VariableLocation::LOCAL:
-      case VariableLocation::CONTEXT: {
-        // Deleting local var/let/const, context variables, and arguments
-        // does not have any effect.
-        if (variable->is_this()) {
-          builder()->LoadTrue();
-        } else {
+    if (variable->is_this()) {
+      builder()->LoadTrue();
+    } else {
+      switch (variable->location()) {
+        case VariableLocation::PARAMETER:
+        case VariableLocation::LOCAL:
+        case VariableLocation::CONTEXT: {
+          // Deleting local var/let/const, context variables, and arguments
+          // does not have any effect.
           builder()->LoadFalse();
+          break;
         }
-        break;
-      }
-      case VariableLocation::LOOKUP: {
-        Register name_reg = register_allocator()->NewRegister();
-        builder()
-            ->LoadLiteral(variable->raw_name())
-            .StoreAccumulatorInRegister(name_reg)
-            .CallRuntime(Runtime::kDeleteLookupSlot, name_reg);
-        break;
+        case VariableLocation::UNALLOCATED:
+        // TODO(adamk): Falling through to the runtime results in correct
+        // behavior, but does unnecessary context-walking (since scope
+        // analysis has already proven that the variable doesn't exist in
+        // any non-global scope). Consider adding a DeleteGlobal bytecode
+        // that knows how to deal with ScriptContexts as well as global
+        // object properties.
+        case VariableLocation::LOOKUP: {
+          Register name_reg = register_allocator()->NewRegister();
+          builder()
+              ->LoadLiteral(variable->raw_name())
+              .StoreAccumulatorInRegister(name_reg)
+              .CallRuntime(Runtime::kDeleteLookupSlot, name_reg);
+          break;
+        }
+        default:
+          UNREACHABLE();
       }
-      default:
-        UNREACHABLE();
     }
   } else {
     // Delete of an unresolvable reference returns true.

test/cctest/interpreter/bytecode_expectations/GlobalDelete.golden

@@ -66,17 +66,14 @@ snippet: "
   };
   f();
 "
-frame size: 2
+frame size: 1
 parameter count: 1
-bytecode array length: 16
+bytecode array length: 11
 bytecodes: [
   /*   32 E> */ B(StackCheck),
-  /*   39 S> */ B(LdaImmutableCurrentContextSlot), U8(3),
+  /*   39 S> */ B(LdaConstant), U8(0),
                 B(Star), R(0),
-                B(LdaImmutableContextSlot), R(0), U8(2), U8(0),
-                B(Star), R(1),
-                B(LdaConstant), U8(0),
-                B(DeletePropertySloppy), R(1),
+                B(CallRuntime), U16(Runtime::kDeleteLookupSlot), R(0), U8(1),
   /*   55 S> */ B(Return),
 ]
 constant pool: [
@@ -93,17 +90,14 @@ snippet: "
   };
   f();
 "
-frame size: 2
+frame size: 1
 parameter count: 1
-bytecode array length: 16
+bytecode array length: 11
 bytecodes: [
   /*   18 E> */ B(StackCheck),
-  /*   25 S> */ B(LdaImmutableCurrentContextSlot), U8(3),
+  /*   25 S> */ B(LdaConstant), U8(0),
                 B(Star), R(0),
-                B(LdaImmutableContextSlot), R(0), U8(2), U8(0),
-                B(Star), R(1),
-                B(LdaConstant), U8(0),
-                B(DeletePropertySloppy), R(1),
+                B(CallRuntime), U16(Runtime::kDeleteLookupSlot), R(0), U8(1),
   /*   41 S> */ B(Return),
 ]
 constant pool: [

test/mjsunit/regress/regress-6733.js

+// Copyright 2017 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.
+
+let x;
+Realm.eval(Realm.current(), "let y");
+assertFalse(delete x);
+assertFalse(delete y);
+assertFalse(eval("delete x"));
+assertFalse(eval("delete y"));
+
+(function() {
+  let z;
+  assertFalse(delete x);
+  assertFalse(delete y);
+  assertFalse(delete z);
+  assertFalse(eval("delete x"));
+  assertFalse(eval("delete y"));
+  assertFalse(eval("delete z"));
+})();
+
+assertFalse(eval("let z; delete z"));