Tests for GuardField length check along with bug fixes

runtime/vm/intermediate_language_ia32.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_IA32.
 #if defined(TARGET_ARCH_IA32)
 
 #include "vm/intermediate_language.h"
 
 #include "vm/dart_entry.h"
@@ skipping 1594 matching lines @@
       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());
   }
 
   if (field_cid == kDynamicCid) {
     ASSERT(!compiler->is_optimizing());
     return;  // Nothing to emit.
   }
-
   const intptr_t value_cid = value()->Type()->ToCid();
 
   Register value_reg = locs()->in(0).reg();
 
   Register value_cid_reg = needs_value_temp_reg ?
       locs()->temp(0).reg() : kNoRegister;
 
   Register field_reg = needs_field_temp_reg ?
       locs()->temp(locs()->temp_count() - 1).reg() : kNoRegister;
 
@@ skipping 25 matching lines @@
         field_reg, Field::guarded_list_length_offset());
 
     if (value_cid == kDynamicCid) {
       if (value_cid_reg == kNoRegister) {
         ASSERT(!compiler->is_optimizing());
         value_cid_reg = EDX;
         ASSERT((value_cid_reg != value_reg) && (field_reg != value_cid_reg));
       }
 
       LoadValueCid(compiler, value_cid_reg, value_reg);
-
       Label skip_length_check;
       __ cmpl(value_cid_reg, field_cid_operand);
       // Value CID != Field guard CID, skip length check.
       __ j(NOT_EQUAL, &skip_length_check);
       if (field_has_length) {
         // Field guard may have remembered list length, check it.
         if ((field_cid == kArrayCid) || (field_cid == kImmutableArrayCid)) {
           __ pushl(value_cid_reg);
           __ movl(value_cid_reg,
                   FieldAddress(value_reg, Array::length_offset()));
-          __ cmpl(value_cid_reg, Immediate(field_length));
+          __ cmpl(value_cid_reg, Immediate(Smi::RawValue(field_length)));
           __ popl(value_cid_reg);
         } else if (RawObject::IsTypedDataClassId(field_cid)) {
           __ pushl(value_cid_reg);
           __ movl(value_cid_reg,
                   FieldAddress(value_reg, TypedData::length_offset()));
-          __ cmpl(value_cid_reg, Immediate(field_length));
+          __ cmpl(value_cid_reg, Immediate(Smi::RawValue(field_length)));
           __ popl(value_cid_reg);
         } else {
           ASSERT(field_cid == kIllegalCid);
-          // Following jump cannot not occur, fall through.
+          Label check_array, local_exit, local_fail;

[srdjan, 2013/08/28 16:00:55]

Optional: maybe rename the labels to be self-explanatory:
local_exit -> length_loaded
local_fail -> no_fixed_length

[srdjan, 2013/08/28 16:00:55]

Please remind me how can field_cid be kIllegalCid but the length is known?

[Cutch, 2013/08/28 19:20:36]

field_cid is kIllegalCid and field_length is kUnknownLength at compile time but
this code path can be executed after updating field_cid_operand and
field_length_operand but before we recompile with the new knowledge. At run-time
the following happens:

First time we execute:
1. We jump at 1663 because we field_cid_operand is kIlllegalCid.
2. We end up at 1768 and initialized field_cid_operand and field_length_operand
for the first time.

Second time we execute:
1. We fall through at 1663 because field_cid_operand matches value_cid_reg.
2. We check that field_length_operand matches the length of the list, if not we
set field_length_operand to kNoFixedLength and end up at fail.

[Cutch, 2013/08/29 08:27:17]

Done.

+          // Check field guard.
+          __ cmpl(value_cid_reg, Immediate(kNullCid));
+          __ j(EQUAL, &local_fail);
+          // Check for typed data array.
+          __ cmpl(value_cid_reg, Immediate(kTypedDataFloat32x4ArrayCid));
+          __ j(GREATER, &local_fail);  // Not a typed array or a regular array.
+          __ cmpl(value_cid_reg, Immediate(kTypedDataInt8ArrayCid));
+          __ j(LESS, &check_array);  // Could still be a regular array.
+          __ pushl(value_cid_reg);
+          __ movl(value_cid_reg,
+                  FieldAddress(value_reg, TypedData::length_offset()));
+          __ cmpl(field_length_operand, value_cid_reg);

[srdjan, 2013/08/28 16:00:55]

Eliminate push-pop, use cmpl(field_length, FieldAddress())
Also instead of jump to local_exit, you can do j(NOT_EQUAL, fail).

[Cutch, 2013/08/29 08:27:17]

We do not know field_length at compile time, I've added an ASSERT to this
effect.
If we j(NOT_EQUAL, fail) when the list length is equal we fall through into code
which will destroy the known list length.

[srdjan, 2013/08/29 16:05:25]

I see: field_length_operand is an address, not a register.

******

Currently the code does:

__ cmpl (..)
__ popl(...)
__ jmp(&local_exit);

...
__ Bind(&local_exit);
__ j(NOT_EQUAL, fail);
...

I thought could be safely transformed to:

__ cmpl(...)
__ popl(..)
__ jmp(NOT_EQUAL, &fail).

Also try using NearLabels if possible, it shortens the code.

+          __ popl(value_cid_reg);
+          __ jmp(&local_exit);
+          // Check for regular array.
+          __ Bind(&check_array);
+          __ cmpl(value_cid_reg, Immediate(kImmutableArrayCid));
+          __ j(GREATER, &local_fail);
+          __ cmpl(value_cid_reg, Immediate(kArrayCid));
+          __ j(LESS, &local_fail);
+          __ pushl(value_cid_reg);
+          __ movl(value_cid_reg,
+                  FieldAddress(value_reg, Array::length_offset()));
+          __ cmpl(field_length_operand, value_cid_reg);

[srdjan, 2013/08/28 16:00:55]

Use __ cmpl(field_length_operand, FieldAddress(...)) and avoid push/pop,
followed by j(NOT_EQUAL, fail)

[Cutch, 2013/08/29 08:27:17]

field_length_operand is a FieldAddress there is no cmpl(FieldAddress,
FieldAddress).

+          __ popl(value_cid_reg);
+          __ jmp(&local_exit);
+
+          __ Bind(&local_fail);
+          __ movl(field_length_operand,
+                  Immediate(Smi::RawValue(Field::kNoFixedLength)));
+
+          __ Bind(&local_exit);
         }
         __ j(NOT_EQUAL, fail);
       }
       __ Bind(&skip_length_check);
       __ cmpl(value_cid_reg, field_nullability_operand);
     } else if (value_cid == kNullCid) {
       // Value in graph known to be null.
       // Compare with null.
       __ cmpl(field_nullability_operand, Immediate(value_cid));
     } else {
       // Value in graph known to be non-null.
       Label skip_length_check;
       // Compare class id with guard field class id.
       __ cmpl(field_cid_operand, Immediate(value_cid));
       // If not equal, skip over length check.
       __ j(NOT_EQUAL, &skip_length_check);
       // Insert length check.
       if (field_has_length) {
         if (value_cid_reg == kNoRegister) {
           ASSERT(!compiler->is_optimizing());
           value_cid_reg = EDX;
           ASSERT((value_cid_reg != value_reg) && (field_reg != value_cid_reg));
         }
         ASSERT(value_cid_reg != kNoRegister);
-        if ((field_cid == kArrayCid) || (field_cid == kImmutableArrayCid)) {
+        if ((value_cid == kArrayCid) || (value_cid == kImmutableArrayCid)) {
           __ pushl(value_cid_reg);
           __ movl(value_cid_reg,
                   FieldAddress(value_reg, Array::length_offset()));
-          __ cmpl(value_cid_reg, Immediate(field_length));
+          __ cmpl(value_cid_reg, Immediate(Smi::RawValue(field_length)));

[srdjan, 2013/08/28 16:00:55]

Eliminate push/pop by using __cmpl(FieldAddress(...), Immediate(...));

Also below.

[Cutch, 2013/08/29 08:27:17]

Done.

           __ popl(value_cid_reg);
-        } else if (RawObject::IsTypedDataClassId(field_cid)) {
+        } else if (RawObject::IsTypedDataClassId(value_cid)) {
           __ pushl(value_cid_reg);
           __ movl(value_cid_reg,
                   FieldAddress(value_reg, TypedData::length_offset()));
-          __ cmpl(value_cid_reg, Immediate(field_length));
+          __ cmpl(value_cid_reg, Immediate(Smi::RawValue(field_length)));

[srdjan, 2013/08/28 16:00:55]

Eliminate push/pop

[Cutch, 2013/08/29 08:27:17]

Done.

           __ popl(value_cid_reg);
         } else {
           ASSERT(field_cid == kIllegalCid);
           // Following jump cannot not occur, fall through.
         }
+        __ j(NOT_EQUAL, fail);
       }
       // Not identical, possibly null.
       __ Bind(&skip_length_check);
     }
     // Jump when class id guard and list length guard are okay.
     __ j(EQUAL, &ok);
 
-
     // Check if guard field is uninitialized.
     __ cmpl(field_cid_operand, Immediate(kIllegalCid));
     // Jump to failure path when guard field has been initialized and
     // the field and value class ids do not not match.
     __ j(NOT_EQUAL, fail);
 
     // At this point the field guard is being initialized for the first time.
     if (value_cid == kDynamicCid) {
       // Do not know value's class id.
       __ movl(field_cid_operand, value_cid_reg);
@@ skipping 18 matching lines @@
         __ j(GREATER, &local_fail);
         __ cmpl(value_cid_reg, Immediate(kArrayCid));
         __ j(LESS, &local_fail);
         // Destroy value_cid_reg (safe because we are finished with it).
         __ movl(value_cid_reg,
                 FieldAddress(value_reg, Array::length_offset()));
         __ movl(field_length_operand, value_cid_reg);
         __ jmp(&local_exit);  // Updated field length from regular array.
 
         __ Bind(&local_fail);
-        __ movl(field_length_operand, Immediate(Field::kNoFixedLength));
+        __ movl(field_length_operand,
+                Immediate(Smi::RawValue(Field::kNoFixedLength)));
 
         __ Bind(&local_exit);
       }
     } else {
       if (value_cid_reg == kNoRegister) {
           ASSERT(!compiler->is_optimizing());
           value_cid_reg = EDX;
           ASSERT((value_cid_reg != value_reg) && (field_reg != value_cid_reg));
       }
       ASSERT(value_cid_reg != kNoRegister);
       ASSERT(field_reg != kNoRegister);
       __ movl(field_cid_operand, Immediate(value_cid));
       __ movl(field_nullability_operand, Immediate(value_cid));
-      if ((value_cid == kArrayCid) || (value_cid == kImmutableArrayCid)) {
-        // Destroy value_cid_reg (safe because we are finished with it).
-        __ movl(value_cid_reg,
-                FieldAddress(value_reg, Array::length_offset()));
-        __ movl(field_length_operand, value_cid_reg);
-      } else if (RawObject::IsTypedDataClassId(value_cid)) {
-        // Destroy value_cid_reg (safe because we are finished with it).
-        __ movl(value_cid_reg,
-                FieldAddress(value_reg, TypedData::length_offset()));
-        __ movl(field_length_operand, value_cid_reg);
-      } else {
-        __ movl(field_length_operand, Immediate(Field::kNoFixedLength));
+      if (field_has_length) {
+        if ((value_cid == kArrayCid) || (value_cid == kImmutableArrayCid)) {
+          // Destroy value_cid_reg (safe because we are finished with it).
+          __ movl(value_cid_reg,
+                  FieldAddress(value_reg, Array::length_offset()));
+          __ movl(field_length_operand, value_cid_reg);
+        } else if (RawObject::IsTypedDataClassId(value_cid)) {
+          // Destroy value_cid_reg (safe because we are finished with it).
+          __ movl(value_cid_reg,
+                  FieldAddress(value_reg, TypedData::length_offset()));
+          __ movl(field_length_operand, value_cid_reg);
+        } else {
+          __ movl(field_length_operand,
+                  Immediate(Smi::RawValue(Field::kNoFixedLength)));
+        }
       }
     }
 
     if (!ok_is_fall_through) {
       __ jmp(&ok);
     }
   } else {
     // Field guard class has been initialized and is known.
-
     if (field_reg != kNoRegister) {
       __ LoadObject(field_reg, Field::ZoneHandle(field().raw()));
     }
 
     if (value_cid == kDynamicCid) {
       // Value's class id is not known.
       __ testl(value_reg, Immediate(kSmiTagMask));
 
       if (field_cid != kSmiCid) {
         __ j(ZERO, fail);
@@ skipping 3441 matching lines @@
                          PcDescriptors::kOther,
                          locs());
   __ Drop(2);  // Discard type arguments and receiver.
 }
 
 }  // namespace dart
 
 #undef __
 
 #endif  // defined TARGET_ARCH_IA32