Tests for GuardField length check along with bug fixes

runtime/vm/intermediate_language_arm.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/globals.h"  // Needed here to get TARGET_ARCH_ARM.
 #if defined(TARGET_ARCH_ARM)
 
 #include "vm/intermediate_language.h"
 
 #include "vm/dart_entry.h"
@@ skipping 1519 matching lines @@
   if (need_field_temp_reg) {
     summary->AddTemp(Location::RequiresRegister());
   }
   return summary;
 }
 
 
 void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   const intptr_t field_cid = field().guarded_cid();
   const intptr_t nullability = field().is_nullable() ? kNullCid : kIllegalCid;
-  const intptr_t field_length = field().guarded_list_length();
+  const intptr_t field_length = Smi::RawValue(field().guarded_list_length());

[srdjan, 2013/08/27 17:45:22]

It is not a good idea to hang onto tagged value as integer, unless the name
describes it, e.g., tagged_filed_length. Instead, I would do
Smi::New(field_length) wherever you need to pass length as Smi.

[Cutch, 2013/08/28 12:24:59]

Thanks for this.

   const bool field_has_length = field().needs_length_check();
   const bool needs_value_temp_reg =
       (field_has_length || ((value()->Type()->ToCid() == kDynamicCid) &&
                             (field().guarded_cid() != kSmiCid)));
   const bool needs_field_temp_reg =
       field_has_length || (field().guarded_cid() == kIllegalCid);
   if (field_has_length) {
     // Currently, we should only see final fields that remember length.
     ASSERT(field().is_final());
   }
@@ skipping 28 matching lines @@
     if (!compiler->is_optimizing() && (field_reg == kNoRegister)) {
       // Currently we can't have different location summaries for optimized
       // and non-optimized code. So instead we manually pick up a register
       // that is known to be free because we know how non-optimizing compiler
       // allocates registers.
       field_reg = R2;
       ASSERT((field_reg != value_reg) && (field_reg != value_cid_reg));
     }
 
     __ LoadObject(field_reg, Field::ZoneHandle(field().raw()));
-
+    int32_t raw_no_fixed_length = Smi::RawValue(Field::kNoFixedLength);
     FieldAddress field_cid_operand(field_reg, Field::guarded_cid_offset());
     FieldAddress field_nullability_operand(
         field_reg, Field::is_nullable_offset());
     FieldAddress field_length_operand(
         field_reg, Field::guarded_list_length_offset());
 
     if (value_cid_reg == kNoRegister) {
       ASSERT(!compiler->is_optimizing());
       value_cid_reg = R3;
       ASSERT((value_cid_reg != value_reg) && (field_reg != value_cid_reg));
@@ skipping 11 matching lines @@
         if ((field_cid == kArrayCid) || (field_cid == kImmutableArrayCid)) {
           __ ldr(temp_reg,
                  FieldAddress(value_reg, Array::length_offset()));
           __ CompareImmediate(temp_reg, field_length);
         } else if (RawObject::IsTypedDataClassId(field_cid)) {
           __ ldr(temp_reg,
                  FieldAddress(value_reg, TypedData::length_offset()));
           __ CompareImmediate(temp_reg, field_length);
         } else {
           ASSERT(field_cid == kIllegalCid);
+          Label check_array, local_exit, local_fail;
+          __ CompareImmediate(value_cid_reg, kNullCid);

[srdjan, 2013/08/27 17:45:22]

This code seems identical to the one below, please factor out.

[Cutch, 2013/08/28 12:24:59]

This code is not identical to the block below. This block does the following:

1. Determines what type of list we are dealing with.
2. Compares the guarded field length with the list length.

The code below does the following:

1. Determine what type of list we are dealing with.
2. Sets the guarded field length for the first time.

What happens is that when we compile the function we have no static knowledge
(field_cid and value_cid are unknown). The first time it executes we set the
field length for the first time and the second time we execute it we must guard
against it. Future compilations of the function should have static knowledge
(field_cid or value_cid will be known) and emit much smaller code.

+          __ b(&local_fail, EQ);
+          // Check for typed data array.
+          __ CompareImmediate(value_cid_reg, kTypedDataFloat32x4ArrayCid);
+          __ b(&local_fail, GT);
+          __ CompareImmediate(value_cid_reg, kTypedDataInt8ArrayCid);
+          __ b(&check_array, LT);  // Could still be a regular array.
+          __ ldr(temp_reg,
+                 FieldAddress(value_reg, TypedData::length_offset()));
+          __ ldr(IP, field_length_operand);
+          __ cmp(temp_reg, ShifterOperand(IP));
+          __ b(&local_exit);  // Updated field length typed data array.
+          // Check for regular array.
+          __ Bind(&check_array);
+          __ CompareImmediate(value_cid_reg, kImmutableArrayCid);
+          __ b(&local_fail, GT);
+          __ CompareImmediate(value_cid_reg, kArrayCid);
+          __ b(&local_fail, LT);
+          __ ldr(temp_reg,
+                 FieldAddress(value_reg, Array::length_offset()));
+          __ ldr(IP, field_length_operand);
+          __ cmp(temp_reg, ShifterOperand(IP));
+          __ b(&local_exit);  // Updated field length from regular array.
+
+          __ Bind(&local_fail);
+          __ LoadImmediate(IP, raw_no_fixed_length);
+          __ str(IP, field_length_operand);
+
+          __ Bind(&local_exit);
           // Following branch cannot not occur, fall through.
         }
         __ b(fail, NE);
       }
       __ Bind(&skip_length_check);
       __ ldr(IP, field_nullability_operand);
       __ cmp(value_cid_reg, ShifterOperand(IP));
     } else if (value_cid == kNullCid) {
       __ ldr(value_cid_reg, field_nullability_operand);
       __ CompareImmediate(value_cid_reg, value_cid);
@@ skipping 10 matching lines @@
                   FieldAddress(value_reg, Array::length_offset()));
           __ CompareImmediate(temp_reg, field_length);
         } else if (RawObject::IsTypedDataClassId(field_cid)) {
           __ ldr(temp_reg,
                   FieldAddress(value_reg, TypedData::length_offset()));
           __ CompareImmediate(temp_reg, field_length);
         } else {
           ASSERT(field_cid == kIllegalCid);
           // Following jump cannot not occur, fall through.
         }
+        __ b(fail, NE);
       }
       // Not identical, possibly null.
       __ Bind(&skip_length_check);
     }
     __ b(&ok, EQ);
 
     __ ldr(IP, field_cid_operand);
     __ CompareImmediate(IP, kIllegalCid);
     __ b(fail, NE);
 
@@ skipping 20 matching lines @@
         __ b(&local_fail, GT);
         __ CompareImmediate(value_cid_reg, kArrayCid);
         __ b(&local_fail, LT);
         // Destroy value_cid_reg (safe because we are finished with it).
         __ ldr(value_cid_reg,
                FieldAddress(value_reg, Array::length_offset()));
         __ str(value_cid_reg, field_length_operand);
         __ b(&local_exit);  // Updated field length from regular array.
 
         __ Bind(&local_fail);
-        __ LoadImmediate(IP, Field::kNoFixedLength);
+        __ LoadImmediate(IP, raw_no_fixed_length);
         __ str(IP, field_length_operand);
 
         __ Bind(&local_exit);
       }
     } else {
       __ LoadImmediate(IP, value_cid);
       __ str(IP, field_cid_operand);
       __ str(IP, field_nullability_operand);
-      if ((value_cid == kArrayCid) || (value_cid == kImmutableArrayCid)) {
-        // Destroy value_cid_reg (safe because we are finished with it).
-        __ ldr(value_cid_reg,
-               FieldAddress(value_reg, Array::length_offset()));
-        __ str(value_cid_reg, field_length_operand);
-      } else if (RawObject::IsTypedDataClassId(value_cid)) {
-        // Destroy value_cid_reg (safe because we are finished with it).
-        __ ldr(value_cid_reg,
-                FieldAddress(value_reg, TypedData::length_offset()));
-        __ str(value_cid_reg, field_length_operand);
-      } else {
-        __ LoadImmediate(IP, Field::kNoFixedLength);
-        __ str(IP, field_length_operand);
+      if (field_has_length) {
+        if ((value_cid == kArrayCid) || (value_cid == kImmutableArrayCid)) {
+          // Destroy value_cid_reg (safe because we are finished with it).
+          __ ldr(value_cid_reg,
+                 FieldAddress(value_reg, Array::length_offset()));
+          __ str(value_cid_reg, field_length_operand);
+        } else if (RawObject::IsTypedDataClassId(value_cid)) {
+          // Destroy value_cid_reg (safe because we are finished with it).
+          __ ldr(value_cid_reg,
+                  FieldAddress(value_reg, TypedData::length_offset()));
+          __ str(value_cid_reg, field_length_operand);
+        } else {
+          __ LoadImmediate(IP, raw_no_fixed_length);
+          __ str(IP, field_length_operand);
+        }
       }
     }
-
     if (!ok_is_fall_through) {
       __ b(&ok);
     }
   } else {
     if (field_reg != kNoRegister) {
       __ LoadObject(field_reg, Field::ZoneHandle(field().raw()));
     }
     if (value_cid == kDynamicCid) {
       // Field's guarded class id is fixed by value's class id is not known.
       __ tst(value_reg, ShifterOperand(kSmiTagMask));
@@ skipping 2917 matching lines @@
   compiler->GenerateCall(token_pos(),
                          &label,
                          PcDescriptors::kOther,
                          locs());
   __ Drop(2);  // Discard type arguments and receiver.
 }
 
 }  // namespace dart
 
 #endif  // defined TARGET_ARCH_ARM