macOS: Enable wasm and allow to load .wasm on Apple silicon #23257

dio · 2022-09-27T03:49:23Z

Commit Message: This applies changes from https://chromium-review.googlesource.com/c/v8/v8/+/3700352 as a fix for MemoryAllocator::PartialFreeMemory() which shouldn't try to change permissions of RWX pages.

This mainly affects macOS > 11.2 due to mprotect() behavior changes (#23243, https://chromium-review.googlesource.com/c/v8/v8/+/3610445/comment/6294ada3_a3cab643/) on Apple silicon.

This also flips the Wasm feature to be enabled by default on macOS.

The future patch removal task is tracked in #23258.

Signed-off-by: Dhi Aurrahman [email protected]

Additional description:
By applying this patch on my local (macOS, 12.6 (21G115), M1), I can run envoy to load a .wasm from the examples/wasm-cc. Before this patch, it was caught by: EACCESS error. See #23243 for more details.

$ uname -a
Darwin Dhis-MacBook-Air.local 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:20:05 PDT 2022; root:xnu-8020.140.49~2/RELEASE_ARM64_T8101 arm64
$ pwd
/Users/dio/src/envoy/examples/wasm-cc
$ ~/src/envoy/bazel-bin/source/exe/envoy-static -c envoy.yaml

Also, I think we need to backport this to release/1.23 (which has the same v8 version), hence the Homebrew build for Apple silicon will have this Wasm feature enabled.

Risk Level: Low, since the current behavior is preserved, and the patch is included on upstream (v8, 10.5-lkgr has it)
Testing: existing, and manual testing on macOS M1
Docs Changes: N/A
Release Notes: N/A (should we add an announcement since we enable wasm by default again?)
Platform Specific Features: enable wasm (v8 runtime) on macOS by default
Fixes #23243

This applies https://chromium-review.googlesource.com/c/v8/v8/+/3700352 as a fix for MemoryAllocator::PartialFreeMemory() which shouldn't try to change permissions of RWX pages. This mainly affects macOS > 11.2 due to mprotect behavior changes (envoyproxy#23243) on Apple silicon. Signed-off-by: Dhi Aurrahman <[email protected]>

repokitteh-read-only · 2022-09-27T03:49:30Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #23257 was opened by dio.

see: more, trace.

lizan · 2022-09-27T05:55:54Z

bazel/v8.patch

@@ -7,13 +7,15 @@
 # 7. Fix build errors in SIMD IndexOf/includes (https://crrev.com/c/3749192).
 # 8. Fix build on arm64.
 # 9. Fix build on older versions of Linux.
+# 10. Fix MemoryAllocator::PartialFreeMemory() which shouldn't try to change permissions of RWX pages,
+#     mainly affecting macOS on Apple silicon (https://chromium-review.googlesource.com/c/v8/v8/+/3700352).


Can you file an issue to track when this is released?

Tracked here: #23258.

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2022-09-27T11:00:46Z

Thanks, @lizan! 🙇🏾

RyanTheOptimist · 2022-09-27T13:58:23Z

/lgtm deps

PiotrSikora

LGTM, but quite frankly I'd first try upgrading to v10.6 (or even v10.7, really).

PiotrSikora · 2022-09-28T02:17:42Z

bazel/v8.patch

   const int target_align = 32;
   // Scalar loop to reach desired alignment
 @@ -256,21 +266,21 @@ TARGET_AVX2 inline uintptr_t fast_search_avx(T* array, uintptr_t array_len,
   }
- 
+


Nit: This PR contains a dozen of unnecessary (whitespace) changes that make this much less readable, consider unstaging them.

Yeah sorry, my editor seems too eager to remove "trailing" whitespaces.

Much better now, thanks!

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2022-09-28T03:04:47Z

@PiotrSikora sure. I'll try. However, since release/1.23 is on this 10.4.132.18 I think we can backport this for release/1.23 first, and not sure if updating deps (with minor release version) can be done in stable branches.

dio · 2022-09-28T03:12:14Z

(sorry for the spammy request for re-reviewing this, seems like there was a glitch on GitHub on my side)

PiotrSikora · 2022-09-28T03:47:27Z

@PiotrSikora sure. I'll try. However, since release/1.23 is on this 10.4.132.18 I think we can backport this for release/1.23 first, and not sure if updating deps (with minor release version) can be done in stable branches.

Good point. I guess we could merge this (since it's already approved) and then update V8 in a follow-up PR.

dio · 2022-09-28T12:52:58Z

@RyanTheOptimist seems like we need another stamp (for deps).

RyanTheOptimist · 2022-09-28T13:45:15Z

/lgtm deps

dio · 2022-09-28T13:57:43Z

@lizan could you help to approve and merge this? 🙇🏾

wbpcode

LGTM. Thanks.

…xy#23257) * macOS: Allow to load .wasm on Apple silicon This applies https://chromium-review.googlesource.com/c/v8/v8/+/3700352 as a fix for MemoryAllocator::PartialFreeMemory() which shouldn't try to change permissions of RWX pages. This mainly affects macOS > 11.2 due to mprotect behavior changes (envoyproxy#23243) on Apple silicon.

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 27, 2022

repokitteh-read-only bot assigned RyanTheOptimist Sep 27, 2022

lizan reviewed Sep 27, 2022

View reviewed changes

dio mentioned this pull request Sep 27, 2022

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

Closed

Fix comment

43fac2b

Signed-off-by: Dhi Aurrahman <[email protected]>

lizan previously approved these changes Sep 27, 2022

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 27, 2022

PiotrSikora previously approved these changes Sep 28, 2022

View reviewed changes

dio added 2 commits September 28, 2022 10:00

Remove whitespace changes

335ba8c

Signed-off-by: Dhi Aurrahman <[email protected]>

Add comment

b40a61d

Signed-off-by: Dhi Aurrahman <[email protected]>

dio dismissed stale reviews from PiotrSikora and lizan via b40a61d September 28, 2022 03:01

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 28, 2022

dio requested review from PiotrSikora and lizan and removed request for PiotrSikora and lizan September 28, 2022 03:06

PiotrSikora approved these changes Sep 28, 2022

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 28, 2022

wbpcode approved these changes Sep 29, 2022

View reviewed changes

wbpcode merged commit 63f27a6 into envoyproxy:main Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

macOS: Enable wasm and allow to load .wasm on Apple silicon #23257

macOS: Enable wasm and allow to load .wasm on Apple silicon #23257

dio commented Sep 27, 2022 •

edited

Loading

repokitteh-read-only bot commented Sep 27, 2022

lizan Sep 27, 2022

dio Sep 27, 2022

dio commented Sep 27, 2022

RyanTheOptimist commented Sep 27, 2022

PiotrSikora left a comment

PiotrSikora Sep 28, 2022

dio Sep 28, 2022

PiotrSikora Sep 28, 2022

dio commented Sep 28, 2022 •

edited

Loading

dio commented Sep 28, 2022

PiotrSikora commented Sep 28, 2022

dio commented Sep 28, 2022

RyanTheOptimist commented Sep 28, 2022

dio commented Sep 28, 2022

wbpcode left a comment

macOS: Enable wasm and allow to load .wasm on Apple silicon #23257

macOS: Enable wasm and allow to load .wasm on Apple silicon #23257

Conversation

dio commented Sep 27, 2022 • edited Loading

repokitteh-read-only bot commented Sep 27, 2022

lizan Sep 27, 2022

Choose a reason for hiding this comment

dio Sep 27, 2022

Choose a reason for hiding this comment

dio commented Sep 27, 2022

RyanTheOptimist commented Sep 27, 2022

PiotrSikora left a comment

Choose a reason for hiding this comment

PiotrSikora Sep 28, 2022

Choose a reason for hiding this comment

dio Sep 28, 2022

Choose a reason for hiding this comment

PiotrSikora Sep 28, 2022

Choose a reason for hiding this comment

dio commented Sep 28, 2022 • edited Loading

dio commented Sep 28, 2022

PiotrSikora commented Sep 28, 2022

dio commented Sep 28, 2022

RyanTheOptimist commented Sep 28, 2022

dio commented Sep 28, 2022

wbpcode left a comment

Choose a reason for hiding this comment

dio commented Sep 27, 2022 •

edited

Loading

dio commented Sep 28, 2022 •

edited

Loading