[Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block.

src/interpreter/bytecode-array-builder.cc

 // Copyright 2015 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.
 
 #include "src/interpreter/bytecode-array-builder.h"
 
 namespace v8 {
 namespace internal {
 namespace interpreter {
 
 class BytecodeArrayBuilder::PreviousBytecodeHelper {
  public:
   explicit PreviousBytecodeHelper(const BytecodeArrayBuilder& array_builder)
       : array_builder_(array_builder) {}

[oth, 2015/12/08 09:54:14]

It would be better to have:

 CHECK(array_builder->LastBytecodeIsInSameBlock())

in the constructor and DCHECK in the getters, leaving GetBytecode and GetOperand
alone. The code would be more readable and realiable if it explicitly checked
the validity of the previous bytecode before instantiating
PreviousBytecodeHelper.

The PreviousBytecode class should have DISALLOW_COPY_AND_ASSIGN.

[mythria, 2015/12/08 11:24:42]

Done.

 
-  Bytecode GetBytecode() const {
+  bool GetBytecode(Bytecode* bytecode) const {

[oth, 2015/12/08 09:54:15]

Propose leaving this as Bytecode, adding MUST_USE_RESULT, and DCHECK'ing in the
same block here.

[mythria, 2015/12/08 11:24:42]

Done.

     // Returns the previous bytecode in the same basicblock. If there is none it
-    // returns Bytecode::kLast.
+    // returns false and does not change bytecode.
     if (!array_builder_.LastBytecodeInSameBlock()) {
-      return Bytecode::kLast;
+      return false;
     }
-    return Bytecodes::FromByte(
+    *bytecode = Bytecodes::FromByte(
         array_builder_.bytecodes()->at(array_builder_.last_bytecode_start_));
+    return true;
   }
 
   uint32_t GetOperand(int operand_index) const {

[oth, 2015/12/08 09:54:14]

Propse adding MUST_USE_RESULT and DCHECK'ing in the same block here.

[mythria, 2015/12/08 11:24:42]

The DCHECK is done when we call GetBytecode() from this function. So did not add
another one here. 
Done.

-    Bytecode bytecode = GetBytecode();
+    Bytecode bytecode = Bytecode::kLast;
+    CHECK(GetBytecode(&bytecode));
     DCHECK_GE(operand_index, 0);
     DCHECK_LT(operand_index, Bytecodes::NumberOfOperands(bytecode));
     size_t operand_offset =
         array_builder_.last_bytecode_start_ +
         Bytecodes::GetOperandOffset(bytecode, operand_index);
     OperandSize size = Bytecodes::GetOperandSize(bytecode, operand_index);
     switch (size) {
       default:
       case OperandSize::kNone:
         UNREACHABLE();
@@ skipping 536 matching lines @@
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::PopContext(Register context) {
   Output(Bytecode::kPopContext, context.ToOperand());
   return *this;
 }
 
 
 bool BytecodeArrayBuilder::NeedToBooleanCast() {
   PreviousBytecodeHelper previous_bytecode(*this);
-  switch (previous_bytecode.GetBytecode()) {
+  Bytecode bytecode = Bytecode::kLast;
+  if (!previous_bytecode.GetBytecode(&bytecode)) {
+    return true;
+  }
+  switch (bytecode) {
     case Bytecode::kToBoolean:
       UNREACHABLE();
     // If the previous bytecode puts a boolean in the accumulator return true.
     case Bytecode::kLdaTrue:
     case Bytecode::kLdaFalse:
     case Bytecode::kLogicalNot:
     case Bytecode::kTestEqual:
     case Bytecode::kTestNotEqual:
     case Bytecode::kTestEqualStrict:
     case Bytecode::kTestNotEqualStrict:
     case Bytecode::kTestLessThan:
     case Bytecode::kTestLessThanOrEqual:
     case Bytecode::kTestGreaterThan:
     case Bytecode::kTestGreaterThanOrEqual:
     case Bytecode::kTestInstanceOf:
     case Bytecode::kTestIn:
     case Bytecode::kForInDone:
       return false;
-    // Also handles the case where the previous bytecode was in a different
-    // block.
     default:
       return true;
   }
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToBoolean() {
   PreviousBytecodeHelper previous_bytecode(*this);
   // If the previous bytecode puts a boolean in the accumulator
   // there is no need to emit an instruction.
   if (NeedToBooleanCast()) {
-    switch (previous_bytecode.GetBytecode()) {
+    Bytecode bytecode = Bytecode::kLast;
+    if (!previous_bytecode.GetBytecode(&bytecode)) {
+      Output(Bytecode::kToBoolean);
+      return *this;
+    }
+    switch (bytecode) {
       // If the previous bytecode is a constant evaluate it and return false.
       case Bytecode::kLdaZero: {
         LoadFalse();
         break;
       }
       case Bytecode::kLdaSmi8: {
         LoadBooleanConstant(previous_bytecode.GetOperand(0) != 0);
         break;
       }
       case Bytecode::kLdaConstant: {
@@ skipping 10 matching lines @@
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToJSObject() {
   Output(Bytecode::kToObject);
   return *this;
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToName() {
   PreviousBytecodeHelper previous_bytecode(*this);
-  switch (previous_bytecode.GetBytecode()) {
+  Bytecode bytecode = Bytecode::kLast;
+  if (!previous_bytecode.GetBytecode(&bytecode)) {
+    Output(Bytecode::kToName);
+    return *this;
+  }
+  switch (bytecode) {
+    case Bytecode::kToName:
+    case Bytecode::kTypeOf:
+      return *this;
     case Bytecode::kLdaConstantWide:
     case Bytecode::kLdaConstant: {
       Handle<Object> object = previous_bytecode.GetConstantForIndexOperand(0);
       if (object->IsName()) return *this;
       break;
     }
-    case Bytecode::kToName:
-    case Bytecode::kTypeOf:
-      return *this;
     default:
       break;
   }
   Output(Bytecode::kToName);
   return *this;
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToNumber() {
   // TODO(rmcilroy): consider omitting if the preceeding bytecode always returns
@@ skipping 416 matching lines @@
 
 
 bool BytecodeArrayBuilder::LastBytecodeInSameBlock() const {
   return last_bytecode_start_ < bytecodes()->size() &&
          last_bytecode_start_ >= last_block_end_;
 }
 
 
 bool BytecodeArrayBuilder::IsRegisterInAccumulator(Register reg) {
   PreviousBytecodeHelper previous_bytecode(*this);
-  if (previous_bytecode.GetBytecode() == Bytecode::kLdar ||
-      previous_bytecode.GetBytecode() == Bytecode::kStar) {
-    if (reg == Register::FromOperand(previous_bytecode.GetOperand(0))) {
-      return true;
-    }
+  Bytecode bytecode = Bytecode::kLast;
+  if (!previous_bytecode.GetBytecode(&bytecode)) {
+    return false;
+  }
+  if ((bytecode == Bytecode::kLdar || bytecode == Bytecode::kStar) &&
+      (reg == Register::FromOperand(previous_bytecode.GetOperand(0)))) {
+    return true;
   }
   return false;
 }
 
 
 // static
 Bytecode BytecodeArrayBuilder::BytecodeForBinaryOperation(Token::Value op) {
   switch (op) {
     case Token::Value::ADD:
       return Bytecode::kAdd;
@@ skipping 316 matching lines @@
   DCHECK_GT(next_consecutive_count_, 0);
   builder_->BorrowConsecutiveTemporaryRegister(next_consecutive_register_);
   allocated_.push_back(next_consecutive_register_);
   next_consecutive_count_--;
   return Register(next_consecutive_register_++);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8