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

Remove mprotect()/rwx-related patch entry from bazel/v8.patch #23258

Closed
dio opened this issue Sep 27, 2022 · 10 comments · Fixed by #23434
Closed

Remove mprotect()/rwx-related patch entry from bazel/v8.patch #23258

dio opened this issue Sep 27, 2022 · 10 comments · Fixed by #23434
Labels
area/wasm no stalebot Disables stalebot from closing an issue

Comments

@dio
Copy link
Member

dio commented Sep 27, 2022

A patch is introduced by #23257 to fix MemoryAllocator::PartialFreeMemory() on v8 10.4.132.18 (

version = "10.4.132.18",
). See #23243 for more details.

That patch is copied from https://chromium-review.googlesource.com/c/v8/v8/+/3700352. That commit on v8 upstream is included in 10.5 (can be checked from 10.5-lkgr: https://chromium.googlesource.com/v8/v8.git/+/refs/heads/10.5-lkgr/src/heap/memory-allocator.cc#407) hence when we adopt 10.5 or higher, we should remove that patch (probably most of the patches will be removed/adapted anyway).

@dio dio added the triage Issue requires triage label Sep 27, 2022
@yanavlasov yanavlasov added area/wasm no stalebot Disables stalebot from closing an issue and removed triage Issue requires triage labels Sep 27, 2022
@dio
Copy link
Member Author

dio commented Sep 28, 2022

Seems like we are aiming for 10.7: #23257 (review)

@dio
Copy link
Member Author

dio commented Oct 1, 2022

@PiotrSikora I tried to upgrade v8, however, I stumbled upon: https://chromium-review.googlesource.com/c/v8/v8/+/3834031. Do you think we need to have this enabled by default too (hence reverting that commit) + adding a flag to enable V8_USE_ZLIB?

Note: zlib is enabled by default in 10.7-lkgr: https://github.com/v8/v8/blob/6c8b357a84847a479cd329478522feefc1c3195a/BUILD.gn#L373-L376.

@PiotrSikora
Copy link
Contributor

@dio you can ignore it, snapshot compression was never enabled in the Bazel build anyway. Did you run into any issues because of that change?

@dio
Copy link
Member Author

dio commented Oct 3, 2022

Did you run into any issues because of that change?

No. Without zlib we're good. Thanks!

@PiotrSikora I asked since previously we patched zlib header path.

@dio
Copy link
Member Author

dio commented Oct 5, 2022

Submitted proxy-wasm/proxy-wasm-cpp-host#310 to have proxy-wasm-cpp-host to use newer V8, 10.7.193.13 (with some updated --test_timeouts).

@dio
Copy link
Member Author

dio commented Oct 10, 2022

Seems like proxy-wasm/proxy-wasm-cpp-host#310 is merged (the updated v8 version is v10.7.193.13). I can set up my own bucket at first to serve the required snapshot for submitting a PR (to upgrade the v8 version for Envoy), but then we need someone who has access to provide the snapshot on https://storage.googleapis.com/envoyproxy-wee8 (ref:

urls = ["https://storage.googleapis.com/envoyproxy-wee8/v8-{version}.tar.gz"],
) using https://storage.googleapis.com/envoyproxy-wee8/wee8-fetch-deps.sh.

cc. @envoyproxy/wasm-dev

@PiotrSikora
Copy link
Contributor

@mpwarres should have access to generate and upload them.

@mpwarres
Copy link
Contributor

@mpwarres should have access to generate and upload them.

Ack, ran out of time to do this today, but will get to it tomorrow.

@mpwarres
Copy link
Contributor

The following files have been added to the https://storage.googleapis.com/envoyproxy-wee8 bucket:

v8-10.7.193.13.tar.gz
v8-10.7.193.13-deps.sha256
chromium-base_trace_event_common-521ac34ebd795939c7e16b37d9d3ddb40e8ed556.tar.gz
chromium-icu-20f8ac695af59b6c830def7d4e95bfeb13dd7be5.tar.gz
chromium-zlib-f48cb14d487038d20c85680e29351e095a0fea8b.tar.gz

@dio
Copy link
Member Author

dio commented Oct 12, 2022

Thanks, @mpwarres!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/wasm no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants