Conversation
|
Review requested:
|
|
Compared to version 14.5, there is a new test failure that is not obvious. Local run: |
|
Additionally, snapshot is no longer reproducible: Edit: not macOS-specific: https://github.com/nodejs/node/actions/runs/22221811293/job/64279253109 |
|
Note that this changeset does not compile with GCC <14.1, unless the following patch is applied: diff --git a/deps/v8/src/objects/js-duration-format.cc b/deps/v8/src/objects/js-duration-format.cc
index 41bdbc2cc6..c9ef451713 100644
--- a/deps/v8/src/objects/js-duration-format.cc
+++ b/deps/v8/src/objects/js-duration-format.cc
@@ -807,7 +807,7 @@ void OutputFractional(const char* type, int64_t integer, int32_t powerOfTen,
// Pass in the value as int64_t and ask ICU to scale down.
nfOpts = nfOpts.scale(icu::number::Scale::powerOfTen(-powerOfTen));
- int64_t factor = static_cast<int64_t>(std::powl(10, powerOfTen));
+ int64_t factor = static_cast<int64_t>(std::pow(10.0L, powerOfTen));
int64_t bound = std::numeric_limits<int64_t>::max() / factor - 1;
UErrorCode status = U_ZERO_ERROR;
// Use faster ICU API formatInt if the value fit the precision int64_t, |
FYI @joyeecheung |
|
For what it's worth this branch seems to build ok with a RISC-V cross compiler too 👍🏻 (An experimental platform but I thought I'd mention it anyway ;-) ) |
|
@nodejs/platform-windows Can you please have a look at the Windows build failure? |
|
There's still this REPL strict mode issue: #61898 (comment) @nodejs/repl |
|
And the reproducible snapshot: #61898 (comment) @nodejs/startup (for lack of a more specific team) |
It's not so much a REPL issue as an issue with the VM global sandbox interceptors, introduced as a regression with 113b5cf. Previously, we were simulating CheckContextualStoreToJSGlobalObject-esque behaviour by rejecting interceptions on global proxy properties in strict mode if the receiver was not the global object. Now that we can no longer observe the receiver, that mechanism doesn't exist, and statements like (We should probably have VM tests for this.) |
We will either need to merge (or upstream) this, or change the GCC build requirement to >=14.1. |
|
Would you like to try and upstream it? |
|
It would be easier if someone with existing Chromium contributor status did the honours, I'd rather not jump through the hoops for a one-liner! |
|
Locally this fixes the snapshot reproducibility test for me See diffdiff --git a/deps/v8/src/builtins/builtins-proxy-gen.cc b/deps/v8/src/builtins/builtins-proxy-gen.cc
index 0bc45bac300..f0047f044f2 100644
--- a/deps/v8/src/builtins/builtins-proxy-gen.cc
+++ b/deps/v8/src/builtins/builtins-proxy-gen.cc
@@ -63,6 +63,10 @@ TNode<JSProxy> ProxiesCodeStubAssembler::AllocateProxy(
StoreObjectFieldNoWriteBarrier(proxy, JSProxy::kTargetOffset, target);
StoreObjectFieldNoWriteBarrier(proxy, JSProxy::kHandlerOffset, handler);
StoreObjectFieldNoWriteBarrier(proxy, JSProxy::kFlagsOffset, flags);
+#if TAGGED_SIZE_8_BYTES
+ StoreObjectFieldNoWriteBarrier(proxy, JSProxy::kPaddingOffset,
+ Int32Constant(0));
+#endif
return CAST(proxy);
}
diff --git a/deps/v8/src/heap/factory.cc b/deps/v8/src/heap/factory.cc
index b6f6938450c..e0117df19f9 100644
--- a/deps/v8/src/heap/factory.cc
+++ b/deps/v8/src/heap/factory.cc
@@ -3945,6 +3945,9 @@ Handle<JSProxy> Factory::NewJSProxy(DirectHandle<JSReceiver> target,
result->set_target(*target, SKIP_WRITE_BARRIER);
result->set_handler(*handler, SKIP_WRITE_BARRIER);
result->set_flags(JSProxy::IsRevocableBit::encode(revocable));
+#if TAGGED_SIZE_8_BYTES
+ result->set_padding(0);
+#endif
return handle(result, isolate());
}
Uploaded https://chromium-review.googlesource.com/c/v8/v8/+/7666243 |
Also uploaded https://chromium-review.googlesource.com/c/v8/v8/+/7666244 (IIUC, it was a libstdc++ issue fixed by https://gcc.gnu.org/pipermail/libstdc++/2023-February/055493.html) |
I'll take a look. Thanks for the ping. |
Will look into it. Thanks for the ping. |
So that snapshots with proxies can be reproducible. Refs: nodejs/node#61898 Change-Id: I01fac5e18c73cd482a1ae63750dbadf42a12e08a Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7666243 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/main@{#105830}
|
@targos, here is the patch to enable building on Windows: v8-146-fix.patch. Two small changes were needed. |
Original commit message:
Zero-initialize proxy padding
So that snapshots with proxies can be reproducible.
Refs: nodejs#61898
Change-Id: I01fac5e18c73cd482a1ae63750dbadf42a12e08a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7666243
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#105830}
Refs: v8/v8@edeb0a4
|
Thanks @StefanStojanovic and @joyeecheung. I pushed your fixes. Let's see how it goes on GH runners. |
Use the method without context parameter; the old API is deprecated. Refs: https://crrev.com/c/7141498
Use the new API which gets a `ModuleCachingCallback` parameter. Refs: https://crrev.com/c/7078551
|
I opened nodejs/build#4290 to skip smartos22 So we're waiting for these to get a green CI, but this PR is now ready! /cc @nodejs/v8-update @nodejs/tsc |
Original commit message:
[runtime] always sort transition arrays during rehashing
After rehashing, the arrays are no longer in hash-sorted order.
In this case, we need to force a re-sort even for small arrays,
so that subsequent linear searches can find the correct transition
and avoid inserting duplicates.
Refs: nodejs#61898 (comment)
Change-Id: Ia813d1fb9d23e08012811d672052d235c0e0bf4d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7723678
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#106255}
Refs: v8/v8@00f6e83
|
Needs a new change after #59034 |
PR for previous version: #61681