Merged: [compiler] Fix bad store because of concurrently finish slack…

… tracking

Here are the steps that lead to the bug:
  - main thread: map `a` was being slack-tracked
  - background: a compilation job serializes `a` into a MapRef `aRef`
  - main thread: slack tracking finished for this map.
  - main thread: a store to an object of map `a` created a transition
    from map `a` to map `b`, and the property stored was stored as the
    1st item of the out-of-object properties.
  - background: compilation reached JSNativeContextSpecialization,
    which tried to optimize a JSSetNamedProperty (specifically, the
    same operation that lead to the map transition on the main
    thread). There was no feedback for this operation since it hadn't
    been executed before (otherwise, the map transition would have had
    happened before, and the MapRef would not have been out of date).
    JSNativeCtxtSpec inferred maps of the receiver from previous
    CheckMaps, and realized that the store was transitioning (from `a`
    to `b`). It looked at the MapRef `aRef` to see how much unused
    properties the object had. `aRef` still had the cached
    slack-tracking data, and thus thought that it still had unused
    properties, whereas in reality, `a` didn't have any left, and a
    new property backing store should have been allocated.
  - main thread: when executing the store generated, we tried to write
    to the 1st item of the out-of-object properties of an object
    with map `a`, which was the EmptyFixedArray root, which caused a
    segfault, since this is in read-only space.

The fix is to add a compilation dependency for map slack-tracking when
deciding to extend (or not) the property backing store of an object.
At the end of compilation, if the construction_counter of the Map is 0
and the one of the MapRef is non-0, then slack tracking finished
during compilation, and we discard the optimized code.

While fixing this, I also found out that UnusedPropertyFields and
construction_counter were sometimes incoherent in the background,
because CSA was updating construction_counter without taking the
map_updater_access mutex (which means that when construction_counter
was 0 in the background, it wasn't always safe to look at
UnusedPropertyFields, since it could contain the old value).
Similarly, MapRef::IsInobjectSlackTrackingInProgress was looking at
the Map rather than the cached value for construction_counter, which
means that it could also be out of sync with UnusedPropertyFields.

Bug: chromium:1444366
(cherry picked from commit 7effdbf)

Change-Id: I6005ccf87b3bffdcf5a21c49afe4a5abc0c05789
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4667386
Bot-Commit: Rubber Stamper <[email protected]>
Reviewed-by: Jakob Kummerow <[email protected]>
Commit-Queue: Darius Mercadier <[email protected]>
Cr-Commit-Position: refs/branch-heads/11.5@{v8#29}
Cr-Branched-From: 0c4044b-refs/heads/11.5.150@{#1}
Cr-Branched-From: b71d303-refs/heads/main@{#87781}

src/codegen/code-stub-assembler.cc

@@ -4152,16 +4152,14 @@ void CodeStubAssembler::InitializeJSObjectBodyWithSlackTracking(
     TNode<Word32T> new_bit_field3 = Int32Sub(
         bit_field3,
         Int32Constant(1 << Map::Bits3::ConstructionCounterBits::kShift));
-    StoreObjectFieldNoWriteBarrier(map, Map::kBitField3Offset, new_bit_field3);
-    static_assert(Map::kSlackTrackingCounterEnd == 1);
 
     // The object still has in-object slack therefore the |unsed_or_unused|
     // field contain the "used" value.
     TNode<IntPtrT> used_size =
         Signed(TimesTaggedSize(ChangeUint32ToWord(LoadObjectField<Uint8T>(
             map, Map::kUsedOrUnusedInstanceSizeInWordsOffset))));
 
-    Comment("iInitialize filler fields");
+    Comment("Initialize filler fields");
     InitializeFieldsWithRoot(object, used_size, instance_size,
                              RootIndex::kOnePointerFillerMap);
 
@@ -4172,6 +4170,14 @@ void CodeStubAssembler::InitializeJSObjectBodyWithSlackTracking(
     static_assert(Map::kNoSlackTracking == 0);
     GotoIf(IsClearWord32<Map::Bits3::ConstructionCounterBits>(new_bit_field3),
            &complete);
+
+    // Setting ConstructionCounterBits to 0 requires taking the
+    // map_updater_access mutex, which we can't do from CSA, so we only manually
+    // update ConstructionCounterBits when its result is non-zero; otherwise we
+    // let the runtime do it (with the GotoIf right above this comment).
+    StoreObjectFieldNoWriteBarrier(map, Map::kBitField3Offset, new_bit_field3);
+    static_assert(Map::kSlackTrackingCounterEnd == 1);
+
     Goto(&end);
   }
 

src/compiler/compilation-dependencies.cc

@@ -29,6 +29,7 @@ namespace compiler {
   V(GlobalProperty)                     \
   V(InitialMap)                         \
   V(InitialMapInstanceSizePrediction)   \
+  V(NoSlackTrackingChange)              \
   V(OwnConstantDataProperty)            \
   V(OwnConstantDictionaryProperty)      \
   V(OwnConstantElement)                 \
@@ -994,6 +995,41 @@ class OwnConstantElementDependency final : public CompilationDependency {
   const ObjectRef element_;
 };
 
+class NoSlackTrackingChangeDependency final : public CompilationDependency {
+ public:
+  explicit NoSlackTrackingChangeDependency(MapRef map)
+      : CompilationDependency(kNoSlackTrackingChange), map_(map) {}
+
+  bool IsValid(JSHeapBroker* broker) const override {
+    if (map_.construction_counter() != 0 &&
+        map_.object()->construction_counter() == 0) {
+      // Slack tracking finished during compilation.
+      return false;
+    }
+    return map_.UnusedPropertyFields() ==
+               map_.object()->UnusedPropertyFields() &&
+           map_.GetInObjectProperties() ==
+               map_.object()->GetInObjectProperties();
+  }
+
+  void PrepareInstall(JSHeapBroker*) const override {}
+  void Install(JSHeapBroker*, PendingDependencies*) const override {}
+
+ private:
+  size_t Hash() const override {
+    ObjectRef::Hash h;
+    return base::hash_combine(h(map_));
+  }
+
+  bool Equals(const CompilationDependency* that) const override {
+    const NoSlackTrackingChangeDependency* const zat =
+        that->AsNoSlackTrackingChange();
+    return map_.equals(zat->map_);
+  }
+
+  const MapRef map_;
+};
+
 class InitialMapInstanceSizePredictionDependency final
     : public CompilationDependency {
  public:
@@ -1351,6 +1387,12 @@ void CompilationDependencies::DependOnConsistentJSFunctionView(
   RecordDependency(zone_->New<ConsistentJSFunctionViewDependency>(function));
 }
 
+void CompilationDependencies::DependOnNoSlackTrackingChange(MapRef map) {
+  if (map.construction_counter() == 0) return;
+  RecordDependency(zone_->New<NoSlackTrackingChangeDependency>(map));
+  return;
+}
+
 SlackTrackingPrediction::SlackTrackingPrediction(MapRef initial_map,
                                                  int instance_size)
     : instance_size_(instance_size),

src/compiler/compilation-dependencies.h

@@ -44,6 +44,12 @@ class V8_EXPORT_PRIVATE CompilationDependencies : public ZoneObject {
   // Record the assumption that {map} stays stable.
   void DependOnStableMap(MapRef map);
 
+  // Record the assumption that slack tracking for {map} doesn't change during
+  // compilation. This gives no guarantees about slack tracking changes after
+  // the compilation is finished (ie, it Validates the dependency, but doesn't
+  // Install anything).
+  void DependOnNoSlackTrackingChange(MapRef map);
+
   // Depend on the fact that accessing property |property_name| from
   // |receiver_map| yields the constant value |constant|, which is held by
   // |holder|. Therefore, must be invalidated if |property_name| is added to any

src/compiler/heap-refs.cc

@@ -1493,6 +1493,8 @@ HEAP_ACCESSOR_B(Map, bit_field3, NumberOfOwnDescriptors,
                 Map::Bits3::NumberOfOwnDescriptorsBits)
 HEAP_ACCESSOR_B(Map, bit_field3, is_migration_target,
                 Map::Bits3::IsMigrationTargetBit)
+BIMODAL_ACCESSOR_B(Map, bit_field3, construction_counter,
+                   Map::Bits3::ConstructionCounterBits)
 HEAP_ACCESSOR_B(Map, bit_field, has_prototype_slot,
                 Map::Bits1::HasPrototypeSlotBit)
 HEAP_ACCESSOR_B(Map, bit_field, is_access_check_needed,
@@ -1682,7 +1684,7 @@ void* JSTypedArrayRef::data_ptr() const {
 }
 
 bool MapRef::IsInobjectSlackTrackingInProgress() const {
-  return object()->IsInobjectSlackTrackingInProgress();
+  return construction_counter() != Map::kNoSlackTracking;
 }
 
 int MapRef::constructor_function_index() const {

src/compiler/heap-refs.h

@@ -813,6 +813,7 @@ class V8_EXPORT_PRIVATE MapRef : public HeapObjectRef {
   bool is_undetectable() const;
   bool is_callable() const;
   bool has_indexed_interceptor() const;
+  int construction_counter() const;
   bool is_migration_target() const;
   bool supports_fast_array_iteration(JSHeapBroker* broker) const;
   bool supports_fast_array_resize(JSHeapBroker* broker) const;

src/compiler/js-native-context-specialization.cc

@@ -3052,6 +3052,16 @@ JSNativeContextSpecialization::BuildPropertyStore(
       // with this transitioning store.
       MapRef transition_map_ref = transition_map.value();
       MapRef original_map = transition_map_ref.GetBackPointer(broker()).AsMap();
+      if (!field_index.is_inobject()) {
+        // If slack tracking ends after this compilation started but before it's
+        // finished, then we could {original_map} could be out-of-sync with
+        // {transition_map_ref}. In particular, its UnusedPropertyFields could
+        // be non-zero, which would lead us to not extend the property backing
+        // store, while the underlying Map has actually zero
+        // UnusedPropertyFields. Thus, we install a dependency on {orininal_map}
+        // now, so that if such a situation happens, we'll throw away the code.
+        dependencies()->DependOnNoSlackTrackingChange(original_map);
+      }
       if (original_map.UnusedPropertyFields() == 0) {
         DCHECK(!field_index.is_inobject());
 
@@ -3882,8 +3892,7 @@ Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore(
   // difficult for escape analysis to get rid of the backing stores used
   // for intermediate states of chains of property additions. That makes
   // it unclear what the best approach is here.
-  DCHECK_EQ(0, map.UnusedPropertyFields());
-  // Compute the length of the old {properties} and the new properties.
+  DCHECK_EQ(map.UnusedPropertyFields(), 0);
   int length = map.NextFreePropertyIndex() - map.GetInObjectProperties();
   int new_length = length + JSObject::kFieldsAdded;
   // Collect the field values from the {properties}.