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

src/interpreter/bytecode-array-builder.cc

Index: src/interpreter/bytecode-array-builder.cc
diff --git a/src/interpreter/bytecode-array-builder.cc b/src/interpreter/bytecode-array-builder.cc
index 594d8faa25e02dec182eae96e94f802eaa01dd58..601fc68a6804685d6721c3121981062654a21771 100644
--- a/src/interpreter/bytecode-array-builder.cc
+++ b/src/interpreter/bytecode-array-builder.cc
@@ -13,18 +13,20 @@ class BytecodeArrayBuilder::PreviousBytecodeHelper {
   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 =
@@ -581,7 +583,11 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::PopContext(Register context) {
 
 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.
@@ -600,8 +606,6 @@ bool BytecodeArrayBuilder::NeedToBooleanCast() {
     case Bytecode::kTestIn:
     case Bytecode::kForInDone:
       return false;
-    // Also handles the case where the previous bytecode was in a different
-    // block.
     default:
       return true;
   }
@@ -613,7 +617,12 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToBoolean() {
   // 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();
@@ -644,16 +653,21 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToJSObject() {
 
 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;
   }
@@ -1090,11 +1104,13 @@ bool BytecodeArrayBuilder::LastBytecodeInSameBlock() const {
 
 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;
 }