[const-tracking] Mark const field as mutable when reconfiguring

... to different attributes or different property kind.

Bug: chromium:1161847, v8:9233
Change-Id: I5a6e1e012c6afcf09ed9da6bbf9f33c1007c3d99
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2727272
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73220}

src/objects/map-updater.cc

@@ -125,6 +125,37 @@ Handle<Map> MapUpdater::ReconfigureToDataField(InternalIndex descriptor,
   PropertyDetails old_details =
       old_descriptors_->GetDetails(modified_descriptor_);
 
+  // If the {descriptor} was "const" data field so far, we need to update the
+  // {old_map_} here, otherwise we could get the constants wrong, i.e.
+  //
+  //   o.x = 1;
+  //   change o.x's attributes to something else
+  //   delete o.x;
+  //   o.x = 2;
+  //
+  // could trick V8 into thinking that `o.x` is still 1 even after the second
+  // assignment.
+  // This situation is similar to what might happen with property deletion.
+  if (old_details.constness() == PropertyConstness::kConst &&
+      old_details.location() == kField &&
+      old_details.attributes() != new_attributes_) {
+    Handle<FieldType> field_type(
+        old_descriptors_->GetFieldType(modified_descriptor_), isolate_);
+    Map::GeneralizeField(isolate_, old_map_, descriptor,
+                         PropertyConstness::kMutable,
+                         old_details.representation(), field_type);
+    // The old_map_'s property must become mutable.
+    DCHECK_EQ(PropertyConstness::kMutable,
+              old_descriptors_->GetDetails(modified_descriptor_).constness());
+    // Although the property in the old map is marked as mutable we still
+    // treat it as constant when merging with the new path in transition tree.
+    // This is fine because up until this reconfiguration the field was
+    // known to be constant, so it's fair to proceed treating it as such
+    // during this reconfiguration session. The issue is that after the
+    // reconfiguration the original field might become mutable (see the delete
+    // example above).
+  }
+
   // If property kind is not reconfigured merge the result with
   // representation/field type from the old descriptor.
   if (old_details.kind() == new_kind_) {

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

@@ -1066,20 +1066,31 @@ void TestReconfigureDataFieldAttribute_GeneralizeField(
   Handle<Code> code_field_type = CreateDummyOptimizedCode(isolate);
   Handle<Code> code_field_repr = CreateDummyOptimizedCode(isolate);
   Handle<Code> code_field_const = CreateDummyOptimizedCode(isolate);
-  Handle<Map> field_owner(
-      map->FindFieldOwner(isolate, InternalIndex(kSplitProp)), isolate);
-  DependentCode::InstallDependency(isolate,
-                                   MaybeObjectHandle::Weak(code_field_type),
-                                   field_owner, DependentCode::kFieldTypeGroup);
-  DependentCode::InstallDependency(
-      isolate, MaybeObjectHandle::Weak(code_field_repr), field_owner,
-      DependentCode::kFieldRepresentationGroup);
-  DependentCode::InstallDependency(
-      isolate, MaybeObjectHandle::Weak(code_field_const), field_owner,
-      DependentCode::kFieldConstGroup);
+  Handle<Code> code_src_field_const = CreateDummyOptimizedCode(isolate);
+  {
+    Handle<Map> field_owner(
+        map->FindFieldOwner(isolate, InternalIndex(kSplitProp)), isolate);
+    DependentCode::InstallDependency(
+        isolate, MaybeObjectHandle::Weak(code_field_type), field_owner,
+        DependentCode::kFieldTypeGroup);
+    DependentCode::InstallDependency(
+        isolate, MaybeObjectHandle::Weak(code_field_repr), field_owner,
+        DependentCode::kFieldRepresentationGroup);
+    DependentCode::InstallDependency(
+        isolate, MaybeObjectHandle::Weak(code_field_const), field_owner,
+        DependentCode::kFieldConstGroup);
+  }
+  {
+    Handle<Map> field_owner(
+        map2->FindFieldOwner(isolate, InternalIndex(kSplitProp)), isolate);
+    DependentCode::InstallDependency(
+        isolate, MaybeObjectHandle::Weak(code_src_field_const), field_owner,
+        DependentCode::kFieldConstGroup);
+  }
   CHECK(!code_field_type->marked_for_deoptimization());
   CHECK(!code_field_repr->marked_for_deoptimization());
   CHECK(!code_field_const->marked_for_deoptimization());
+  CHECK(!code_src_field_const->marked_for_deoptimization());
 
   // Reconfigure attributes of property |kSplitProp| of |map2| to NONE, which
   // should generalize representations in |map1|.
@@ -1087,10 +1098,21 @@ void TestReconfigureDataFieldAttribute_GeneralizeField(
       Map::ReconfigureExistingProperty(isolate, map2, InternalIndex(kSplitProp),
                                        kData, NONE, PropertyConstness::kConst);
 
-  // |map2| should be left unchanged but marked unstable.
+  // |map2| should be mosly left unchanged but marked unstable and if the
+  // source property was constant it should also be transitioned to kMutable.
   CHECK(!map2->is_stable());
   CHECK(!map2->is_deprecated());
   CHECK_NE(*map2, *new_map);
+  // If the "source" property was const then update constness expectations for
+  // "source" map and ensure the deoptimization dependency was triggered.
+  if (to.constness == PropertyConstness::kConst) {
+    expectations2.SetDataField(kSplitProp, READ_ONLY,
+                               PropertyConstness::kMutable, to.representation,
+                               to.type);
+    CHECK(code_src_field_const->marked_for_deoptimization());
+  } else {
+    CHECK(!code_src_field_const->marked_for_deoptimization());
+  }
   CHECK(expectations2.Check(*map2));
 
   for (int i = kSplitProp; i < kPropCount; i++) {

test/mjsunit/regress/regress-crbug-1161847-1.js

@@ -0,0 +1,19 @@
+// 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
+
+function foo(first_run) {
+  let o = { x: 0 };
+  if (first_run) assertTrue(%HasOwnConstDataProperty(o, 'x'));
+  Object.defineProperty(o, 'x', { writable: false });
+  delete o.x;
+  o.x = 23;
+  if (first_run) assertFalse(%HasOwnConstDataProperty(o, 'x'));
+}
+%PrepareFunctionForOptimization(foo);
+foo(true);
+foo(false);
+%OptimizeFunctionOnNextCall(foo);
+foo(false);

test/mjsunit/regress/regress-crbug-1161847-2.js

@@ -0,0 +1,19 @@
+// 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
+
+function foo(first_run) {
+  let o = { x: 0 };
+  if (first_run) assertTrue(%HasOwnConstDataProperty(o, 'x'));
+  Object.defineProperty(o, 'x', { get() { return 1; }, configurable: true, enumerable: true });
+  delete o.x;
+  o.x = 23;
+  if (first_run) assertFalse(%HasOwnConstDataProperty(o, 'x'));
+}
+%PrepareFunctionForOptimization(foo);
+foo(true);
+foo(false);
+%OptimizeFunctionOnNextCall(foo);
+foo(false);