Browse Source

chore: [32-x-y] cherry-pick 1 changes from 3-M126

* cdbc1d9684a3 from v8
Keeley Hammond 8 months ago
parent
commit
811a4eddf2
2 changed files with 149 additions and 0 deletions
  1. 1 0
      patches/v8/.patches
  2. 148 0
      patches/v8/cherry-pick-cdbc1d9684a3.patch

+ 1 - 0
patches/v8/.patches

@@ -1,3 +1,4 @@
 chore_allow_customizing_microtask_policy_per_context.patch
 deps_add_v8_object_setinternalfieldfornodecore.patch
 fix_disable_scope_reuse_associated_dchecks.patch
+cherry-pick-cdbc1d9684a3.patch

+ 148 - 0
patches/v8/cherry-pick-cdbc1d9684a3.patch

@@ -0,0 +1,148 @@
+From cdbc1d9684a3602c77c39d23b4e95a8522a0cc90 Mon Sep 17 00:00:00 2001
+From: Darius Mercadier <[email protected]>
+Date: Tue, 18 Jun 2024 16:10:26 +0200
+Subject: [PATCH] [turboshaft] Lower LoadStackArgument to a Tagged load
+
+If we start with a graph that looks like
+
+```
+x = LoadStackArgument(a, 40)
+...
+Allocate()
+...
+y = LoadStackArgument(a, 40)
+```
+
+This used to be lowered to
+
+```
+x1 = Load<WordPtr>(a, 40)
+x2 = TaggedBitcast(x1, WordPtr->Tagged)
+...
+Allocate()
+...
+y1 = Load<WordPtr>(a, 40)
+y2 = TaggedBitcast(y1, WordPtr->Tagged)
+```
+
+And then, Load Elimination would remove the second Load, and we'd get:
+
+```
+x1 = Load<WordPtr>(a, 40)
+x2 = TaggedBitcast(x1, WordPtr->Tagged)
+...
+Allocate()
+...
+y2 = TaggedBitcast(x1, WordPtr->Tagged)
+```
+
+And now we would be in trouble: if the allocation in the middle
+triggers a GC, then `x1` could move, and thus `y2` could refer to a
+stale pointer. In theory, Turbofan knows where tagged values are, and
+can thus update them when the GC moves things, but here, `x1` is not
+marked as Tagged (but rather as a raw WordPtr).
+
+This CL fixes this issue by doing a Tagged load from the start, since
+the value we're loading is clearly tagged.
+
+Fixed: chromium:347724915
+Change-Id: Ia659155fbc602907ab9a50fb992c79df6ccdaa44
+Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5630530
+Reviewed-by: Nico Hartmann <[email protected]>
+Auto-Submit: Darius Mercadier <[email protected]>
+Commit-Queue: Nico Hartmann <[email protected]>
+Cr-Commit-Position: refs/heads/main@{#94527}
+---
+
+diff --git a/src/compiler/access-builder.cc b/src/compiler/access-builder.cc
+index 258a9e5..6bdf19b 100644
+--- a/src/compiler/access-builder.cc
++++ b/src/compiler/access-builder.cc
+@@ -1153,16 +1153,6 @@
+ }
+ 
+ // static
+-ElementAccess AccessBuilder::ForStackArgument() {
+-  ElementAccess access = {
+-      kUntaggedBase,
+-      CommonFrameConstants::kFixedFrameSizeAboveFp - kSystemPointerSize,
+-      Type::NonInternal(), MachineType::Pointer(),
+-      WriteBarrierKind::kNoWriteBarrier};
+-  return access;
+-}
+-
+-// static
+ ElementAccess AccessBuilder::ForFixedDoubleArrayElement() {
+   ElementAccess access = {kTaggedBase, FixedDoubleArray::kHeaderSize,
+                           TypeCache::Get()->kFloat64, MachineType::Float64(),
+diff --git a/src/compiler/access-builder.h b/src/compiler/access-builder.h
+index 37332f1..0c9558a 100644
+--- a/src/compiler/access-builder.h
++++ b/src/compiler/access-builder.h
+@@ -337,9 +337,6 @@
+   // Provides access to SloppyArgumentsElements elements.
+   static ElementAccess ForSloppyArgumentsElementsMappedEntry();
+ 
+-  // Provides access to stack arguments
+-  static ElementAccess ForStackArgument();
+-
+   // Provides access to FixedDoubleArray elements.
+   static ElementAccess ForFixedDoubleArrayElement();
+ 
+diff --git a/src/compiler/turboshaft/machine-lowering-reducer-inl.h b/src/compiler/turboshaft/machine-lowering-reducer-inl.h
+index ce6dc64..1a6059a 100644
+--- a/src/compiler/turboshaft/machine-lowering-reducer-inl.h
++++ b/src/compiler/turboshaft/machine-lowering-reducer-inl.h
+@@ -2445,9 +2445,17 @@
+   }
+ 
+   V<Object> REDUCE(LoadStackArgument)(V<WordPtr> base, V<WordPtr> index) {
+-    V<WordPtr> argument = __ template LoadNonArrayBufferElement<WordPtr>(
+-        base, AccessBuilder::ForStackArgument(), index);
+-    return __ BitcastWordPtrToTagged(argument);
++    // Note that this is a load of a Tagged value
++    // (MemoryRepresentation::TaggedPointer()), but since it's on the stack
++    // where stack slots are all kSystemPointerSize, we use kSystemPointerSize
++    // for element_size_log2. On 64-bit plateforms with pointer compression,
++    // this means that we're kinda loading a 32-bit value from an array of
++    // 64-bit values.
++    return __ Load(
++        base, index, LoadOp::Kind::RawAligned(),
++        MemoryRepresentation::TaggedPointer(),
++        CommonFrameConstants::kFixedFrameSizeAboveFp - kSystemPointerSize,
++        kSystemPointerSizeLog2);
+   }
+ 
+   OpIndex REDUCE(StoreTypedElement)(OpIndex buffer, V<Object> base,
+diff --git a/test/mjsunit/compiler/regress-347724915.js b/test/mjsunit/compiler/regress-347724915.js
+new file mode 100644
+index 0000000..4a5d1a9
+--- /dev/null
++++ b/test/mjsunit/compiler/regress-347724915.js
+@@ -0,0 +1,26 @@
++// Copyright 2024 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
++
++function f(...args) {
++  let arr1 = [ undefined, undefined, undefined ];
++  %SimulateNewspaceFull();
++  arr1[0] = args[0];
++  // The following allocation will trigger a full GC, which will move the
++  // argument passed to the function (because it was a young object).
++  let arr2 = [ arr1 ];
++  // Here we're accessing `args[0]` again. This might be load-eliminated with
++  // the `args[0]` load from a few lines above, which has been moved by the GC
++  // since then. This should be fine though, as the loaded value should be
++  // marked as Tagged, which means that it shouldn't point to the stale value
++  // but instead have been updated during GC.
++  arr1[1] = args[0]
++  return arr2;
++}
++
++%PrepareFunctionForOptimization(f);
++let expected = f({ x : 42 });
++%OptimizeFunctionOnNextCall(f);
++assertEquals(expected, f({x : 42}));