From afa60ff604bf41cacd08a8364e8a70a2b5f69d30 Mon Sep 17 00:00:00 2001 From: jkummerow Date: Wed, 23 Sep 2015 05:35:21 -0700 Subject: [PATCH] [field type tracking] Fix handling of cleared WeakCells Whenever a generalization is computed, the inputs must be checked for being cleared, and if they are, the generalization must be Type::Any. Hopefully this fixes Chromium issue 527994 as well. BUG=v8:4325,chromium:527994 LOG=n Review URL: https://codereview.chromium.org/1361103002 Cr-Commit-Position: refs/heads/master@{#30887} --- src/accessors.cc | 1 - src/lookup.cc | 13 ++++++ src/objects.cc | 83 +++++++++++++++++++++++++----------- src/objects.h | 5 +-- test/mjsunit/mjsunit.status | 3 -- test/mjsunit/regress/regress-4325.js | 48 +++++++++++++++++++++ 6 files changed, 121 insertions(+), 32 deletions(-) create mode 100644 test/mjsunit/regress/regress-4325.js diff --git a/src/accessors.cc b/src/accessors.cc index 7d5d27a..441ff4d 100644 --- a/src/accessors.cc +++ b/src/accessors.cc @@ -1010,7 +1010,6 @@ MUST_USE_RESULT static MaybeHandle ReplaceAccessorWithDataProperty( CHECK_EQ(LookupIterator::ACCESSOR, it.state()); DCHECK(it.HolderIsReceiverOrHiddenPrototype()); it.ReconfigureDataProperty(value, it.property_details().attributes()); - it.WriteDataValue(value); if (is_observed && !old_value->SameValue(*value)) { return JSObject::EnqueueChangeRecord(object, "update", name, old_value); diff --git a/src/lookup.cc b/src/lookup.cc index 69b733e..809c35e 100644 --- a/src/lookup.cc +++ b/src/lookup.cc @@ -178,6 +178,13 @@ void LookupIterator::ReconfigureDataProperty(Handle value, } ReloadPropertyInformation(); + WriteDataValue(value); + +#if VERIFY_HEAP + if (FLAG_verify_heap) { + holder->JSObjectVerify(); + } +#endif } @@ -290,6 +297,12 @@ void LookupIterator::TransitionToAccessorProperty( } TransitionToAccessorPair(pair, attributes); + +#if VERIFY_HEAP + if (FLAG_verify_heap) { + receiver->JSObjectVerify(); + } +#endif } diff --git a/src/objects.cc b/src/objects.cc index 6b057c1..d5cbcda 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -2726,10 +2726,23 @@ void Map::UpdateFieldType(int descriptor, Handle name, } +bool FieldTypeIsCleared(Representation rep, Handle type) { + return type->Is(HeapType::None()) && rep.IsHeapObject(); +} + + // static -Handle Map::GeneralizeFieldType(Handle type1, +Handle Map::GeneralizeFieldType(Representation rep1, + Handle type1, + Representation rep2, Handle type2, Isolate* isolate) { + // Cleared field types need special treatment. They represent lost knowledge, + // so we must be conservative, so their generalization with any other type + // is "Any". + if (FieldTypeIsCleared(rep1, type1) || FieldTypeIsCleared(rep2, type2)) { + return HeapType::Any(isolate); + } if (type1->NowIs(type2)) return type2; if (type2->NowIs(type1)) return type1; return HeapType::Any(isolate); @@ -2750,10 +2763,13 @@ void Map::GeneralizeFieldType(Handle map, int modify_index, isolate); if (old_representation.Equals(new_representation) && + !FieldTypeIsCleared(new_representation, new_field_type) && + // Checking old_field_type for being cleared is not necessary because + // the NowIs check below would fail anyway in that case. new_field_type->NowIs(old_field_type)) { - DCHECK(Map::GeneralizeFieldType(old_field_type, - new_field_type, - isolate)->NowIs(old_field_type)); + DCHECK(Map::GeneralizeFieldType(old_representation, old_field_type, + new_representation, new_field_type, isolate) + ->NowIs(old_field_type)); return; } @@ -2762,17 +2778,10 @@ void Map::GeneralizeFieldType(Handle map, int modify_index, Handle descriptors( field_owner->instance_descriptors(), isolate); DCHECK_EQ(*old_field_type, descriptors->GetFieldType(modify_index)); - bool old_field_type_was_cleared = - old_field_type->Is(HeapType::None()) && old_representation.IsHeapObject(); - // Determine the generalized new field type. Conservatively assume type Any - // for cleared field types because the cleared type could have been a - // deprecated map and there still could be live instances with a non- - // deprecated version of the map. new_field_type = - old_field_type_was_cleared - ? HeapType::Any(isolate) - : Map::GeneralizeFieldType(old_field_type, new_field_type, isolate); + Map::GeneralizeFieldType(old_representation, old_field_type, + new_representation, new_field_type, isolate); PropertyDetails details = descriptors->GetDetails(modify_index); Handle name(descriptors->GetKey(modify_index)); @@ -2996,8 +3005,10 @@ Handle Map::ReconfigureProperty(Handle old_map, int modify_index, Handle old_field_type = GetFieldType(isolate, old_descriptors, i, old_details.location(), tmp_representation); - next_field_type = - GeneralizeFieldType(next_field_type, old_field_type, isolate); + Representation old_representation = old_details.representation(); + next_field_type = GeneralizeFieldType( + old_representation, old_field_type, new_representation, + next_field_type, isolate); } } else { Handle old_field_type = @@ -3161,21 +3172,24 @@ Handle Map::ReconfigureProperty(Handle old_map, int modify_index, Handle next_field_type; if (modify_index == i) { - next_field_type = - GeneralizeFieldType(target_field_type, new_field_type, isolate); + next_field_type = GeneralizeFieldType( + target_details.representation(), target_field_type, + new_representation, new_field_type, isolate); if (!property_kind_reconfiguration) { Handle old_field_type = GetFieldType(isolate, old_descriptors, i, old_details.location(), next_representation); - next_field_type = - GeneralizeFieldType(next_field_type, old_field_type, isolate); + next_field_type = GeneralizeFieldType( + old_details.representation(), old_field_type, + next_representation, next_field_type, isolate); } } else { Handle old_field_type = GetFieldType(isolate, old_descriptors, i, old_details.location(), next_representation); - next_field_type = - GeneralizeFieldType(target_field_type, old_field_type, isolate); + next_field_type = GeneralizeFieldType( + old_details.representation(), old_field_type, next_representation, + target_field_type, isolate); } Handle wrapped_type(WrapType(next_field_type)); DataDescriptor d(target_key, current_offset, wrapped_type, @@ -3236,8 +3250,9 @@ Handle Map::ReconfigureProperty(Handle old_map, int modify_index, Handle old_field_type = GetFieldType(isolate, old_descriptors, i, old_details.location(), next_representation); - next_field_type = - GeneralizeFieldType(next_field_type, old_field_type, isolate); + next_field_type = GeneralizeFieldType( + old_details.representation(), old_field_type, + next_representation, next_field_type, isolate); } } else { Handle old_field_type = @@ -3798,6 +3813,11 @@ MaybeHandle Object::SetDataProperty(LookupIterator* it, Object); } +#if VERIFY_HEAP + if (FLAG_verify_heap) { + receiver->JSObjectVerify(); + } +#endif return value; } @@ -3920,6 +3940,11 @@ MaybeHandle Object::AddDataProperty(LookupIterator* it, it->factory()->the_hole_value()), Object); } +#if VERIFY_HEAP + if (FLAG_verify_heap) { + receiver->JSObjectVerify(); + } +#endif } return value; @@ -4572,6 +4597,11 @@ void JSObject::MigrateInstance(Handle object) { if (FLAG_trace_migration) { object->PrintInstanceMigration(stdout, *original_map, *map); } +#if VERIFY_HEAP + if (FLAG_verify_heap) { + object->JSObjectVerify(); + } +#endif } @@ -4588,6 +4618,11 @@ bool JSObject::TryMigrateInstance(Handle object) { if (FLAG_trace_migration) { object->PrintInstanceMigration(stdout, *original_map, object->map()); } +#if VERIFY_HEAP + if (FLAG_verify_heap) { + object->JSObjectVerify(); + } +#endif return true; } @@ -4696,7 +4731,6 @@ MaybeHandle JSObject::DefineOwnPropertyIgnoreAttributes( it->TransitionToAccessorPair(new_data, attributes); } else { it->ReconfigureDataProperty(value, attributes); - it->WriteDataValue(value); } if (is_observed) { @@ -4732,7 +4766,6 @@ MaybeHandle JSObject::DefineOwnPropertyIgnoreAttributes( if (is_observed) old_value = it->GetDataValue(); it->ReconfigureDataProperty(value, attributes); - it->WriteDataValue(value); if (is_observed) { if (old_value->SameValue(*value)) { diff --git a/src/objects.h b/src/objects.h index a24d029..f859318 100644 --- a/src/objects.h +++ b/src/objects.h @@ -5469,9 +5469,8 @@ class Map: public HeapObject { // TODO(ishell): moveit! static Handle GeneralizeAllFieldRepresentations(Handle map); MUST_USE_RESULT static Handle GeneralizeFieldType( - Handle type1, - Handle type2, - Isolate* isolate); + Representation rep1, Handle type1, Representation rep2, + Handle type2, Isolate* isolate); static void GeneralizeFieldType(Handle map, int modify_index, Representation new_representation, Handle new_field_type); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index b045803..2a616b0 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -154,9 +154,6 @@ # issue 4078: 'allocation-site-info': [PASS, NO_VARIANTS], - # issue 4325: - 'regress/regress-3960': [PASS, NO_VARIANTS], - ############################################################################## # Too slow in debug mode with --stress-opt mode. 'compiler/regress-stacktrace-methods': [PASS, ['mode == debug', SKIP]], diff --git a/test/mjsunit/regress/regress-4325.js b/test/mjsunit/regress/regress-4325.js new file mode 100644 index 0000000..e88bdd3 --- /dev/null +++ b/test/mjsunit/regress/regress-4325.js @@ -0,0 +1,48 @@ +// Copyright 2015 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 --expose-gc + +function Inner() { + this.p1 = 0; + this.p2 = 3; +} + +function Outer() { + this.p3 = 0; +} + +var i1 = new Inner(); +var i2 = new Inner(); +var o1 = new Outer(); +o1.inner = i1; +// o1.map now thinks "inner" has type Inner.map1. +// Deprecate Inner.map1: +i1.p1 = 0.5; +// Let Inner.map1 die by migrating i2 to Inner.map2: +print(i2.p1); +gc(); +// o1.map's descriptor for "inner" is now a cleared WeakCell; +// o1.inner's actual map is Inner.map2. +// Prepare Inner.map3, deprecating Inner.map2. +i2.p2 = 0.5; +// Deprecate o1's map. +var o2 = new Outer(); +o2.p3 = 0.5; +o2.inner = i2; +// o2.map (Outer.map2) now says that o2.inner's type is Inner.map3. +// Migrate o1 to Outer.map2. +print(o1.p3); +// o1.map now thinks that o1.inner has map Inner.map3 just like o2.inner, +// but in fact o1.inner.map is still Inner.map2! + +function loader(o) { + return o.inner.p2; +} +loader(o2); +loader(o2); +%OptimizeFunctionOnNextCall(loader); +assertEquals(0.5, loader(o2)); +assertEquals(3, loader(o1)); +gc(); // Crashes with --verify-heap. -- 2.7.4