[ext-code-space] Fix Code vs non-Code comparisons

When external code space is enabled comparing Code and non-Code objects
by looking at compressed values is not always correct. Such an approach
works only for comparing Code vs Code objects or non-Code vs non-Code
objects.

This CL instroduces SLOW_DCHECK into Object comparison operators to
ensure that such a comparison is allowed. Also, this CL instroduces
an Object::SafeEquals() method which compares uncompressed values
and thus is safe to be used for comparing Code with non-Code objects.

Bug: v8:11880
Change-Id: I7ccf1f90f927beb2bb9f45efb303e902b1838d02
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3838172Reviewed-by: Jakob Linke <jgruber@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82611}

src/codegen/compiler.cc

@@ -297,8 +297,7 @@ void Compiler::LogFunctionCompilation(Isolate* isolate,
                                       Handle<FeedbackVector> vector,
                                       Handle<AbstractCode> abstract_code,
                                       CodeKind kind, double time_taken_ms) {
-  DCHECK(!abstract_code.is_null());
-  DCHECK(!abstract_code.is_identical_to(BUILTIN_CODE(isolate, CompileLazy)));
+  DCHECK_NE(*abstract_code, *BUILTIN_CODE(isolate, CompileLazy));
   if (!V8_REMOVE_BUILTINS_CODE_OBJECTS && V8_EXTERNAL_CODE_SPACE_BOOL) {
     DCHECK_NE(*abstract_code, FromCodeT(*BUILTIN_CODE(isolate, CompileLazy)));
   }

src/heap/object-stats.cc

@@ -451,7 +451,8 @@ class ObjectStatsCollectorImpl {
   Heap* heap_;
   ObjectStats* stats_;
   NonAtomicMarkingState* marking_state_;
-  std::unordered_set<HeapObject, Object::Hasher> virtual_objects_;
+  std::unordered_set<HeapObject, Object::Hasher, Object::KeyEqualSafe>
+      virtual_objects_;
   std::unordered_set<Address> external_resources_;
   FieldStatsCollector field_stats_collector_;
 };
@@ -475,7 +476,7 @@ bool ObjectStatsCollectorImpl::ShouldRecordObject(HeapObject obj,
     bool cow_check = check_cow_array == kIgnoreCow || !IsCowArray(fixed_array);
     return CanRecordFixedArray(fixed_array) && cow_check;
   }
-  if (obj == ReadOnlyRoots(heap_).empty_property_array()) return false;
+  if (obj.SafeEquals(ReadOnlyRoots(heap_).empty_property_array())) return false;
   return true;
 }
 

src/heap/reference-summarizer.cc

@@ -77,9 +77,8 @@ class ReferenceSummarizerMarkingState final {
   WeakObjects::Local* local_weak_objects() { return &local_weak_objects_; }
 
  private:
-  void AddReference(
-      HeapObject host, HeapObject obj,
-      std::unordered_set<HeapObject, Object::Hasher>& references) {
+  void AddReference(HeapObject host, HeapObject obj,
+                    ReferenceSummary::UnorderedHeapObjectSet& references) {
     // It's possible that the marking visitor handles multiple objects at once,
     // such as a Map and its DescriptorArray, but we're only interested in
     // references from the primary object.

src/heap/reference-summarizer.h

@@ -26,17 +26,16 @@ class ReferenceSummary {
   // behavior. Intended only for verification.
   static ReferenceSummary SummarizeReferencesFrom(Heap* heap, HeapObject obj);
 
+  using UnorderedHeapObjectSet =
+      std::unordered_set<HeapObject, Object::Hasher, Object::KeyEqualSafe>;
+
   // All objects which the chosen object has strong pointers to.
-  std::unordered_set<HeapObject, Object::Hasher>& strong_references() {
-    return strong_references_;
-  }
+  UnorderedHeapObjectSet& strong_references() { return strong_references_; }
 
   // All objects which the chosen object has weak pointers to. The values in
   // ephemeron hash tables are also included here, even though they aren't
   // normal weak pointers.
-  std::unordered_set<HeapObject, Object::Hasher>& weak_references() {
-    return weak_references_;
-  }
+  UnorderedHeapObjectSet& weak_references() { return weak_references_; }
 
   void Clear() {
     strong_references_.clear();
@@ -44,8 +43,8 @@ class ReferenceSummary {
   }
 
  private:
-  std::unordered_set<HeapObject, Object::Hasher> strong_references_;
-  std::unordered_set<HeapObject, Object::Hasher> weak_references_;
+  UnorderedHeapObjectSet strong_references_;
+  UnorderedHeapObjectSet weak_references_;
   DISALLOW_GARBAGE_COLLECTION(no_gc)
 };
 

src/logging/log.cc

@@ -2334,8 +2334,10 @@ void ExistingCodeLogger::LogCompiledFunctions() {
                                           shared->baseline_code(kAcquireLoad))),
                                       isolate_));
     }
-    if (pair.second.is_identical_to(BUILTIN_CODE(isolate_, CompileLazy)))
-      continue;
+    // Can't use .is_identical_to() because AbstractCode might be both Code and
+    // non-Code object and regular tagged comparison or compressed values might
+    // not be correct when V8_EXTERNAL_CODE_SPACE is enabled.
+    if (*pair.second == *BUILTIN_CODE(isolate_, CompileLazy)) continue;
     LogExistingFunction(pair.first, pair.second);
   }
 

src/objects/code.h

@@ -1093,10 +1093,16 @@ class AbstractCode : public HeapObject {
   inline CodeT GetCodeT();
   inline BytecodeArray GetBytecodeArray();
 
-  OBJECT_CONSTRUCTORS(AbstractCode, HeapObject);
+  // AbstractCode might be represented by both Code and non-Code objects and
+  // thus regular comparison of tagged values might not be correct when
+  // V8_EXTERNAL_CODE_SPACE is enabled. SafeEquals() must be used instead.
+  constexpr bool operator==(Object other) const { return SafeEquals(other); }
+  constexpr bool operator!=(Object other) const { return !SafeEquals(other); }
 
  private:
   inline ByteArray SourcePositionTableInternal(PtrComprCageBase cage_base);
+
+  OBJECT_CONSTRUCTORS(AbstractCode, HeapObject);
 };
 
 // Dependent code is conceptually the list of {Code, DependencyGroup} tuples

src/objects/objects-inl.h

@@ -102,7 +102,7 @@ IS_TYPE_FUNCTION_DEF(CodeT)
     return Is##Type(ReadOnlyRoots(isolate));                     \
   }                                                              \
   bool Object::Is##Type(ReadOnlyRoots roots) const {             \
-    return *this == roots.Value();                               \
+    return SafeEquals(roots.Value());                            \
   }                                                              \
   bool Object::Is##Type() const {                                \
     return IsHeapObject() && HeapObject::cast(*this).Is##Type(); \

src/objects/objects.h

@@ -681,6 +681,14 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
     }
   };
 
+  // For use with std::unordered_set/unordered_map when using both Code and
+  // non-Code objects as keys.
+  struct KeyEqualSafe {
+    bool operator()(const Object a, const Object b) const {
+      return a.SafeEquals(b);
+    }
+  };
+
   // For use with std::map.
   struct Comparer {
     bool operator()(const Object a, const Object b) const { return a < b; }

src/objects/tagged-impl.cc

@@ -10,9 +10,30 @@
 #include "src/strings/string-stream.h"
 #include "src/utils/ostreams.h"
 
+#ifdef V8_EXTERNAL_CODE_SPACE
+// For IsCodeSpaceObject().
+#include "src/heap/heap-write-barrier-inl.h"
+#endif
+
 namespace v8 {
 namespace internal {
 
+#ifdef V8_EXTERNAL_CODE_SPACE
+bool CheckObjectComparisonAllowed(Address a, Address b) {
+  if (!HAS_STRONG_HEAP_OBJECT_TAG(a) || !HAS_STRONG_HEAP_OBJECT_TAG(b)) {
+    return true;
+  }
+  HeapObject obj_a = HeapObject::unchecked_cast(Object(a));
+  HeapObject obj_b = HeapObject::unchecked_cast(Object(b));
+  // This check might fail when we try to compare Code object with non-Code
+  // object. The main legitimate case when such "mixed" comparison could happen
+  // is comparing two AbstractCode objects. If that's the case one must use
+  // AbstractCode's == operator instead of Object's one or SafeEquals().
+  CHECK_EQ(IsCodeSpaceObject(obj_a), IsCodeSpaceObject(obj_b));
+  return true;
+}
+#endif  // V8_EXTERNAL_CODE_SPACE
+
 template <HeapObjectReferenceType kRefType, typename StorageType>
 void TaggedImpl<kRefType, StorageType>::ShortPrint(FILE* out) {
   OFStream os(out);

src/objects/tagged-impl.h

@@ -6,11 +6,19 @@
 #define V8_OBJECTS_TAGGED_IMPL_H_
 
 #include "include/v8-internal.h"
+#include "src/common/checks.h"
 #include "src/common/globals.h"
 
 namespace v8 {
 namespace internal {
 
+#ifdef V8_EXTERNAL_CODE_SPACE
+// When V8_EXTERNAL_CODE_SPACE is enabled comparing Code and non-Code objects
+// by looking only at compressed values it not correct.
+// Full pointers must be compared instead.
+bool V8_EXPORT_PRIVATE CheckObjectComparisonAllowed(Address a, Address b);
+#endif
+
 // An TaggedImpl is a base class for Object (which is either a Smi or a strong
 // reference to a HeapObject) and MaybeObject (which is either a Smi, a strong
 // reference to a HeapObject, a weak reference to a HeapObject, or a cleared
@@ -44,6 +52,13 @@ class TaggedImpl {
     static_assert(
         std::is_same<U, Address>::value || std::is_same<U, Tagged_t>::value,
         "U must be either Address or Tagged_t");
+#ifdef V8_EXTERNAL_CODE_SPACE
+    // When comparing two full pointer values ensure that it's allowed.
+    if (std::is_same<StorageType, Address>::value &&
+        std::is_same<U, Address>::value) {
+      SLOW_DCHECK(CheckObjectComparisonAllowed(ptr_, other.ptr()));
+    }
+#endif  // V8_EXTERNAL_CODE_SPACE
     return static_cast<Tagged_t>(ptr_) == static_cast<Tagged_t>(other.ptr());
   }
   template <typename U>
@@ -51,11 +66,34 @@ class TaggedImpl {
     static_assert(
         std::is_same<U, Address>::value || std::is_same<U, Tagged_t>::value,
         "U must be either Address or Tagged_t");
+#ifdef V8_EXTERNAL_CODE_SPACE
+    // When comparing two full pointer values ensure that it's allowed.
+    if (std::is_same<StorageType, Address>::value &&
+        std::is_same<U, Address>::value) {
+      SLOW_DCHECK(CheckObjectComparisonAllowed(ptr_, other.ptr()));
+    }
+#endif  // V8_EXTERNAL_CODE_SPACE
     return static_cast<Tagged_t>(ptr_) != static_cast<Tagged_t>(other.ptr());
   }
+  // A variant of operator== which allows comparing Code object with non-Code
+  // objects even if the V8_EXTERNAL_CODE_SPACE is enabled.
+  constexpr bool SafeEquals(TaggedImpl other) const {
+    static_assert(std::is_same<StorageType, Address>::value,
+                  "Safe comparison is allowed only for full tagged values");
+    if (V8_EXTERNAL_CODE_SPACE_BOOL) {
+      return ptr_ == other.ptr();
+    }
+    return this->operator==(other);
+  }
 
   // For using in std::set and std::map.
   constexpr bool operator<(TaggedImpl other) const {
+#ifdef V8_EXTERNAL_CODE_SPACE
+    // When comparing two full pointer values ensure that it's allowed.
+    if (std::is_same<StorageType, Address>::value) {
+      SLOW_DCHECK(CheckObjectComparisonAllowed(ptr_, other.ptr()));
+    }
+#endif  // V8_EXTERNAL_CODE_SPACE
     return static_cast<Tagged_t>(ptr_) < static_cast<Tagged_t>(other.ptr());
   }
 

src/profiler/heap-snapshot-generator.cc

@@ -78,7 +78,7 @@ class HeapEntryVerifier {
     // property getter that bypasses the property array and accessor info. At
     // this point, we must check for those indirect references.
     for (size_t level = 0; level < 3; ++level) {
-      const std::unordered_set<HeapObject, Object::Hasher>& indirect =
+      const UnorderedHeapObjectSet& indirect =
           GetIndirectStrongReferences(level);
       if (indirect.find(target) != indirect.end()) {
         return;
@@ -132,14 +132,16 @@ class HeapEntryVerifier {
   }
 
  private:
-  const std::unordered_set<HeapObject, Object::Hasher>&
-  GetIndirectStrongReferences(size_t level) {
+  using UnorderedHeapObjectSet =
+      std::unordered_set<HeapObject, Object::Hasher, Object::KeyEqualSafe>;
+
+  const UnorderedHeapObjectSet& GetIndirectStrongReferences(size_t level) {
     CHECK_GE(indirect_strong_references_.size(), level);
 
     if (indirect_strong_references_.size() == level) {
       // Expansion is needed.
       indirect_strong_references_.resize(level + 1);
-      const std::unordered_set<HeapObject, Object::Hasher>& previous =
+      const UnorderedHeapObjectSet& previous =
           level == 0 ? reference_summary_.strong_references()
                      : indirect_strong_references_[level - 1];
       for (HeapObject obj : previous) {
@@ -183,8 +185,7 @@ class HeapEntryVerifier {
   // Objects transitively retained by the primary object. The objects in the set
   // at index i are retained by the primary object via a chain of i+1
   // intermediate objects.
-  std::vector<std::unordered_set<HeapObject, Object::Hasher>>
-      indirect_strong_references_;
+  std::vector<UnorderedHeapObjectSet> indirect_strong_references_;
 };
 #endif
 
@@ -2124,10 +2125,15 @@ bool V8HeapExplorer::IterateAndExtractReferences(
 }
 
 bool V8HeapExplorer::IsEssentialObject(Object object) {
+  if (!object.IsHeapObject()) return false;
+  // Avoid comparing Code objects with non-Code objects below.
+  if (V8_EXTERNAL_CODE_SPACE_BOOL &&
+      IsCodeSpaceObject(HeapObject::cast(object))) {
+    return true;
+  }
   Isolate* isolate = heap_->isolate();
   ReadOnlyRoots roots(isolate);
-  return object.IsHeapObject() && !object.IsOddball(isolate) &&
-         object != roots.empty_byte_array() &&
+  return !object.IsOddball(isolate) && object != roots.empty_byte_array() &&
          object != roots.empty_fixed_array() &&
          object != roots.empty_weak_fixed_array() &&
          object != roots.empty_descriptor_array() &&

src/snapshot/serializer.cc

@@ -417,7 +417,7 @@ void Serializer::ObjectSerializer::SerializePrologue(SnapshotSpace space,
         CodeNameEvent(object_->address(), sink_->Position(), code_name));
   }
 
-  if (map == *object_) {
+  if (map.SafeEquals(*object_)) {
     DCHECK_EQ(*object_, ReadOnlyRoots(isolate()).meta_map());
     DCHECK_EQ(space, SnapshotSpace::kReadOnlyHeap);
     sink_->Put(kNewMetaMap, "NewMetaMap");
@@ -907,10 +907,11 @@ void Serializer::ObjectSerializer::VisitPointers(HeapObject host,
       if (repeat_end < end &&
           serializer_->root_index_map()->Lookup(*obj, &root_index) &&
           RootsTable::IsImmortalImmovable(root_index) &&
-          *current == *repeat_end) {
+          current.load(cage_base) == repeat_end.load(cage_base)) {
         DCHECK_EQ(reference_type, HeapObjectReferenceType::STRONG);
         DCHECK(!Heap::InYoungGeneration(*obj));
-        while (repeat_end < end && *repeat_end == *current) {
+        while (repeat_end < end &&
+               repeat_end.load(cage_base) == current.load(cage_base)) {
           repeat_end++;
         }
         int repeat_count = static_cast<int>(repeat_end - current);

test/cctest/heap/test-heap.cc

@@ -653,8 +653,8 @@ TEST(BytecodeArray) {
   }
 
   // Constant pool should have been migrated.
-  CHECK_EQ(array->constant_pool(), *constant_pool);
-  CHECK_NE(array->constant_pool(), old_constant_pool_address);
+  CHECK_EQ(array->constant_pool().ptr(), constant_pool->ptr());
+  CHECK_NE(array->constant_pool().ptr(), old_constant_pool_address.ptr());
 }
 
 TEST(BytecodeArrayAging) {
@@ -3869,8 +3869,8 @@ TEST(LargeObjectSlotRecording) {
 
   // Verify that the pointers in the large object got updated.
   for (int i = 0; i < size; i += kStep) {
-    CHECK_EQ(lo->get(i), *lit);
-    CHECK(lo->get(i) != old_location);
+    CHECK_EQ(lo->get(i).ptr(), lit->ptr());
+    CHECK_NE(lo->get(i).ptr(), old_location.ptr());
   }
 }
 
@@ -6401,7 +6401,7 @@ TEST(RememberedSet_OldToOld) {
   // This GC pass will evacuate the page with 'arr'/'ref' so it will have to
   // create OLD_TO_OLD remembered set to track the reference.
   CcTest::CollectAllGarbage();
-  CHECK_NE(prev_location, *arr);
+  CHECK_NE(prev_location.ptr(), arr->ptr());
 }
 
 TEST(RememberedSetRemoveRange) {

test/cctest/heap/test-iterators.cc

@@ -77,7 +77,7 @@ TEST(HeapObjectIterator) {
        obj = iterator.Next()) {
     CHECK_IMPLIES(!FLAG_enable_third_party_heap, !ReadOnlyHeap::Contains(obj));
     CHECK(CcTest::heap()->Contains(obj));
-    if (sample_object == obj) seen_sample_object = true;
+    if (sample_object.SafeEquals(obj)) seen_sample_object = true;
   }
   CHECK(seen_sample_object);
 }
@@ -92,7 +92,7 @@ TEST(CombinedHeapObjectIterator) {
   for (HeapObject obj = iterator.Next(); !obj.is_null();
        obj = iterator.Next()) {
     CHECK(IsValidHeapObject(CcTest::heap(), obj));
-    if (sample_object == obj) seen_sample_object = true;
+    if (sample_object.SafeEquals(obj)) seen_sample_object = true;
   }
   CHECK(seen_sample_object);
 }