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 ea4f69a..bc5d1e4 100644 (file)
--- a/src/factory.cc
+++ b/src/factory.cc
@@ -978,6 +978,14 @@ Handle<FixedArray> Factory::CopyFixedArrayWithMap(Handle<FixedArray> array,
 }
 
 
+Handle<FixedArray> Factory::CopyFixedArrayAndGrow(Handle<FixedArray> array,
+                                                  int grow_by) {
+  CALL_HEAP_FUNCTION(isolate(),
+                     isolate()->heap()->CopyFixedArrayAndGrow(*array, grow_by),
+                     FixedArray);
+}
+
+
 Handle<FixedArray> Factory::CopyFixedArray(Handle<FixedArray> array) {
   CALL_HEAP_FUNCTION(isolate(),
                      isolate()->heap()->CopyFixedArray(*array),

diff --git a/src/factory.h b/src/factory.h
index 9a595c4..c52d6d3 100644 (file)
--- a/src/factory.h
+++ b/src/factory.h
@@ -322,6 +322,9 @@ class Factory final {
   Handle<FixedArray> CopyFixedArrayWithMap(Handle<FixedArray> array,
                                            Handle<Map> map);
 
+  Handle<FixedArray> CopyFixedArrayAndGrow(Handle<FixedArray> array,
+                                           int grow_by);
+
   Handle<FixedArray> CopyFixedArray(Handle<FixedArray> 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 c17a7ac..70a0267 100644 (file)
--- 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 48e3de8..526da06 100644 (file)
--- 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 51675d4..f4a637c 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -9539,9 +9539,9 @@ void SharedFunctionInfo::AddToOptimizedCodeMap(
     // Copy old map and append one new entry.
     Handle<FixedArray> old_code_map = Handle<FixedArray>::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 78de223..ac37f81 100644 (file)
--- 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<FixedArray> CopySize(Handle<FixedArray> array,
                                      int new_length,
                                      PretenureFlag pretenure = NOT_TENURED);

diff --git a/test/cctest/test-heap.cc b/test/cctest/test-heap.cc
index c54d67a..e476bcc 100644 (file)
--- 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<SharedFunctionInfo> shared;
+  {
+    HandleScope inner_scope(isolate);
+    CompileRun("function f() { return 1 }"
+               "f(); %OptimizeFunctionOnNextCall(f); f();");
+
+    Handle<JSFunction> f =
+        v8::Utils::OpenHandle(
+            *v8::Handle<v8::Function>::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> code;
+  {
+    HandleScope inner_scope(isolate);
+    CompileRun("function g() { return 2 }"
+               "g(); %OptimizeFunctionOnNextCall(g); g();");
+
+    Handle<JSFunction> g =
+        v8::Utils::OpenHandle(
+            *v8::Handle<v8::Function>::Cast(
+                CcTest::global()->Get(v8_str("g"))));
+    code = inner_scope.CloseAndEscape(handle(g->code(), isolate));
+    if (!code->is_optimized_code()) return;
+  }
+
+  Handle<FixedArray> lit = isolate->factory()->empty_fixed_array();
+  Handle<Context> 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 (file)
index 0000000..dbf35c9
--- /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.