[compiler] Improve GetOwnFastDataPropertyFromHeap representation check

Added a parameter to Object::FitsRepresentation() to disallow coercion.
Normally, when we ask if a Smi can "fit" into a Double representation
we'd answer yes, because the Smi can be converted to a HeapNumber.
However, from the compilers perspective, the object is found in a
field with a particular representation. In this case, finding a
Smi in a field with representation Double means something is awry.
Therefore, it's useful for the compiler to be able to ask if
the object fits the field without coercion.

Bug: chromium:1227324, v8:7790
Change-Id: I12033736030d904ef9c29516c07999600a5f508a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3015570
Commit-Queue: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75706}

src/compiler/heap-refs.cc

@@ -514,16 +514,17 @@ base::Optional<ObjectRef> GetOwnFastDataPropertyFromHeap(
     // {constant} needs to pass the gc predicate before we can introspect on it.
     if (broker->ObjectMayBeUninitialized(constant.value())) return {};
 
-    // Since we don't have a guarantee that {constant} is the correct value of
-    // the property, we use the expected {representation} to weed out the most
-    // egregious types of wrong values.
-    Representation constant_representation =
-        constant->OptimalRepresentation(broker->isolate());
-    if (!constant_representation.CanBeInPlaceChangedTo(representation)) {
-      TRACE_BROKER_MISSING(broker,
-                           "Mismatched representation for "
-                               << holder << ". Expected " << representation
-                               << ", have: " << constant_representation);
+    // Ensure that {constant} matches the {representation} we expect for the
+    // field.
+    if (!constant->FitsRepresentation(representation, false)) {
+      const char* repString =
+          constant->IsSmi()
+              ? "Smi"
+              : constant->IsHeapNumber() ? "HeapNumber" : "HeapObject";
+      TRACE_BROKER_MISSING(broker, "Mismatched representation for "
+                                       << holder << ". Expected "
+                                       << representation << ", but object is a "
+                                       << repString);
       return {};
     }
   }

src/objects/objects-inl.h

@@ -497,11 +497,12 @@ ElementsKind Object::OptimalElementsKind(PtrComprCageBase cage_base) const {
   return PACKED_ELEMENTS;
 }
 
-bool Object::FitsRepresentation(Representation representation) {
+bool Object::FitsRepresentation(Representation representation,
+                                bool allow_coercion) const {
   if (FLAG_track_fields && representation.IsSmi()) {
     return IsSmi();
   } else if (FLAG_track_double_fields && representation.IsDouble()) {
-    return IsNumber();
+    return allow_coercion ? IsNumber() : IsHeapNumber();
   } else if (FLAG_track_heap_object_fields && representation.IsHeapObject()) {
     return IsHeapObject();
   } else if (FLAG_track_fields && representation.IsNone()) {

src/objects/objects.h

@@ -327,7 +327,11 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
 
   inline ElementsKind OptimalElementsKind(PtrComprCageBase cage_base) const;
 
-  inline bool FitsRepresentation(Representation representation);
+  // If {allow_coercion} is true, then a Smi will be considered to fit
+  // a Double representation, since it can be converted to a HeapNumber
+  // and stored.
+  inline bool FitsRepresentation(Representation representation,
+                                 bool allow_coercion = true) const;
 
   inline bool FilterKey(PropertyFilter filter);
 

test/cctest/test-field-type-tracking.cc

@@ -3145,6 +3145,48 @@ TEST(DeletePropertyGeneralizesConstness) {
   }
 }
 
+#define CHECK_SAME(object, rep, expected)           \
+  CHECK_EQ(object->FitsRepresentation(rep, true),   \
+           object->FitsRepresentation(rep, false)); \
+  CHECK_EQ(object->FitsRepresentation(rep, true), expected)
+
+TEST(CheckFitsRepresentationPredicate) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+  i::Factory* factory = CcTest::i_isolate()->factory();
+
+  Handle<Smi> smi_value = factory->last_script_id();
+  Handle<HeapNumber> double_value = factory->nan_value();
+  Handle<OrderedHashMap> heapobject_value = factory->empty_ordered_hash_map();
+
+  Representation rep_smi = Representation::Smi();
+  Representation rep_double = Representation::Double();
+  Representation rep_heapobject = Representation::HeapObject();
+  Representation rep_tagged = Representation::Tagged();
+
+  // Verify the behavior of Object::FitsRepresentation() with and
+  // without coercion. A Smi can be "coerced" into a Double
+  // representation by converting it to a HeapNumber. If coercion is
+  // disallowed, that query should fail.
+  CHECK_SAME(smi_value, rep_smi, true);
+  CHECK_EQ(smi_value->FitsRepresentation(rep_double, true), true);
+  CHECK_EQ(smi_value->FitsRepresentation(rep_double, false), false);
+  CHECK_SAME(smi_value, rep_heapobject, false);
+  CHECK_SAME(smi_value, rep_tagged, true);
+
+  CHECK_SAME(double_value, rep_smi, false);
+  CHECK_SAME(double_value, rep_double, true);
+  CHECK_SAME(double_value, rep_heapobject, true);
+  CHECK_SAME(double_value, rep_tagged, true);
+
+  CHECK_SAME(heapobject_value, rep_smi, false);
+  CHECK_SAME(heapobject_value, rep_double, false);
+  CHECK_SAME(heapobject_value, rep_heapobject, true);
+  CHECK_SAME(heapobject_value, rep_tagged, true);
+}
+
+#undef CHECK_SAME
+
 }  // namespace test_field_type_tracking
 }  // namespace compiler
 }  // namespace internal

test/mjsunit/compiler/regress-1227324.js

@@ -0,0 +1,23 @@
+// Copyright 2021 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.
+
+// Flags: --allow-natives-syntax --concurrent-inlining
+
+(function() {
+  var use_symbol = {
+    [Symbol.hasInstance] : () => true
+  };
+  var use_symbol_double = {
+    [Symbol.hasInstance] : 0.1
+  };
+  function bar(b) {
+    if (b) return {} instanceof use_symbol_double;
+  }
+  %PrepareFunctionForOptimization(bar);
+  function foo(b) { return bar(); }
+  %PrepareFunctionForOptimization(foo);
+  foo();
+  %OptimizeFunctionOnNextCall(foo);
+  foo();
+})();