[interpreter] Store accumulator to callee after optional chain checks

mythrialle · Commit Bot · commit df98901c19ce · 2021-02-02T11:27:13.000Z
Optional chain checks check if the object is null or undefined and if it is we don't perform the load but just load accumulator with undefined. For calls the value of the accumulator needs to be stored in the callee register. We were doing this only when the object isn't null or undefined. This cl fixes it by storing it to callee always. Bug: chromium:1171954 Change-Id: I391af18e783486fed70be561027bd8aba97b93cc Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2665466 Commit-Queue: Mythri Alle <mythria@chromium.org> Reviewed-by: Shu-yu Guo <syg@chromium.org> Cr-Commit-Position: refs/heads/master@{#72475}
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
@@ -5043,8 +5043,9 @@ void BytecodeGenerator::VisitCall(Call* expr) {
       Property* property = chain->expression()->AsProperty();
       BuildOptionalChain([&]() {
         VisitAndPushIntoRegisterList(property->obj(), &args);
-        VisitPropertyLoadForRegister(args.last_register(), property, callee);
+        VisitPropertyLoad(args.last_register(), property);
       });
+      builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::SUPER_CALL:
diff --git a/test/mjsunit/regress/regress-crbug-1038178.js b/test/mjsunit/regress/regress-crbug-1038178.js
@@ -15,7 +15,7 @@ function opt(){
     (((function(){})())?.v)()
 }
 %PrepareFunctionForOptimization(opt)
-assertThrows(opt());
-assertThrows(opt());
+assertThrows(() => opt());
+assertThrows(() => opt());
 %OptimizeFunctionOnNextCall(opt)
-assertThrows(opt());
+assertThrows(() => opt());
diff --git a/test/mjsunit/regress/regress-crbug-1171954.js b/test/mjsunit/regress/regress-crbug-1171954.js
@@ -0,0 +1,19 @@
+// Copyright 2021 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: --always-opt
+
+// This causes the register used by the call in the later try-catch block to be
+// used by the ToName conversion for null which causes a DCHECK fail when
+// compiling. If register allocation changes, this test may no longer reproduce
+// the crash but it is not easy write a proper test because it is linked to
+// register allocation. This test should always work, so shouldn't cause any
+// flakes.
+try {
+  var { [null]: __v_12, } = {};
+} catch (e) {}
+
+try {
+  assertEquals((__v_40?.o?.m)().p);
+} catch (e) {}
