V8 v12.4 patch #16
V8 v12.4 patch #16
Conversation
Original commit message:
[compiler] reset script details in functions deserialized from code cache
During the serialization of the code cache, V8 would wipe out the
host-defined options, so after a script id deserialized from the
code cache, the host-defined options need to be reset on the script
using what's provided by the embedder when doing the deserializing
compilation, otherwise the HostImportModuleDynamically callbacks
can't get the data it needs to implement dynamic import().
Change-Id: I33cc6a5e43b6469d3527242e083f7ae6d8ed0c6a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5401780
Reviewed-by: Leszek Swirski <[email protected]>
Commit-Queue: Joyee Cheung <[email protected]>
Cr-Commit-Position: refs/heads/main@{#93323}
Refs: v8/v8@cd10ad7
PR-URL: nodejs#52535
Refs: nodejs#47472
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Refs: v8/v8@6196de8 Refs: v8/v8@4af6461 Refs: v8/v8@7ba16ea
Our Linux build infra is not ready for it yet, but V8 is making it difficult to compile on Windows without it. Refs: nodejs#45402
After enabling -std:c++20 on Windows patch is now much smaller.
|
A quick update on this V8 update. After enabling C++20, I noticed the debug build crashed when building a simple Hello World add-on. That got me thinking about a huge patch I had and I started fresh. After making changes to 2 files, the compilation was successful. The problem that is still here, is that the debug binary still crashes when building a simple addon, and now, there are very few changes I did so I doubt this PR introduced it. I'll investigate this further to try and get to the bottom of it. Release build builds simple add-on without any issues. Btw, here is the debug assertion that fails. If ignored, building the add-on will pass, but since this is ran as sanity check in debug compilation in CI, there is no way to click |
|
If you can get a stack trace that points to our source, it will probably helkp a lot |
Yep, I debugged it a bit in VS and I plan to investigate this more later in the evening, will be away until then. |
|
It's very easy to trigger the crash: |
|
|
|
|
|
It seems to happen because begin and end of the iterator are the same? |
|
Thanks for the investigation @targos! Your example (
This is correct. Making an empty I see you've already reverted the V8 CL introducing this, but just in case CL has some useful stuff, I have a simple one-liner fix I pushed to my branch in JaneaSystems@03cc860 that should work (tested it locally). |
|
I'm not entirely confident in adding a condition to the branch. I think it's safe to revert the change as it is relatively small and focused on performance. Closing the PR, as I incorporated your changes to my branch already. Thanks a lot! |
|
I created a V8 issue: https://issues.chromium.org/issues/336349658 |
This patches V8 v12.4 for Windows, by fixing compilation errors. After enabling
C++20, this patch is much smaller than previous ones as most of the previously patched compilation errors were C++20-specific code as it turned out.The changes introduced here are strictly meant as a patch only, so they shouldn't be pushed upstream.
Refs: #13
Refs: #14
Refs: #15