From 0b300d4b3dc8b2073f8acd4ccb51b1b9fd1d33d5 Mon Sep 17 00:00:00 2001 From: Paolo Severini Date: Wed, 24 Apr 2019 15:01:00 -0700 Subject: [PATCH] Reland "Generate unwind info on Win/x64 by default" The original CL title was updated to reflect CL contents. The --win64-unwinding-info flag still exists but it is set by default. This is a reland of efd8c2d9752c4206966dfd72e4794e025b9843e1 Original change's description: > Remove --win64-unwinding-info flag and always generate unwind info on Win/x64 > > The generation of unwind info to enable stack walking on Windows/x64 > (https://chromium-review.googlesource.com/c/v8/v8/+/1469329) was implemented > behind a temporary flag, in order to coordinate these changes with the > corresponding changes in Chromium. > > The required changes to Chromium > (https://chromium-review.googlesource.com/c/chromium/src/+/1474703) have also > been merged, so we can now remove the flag and enable the generation of stack > unwinding info by default on Windows/x64. > > Bug: v8:3598 > Change-Id: I88814aaeabecc007f5262227aa0681a1d16156d5 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1573138 > Reviewed-by: Michael Starzinger > Reviewed-by: Jakob Gruber > Reviewed-by: Ulan Degenbaev > Commit-Queue: Paolo Severini > Cr-Commit-Position: refs/heads/master@{#61020} Bug: v8:3598, chromium:958035 Change-Id: Ie53b39f3bb31567797a61e5110685284c266c1f9 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1599596 Commit-Queue: Jakob Gruber Reviewed-by: Jakob Gruber Reviewed-by: Michael Starzinger Reviewed-by: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#61368} --- BUILD.gn | 6 +----- src/flag-definitions.h | 3 +-- src/wasm/wasm-code-manager.cc | 21 +++++++++++++------ src/wasm/wasm-code-manager.h | 14 +++++++++++++ .../wasm/wasm-code-manager-unittest.cc | 16 ++++++++++++++ 5 files changed, 47 insertions(+), 13 deletions(-) diff --git a/BUILD.gn b/BUILD.gn index cf732be625..15acbdd21c 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -89,7 +89,7 @@ declare_args() { v8_enable_embedded_builtins = true # Enable the registration of unwinding info for Windows/x64. - v8_win64_unwinding_info = false + v8_win64_unwinding_info = true # Enable code comments for builtins in the snapshot (impacts performance). v8_enable_snapshot_code_comments = false @@ -1183,10 +1183,6 @@ template("run_mksnapshot") { args += invoker.args - if (v8_win64_unwinding_info) { - args += [ "--win64-unwinding-info" ] - } - if (v8_enable_embedded_builtins) { outputs += [ "$target_gen_dir/embedded${suffix}.S" ] args += [ diff --git a/src/flag-definitions.h b/src/flag-definitions.h index f7c4e715d7..f2df427a4d 100644 --- a/src/flag-definitions.h +++ b/src/flag-definitions.h @@ -1403,8 +1403,7 @@ DEFINE_STRING(redirect_code_traces_to, nullptr, DEFINE_BOOL(print_opt_source, false, "print source code of optimized and inlined functions") -DEFINE_BOOL(win64_unwinding_info, false, - "Enable unwinding info for Windows/x64 (experimental).") +DEFINE_BOOL(win64_unwinding_info, true, "Enable unwinding info for Windows/x64") #ifdef V8_TARGET_ARCH_ARM // Unsupported on arm. See https://crbug.com/v8/8713. diff --git a/src/wasm/wasm-code-manager.cc b/src/wasm/wasm-code-manager.cc index e078778307..e8815106d1 100644 --- a/src/wasm/wasm-code-manager.cc +++ b/src/wasm/wasm-code-manager.cc @@ -431,8 +431,8 @@ NativeModule::NativeModule(WasmEngine* engine, const WasmFeatures& enabled, // See src/heap/spaces.cc, MemoryAllocator::InitializeCodePageAllocator() and // https://cs.chromium.org/chromium/src/components/crash/content/app/crashpad_win.cc?rcl=fd680447881449fba2edcf0589320e7253719212&l=204 // for details. - if (win64_unwindinfo::CanRegisterUnwindInfoForNonABICompliantCodeRange() && - FLAG_win64_unwinding_info) { + if (engine_->code_manager() + ->CanRegisterUnwindInfoForNonABICompliantCodeRange()) { AllocateForCode(Heap::GetCodeRangeReservedAreaSize()); } #endif @@ -1040,11 +1040,22 @@ WasmCodeManager::WasmCodeManager(WasmMemoryTracker* memory_tracker, size_t max_committed) : memory_tracker_(memory_tracker), max_committed_code_space_(max_committed), +#if defined(V8_OS_WIN_X64) + is_win64_unwind_info_disabled_for_testing_(false), +#endif total_committed_code_space_(0), critical_committed_code_space_(max_committed / 2) { DCHECK_LE(max_committed, kMaxWasmCodeMemory); } +#if defined(V8_OS_WIN_X64) +bool WasmCodeManager::CanRegisterUnwindInfoForNonABICompliantCodeRange() const { + return win64_unwindinfo::CanRegisterUnwindInfoForNonABICompliantCodeRange() && + FLAG_win64_unwinding_info && + !is_win64_unwind_info_disabled_for_testing_; +} +#endif + bool WasmCodeManager::Commit(Address start, size_t size) { // TODO(v8:8462) Remove eager commit once perf supports remapping. if (FLAG_perf_prof) return true; @@ -1198,8 +1209,7 @@ std::shared_ptr WasmCodeManager::NewNativeModule( size); #if defined(V8_OS_WIN_X64) - if (win64_unwindinfo::CanRegisterUnwindInfoForNonABICompliantCodeRange() && - FLAG_win64_unwinding_info) { + if (CanRegisterUnwindInfoForNonABICompliantCodeRange()) { win64_unwindinfo::RegisterNonABICompliantCodeRange( reinterpret_cast(start), size); } @@ -1386,8 +1396,7 @@ void WasmCodeManager::FreeNativeModule(NativeModule* native_module) { code_space.address(), code_space.end(), code_space.size()); #if defined(V8_OS_WIN_X64) - if (win64_unwindinfo::CanRegisterUnwindInfoForNonABICompliantCodeRange() && - FLAG_win64_unwinding_info) { + if (CanRegisterUnwindInfoForNonABICompliantCodeRange()) { win64_unwindinfo::UnregisterNonABICompliantCodeRange( reinterpret_cast(code_space.address())); } diff --git a/src/wasm/wasm-code-manager.h b/src/wasm/wasm-code-manager.h index 1417986f8c..9fef25e6b9 100644 --- a/src/wasm/wasm-code-manager.h +++ b/src/wasm/wasm-code-manager.h @@ -555,6 +555,10 @@ class V8_EXPORT_PRIVATE WasmCodeManager final { } #endif +#if defined(V8_OS_WIN_X64) + bool CanRegisterUnwindInfoForNonABICompliantCodeRange() const; +#endif + NativeModule* LookupNativeModule(Address pc) const; WasmCode* LookupCode(Address pc) const; size_t committed_code_space() const { @@ -563,6 +567,12 @@ class V8_EXPORT_PRIVATE WasmCodeManager final { void SetMaxCommittedMemoryForTesting(size_t limit); +#if defined(V8_OS_WIN_X64) + void DisableWin64UnwindInfoForTesting() { + is_win64_unwind_info_disabled_for_testing_ = true; + } +#endif + static size_t EstimateNativeModuleCodeSize(const WasmModule* module); static size_t EstimateNativeModuleNonCodeSize(const WasmModule* module); @@ -590,6 +600,10 @@ class V8_EXPORT_PRIVATE WasmCodeManager final { size_t max_committed_code_space_; +#if defined(V8_OS_WIN_X64) + bool is_win64_unwind_info_disabled_for_testing_; +#endif + std::atomic total_committed_code_space_; // If the committed code space exceeds {critical_committed_code_space_}, then // we trigger a GC before creating the next module. This value is set to the diff --git a/test/unittests/wasm/wasm-code-manager-unittest.cc b/test/unittests/wasm/wasm-code-manager-unittest.cc index 9c7ed8a702..63a50edc9e 100644 --- a/test/unittests/wasm/wasm-code-manager-unittest.cc +++ b/test/unittests/wasm/wasm-code-manager-unittest.cc @@ -193,6 +193,12 @@ class WasmCodeManagerTest : public TestWithContext, void SetMaxCommittedMemory(size_t limit) { manager()->SetMaxCommittedMemoryForTesting(limit); } + + void DisableWin64UnwindInfoForTesting() { +#if defined(V8_OS_WIN_X64) + manager()->DisableWin64UnwindInfoForTesting(); +#endif + } }; // static @@ -212,6 +218,8 @@ TEST_P(WasmCodeManagerTest, EmptyCase) { TEST_P(WasmCodeManagerTest, AllocateAndGoOverLimit) { SetMaxCommittedMemory(page_size); + DisableWin64UnwindInfoForTesting(); + CHECK_EQ(0, manager()->committed_code_space()); NativeModulePtr native_module = AllocModule(page_size, GetParam()); CHECK(native_module); @@ -241,6 +249,8 @@ TEST_P(WasmCodeManagerTest, AllocateAndGoOverLimit) { TEST_P(WasmCodeManagerTest, TotalLimitIrrespectiveOfModuleCount) { SetMaxCommittedMemory(3 * page_size); + DisableWin64UnwindInfoForTesting(); + NativeModulePtr nm1 = AllocModule(2 * page_size, GetParam()); NativeModulePtr nm2 = AllocModule(2 * page_size, GetParam()); CHECK(nm1); @@ -255,6 +265,8 @@ TEST_P(WasmCodeManagerTest, TotalLimitIrrespectiveOfModuleCount) { TEST_P(WasmCodeManagerTest, GrowingVsFixedModule) { SetMaxCommittedMemory(3 * page_size); + DisableWin64UnwindInfoForTesting(); + NativeModulePtr nm = AllocModule(page_size, GetParam()); size_t module_size = GetParam() == Fixed ? kMaxWasmCodeMemory : page_size; size_t remaining_space_in_module = module_size - kJumpTableSize; @@ -275,6 +287,8 @@ TEST_P(WasmCodeManagerTest, GrowingVsFixedModule) { TEST_P(WasmCodeManagerTest, CommitIncrements) { SetMaxCommittedMemory(10 * page_size); + DisableWin64UnwindInfoForTesting(); + NativeModulePtr nm = AllocModule(3 * page_size, GetParam()); WasmCodeRefScope code_ref_scope; WasmCode* code = AddCode(nm.get(), 0, kCodeAlignment); @@ -290,6 +304,7 @@ TEST_P(WasmCodeManagerTest, CommitIncrements) { TEST_P(WasmCodeManagerTest, Lookup) { SetMaxCommittedMemory(2 * page_size); + DisableWin64UnwindInfoForTesting(); NativeModulePtr nm1 = AllocModule(page_size, GetParam()); NativeModulePtr nm2 = AllocModule(page_size, GetParam()); @@ -335,6 +350,7 @@ TEST_P(WasmCodeManagerTest, Lookup) { TEST_P(WasmCodeManagerTest, LookupWorksAfterRewrite) { SetMaxCommittedMemory(2 * page_size); + DisableWin64UnwindInfoForTesting(); NativeModulePtr nm1 = AllocModule(page_size, GetParam());