Tests for GuardField length check along with bug fixes

runtime/vm/intermediate_language_arm.cc

Index: runtime/vm/intermediate_language_arm.cc
diff --git a/runtime/vm/intermediate_language_arm.cc b/runtime/vm/intermediate_language_arm.cc
index 830ade19e433bb231929621a9a5a9457ee6922b6..379a36820e599ffbf1e7da2c72175d942cbfe901 100644
--- a/runtime/vm/intermediate_language_arm.cc
+++ b/runtime/vm/intermediate_language_arm.cc
@@ -1537,7 +1537,7 @@ LocationSummary* GuardFieldInstr::MakeLocationSummary() const {
 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) &&
@@ -1586,7 +1586,7 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
     }
 
     __ 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());
@@ -1618,6 +1618,36 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
           __ 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);
@@ -1648,6 +1678,7 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
           ASSERT(field_cid == kIllegalCid);
           // Following jump cannot not occur, fall through.
         }
+        __ b(fail, NE);
       }
       // Not identical, possibly null.
       __ Bind(&skip_length_check);
@@ -1688,7 +1719,7 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
         __ 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);
@@ -1697,22 +1728,23 @@ void GuardFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
       __ 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);
     }