Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update V8: 12.6 -> 12.7 #2342

Merged
merged 7 commits into from
Jul 12, 2024
Merged

Update V8: 12.6 -> 12.7 #2342

merged 7 commits into from
Jul 12, 2024

Conversation

harrishancock
Copy link
Collaborator

This PR updates V8 from 12.6 to 12.7. My only concern here is the TracedReference dereferencing API removal. For now, I've just reverted the removal, and things seem to work -- but we need to find a way to move off of it soon.

@harrishancock harrishancock requested review from a team as code owners June 28, 2024 15:14
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a possible windows issue to investigate but over all LGTM once CI comes back green

@fhanau
Copy link
Collaborator

fhanau commented Jul 2, 2024

Based on https://chromium-review.googlesource.com/c/v8/v8/+/5600069 and https://chromium-review.googlesource.com/c/v8/v8/+/5596550 we should also update FP16 and abseil here to keep their versions in sync with V8.

@harrishancock
Copy link
Collaborator Author

Thanks @fhanau, I added an Abseil and FP16 update. I also updated to V8 12.7.224.11, which is now the latest patch release.

@fhanau
Copy link
Collaborator

fhanau commented Jul 3, 2024

The Windows build was stalled due to a transient bazel issue, I kicked it and it passed. PR LGTM.

@harrishancock harrishancock force-pushed the harris/update-v8-12.7 branch 5 times, most recently from 36a8f6a to 84519f2 Compare July 12, 2024 14:16
V8 update from 12.6.228.7 -> 12.7.224.12.

I noticed that V8 tests did not pass for either the current or new version of V8, and saw that our v8::Locker patch was to blame. I initially modified it to work, by exiting and re-entering in v8::Unlocker, but then decided to try just removing the patch entirely. All tests pass with it removed, so let's try. Removing the patch required adding a couple Isolate::Scopes in our code that were previously implicit.

V8 12.7 also ships with the AsyncDispose symbol, so I removed our polyfill.

Lastly, V8 now provides a SetFatalErrorHandler() function, which I call with our DCHECK-handling function. This will allow us to drop a separate patch from our internal repo.
This API was removed in V8 12.7, but we don't know how to safely replace our use of it yet.
With this new V8 commit, our Windows build broke with the following
error:

    external/v8/src/heap/base/asm/x64/push_registers_asm.cc(24,2): error: "The masm based version must be used for Windows"
@harrishancock harrishancock merged commit 59d61ab into main Jul 12, 2024
9 checks passed
@harrishancock harrishancock deleted the harris/update-v8-12.7 branch July 12, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants