Conversation
|
Review requested:
|
nodejs-github-bot
added
build
|
Compared to version 14.5, there is a new test failure that is not obvious. Local run: |
targos
added
the
semver-major
|
Additionally, snapshot is no longer reproducible: Edit: not macOS-specific: https://github.com/nodejs/node/actions/runs/22221811293/job/64279253109 |
targos
added
the
help wanted
|
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. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Okay, now we're back to a debug build error that already happened with earlier V8 versions. @joyeecheung found a fix for it (093b60b) but that doesn't work anymore, because debug builds also create a release binary and the |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Original commit message:
Mark MemCopyAndSwitchEndianness src input as `const`
Needed after this CL: http://crrev.com/c/7600437
Currently getting the following error:
```
candidate function not viable: no known conversion from 'const unsigned char *' to 'void *'
```
Change-Id: I0c0f065b822e0f95ffd06207d280fc8b7bab4403
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7601332
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Milad Farazmand <mfarazma@ibm.com>
Cr-Commit-Position: refs/heads/main@{#105413}
Refs: v8/v8@daf4656
|
@nodejs/platform-smartos PTAL: On SmartOS 22, there is a compile error: https://ci.nodejs.org/job/node-test-commit-smartos/64813/nodes=smartos22-x64/console On SmartOS 23, there is a linker error: https://ci.nodejs.org/job/node-test-commit-smartos/64813/nodes=smartos23-x64/console |
Running this build on a SmartOS 24.4 LTS zone locally. If it fails I will provide appropriate diffs. If it succeeds, we'll need to see what is differeing in 23.4 LTS pkgsrc, OR the platform image your jenkins agent(s) are running on. |
But in this way, Node.js can no longer distinguish between |
24.4 LTS misbehaves the same as 23.4 LTS. Duplicate symbols. Here's an example error: I don't know v8 well enough but why do we have both |
|
It is expected to have both. Lines 295 to 363 in 53bcd11 |
|
I can try to remove this hack. Maybe V8 has changed enough to make the problem disappear? |
It now builds (have not run |
|
@legendecas: The difference between contextual stores ( I think I don't understand the motivation for the necessity to distinguish the contextual stores ( BTW, given that |
With a code snippet like const vm = require('node:vm')
const ctx = {};
vm.createContext(ctx);
vm.runInContext('"use strict"; x = 5', ctx); // ReferenceError
ctx.x // => should be undefinedHere, Both [[Set]] for According to https://tc39.es/ecma262/#sec-object-environment-records-setmutablebinding-n-v-s, if V8 could call But right now, V8 calls |
|
@legendecas: Thank you for the explanation! Let me think what we can do to make |
Seems to be better, but compared with prior releases seems to have a few more failures. Full output available upon request. |
|
@danmcd the tests passed on SmartOS 23 in the CI run: https://ci.nodejs.org/job/node-test-commit-smartos/64850/nodes=smartos23-x64/ |
|
Well actually AIX failed with the same error (/cc @nodejs/platform-aix). |
|
The main blocker is still the debug build, btw /cc @joyeecheung |
13 participants
PR for previous version: #61681