mirror of https://github.com/electron/electron
113 lines
4.1 KiB
Diff
113 lines
4.1 KiB
Diff
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
|
|
From: Darius Mercadier <dmercadier@chromium.org>
|
|
Date: Tue, 18 Jun 2024 16:10:26 +0200
|
|
Subject: 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 <nicohartmann@chromium.org>
|
|
Auto-Submit: Darius Mercadier <dmercadier@chromium.org>
|
|
Commit-Queue: Nico Hartmann <nicohartmann@chromium.org>
|
|
Cr-Commit-Position: refs/heads/main@{#94527}
|
|
|
|
diff --git a/src/compiler/turboshaft/machine-lowering-reducer-inl.h b/src/compiler/turboshaft/machine-lowering-reducer-inl.h
|
|
index 8f37ef00f7edc1395586439fad4c39f426520d87..1f8ecb428f7d53d20369b82d958b2309ab09d2eb 100644
|
|
--- a/src/compiler/turboshaft/machine-lowering-reducer-inl.h
|
|
+++ b/src/compiler/turboshaft/machine-lowering-reducer-inl.h
|
|
@@ -2372,9 +2372,17 @@ class MachineLoweringReducer : public Next {
|
|
}
|
|
|
|
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 0000000000000000000000000000000000000000..4a5d1a9a2e3dd7674bf0872c94a971b5f28ddf72
|
|
--- /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}));
|