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 ea4f69a96b863957f6de9255e4f649e4096875f1..bc5d1e4e0ecbd424df06bcf248678116d5fcbfda 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 9a595c411566288711fccba9a1a8e68908609b18..c52d6d31d94c2543881198c93e5de18ccfc1dca7 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 c17a7ac8789cb37ec499407fd2aad964611f7b81..70a02679f1d888a8e9902a9e66c5e0542f7505d9 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 48e3de8ea4edbbc4abfd9ae85132d4ade9f1462d..526da067b7d8e21ceea9686d63e4d7c2ac27952c 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 51675d4c21597465a1e4fa790127b4e33d411d6f..f4a637c008612f9faaf858d14d00855c3865576e 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 78de2235b7c991b5b36c395bcb3a9d12116a18a8..ac37f817798ae4ba50b1f37db7f18f412e0e93b6 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 c54d67aeeb32f100a391369a10625be93b39d665..e476bcca84665990347988a1780298a134086fe5 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.