Skip to content

Commit afa60ff

Browse files
jakobkummerowCommit bot
jakobkummerow
authored and
Commit bot
committed
[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}
1 parent 0e84241 commit afa60ff

File tree

6 files changed

+121
-32
lines changed

6 files changed

+121
-32
lines changed

‎src/accessors.cc‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1010,7 +1010,6 @@ MUST_USE_RESULT static MaybeHandle<Object> ReplaceAccessorWithDataProperty(
10101010
CHECK_EQ(LookupIterator::ACCESSOR, it.state());
10111011
DCHECK(it.HolderIsReceiverOrHiddenPrototype());
10121012
it.ReconfigureDataProperty(value, it.property_details().attributes());
1013-
it.WriteDataValue(value);
10141013

10151014
if (is_observed && !old_value->SameValue(*value)) {
10161015
return JSObject::EnqueueChangeRecord(object, "update", name, old_value);

‎src/lookup.cc‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,13 @@ void LookupIterator::ReconfigureDataProperty(Handle<Object> value,
178178
}
179179

180180
ReloadPropertyInformation();
181+
WriteDataValue(value);
182+
183+
#if VERIFY_HEAP
184+
if (FLAG_verify_heap) {
185+
holder->JSObjectVerify();
186+
}
187+
#endif
181188
}
182189

183190

@@ -290,6 +297,12 @@ void LookupIterator::TransitionToAccessorProperty(
290297
}
291298

292299
TransitionToAccessorPair(pair, attributes);
300+
301+
#if VERIFY_HEAP
302+
if (FLAG_verify_heap) {
303+
receiver->JSObjectVerify();
304+
}
305+
#endif
293306
}
294307

295308

‎src/objects.cc‎

Lines changed: 58 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2726,10 +2726,23 @@ void Map::UpdateFieldType(int descriptor, Handle<Name> name,
27262726
}
27272727

27282728

2729+
bool FieldTypeIsCleared(Representation rep, Handle<HeapType> type) {
2730+
return type->Is(HeapType::None()) && rep.IsHeapObject();
2731+
}
2732+
2733+
27292734
// static
2730-
Handle<HeapType> Map::GeneralizeFieldType(Handle<HeapType> type1,
2735+
Handle<HeapType> Map::GeneralizeFieldType(Representation rep1,
2736+
Handle<HeapType> type1,
2737+
Representation rep2,
27312738
Handle<HeapType> type2,
27322739
Isolate* isolate) {
2740+
// Cleared field types need special treatment. They represent lost knowledge,
2741+
// so we must be conservative, so their generalization with any other type
2742+
// is "Any".
2743+
if (FieldTypeIsCleared(rep1, type1) || FieldTypeIsCleared(rep2, type2)) {
2744+
return HeapType::Any(isolate);
2745+
}
27332746
if (type1->NowIs(type2)) return type2;
27342747
if (type2->NowIs(type1)) return type1;
27352748
return HeapType::Any(isolate);
@@ -2750,10 +2763,13 @@ void Map::GeneralizeFieldType(Handle<Map> map, int modify_index,
27502763
isolate);
27512764

27522765
if (old_representation.Equals(new_representation) &&
2766+
!FieldTypeIsCleared(new_representation, new_field_type) &&
2767+
// Checking old_field_type for being cleared is not necessary because
2768+
// the NowIs check below would fail anyway in that case.
27532769
new_field_type->NowIs(old_field_type)) {
2754-
DCHECK(Map::GeneralizeFieldType(old_field_type,
2755-
new_field_type,
2756-
isolate)->NowIs(old_field_type));
2770+
DCHECK(Map::GeneralizeFieldType(old_representation, old_field_type,
2771+
new_representation, new_field_type, isolate)
2772+
->NowIs(old_field_type));
27572773
return;
27582774
}
27592775

@@ -2762,17 +2778,10 @@ void Map::GeneralizeFieldType(Handle<Map> map, int modify_index,
27622778
Handle<DescriptorArray> descriptors(
27632779
field_owner->instance_descriptors(), isolate);
27642780
DCHECK_EQ(*old_field_type, descriptors->GetFieldType(modify_index));
2765-
bool old_field_type_was_cleared =
2766-
old_field_type->Is(HeapType::None()) && old_representation.IsHeapObject();
27672781

2768-
// Determine the generalized new field type. Conservatively assume type Any
2769-
// for cleared field types because the cleared type could have been a
2770-
// deprecated map and there still could be live instances with a non-
2771-
// deprecated version of the map.
27722782
new_field_type =
2773-
old_field_type_was_cleared
2774-
? HeapType::Any(isolate)
2775-
: Map::GeneralizeFieldType(old_field_type, new_field_type, isolate);
2783+
Map::GeneralizeFieldType(old_representation, old_field_type,
2784+
new_representation, new_field_type, isolate);
27762785

27772786
PropertyDetails details = descriptors->GetDetails(modify_index);
27782787
Handle<Name> name(descriptors->GetKey(modify_index));
@@ -2996,8 +3005,10 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
29963005
Handle<HeapType> old_field_type =
29973006
GetFieldType(isolate, old_descriptors, i,
29983007
old_details.location(), tmp_representation);
2999-
next_field_type =
3000-
GeneralizeFieldType(next_field_type, old_field_type, isolate);
3008+
Representation old_representation = old_details.representation();
3009+
next_field_type = GeneralizeFieldType(
3010+
old_representation, old_field_type, new_representation,
3011+
next_field_type, isolate);
30013012
}
30023013
} else {
30033014
Handle<HeapType> old_field_type =
@@ -3161,21 +3172,24 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
31613172

31623173
Handle<HeapType> next_field_type;
31633174
if (modify_index == i) {
3164-
next_field_type =
3165-
GeneralizeFieldType(target_field_type, new_field_type, isolate);
3175+
next_field_type = GeneralizeFieldType(
3176+
target_details.representation(), target_field_type,
3177+
new_representation, new_field_type, isolate);
31663178
if (!property_kind_reconfiguration) {
31673179
Handle<HeapType> old_field_type =
31683180
GetFieldType(isolate, old_descriptors, i,
31693181
old_details.location(), next_representation);
3170-
next_field_type =
3171-
GeneralizeFieldType(next_field_type, old_field_type, isolate);
3182+
next_field_type = GeneralizeFieldType(
3183+
old_details.representation(), old_field_type,
3184+
next_representation, next_field_type, isolate);
31723185
}
31733186
} else {
31743187
Handle<HeapType> old_field_type =
31753188
GetFieldType(isolate, old_descriptors, i, old_details.location(),
31763189
next_representation);
3177-
next_field_type =
3178-
GeneralizeFieldType(target_field_type, old_field_type, isolate);
3190+
next_field_type = GeneralizeFieldType(
3191+
old_details.representation(), old_field_type, next_representation,
3192+
target_field_type, isolate);
31793193
}
31803194
Handle<Object> wrapped_type(WrapType(next_field_type));
31813195
DataDescriptor d(target_key, current_offset, wrapped_type,
@@ -3236,8 +3250,9 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
32363250
Handle<HeapType> old_field_type =
32373251
GetFieldType(isolate, old_descriptors, i,
32383252
old_details.location(), next_representation);
3239-
next_field_type =
3240-
GeneralizeFieldType(next_field_type, old_field_type, isolate);
3253+
next_field_type = GeneralizeFieldType(
3254+
old_details.representation(), old_field_type,
3255+
next_representation, next_field_type, isolate);
32413256
}
32423257
} else {
32433258
Handle<HeapType> old_field_type =
@@ -3798,6 +3813,11 @@ MaybeHandle<Object> Object::SetDataProperty(LookupIterator* it,
37983813
Object);
37993814
}
38003815

3816+
#if VERIFY_HEAP
3817+
if (FLAG_verify_heap) {
3818+
receiver->JSObjectVerify();
3819+
}
3820+
#endif
38013821
return value;
38023822
}
38033823

@@ -3920,6 +3940,11 @@ MaybeHandle<Object> Object::AddDataProperty(LookupIterator* it,
39203940
it->factory()->the_hole_value()),
39213941
Object);
39223942
}
3943+
#if VERIFY_HEAP
3944+
if (FLAG_verify_heap) {
3945+
receiver->JSObjectVerify();
3946+
}
3947+
#endif
39233948
}
39243949

39253950
return value;
@@ -4572,6 +4597,11 @@ void JSObject::MigrateInstance(Handle<JSObject> object) {
45724597
if (FLAG_trace_migration) {
45734598
object->PrintInstanceMigration(stdout, *original_map, *map);
45744599
}
4600+
#if VERIFY_HEAP
4601+
if (FLAG_verify_heap) {
4602+
object->JSObjectVerify();
4603+
}
4604+
#endif
45754605
}
45764606

45774607

@@ -4588,6 +4618,11 @@ bool JSObject::TryMigrateInstance(Handle<JSObject> object) {
45884618
if (FLAG_trace_migration) {
45894619
object->PrintInstanceMigration(stdout, *original_map, object->map());
45904620
}
4621+
#if VERIFY_HEAP
4622+
if (FLAG_verify_heap) {
4623+
object->JSObjectVerify();
4624+
}
4625+
#endif
45914626
return true;
45924627
}
45934628

@@ -4696,7 +4731,6 @@ MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
46964731
it->TransitionToAccessorPair(new_data, attributes);
46974732
} else {
46984733
it->ReconfigureDataProperty(value, attributes);
4699-
it->WriteDataValue(value);
47004734
}
47014735

47024736
if (is_observed) {
@@ -4732,7 +4766,6 @@ MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
47324766
if (is_observed) old_value = it->GetDataValue();
47334767

47344768
it->ReconfigureDataProperty(value, attributes);
4735-
it->WriteDataValue(value);
47364769

47374770
if (is_observed) {
47384771
if (old_value->SameValue(*value)) {

‎src/objects.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5469,9 +5469,8 @@ class Map: public HeapObject {
54695469
// TODO(ishell): moveit!
54705470
static Handle<Map> GeneralizeAllFieldRepresentations(Handle<Map> map);
54715471
MUST_USE_RESULT static Handle<HeapType> GeneralizeFieldType(
5472-
Handle<HeapType> type1,
5473-
Handle<HeapType> type2,
5474-
Isolate* isolate);
5472+
Representation rep1, Handle<HeapType> type1, Representation rep2,
5473+
Handle<HeapType> type2, Isolate* isolate);
54755474
static void GeneralizeFieldType(Handle<Map> map, int modify_index,
54765475
Representation new_representation,
54775476
Handle<HeapType> new_field_type);

‎test/mjsunit/mjsunit.status‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,6 @@
154154
# issue 4078:
155155
'allocation-site-info': [PASS, NO_VARIANTS],
156156

157-
# issue 4325:
158-
'regress/regress-3960': [PASS, NO_VARIANTS],
159-
160157
##############################################################################
161158
# Too slow in debug mode with --stress-opt mode.
162159
'compiler/regress-stacktrace-methods': [PASS, ['mode == debug', SKIP]],

‎test/mjsunit/regress/regress-4325.js‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
// Copyright 2015 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --allow-natives-syntax --expose-gc
6+
7+
function Inner() {
8+
this.p1 = 0;
9+
this.p2 = 3;
10+
}
11+
12+
function Outer() {
13+
this.p3 = 0;
14+
}
15+
16+
var i1 = new Inner();
17+
var i2 = new Inner();
18+
var o1 = new Outer();
19+
o1.inner = i1;
20+
// o1.map now thinks "inner" has type Inner.map1.
21+
// Deprecate Inner.map1:
22+
i1.p1 = 0.5;
23+
// Let Inner.map1 die by migrating i2 to Inner.map2:
24+
print(i2.p1);
25+
gc();
26+
// o1.map's descriptor for "inner" is now a cleared WeakCell;
27+
// o1.inner's actual map is Inner.map2.
28+
// Prepare Inner.map3, deprecating Inner.map2.
29+
i2.p2 = 0.5;
30+
// Deprecate o1's map.
31+
var o2 = new Outer();
32+
o2.p3 = 0.5;
33+
o2.inner = i2;
34+
// o2.map (Outer.map2) now says that o2.inner's type is Inner.map3.
35+
// Migrate o1 to Outer.map2.
36+
print(o1.p3);
37+
// o1.map now thinks that o1.inner has map Inner.map3 just like o2.inner,
38+
// but in fact o1.inner.map is still Inner.map2!
39+
40+
function loader(o) {
41+
return o.inner.p2;
42+
}
43+
loader(o2);
44+
loader(o2);
45+
%OptimizeFunctionOnNextCall(loader);
46+
assertEquals(0.5, loader(o2));
47+
assertEquals(3, loader(o1));
48+
gc(); // Crashes with --verify-heap.

0 commit comments

Comments
 (0)