[deoptimizer] Fix bug in OptimizedFrame::Summarize

GeorgNeis · Commit Bot · commit 3353a7d0b017 · 2021-03-11T08:39:55.000Z
OptimizedFrame::Summarize is used by debugger features etc to inspect the frame of an optimized function (and the virtual frames of functions that got inlined). It could end up materializing a JSArray with the same backing store as one that would later get left-trimmed, resulting in a dangling elements pointer. This CL fixes that by creating a fresh copy of the elements store instead. Bug: chromium:1182647 Change-Id: Iaf329464520a927b0ba33166cad2524d3752c450 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2748593 Reviewed-by: Benedikt Meurer <bmeurer@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Cr-Commit-Position: refs/heads/master@{#73330}
diff --git a/src/deoptimizer/translated-state.cc b/src/deoptimizer/translated-state.cc
@@ -1282,7 +1282,8 @@ Address TranslatedState::DecompressIfNeeded(intptr_t value) {
   }
 }
 
-TranslatedState::TranslatedState(const JavaScriptFrame* frame) {
+TranslatedState::TranslatedState(const JavaScriptFrame* frame)
+    : purpose_(kFrameInspection) {
   int deopt_index = Safepoint::kNoDeoptimizationIndex;
   DeoptimizationData data =
       static_cast<const OptimizedFrame*>(frame)->GetDeoptimizationData(
@@ -1672,25 +1673,63 @@ void TranslatedState::EnsureCapturedObjectAllocatedAt(
     }
 
     default:
-      CHECK(map->IsJSObjectMap());
       EnsureJSObjectAllocated(slot, map);
-      TranslatedValue* properties_slot = &(frame->values_[value_index]);
-      value_index++;
+      int remaining_children_count = slot->GetChildrenCount() - 1;
+
+      TranslatedValue* properties_slot = frame->ValueAt(value_index);
+      value_index++, remaining_children_count--;
       if (properties_slot->kind() == TranslatedValue::kCapturedObject) {
-        // If we are materializing the property array, make sure we put
-        // the mutable heap numbers at the right places.
+        // We are materializing the property array, so make sure we put the
+        // mutable heap numbers at the right places.
         EnsurePropertiesAllocatedAndMarked(properties_slot, map);
         EnsureChildrenAllocated(properties_slot->GetChildrenCount(), frame,
                                 &value_index, worklist);
+      } else {
+        CHECK_EQ(properties_slot->kind(), TranslatedValue::kTagged);
       }
-      // Make sure all the remaining children (after the map and properties) are
-      // allocated.
-      return EnsureChildrenAllocated(slot->GetChildrenCount() - 2, frame,
+
+      TranslatedValue* elements_slot = frame->ValueAt(value_index);
+      value_index++, remaining_children_count--;
+      if (elements_slot->kind() == TranslatedValue::kCapturedObject ||
+          !map->IsJSArrayMap()) {
+        // Handle this case with the other remaining children below.
+        value_index--, remaining_children_count++;
+      } else {
+        CHECK_EQ(elements_slot->kind(), TranslatedValue::kTagged);
+        elements_slot->GetValue();
+        if (purpose_ == kFrameInspection) {
+          // We are materializing a JSArray for the purpose of frame inspection.
+          // If we were to construct it with the above elements value then an
+          // actual deopt later on might create another JSArray instance with
+          // the same elements store. That would violate the key assumption
+          // behind left-trimming.
+          elements_slot->ReplaceElementsArrayWithCopy();
+        }
+      }
+
+      // Make sure all the remaining children (after the map, properties store,
+      // and possibly elements store) are allocated.
+      return EnsureChildrenAllocated(remaining_children_count, frame,
                                      &value_index, worklist);
   }
   UNREACHABLE();
 }
 
+void TranslatedValue::ReplaceElementsArrayWithCopy() {
+  DCHECK_EQ(kind(), TranslatedValue::kTagged);
+  DCHECK_EQ(materialization_state(), TranslatedValue::kFinished);
+  auto elements = Handle<FixedArrayBase>::cast(GetValue());
+  DCHECK(elements->IsFixedArray() || elements->IsFixedDoubleArray());
+  if (elements->IsFixedDoubleArray()) {
+    DCHECK(!elements->IsCowArray());
+    set_storage(isolate()->factory()->CopyFixedDoubleArray(
+        Handle<FixedDoubleArray>::cast(elements)));
+  } else if (!elements->IsCowArray()) {
+    set_storage(isolate()->factory()->CopyFixedArray(
+        Handle<FixedArray>::cast(elements)));
+  }
+}
+
 void TranslatedState::EnsureChildrenAllocated(int count, TranslatedFrame* frame,
                                               int* value_index,
                                               std::stack<int>* worklist) {
@@ -1755,6 +1794,7 @@ Handle<ByteArray> TranslatedState::AllocateStorageFor(TranslatedValue* slot) {
 
 void TranslatedState::EnsureJSObjectAllocated(TranslatedValue* slot,
                                               Handle<Map> map) {
+  CHECK(map->IsJSObjectMap());
   CHECK_EQ(map->instance_size(), slot->GetChildrenCount() * kTaggedSize);
 
   Handle<ByteArray> object_storage = AllocateStorageFor(slot);
diff --git a/src/deoptimizer/translated-state.h b/src/deoptimizer/translated-state.h
@@ -128,6 +128,8 @@ class TranslatedValue {
     return storage_;
   }
 
+  void ReplaceElementsArrayWithCopy();
+
   Kind kind_;
   MaterializationState materialization_state_ = kUninitialized;
   TranslatedState* container_;  // This is only needed for materialization of
@@ -346,7 +348,15 @@ class TranslatedFrame {
 
 class TranslatedState {
  public:
-  TranslatedState() = default;
+  // There are two constructors, each for a different purpose:
+
+  // The default constructor is for the purpose of deoptimizing an optimized
+  // frame (replacing it with one or several unoptimized frames). It is used by
+  // the Deoptimizer.
+  TranslatedState() : purpose_(kDeoptimization) {}
+
+  // This constructor is for the purpose of merely inspecting an optimized
+  // frame. It is used by stack trace generation and various debugging features.
   explicit TranslatedState(const JavaScriptFrame* frame);
 
   void Prepare(Address stack_frame_pointer);
@@ -381,6 +391,12 @@ class TranslatedState {
  private:
   friend TranslatedValue;
 
+  // See the description of the constructors for an explanation of the two
+  // purposes. The only actual difference is that in the kFrameInspection case
+  // extra work is needed to not violate assumptions made by left-trimming.  For
+  // details, see the code around ReplaceElementsArrayWithCopy.
+  enum Purpose { kDeoptimization, kFrameInspection };
+
   TranslatedFrame CreateNextTranslatedFrame(TranslationArrayIterator* iterator,
                                             FixedArray literal_array,
                                             Address fp, FILE* trace_file);
@@ -437,6 +453,7 @@ class TranslatedState {
   static Float32 GetFloatSlot(Address fp, int slot_index);
   static Float64 GetDoubleSlot(Address fp, int slot_index);
 
+  Purpose const purpose_;
   std::vector<TranslatedFrame> frames_;
   Isolate* isolate_ = nullptr;
   Address stack_frame_pointer_ = kNullAddress;
diff --git a/test/mjsunit/compiler/regress-1182647.js b/test/mjsunit/compiler/regress-1182647.js
@@ -0,0 +1,25 @@
+// Copyright 2021 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 --verify-heap
+
+function foo() {
+  const arr = Array(1000);
+
+  function bar() {
+    try { ({a: p4nda, b: arr.length}); } catch(e) {}
+  }
+
+  for (var i = 0; i < 25; i++) bar();
+
+  /p4nda/.test({});  // Deopt here.
+
+  arr.shift();
+}
+
+%PrepareFunctionForOptimization(foo);
+foo();
+foo();
+%OptimizeFunctionOnNextCall(foo);
+foo();
