From 5d4acc4eeac416cbbfd280819c15ec6b1b1efe56 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Fri, 25 Feb 2022 17:04:17 +0100 Subject: [PATCH] [wasm] Avoid passing nullptr to CodeSpaceWriteScope After https://crrev.com/c/3484317, passing {nullptr} to the {CodeSpaceWriteScope} won't work any more. Since the tests do not have a {NativeModule} to pass instead, make them use {pthread_jit_write_protect_np} directly. The jump-table assembler tests have dedicated threads for writing and executing the code, so we just switch once per thread. The icache test switches between writing and executing, so we use a little struct for switching. R=jkummerow@chromium.org, tebbi@chromium.org Bug: v8:12644, v8:11974 Change-Id: I116f3ad75454f749cdc4635802a4617ff91548b2 Cq-Include-Trybots: luci.v8.try:v8_mac_arm64_rel_ng Cq-Include-Trybots: luci.v8.try:v8_mac_arm64_dbg_ng Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3487995 Reviewed-by: Tobias Tebbi Reviewed-by: Jakob Kummerow Commit-Queue: Clemens Backes Cr-Commit-Position: refs/heads/main@{#79290} --- src/wasm/code-space-access.cc | 1 + src/wasm/code-space-access.h | 2 +- test/cctest/test-icache.cc | 28 +++++++++++++------ test/cctest/wasm/test-jump-table-assembler.cc | 21 ++++++++------ 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/wasm/code-space-access.cc b/src/wasm/code-space-access.cc index baae9f5716..f473439441 100644 --- a/src/wasm/code-space-access.cc +++ b/src/wasm/code-space-access.cc @@ -18,6 +18,7 @@ thread_local NativeModule* CodeSpaceWriteScope::current_native_module_ = // writable mode; only the main thread has to switch back and forth. CodeSpaceWriteScope::CodeSpaceWriteScope(NativeModule* native_module) : previous_native_module_(current_native_module_) { + DCHECK_NOT_NULL(native_module); if (previous_native_module_ == native_module) return; current_native_module_ = native_module; if (previous_native_module_ == nullptr || SwitchingPerNativeModule()) { diff --git a/src/wasm/code-space-access.h b/src/wasm/code-space-access.h index 733d5d38f9..502c406b2b 100644 --- a/src/wasm/code-space-access.h +++ b/src/wasm/code-space-access.h @@ -45,7 +45,7 @@ class NativeModule; // permissions for all code pages. class V8_NODISCARD CodeSpaceWriteScope final { public: - explicit V8_EXPORT_PRIVATE CodeSpaceWriteScope(NativeModule* native_module); + explicit V8_EXPORT_PRIVATE CodeSpaceWriteScope(NativeModule*); V8_EXPORT_PRIVATE ~CodeSpaceWriteScope(); // Disable copy constructor and copy-assignment operator, since this manages diff --git a/test/cctest/test-icache.cc b/test/cctest/test-icache.cc index 93e083d73b..ed757fc5ee 100644 --- a/test/cctest/test-icache.cc +++ b/test/cctest/test-icache.cc @@ -184,6 +184,24 @@ TEST(TestFlushICacheOfWritableAndExecutable) { Isolate* isolate = CcTest::i_isolate(); HandleScope handles(isolate); + struct V8_NODISCARD EnableWritePermissionsOnMacArm64Scope { +#if defined(V8_OS_DARWIN) && defined(V8_HOST_ARCH_ARM64) +// Ignoring this warning is considered better than relying on +// __builtin_available. +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wunguarded-availability-new" + EnableWritePermissionsOnMacArm64Scope() { pthread_jit_write_protect_np(0); } + ~EnableWritePermissionsOnMacArm64Scope() { + pthread_jit_write_protect_np(1); + } +#pragma clang diagnostic pop +#else + EnableWritePermissionsOnMacArm64Scope() { + // Define a constructor to avoid unused variable warnings. + } +#endif + }; + for (int i = 0; i < kNumIterations; ++i) { auto buffer = AllocateAssemblerBuffer(kBufferSize, nullptr, VirtualMemory::kMapAsJittable); @@ -194,19 +212,13 @@ TEST(TestFlushICacheOfWritableAndExecutable) { CHECK(SetPermissions(GetPlatformPageAllocator(), buffer->start(), buffer->size(), v8::PageAllocator::kReadWriteExecute)); { -#if defined(V8_OS_DARWIN) && defined(V8_HOST_ARCH_ARM64) - // Make sure to switch memory to writable on M1 hardware. - wasm::CodeSpaceWriteScope code_space_write_scope(nullptr); -#endif + EnableWritePermissionsOnMacArm64Scope write_scope; FloodWithInc(isolate, buffer.get()); FlushInstructionCache(buffer->start(), buffer->size()); } CHECK_EQ(23 + kNumInstr, f.Call(23)); // Call into generated code. { -#if defined(V8_OS_DARWIN) && defined(V8_HOST_ARCH_ARM64) - // Make sure to switch memory to writable on M1 hardware. - wasm::CodeSpaceWriteScope code_space_write_scope(nullptr); -#endif + EnableWritePermissionsOnMacArm64Scope write_scope; FloodWithNop(isolate, buffer.get()); FlushInstructionCache(buffer->start(), buffer->size()); } diff --git a/test/cctest/wasm/test-jump-table-assembler.cc b/test/cctest/wasm/test-jump-table-assembler.cc index 39c89936f8..56619e7763 100644 --- a/test/cctest/wasm/test-jump-table-assembler.cc +++ b/test/cctest/wasm/test-jump-table-assembler.cc @@ -55,6 +55,17 @@ constexpr uint32_t kAvailableBufferSlots = 0; constexpr uint32_t kBufferSlotStartOffset = 0; #endif +void EnsureThreadHasWritePermissions() { +#if defined(V8_OS_DARWIN) && defined(V8_HOST_ARCH_ARM64) +// Ignoring this warning is considered better than relying on +// __builtin_available. +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wunguarded-availability-new" + pthread_jit_write_protect_np(0); +#pragma clang diagnostic pop +#endif +} + Address AllocateJumpTableThunk( Address jump_target, byte* thunk_slot_buffer, std::bitset* used_slots, @@ -203,10 +214,7 @@ class JumpTablePatcher : public v8::base::Thread { void Run() override { TRACE("Patcher %p is starting ...\n", this); -#if defined(V8_OS_DARWIN) && defined(V8_HOST_ARCH_ARM64) - // Make sure to switch memory to writable on M1 hardware. - CodeSpaceWriteScope code_space_write_scope(nullptr); -#endif + EnsureThreadHasWritePermissions(); Address slot_address = slot_start_ + JumpTableAssembler::JumpSlotIndexToOffset(slot_index_); // First, emit code to the two thunks. @@ -261,16 +269,13 @@ TEST(JumpTablePatchingStress) { // Iterate through jump-table slots to hammer at different alignments within // the jump-table, thereby increasing stress for variable-length ISAs. Address slot_start = reinterpret_cast
(buffer->start()); + EnsureThreadHasWritePermissions(); for (int slot = 0; slot < kJumpTableSlotCount; ++slot) { TRACE("Hammering on jump table slot #%d ...\n", slot); uint32_t slot_offset = JumpTableAssembler::JumpSlotIndexToOffset(slot); std::vector> thunk_buffers; std::vector
patcher_thunks; { -#if defined(V8_OS_DARWIN) && defined(V8_HOST_ARCH_ARM64) - // Make sure to switch memory to writable on M1 hardware. - CodeSpaceWriteScope code_space_write_scope(nullptr); -#endif // Patch the jump table slot to jump to itself. This will later be patched // by the patchers. Address slot_addr =