From: mstarzinger Date: Tue, 4 Aug 2015 17:48:42 +0000 (-0700) Subject: Introduce safe interface to "copy and grow" FixedArray. X-Git-Tag: upstream/4.7.83~1028 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=bcad9b547d00320f818ce6ff08016a02ce0b6e5b;p=platform%2Fupstream%2Fv8.git Introduce safe interface to "copy and grow" FixedArray. This introduces a CopyFixedArrayAndGrow method on Factory that takes the "grow amount" instead of the "new size" as an argument. The new interface is safer because it allows for mutations by the GC that potentially trim the source array. This also fixes a bug in SharedFunctionInfo::AddToOptimizedCodeMap where the aformentioned scenario led to unused entries within the optimized code map. Note that FixedArray::CopySize is hereby deprecated because it is considered unsafe and should no longer be used. R=hpayer@chromium.org TEST=mjsunit/regress/regress-crbug-513507 BUG=chromium:513507 LOG=n Review URL: https://codereview.chromium.org/1255173006 Cr-Commit-Position: refs/heads/master@{#30012} --- diff --git a/src/factory.cc b/src/factory.cc index ea4f69a96..bc5d1e4e0 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -978,6 +978,14 @@ Handle Factory::CopyFixedArrayWithMap(Handle array, } +Handle Factory::CopyFixedArrayAndGrow(Handle array, + int grow_by) { + CALL_HEAP_FUNCTION(isolate(), + isolate()->heap()->CopyFixedArrayAndGrow(*array, grow_by), + FixedArray); +} + + Handle Factory::CopyFixedArray(Handle array) { CALL_HEAP_FUNCTION(isolate(), isolate()->heap()->CopyFixedArray(*array), diff --git a/src/factory.h b/src/factory.h index 9a595c411..c52d6d31d 100644 --- a/src/factory.h +++ b/src/factory.h @@ -322,6 +322,9 @@ class Factory final { Handle CopyFixedArrayWithMap(Handle array, Handle map); + Handle CopyFixedArrayAndGrow(Handle array, + int grow_by); + Handle CopyFixedArray(Handle array); // This method expects a COW array in new space, and creates a copy diff --git a/src/heap/heap.cc b/src/heap/heap.cc index c17a7ac87..70a02679f 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -4428,7 +4428,7 @@ AllocationResult Heap::CopyAndTenureFixedCOWArray(FixedArray* src) { FixedArray* result = FixedArray::cast(obj); result->set_length(len); - // Copy the content + // Copy the content. DisallowHeapAllocation no_gc; WriteBarrierMode mode = result->GetWriteBarrierMode(no_gc); for (int i = 0; i < len; i++) result->set(i, src->get(i), mode); @@ -4447,6 +4447,27 @@ AllocationResult Heap::AllocateEmptyFixedTypedArray( } +AllocationResult Heap::CopyFixedArrayAndGrow(FixedArray* src, int grow_by) { + int old_len = src->length(); + int new_len = old_len + grow_by; + HeapObject* obj; + { + AllocationResult allocation = AllocateRawFixedArray(new_len, NOT_TENURED); + if (!allocation.To(&obj)) return allocation; + } + obj->set_map_no_write_barrier(fixed_array_map()); + FixedArray* result = FixedArray::cast(obj); + result->set_length(new_len); + + // Copy the content. + DisallowHeapAllocation no_gc; + WriteBarrierMode mode = result->GetWriteBarrierMode(no_gc); + for (int i = 0; i < old_len; i++) result->set(i, src->get(i), mode); + MemsetPointer(result->data_start() + old_len, undefined_value(), grow_by); + return result; +} + + AllocationResult Heap::CopyFixedArrayWithMap(FixedArray* src, Map* map) { int len = src->length(); HeapObject* obj; @@ -4464,7 +4485,7 @@ AllocationResult Heap::CopyFixedArrayWithMap(FixedArray* src, Map* map) { FixedArray* result = FixedArray::cast(obj); result->set_length(len); - // Copy the content + // Copy the content. DisallowHeapAllocation no_gc; WriteBarrierMode mode = result->GetWriteBarrierMode(no_gc); for (int i = 0; i < len; i++) result->set(i, src->get(i), mode); diff --git a/src/heap/heap.h b/src/heap/heap.h index 48e3de8ea..526da067b 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -2022,17 +2022,18 @@ class Heap { // Allocates an uninitialized fixed array. It must be filled by the caller. MUST_USE_RESULT AllocationResult AllocateUninitializedFixedArray(int length); - // Make a copy of src and return it. Returns - // Failure::RetryAfterGC(requested_bytes, space) if the allocation failed. + // Make a copy of src and return it. MUST_USE_RESULT inline AllocationResult CopyFixedArray(FixedArray* src); - // Make a copy of src, set the map, and return the copy. Returns - // Failure::RetryAfterGC(requested_bytes, space) if the allocation failed. + // Make a copy of src, also grow the copy, and return the copy. + MUST_USE_RESULT AllocationResult + CopyFixedArrayAndGrow(FixedArray* src, int grow_by); + + // Make a copy of src, set the map, and return the copy. MUST_USE_RESULT AllocationResult CopyFixedArrayWithMap(FixedArray* src, Map* map); - // Make a copy of src and return it. Returns - // Failure::RetryAfterGC(requested_bytes, space) if the allocation failed. + // Make a copy of src and return it. MUST_USE_RESULT inline AllocationResult CopyFixedDoubleArray( FixedDoubleArray* src); diff --git a/src/objects.cc b/src/objects.cc index 51675d4c2..f4a637c00 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -9539,9 +9539,9 @@ void SharedFunctionInfo::AddToOptimizedCodeMap( // Copy old map and append one new entry. Handle old_code_map = Handle::cast(value); DCHECK(!shared->SearchOptimizedCodeMap(*native_context, osr_ast_id).code); + new_code_map = + isolate->factory()->CopyFixedArrayAndGrow(old_code_map, kEntryLength); old_length = old_code_map->length(); - new_code_map = FixedArray::CopySize( - old_code_map, old_length + kEntryLength); // Zap the old map for the sake of the heap verifier. if (Heap::ShouldZapGarbage()) { Object** data = old_code_map->data_start(); diff --git a/src/objects.h b/src/objects.h index 78de2235b..ac37f8177 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2399,6 +2399,7 @@ class FixedArray: public FixedArrayBase { void Shrink(int length); // Copy operation. + // TODO(mstarzinger): Deprecated, use Factory::CopyFixedArrayAndGrow! static Handle CopySize(Handle array, int new_length, PretenureFlag pretenure = NOT_TENURED); diff --git a/test/cctest/test-heap.cc b/test/cctest/test-heap.cc index c54d67aee..e476bcca8 100644 --- a/test/cctest/test-heap.cc +++ b/test/cctest/test-heap.cc @@ -4501,6 +4501,61 @@ TEST(Regress173458) { } +#ifdef DEBUG +TEST(Regress513507) { + i::FLAG_flush_optimized_code_cache = false; + i::FLAG_allow_natives_syntax = true; + i::FLAG_gc_global = true; + CcTest::InitializeVM(); + Isolate* isolate = CcTest::i_isolate(); + Heap* heap = isolate->heap(); + HandleScope scope(isolate); + + // Prepare function whose optimized code map we can use. + Handle shared; + { + HandleScope inner_scope(isolate); + CompileRun("function f() { return 1 }" + "f(); %OptimizeFunctionOnNextCall(f); f();"); + + Handle f = + v8::Utils::OpenHandle( + *v8::Handle::Cast( + CcTest::global()->Get(v8_str("f")))); + shared = inner_scope.CloseAndEscape(handle(f->shared(), isolate)); + CompileRun("f = null"); + } + + // Prepare optimized code that we can use. + Handle code; + { + HandleScope inner_scope(isolate); + CompileRun("function g() { return 2 }" + "g(); %OptimizeFunctionOnNextCall(g); g();"); + + Handle g = + v8::Utils::OpenHandle( + *v8::Handle::Cast( + CcTest::global()->Get(v8_str("g")))); + code = inner_scope.CloseAndEscape(handle(g->code(), isolate)); + if (!code->is_optimized_code()) return; + } + + Handle lit = isolate->factory()->empty_fixed_array(); + Handle context(isolate->context()); + + // Add the new code several times to the optimized code map and also set an + // allocation timeout so that expanding the code map will trigger a GC. + heap->set_allocation_timeout(5); + FLAG_gc_interval = 1000; + for (int i = 0; i < 10; ++i) { + BailoutId id = BailoutId(i); + SharedFunctionInfo::AddToOptimizedCodeMap(shared, context, code, lit, id); + } +} +#endif // DEBUG + + class DummyVisitor : public ObjectVisitor { public: void VisitPointers(Object** start, Object** end) { } diff --git a/test/mjsunit/regress/regress-crbug-513507.js b/test/mjsunit/regress/regress-crbug-513507.js new file mode 100644 index 000000000..dbf35c91f --- /dev/null +++ b/test/mjsunit/regress/regress-crbug-513507.js @@ -0,0 +1,24 @@ +// 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: --noflush-optimized-code-cache --allow-natives-syntax + +// The following triggers a GC in SharedFunctionInfo::AddToOptimizedCodeMap. +// Flags: --gc-interval=1234 --gc-global + +function makeFun() { + function fun(osr_fuse) { + for (var i = 0; i < 3; ++i) { + if (i == osr_fuse) %OptimizeOsr(); + } + for (var i = 3; i < 6; ++i) { + if (i == osr_fuse) %OptimizeOsr(); + } + } + return fun; +} + +makeFun()(7); // Warm up. +makeFun()(4); // Optimize once. +makeFun()(1); // Optimize again.