From 2f327a5cb4f2ecbb163979fc9b788af765396a62 Mon Sep 17 00:00:00 2001 From: ulan Date: Mon, 13 Apr 2015 02:43:55 -0700 Subject: [PATCH] Do not inline store if field map was cleared. BUG=v8:4023 LOG=NO Review URL: https://codereview.chromium.org/1081033004 Cr-Commit-Position: refs/heads/master@{#27779} --- src/hydrogen.cc | 37 ++++++++------- src/hydrogen.h | 2 +- src/objects-debug.cc | 2 +- test/mjsunit/regress/regress-4023.js | 67 ++++++++++++++++++++++++++++ 4 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 test/mjsunit/regress/regress-4023.js diff --git a/src/hydrogen.cc b/src/hydrogen.cc index b9e032487d..c79a8170b6 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -6014,7 +6014,7 @@ bool HOptimizedGraphBuilder::PropertyAccessInfo::LoadResult(Handle map) { access_ = HObjectAccess::ForField(map, index, representation(), name_); // Load field map for heap objects. - LoadFieldMaps(map); + return LoadFieldMaps(map); } else if (IsAccessorConstant()) { Handle accessors = GetAccessorsFromMap(map); if (!accessors->IsAccessorPair()) return false; @@ -6040,7 +6040,7 @@ bool HOptimizedGraphBuilder::PropertyAccessInfo::LoadResult(Handle map) { } -void HOptimizedGraphBuilder::PropertyAccessInfo::LoadFieldMaps( +bool HOptimizedGraphBuilder::PropertyAccessInfo::LoadFieldMaps( Handle map) { // Clear any previously collected field maps/type. field_maps_.Clear(); @@ -6051,19 +6051,26 @@ void HOptimizedGraphBuilder::PropertyAccessInfo::LoadFieldMaps( // Collect the (stable) maps from the field type. int num_field_maps = field_type->NumClasses(); - if (num_field_maps == 0) return; - DCHECK(access_.representation().IsHeapObject()); - field_maps_.Reserve(num_field_maps, zone()); - HeapType::Iterator it = field_type->Classes(); - while (!it.Done()) { - Handle field_map = it.Current(); - if (!field_map->is_stable()) { - field_maps_.Clear(); - return; + if (num_field_maps > 0) { + DCHECK(access_.representation().IsHeapObject()); + field_maps_.Reserve(num_field_maps, zone()); + HeapType::Iterator it = field_type->Classes(); + while (!it.Done()) { + Handle field_map = it.Current(); + if (!field_map->is_stable()) { + field_maps_.Clear(); + break; + } + field_maps_.Add(field_map, zone()); + it.Advance(); } - field_maps_.Add(field_map, zone()); - it.Advance(); } + + if (field_maps_.is_empty()) { + // Store is not safe if the field map was cleared. + return IsLoad() || !field_type->Is(HeapType::None()); + } + field_maps_.Sort(); DCHECK_EQ(num_field_maps, field_maps_.length()); @@ -6074,6 +6081,7 @@ void HOptimizedGraphBuilder::PropertyAccessInfo::LoadFieldMaps( // Add dependency on the map that introduced the field. Map::AddDependentCompilationInfo(GetFieldOwnerFromMap(map), DependentCode::kFieldTypeGroup, top_info()); + return true; } @@ -6132,8 +6140,7 @@ bool HOptimizedGraphBuilder::PropertyAccessInfo::CanAccessMonomorphic() { access_ = HObjectAccess::ForField(map_, index, representation, name_); // Load field map for heap objects. - LoadFieldMaps(transition()); - return true; + return LoadFieldMaps(transition()); } return false; } diff --git a/src/hydrogen.h b/src/hydrogen.h index c70ea2d96c..97231a604c 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -2644,7 +2644,7 @@ class HOptimizedGraphBuilder : public HGraphBuilder, public AstVisitor { CompilationInfo* current_info() { return builder_->current_info(); } bool LoadResult(Handle map); - void LoadFieldMaps(Handle map); + bool LoadFieldMaps(Handle map); bool LookupDescriptor(); bool LookupInPrototypes(); bool IsIntegerIndexedExotic(); diff --git a/src/objects-debug.cc b/src/objects-debug.cc index 69699885d0..3495ad65a6 100644 --- a/src/objects-debug.cc +++ b/src/objects-debug.cc @@ -290,7 +290,7 @@ void JSObject::JSObjectVerify() { if (r.IsHeapObject()) DCHECK(value->IsHeapObject()); HeapType* field_type = descriptors->GetFieldType(i); bool type_is_none = field_type->Is(HeapType::None()); - bool type_is_any = field_type->Is(HeapType::Any()); + bool type_is_any = HeapType::Any()->Is(field_type); if (r.IsNone()) { CHECK(type_is_none); } else if (!type_is_any && !(type_is_none && r.IsHeapObject())) { diff --git a/test/mjsunit/regress/regress-4023.js b/test/mjsunit/regress/regress-4023.js new file mode 100644 index 0000000000..902741f6f5 --- /dev/null +++ b/test/mjsunit/regress/regress-4023.js @@ -0,0 +1,67 @@ +// 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 --block-concurrent-recompilation + +function Inner() { + this.property = "OK"; + this.prop2 = 1; +} + +function Outer() { + this.o = "u"; +} +function KeepMapAlive(o) { + return o.o; +} +function SetInner(o, i) { + o.inner_field = i; +} +function Crash(o) { + return o.inner_field.property; +} + +var inner = new Inner(); +var outer = new Outer(); + +// Collect type feedback. +SetInner(new Outer(), inner); +SetInner(outer, inner); + +// This function's only purpose is to stash away a Handle that keeps +// outer's map alive during the gc() call below. We store this handle +// on the compiler thread :-) +KeepMapAlive(outer); +KeepMapAlive(outer); +%OptimizeFunctionOnNextCall(KeepMapAlive, "concurrent"); +KeepMapAlive(outer); + +// So far, all is well. Collect type feedback and optimize. +print(Crash(outer)); +print(Crash(outer)); +%OptimizeFunctionOnNextCall(Crash); +print(Crash(outer)); + +// Null out references and perform GC. This will keep outer's map alive +// (due to the handle created above), but will let inner's map die. Hence, +// inner_field's field type stored in outer's map will get cleared. +inner = undefined; +outer = undefined; +gc(); + +// We could unblock the compiler thread now. But why bother? + +// Now optimize SetInner while inner_field's type is still cleared! +// This will generate optimized code that stores arbitrary objects +// into inner_field without checking their type against the field type. +%OptimizeFunctionOnNextCall(SetInner); + +// Use the optimized code to store an arbitrary object into +// o2's inner_field, without triggering any dependent code deopts... +var o2 = new Outer(); +SetInner(o2, { invalid: 1.51, property: "OK" }); +// ...and then use the existing code expecting an Inner-class object to +// read invalid data (in this case, a raw double). +// We crash trying to convert the raw double into a printable string. +print(Crash(o2));