From 294c76f7c4b4c5a0aa45372a2d32a2fdc48bbf00 Mon Sep 17 00:00:00 2001 From: Triang3l Date: Wed, 16 Feb 2022 20:37:53 +0300 Subject: [PATCH 01/24] [UI] Remove `virtual` from Window::IsFullscreen (tracked entirely by common code) --- src/xenia/ui/window.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xenia/ui/window.h b/src/xenia/ui/window.h index 564eaeaea..f3f3bb348 100644 --- a/src/xenia/ui/window.h +++ b/src/xenia/ui/window.h @@ -280,7 +280,7 @@ class Window { // Desired state stored by the common Window, modifiable both externally and // by the implementation (including from SetFullscreen itself). - virtual bool IsFullscreen() const { return fullscreen_; } + bool IsFullscreen() const { return fullscreen_; } void SetFullscreen(bool new_fullscreen); // Desired state stored by the common Window, externally modifiable, read-only From d64848245d324a50835dc5cd96cb00513a8249b5 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Wed, 16 Feb 2022 00:17:28 +0100 Subject: [PATCH 02/24] [CPU] Improve vrefp accuracy --- src/xenia/cpu/backend/x64/x64_emitter.cc | 3 ++- src/xenia/cpu/backend/x64/x64_emitter.h | 3 ++- src/xenia/cpu/backend/x64/x64_sequences.cc | 28 +++++++++++++++++----- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/xenia/cpu/backend/x64/x64_emitter.cc b/src/xenia/cpu/backend/x64/x64_emitter.cc index 5cd6780be..97b14e03e 100644 --- a/src/xenia/cpu/backend/x64/x64_emitter.cc +++ b/src/xenia/cpu/backend/x64/x64_emitter.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2020 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -653,6 +653,7 @@ void X64Emitter::MovMem64(const Xbyak::RegExp& addr, uint64_t v) { static const vec128_t xmm_consts[] = { /* XMMZero */ vec128f(0.0f), /* XMMOne */ vec128f(1.0f), + /* XMMOnePD */ vec128d(1.0), /* XMMNegativeOne */ vec128f(-1.0f, -1.0f, -1.0f, -1.0f), /* XMMFFFF */ vec128i(0xFFFFFFFFu, 0xFFFFFFFFu, 0xFFFFFFFFu, 0xFFFFFFFFu), diff --git a/src/xenia/cpu/backend/x64/x64_emitter.h b/src/xenia/cpu/backend/x64/x64_emitter.h index 840e355fc..9117beea3 100644 --- a/src/xenia/cpu/backend/x64/x64_emitter.h +++ b/src/xenia/cpu/backend/x64/x64_emitter.h @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2019 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -49,6 +49,7 @@ enum RegisterFlags { enum XmmConst { XMMZero = 0, XMMOne, + XMMOnePD, XMMNegativeOne, XMMFFFF, XMMMaskX16Y16, diff --git a/src/xenia/cpu/backend/x64/x64_sequences.cc b/src/xenia/cpu/backend/x64/x64_sequences.cc index 62f6e9a3f..08badaa9c 100644 --- a/src/xenia/cpu/backend/x64/x64_sequences.cc +++ b/src/xenia/cpu/backend/x64/x64_sequences.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2020 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -2376,21 +2376,37 @@ EMITTER_OPCODE_TABLE(OPCODE_RSQRT, RSQRT_F32, RSQRT_F64, RSQRT_V128); // ============================================================================ // OPCODE_RECIP // ============================================================================ +// Altivec guarantees an error of < 1/4096 for vrefp while AVX only gives +// < 1.5*2^-12 ≈ 1/2730 for rcpps. This breaks camp, horse and random event +// spawning, breaks cactus collision as well as flickering grass in 5454082B struct RECIP_F32 : Sequence> { static void Emit(X64Emitter& e, const EmitArgType& i) { - e.vrcpss(i.dest, i.src1); + if (e.IsFeatureEnabled(kX64EmitAVX512Ortho)) { + e.vrcp14ss(i.dest, i.src1, i.src1); + } else { + e.vmovaps(e.xmm0, e.GetXmmConstPtr(XMMOne)); + e.vdivss(i.dest, e.xmm0, i.src1); + } } }; struct RECIP_F64 : Sequence> { static void Emit(X64Emitter& e, const EmitArgType& i) { - e.vcvtsd2ss(i.dest, i.src1); - e.vrcpss(i.dest, i.dest); - e.vcvtss2sd(i.dest, i.dest); + if (e.IsFeatureEnabled(kX64EmitAVX512Ortho)) { + e.vrcp14sd(i.dest, i.src1, i.src1); + } else { + e.vmovapd(e.xmm0, e.GetXmmConstPtr(XMMOnePD)); + e.vdivsd(i.dest, e.xmm0, i.src1); + } } }; struct RECIP_V128 : Sequence> { static void Emit(X64Emitter& e, const EmitArgType& i) { - e.vrcpps(i.dest, i.src1); + if (e.IsFeatureEnabled(kX64EmitAVX512Ortho)) { + e.vrcp14ps(i.dest, i.src1); + } else { + e.vmovaps(e.xmm0, e.GetXmmConstPtr(XMMOne)); + e.vdivps(i.dest, e.xmm0, i.src1); + } } }; EMITTER_OPCODE_TABLE(OPCODE_RECIP, RECIP_F32, RECIP_F64, RECIP_V128); From 00e7de9297f9353aafd08ecfa99d4c5efb340c47 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Wed, 16 Feb 2022 00:51:24 +0100 Subject: [PATCH 03/24] [CPU] Improve vrsqrtefp accuracy --- src/xenia/cpu/backend/x64/x64_sequences.cc | 30 +++++++++++++++++----- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/xenia/cpu/backend/x64/x64_sequences.cc b/src/xenia/cpu/backend/x64/x64_sequences.cc index 08badaa9c..34cef4f7d 100644 --- a/src/xenia/cpu/backend/x64/x64_sequences.cc +++ b/src/xenia/cpu/backend/x64/x64_sequences.cc @@ -1,4 +1,4 @@ -/** +/** ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** @@ -2354,21 +2354,39 @@ EMITTER_OPCODE_TABLE(OPCODE_SQRT, SQRT_F32, SQRT_F64, SQRT_V128); // ============================================================================ // OPCODE_RSQRT // ============================================================================ +// Altivec guarantees an error of < 1/4096 for vrsqrtefp while AVX only gives +// < 1.5*2^-12 ≈ 1/2730 for vrsqrtps. struct RSQRT_F32 : Sequence> { static void Emit(X64Emitter& e, const EmitArgType& i) { - e.vrsqrtss(i.dest, i.src1); + if (e.IsFeatureEnabled(kX64EmitAVX512Ortho)) { + e.vrsqrt14ss(i.dest, i.src1, i.src1); + } else { + e.vmovaps(e.xmm0, e.GetXmmConstPtr(XMMOne)); + e.vsqrtss(e.xmm1, i.src1, i.src1); + e.vdivss(i.dest, e.xmm0, e.xmm1); + } } }; struct RSQRT_F64 : Sequence> { static void Emit(X64Emitter& e, const EmitArgType& i) { - e.vcvtsd2ss(i.dest, i.src1); - e.vrsqrtss(i.dest, i.dest); - e.vcvtss2sd(i.dest, i.dest); + if (e.IsFeatureEnabled(kX64EmitAVX512Ortho)) { + e.vrsqrt14sd(i.dest, i.src1, i.src1); + } else { + e.vmovapd(e.xmm0, e.GetXmmConstPtr(XMMOnePD)); + e.vsqrtsd(e.xmm1, i.src1, i.src1); + e.vdivsd(i.dest, e.xmm0, e.xmm1); + } } }; struct RSQRT_V128 : Sequence> { static void Emit(X64Emitter& e, const EmitArgType& i) { - e.vrsqrtps(i.dest, i.src1); + if (e.IsFeatureEnabled(kX64EmitAVX512Ortho)) { + e.vrsqrt14ps(i.dest, i.src1); + } else { + e.vmovaps(e.xmm0, e.GetXmmConstPtr(XMMOne)); + e.vsqrtps(e.xmm1, i.src1); + e.vdivps(i.dest, e.xmm0, e.xmm1); + } } }; EMITTER_OPCODE_TABLE(OPCODE_RSQRT, RSQRT_F32, RSQRT_F64, RSQRT_V128); From ba28ef971717af110a6975129fb312c7565f1f7b Mon Sep 17 00:00:00 2001 From: Triang3l Date: Thu, 17 Feb 2022 20:38:52 +0300 Subject: [PATCH 04/24] [Win32] Declare Windows 7-11 support in the manifest --- src/xenia/base/app_win32.manifest | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/xenia/base/app_win32.manifest b/src/xenia/base/app_win32.manifest index 1867a1fd0..bad9294e6 100644 --- a/src/xenia/base/app_win32.manifest +++ b/src/xenia/base/app_win32.manifest @@ -1,5 +1,18 @@ + + + + + + + + + + + + + From 6b45cf84472c26436d4a5db61a7b50dab301e398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Gli=C5=84ski?= Date: Fri, 18 Feb 2022 00:38:04 +0100 Subject: [PATCH 05/24] [Base] Match exactly when no pattern in wildcard --- src/xenia/base/filesystem_wildcard.cc | 4 +++- src/xenia/base/filesystem_wildcard.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/xenia/base/filesystem_wildcard.cc b/src/xenia/base/filesystem_wildcard.cc index 1954060dc..bdaafacaa 100644 --- a/src/xenia/base/filesystem_wildcard.cc +++ b/src/xenia/base/filesystem_wildcard.cc @@ -19,6 +19,7 @@ namespace xe::filesystem { WildcardFlags WildcardFlags::FIRST(true, false, false); WildcardFlags WildcardFlags::LAST(false, true, false); WildcardFlags WildcardFlags::ANY(false, false, true); +WildcardFlags WildcardFlags::FIRST_AND_LAST(true, true, false); WildcardFlags::WildcardFlags() : FromStart(false), ToEnd(false), ExactLength(false) {} @@ -89,7 +90,8 @@ void WildcardEngine::PreparePattern(const std::string_view pattern) { } if (last != pattern.size()) { std::string str_str(pattern.substr(last)); - rules_.push_back(WildcardRule(str_str, WildcardFlags::LAST)); + rules_.push_back(WildcardRule( + str_str, last ? WildcardFlags::LAST : WildcardFlags::FIRST_AND_LAST)); } } diff --git a/src/xenia/base/filesystem_wildcard.h b/src/xenia/base/filesystem_wildcard.h index fdad44604..545321fee 100644 --- a/src/xenia/base/filesystem_wildcard.h +++ b/src/xenia/base/filesystem_wildcard.h @@ -30,6 +30,7 @@ class WildcardFlags { static WildcardFlags FIRST; static WildcardFlags LAST; static WildcardFlags ANY; + static WildcardFlags FIRST_AND_LAST; }; class WildcardRule { From 49efbeaca81db1223debe1dd42b2ce8028850a88 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Mon, 21 Feb 2022 22:26:42 +0100 Subject: [PATCH 06/24] [Base] Add spin wait helper to threading test --- src/xenia/base/testing/threading_test.cc | 31 +++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/xenia/base/testing/threading_test.cc b/src/xenia/base/testing/threading_test.cc index d1380d602..44ce8e71d 100644 --- a/src/xenia/base/testing/threading_test.cc +++ b/src/xenia/base/testing/threading_test.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** -* Copyright 2021 Ben Vanik. All rights reserved. * +* Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -19,6 +19,35 @@ namespace test { using namespace threading; using namespace std::chrono_literals; +// Helpers to wait on a predicate which do not depend on complex sync primitives +template +bool spin_wait_until( + const std::chrono::time_point& timeout_time, + Predicate stop_waiting) { + while (!stop_waiting()) { + if (std::chrono::steady_clock::now() >= timeout_time) { + return false; + } + // Needed for valgrind because it basically runs one thread: + MaybeYield(); + } + return true; +} +template +bool spin_wait_for(const std::chrono::duration& rel_time, + Predicate stop_waiting) { + return spin_wait_until(std::chrono::steady_clock::now() + rel_time, + stop_waiting); +} + +template +void spin_wait(Predicate stop_waiting) { + while (!stop_waiting()) { + // Needed for valgrind because it basically runs one thread: + MaybeYield(); + } +} + TEST_CASE("Fence") { std::unique_ptr pFence; std::unique_ptr pTimer; From ca6296089e9779e6f40447a786bfc3d67738f447 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Sun, 20 Feb 2022 18:51:13 +0100 Subject: [PATCH 07/24] [Base] Remove timing dependency from test - Use atomics and spin waits to synchronize threads for tests - Improves test stability on CI --- src/xenia/base/testing/threading_test.cc | 121 ++++++++++++++--------- 1 file changed, 74 insertions(+), 47 deletions(-) diff --git a/src/xenia/base/testing/threading_test.cc b/src/xenia/base/testing/threading_test.cc index 44ce8e71d..b51df72d4 100644 --- a/src/xenia/base/testing/threading_test.cc +++ b/src/xenia/base/testing/threading_test.cc @@ -92,8 +92,7 @@ TEST_CASE("Fence") { std::thread(func), }); - Sleep(100ms); - REQUIRE(started.load() == threads.size()); + REQUIRE(spin_wait_for(1s, [&] { return started == threads.size(); })); REQUIRE(finished.load() == 0); pFence->Signal(); @@ -320,6 +319,7 @@ TEST_CASE("Wait on Multiple Events", "[event]") { Event::CreateManualResetEvent(false), }; + std::atomic_uint threads_started(0); std::array order = {0}; std::atomic_uint index(0); auto sign_in = [&order, &index](uint32_t id) { @@ -328,40 +328,53 @@ TEST_CASE("Wait on Multiple Events", "[event]") { }; auto threads = std::array{ - std::thread([&events, &sign_in] { - auto res = WaitAll({events[1].get(), events[3].get()}, false, 100ms); + std::thread([&events, &sign_in, &threads_started] { + set_name("1"); + threads_started++; + auto res = WaitAll({events[1].get(), events[3].get()}, false); + REQUIRE(res == WaitResult::kSuccess); if (res == WaitResult::kSuccess) { sign_in(1); } }), - std::thread([&events, &sign_in] { - auto res = WaitAny({events[0].get(), events[2].get()}, false, 100ms); + std::thread([&events, &sign_in, &threads_started] { + set_name("2"); + threads_started++; + auto res = WaitAny({events[0].get(), events[2].get()}, false); + REQUIRE(res.first == WaitResult::kSuccess); if (res.first == WaitResult::kSuccess) { sign_in(2); } }), - std::thread([&events, &sign_in] { - auto res = WaitAll({events[0].get(), events[2].get(), events[3].get()}, - false, 100ms); + std::thread([&events, &sign_in, &threads_started] { + set_name("3"); + threads_started++; + auto res = + WaitAll({events[0].get(), events[2].get(), events[3].get()}, false); + REQUIRE(res == WaitResult::kSuccess); if (res == WaitResult::kSuccess) { sign_in(3); } }), - std::thread([&events, &sign_in] { - auto res = WaitAny({events[1].get(), events[3].get()}, false, 100ms); + std::thread([&events, &sign_in, &threads_started] { + set_name("4"); + threads_started++; + auto res = WaitAny({events[1].get(), events[3].get()}, false); + REQUIRE(res.first == WaitResult::kSuccess); if (res.first == WaitResult::kSuccess) { sign_in(4); } }), }; - Sleep(10ms); + // wait for all threads starting up + REQUIRE(spin_wait_for(1s, [&] { return threads_started == 4; })); events[3]->Set(); // Signals thread id=4 and stays on for 1 and 3 - Sleep(10ms); + REQUIRE(spin_wait_for(1s, [&] { return index == 1; })); events[1]->Set(); // Signals thread id=1 - Sleep(10ms); + REQUIRE(spin_wait_for(1s, [&] { return index == 2; })); events[0]->Set(); // Signals thread id=2 - Sleep(10ms); + REQUIRE(spin_wait_for(1s, [&] { return index == 3; })); events[2]->Set(); // Partial signals thread id=3 events[0]->Set(); // Signals thread id=3 @@ -369,12 +382,13 @@ TEST_CASE("Wait on Multiple Events", "[event]") { t.join(); } + REQUIRE(index == 4); + INFO(order.data()); REQUIRE(order[0] == '4'); - // TODO(bwrsandman): Order is not always maintained on linux - // REQUIRE(order[1] == '1'); - // REQUIRE(order[2] == '2'); - // REQUIRE(order[3] == '3'); + REQUIRE(order[1] == '1'); + REQUIRE(order[2] == '2'); + REQUIRE(order[3] == '3'); } TEST_CASE("Wait on Semaphore", "[semaphore]") { @@ -433,26 +447,24 @@ TEST_CASE("Wait on Semaphore", "[semaphore]") { // Semaphore between threads sem = Semaphore::Create(5, 5); - Sleep(10ms); // Occupy the semaphore with 5 threads std::atomic wait_count(0); - volatile bool threads_terminate(false); + std::atomic threads_terminate(false); auto func = [&sem, &wait_count, &threads_terminate] { auto res = Wait(sem.get(), false, 100ms); wait_count++; - while (!threads_terminate) { - } - if (res == WaitResult::kSuccess) { - sem->Release(1, nullptr); - } + + REQUIRE(spin_wait_for(2s, [&] { return threads_terminate.load(); })); + + REQUIRE(res == WaitResult::kSuccess); + sem->Release(1, nullptr); }; auto threads = std::array{ std::thread(func), std::thread(func), std::thread(func), std::thread(func), std::thread(func), }; // Wait for threads to finish semaphore calls - while (wait_count != 5) { - } + REQUIRE(spin_wait_for(1s, [&] { return wait_count == 5; })); // Attempt to acquire full semaphore with current (6th) thread result = Wait(sem.get(), false, 20ms); REQUIRE(result == WaitResult::kTimeout); @@ -465,18 +477,23 @@ TEST_CASE("Wait on Semaphore", "[semaphore]") { REQUIRE(result == WaitResult::kSuccess); sem->Release(1, &previous_count); REQUIRE(previous_count == 4); +} + +TEST_CASE("Invalid semaphore parameters", "[semaphore]") { + std::unique_ptr sem; // Test invalid construction parameters // These are invalid according to documentation - // TODO(bwrsandman): Many of these invalid invocations succeed sem = Semaphore::Create(-1, 5); - // REQUIRE(sem.get() == nullptr); + REQUIRE(sem == nullptr); sem = Semaphore::Create(10, 5); - // REQUIRE(sem.get() == nullptr); + REQUIRE(sem == nullptr); sem = Semaphore::Create(0, 0); - // REQUIRE(sem.get() == nullptr); + REQUIRE(sem == nullptr); sem = Semaphore::Create(0, -1); - // REQUIRE(sem.get() == nullptr); + REQUIRE(sem == nullptr); + sem = Semaphore::Create(-1, 0); + REQUIRE(sem == nullptr); } TEST_CASE("Wait on Multiple Semaphores", "[semaphore]") { @@ -576,17 +593,18 @@ TEST_CASE("Wait on Mutant", "[mutant]") { REQUIRE_FALSE(mut->Release()); // Test mutants on other threads - auto thread1 = std::thread([&mut] { - Sleep(5ms); + std::atomic step(0); + auto thread1 = std::thread([&mut, &step] { mut = Mutant::Create(true); - Sleep(100ms); + step++; // 1 + REQUIRE(spin_wait_for(2s, [&] { return step == 2; })); mut->Release(); }); - Sleep(10ms); + REQUIRE(spin_wait_for(1s, [&] { return step == 1; })); REQUIRE_FALSE(mut->Release()); - Sleep(10ms); result = Wait(mut.get(), false, 50ms); REQUIRE(result == WaitResult::kTimeout); + step++; // 2 thread1.join(); result = Wait(mut.get(), false, 1ms); REQUIRE(result == WaitResult::kSuccess); @@ -597,16 +615,18 @@ TEST_CASE("Wait on Multiple Mutants", "[mutant]") { WaitResult all_result; std::pair any_result; std::unique_ptr mut0, mut1; + std::atomic step(0); // Test which should fail for WaitAll and WaitAny - auto thread0 = std::thread([&mut0, &mut1] { + auto thread0 = std::thread([&mut0, &mut1, &step] { mut0 = Mutant::Create(true); mut1 = Mutant::Create(true); - Sleep(50ms); + step++; // 1 + REQUIRE(spin_wait_for(2s, [&] { return step == 2; })); mut0->Release(); mut1->Release(); }); - Sleep(10ms); + REQUIRE(spin_wait_for(1s, [&] { return step == 1; })); all_result = WaitAll({mut0.get(), mut1.get()}, false, 10ms); REQUIRE(all_result == WaitResult::kTimeout); REQUIRE_FALSE(mut0->Release()); @@ -616,16 +636,19 @@ TEST_CASE("Wait on Multiple Mutants", "[mutant]") { REQUIRE(any_result.second == 0); REQUIRE_FALSE(mut0->Release()); REQUIRE_FALSE(mut1->Release()); + step++; // 2 thread0.join(); // Test which should fail for WaitAll but not WaitAny - auto thread1 = std::thread([&mut0, &mut1] { + step = 0; + auto thread1 = std::thread([&mut0, &mut1, &step] { mut0 = Mutant::Create(true); mut1 = Mutant::Create(false); - Sleep(50ms); + step++; // 1 + REQUIRE(spin_wait_for(2s, [&] { return step == 2; })); mut0->Release(); }); - Sleep(10ms); + REQUIRE(spin_wait_for(1s, [&] { return step == 1; })); all_result = WaitAll({mut0.get(), mut1.get()}, false, 10ms); REQUIRE(all_result == WaitResult::kTimeout); REQUIRE_FALSE(mut0->Release()); @@ -635,15 +658,18 @@ TEST_CASE("Wait on Multiple Mutants", "[mutant]") { REQUIRE(any_result.second == 1); REQUIRE_FALSE(mut0->Release()); REQUIRE(mut1->Release()); + step++; // 2 thread1.join(); // Test which should pass for WaitAll and WaitAny - auto thread2 = std::thread([&mut0, &mut1] { + step = 0; + auto thread2 = std::thread([&mut0, &mut1, &step] { mut0 = Mutant::Create(false); mut1 = Mutant::Create(false); - Sleep(50ms); + step++; // 1 + REQUIRE(spin_wait_for(2s, [&] { return step == 2; })); }); - Sleep(10ms); + REQUIRE(spin_wait_for(1s, [&] { return step == 1; })); all_result = WaitAll({mut0.get(), mut1.get()}, false, 10ms); REQUIRE(all_result == WaitResult::kSuccess); REQUIRE(mut0->Release()); @@ -653,6 +679,7 @@ TEST_CASE("Wait on Multiple Mutants", "[mutant]") { REQUIRE(any_result.second == 0); REQUIRE(mut0->Release()); REQUIRE_FALSE(mut1->Release()); + step++; // 2 thread2.join(); } From bb42829308890268f2c6495785886e2985c5c555 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Mon, 21 Feb 2022 23:09:20 +0100 Subject: [PATCH 08/24] [Base] Fix WaitMultiple on POSIX - Never use `cond_.notify_one()` because it may wake a thread that is unrelated to the signalled wait handle, resulting in a lost wake and possible deadlock. Wait conditions are to be checked by the threads themselves. - Refactor and simplify `WaitMultiple` --- src/xenia/base/threading_posix.cc | 61 +++++++++++++------------------ 1 file changed, 26 insertions(+), 35 deletions(-) diff --git a/src/xenia/base/threading_posix.cc b/src/xenia/base/threading_posix.cc index 813758a4b..4e8fdfd51 100644 --- a/src/xenia/base/threading_posix.cc +++ b/src/xenia/base/threading_posix.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2020 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -253,37 +253,37 @@ class PosixConditionBase { static std::pair WaitMultiple( std::vector&& handles, bool wait_all, std::chrono::milliseconds timeout) { - using iter_t = std::vector::const_iterator; - bool executed; - auto predicate = [](auto h) { return h->signaled(); }; + assert_true(handles.size() > 0); // Construct a condition for all or any depending on wait_all - auto operation = wait_all ? std::all_of - : std::any_of; - auto aggregate = [&handles, operation, predicate] { - return operation(handles.cbegin(), handles.cend(), predicate); - }; + std::function predicate; + { + using iter_t = std::vector::const_iterator; + const auto predicate_inner = [](auto h) { return h->signaled(); }; + const auto operation = + wait_all ? std::all_of + : std::any_of; + predicate = [&handles, operation, predicate_inner] { + return operation(handles.cbegin(), handles.cend(), predicate_inner); + }; + } // TODO(bwrsandman, Triang3l) This is controversial, see issue #1677 // This will probably cause a deadlock on the next thread doing any waiting // if the thread is suspended between locking and waiting std::unique_lock lock(PosixConditionBase::mutex_); - // Check if the aggregate lambda (all or any) is already satisfied - if (aggregate()) { - executed = true; + bool wait_success = true; + // If the timeout is infinite, wait without timeout. + // The predicate will be checked before beginning the wait + if (timeout == std::chrono::milliseconds::max()) { + PosixConditionBase::cond_.wait(lock, predicate); } else { - // If the aggregate is not yet satisfied and the timeout is infinite, - // wait without timeout. - if (timeout == std::chrono::milliseconds::max()) { - PosixConditionBase::cond_.wait(lock, aggregate); - executed = true; - } else { - // Wait with timeout. - executed = PosixConditionBase::cond_.wait_for(lock, timeout, aggregate); - } + // Wait with timeout. + wait_success = + PosixConditionBase::cond_.wait_for(lock, timeout, predicate); } - if (executed) { + if (wait_success) { auto first_signaled = std::numeric_limits::max(); for (auto i = 0u; i < handles.size(); ++i) { if (handles[i]->signaled()) { @@ -294,6 +294,7 @@ class PosixConditionBase { if (!wait_all) break; } } + assert_true(std::numeric_limits::max() != first_signaled); return std::make_pair(WaitResult::kSuccess, first_signaled); } else { return std::make_pair(WaitResult::kTimeout, 0); @@ -329,13 +330,7 @@ class PosixCondition : public PosixConditionBase { bool Signal() override { auto lock = std::unique_lock(mutex_); signal_ = true; - if (manual_reset_) { - cond_.notify_all(); - } else { - // FIXME(bwrsandman): Potential cause for deadlock - // See issue #1678 for possible fix and discussion - cond_.notify_one(); - } + cond_.notify_all(); return true; } @@ -402,7 +397,7 @@ class PosixCondition : public PosixConditionBase { --count_; // Free to be acquired by another thread if (count_ == 0) { - cond_.notify_one(); + cond_.notify_all(); } return true; } @@ -471,11 +466,7 @@ class PosixCondition : public PosixConditionBase { // Store callback if (callback_) callback = callback_; signal_ = true; - if (manual_reset_) { - cond_.notify_all(); - } else { - cond_.notify_one(); - } + cond_.notify_all(); } // Call callback if (callback) callback(); From e75e0eb39c1ac9a409f4c24533404652f08ef9a8 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Mon, 21 Feb 2022 23:02:38 +0100 Subject: [PATCH 09/24] [Base] Fix `Semaphore::Create` invalid parameters --- src/xenia/base/threading_posix.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/xenia/base/threading_posix.cc b/src/xenia/base/threading_posix.cc index 4e8fdfd51..fac86abc8 100644 --- a/src/xenia/base/threading_posix.cc +++ b/src/xenia/base/threading_posix.cc @@ -975,6 +975,10 @@ class PosixSemaphore : public PosixConditionHandle { std::unique_ptr Semaphore::Create(int initial_count, int maximum_count) { + if (initial_count < 0 || initial_count > maximum_count || + maximum_count <= 0) { + return nullptr; + } return std::make_unique(initial_count, maximum_count); } From 4ea6e45e0c5528f5885eee32c1742ec92120bcde Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Tue, 22 Feb 2022 22:11:17 +0100 Subject: [PATCH 10/24] [Base] Remove `Sleep`s from more test cases Timing dependencies in this tests were causing spurious test failures: - Create and Run Thread - Test Thread QueueUserCallback They have been largely replaced by spin waits. --- src/xenia/base/testing/threading_test.cc | 61 +++++++++++++++--------- 1 file changed, 38 insertions(+), 23 deletions(-) diff --git a/src/xenia/base/testing/threading_test.cc b/src/xenia/base/testing/threading_test.cc index b51df72d4..17a17a8e5 100644 --- a/src/xenia/base/testing/threading_test.cc +++ b/src/xenia/base/testing/threading_test.cc @@ -829,18 +829,25 @@ TEST_CASE("Create and Run Thread", "[thread]") { std::unique_ptr thread; WaitResult result; Thread::CreationParameters params = {}; - auto func = [] { Sleep(20ms); }; + std::atomic fence(0); + auto func = [&fence] { + REQUIRE(spin_wait_for(1s, [&] { return fence == 1; })); + fence++; + }; SECTION("Create most basic case of thread") { + fence = 0; thread = Thread::Create(params, func); REQUIRE(thread->native_handle() != nullptr); REQUIRE_NOTHROW(thread->affinity_mask()); REQUIRE(thread->name().empty()); - result = Wait(thread.get(), false, 50ms); + fence++; + result = Wait(thread.get(), false, 1s); REQUIRE(result == WaitResult::kSuccess); } SECTION("Add thread name") { + fence = 0; std::string new_name = "Test thread name"; thread = Thread::Create(params, func); auto name = thread->name(); @@ -848,7 +855,8 @@ TEST_CASE("Create and Run Thread", "[thread]") { REQUIRE(name.empty()); thread->set_name(new_name); REQUIRE(thread->name() == new_name); - result = Wait(thread.get(), false, 50ms); + fence++; + result = Wait(thread.get(), false, 1s); REQUIRE(result == WaitResult::kSuccess); } @@ -858,10 +866,10 @@ TEST_CASE("Create and Run Thread", "[thread]") { Sleep(1ms); } }); - result = Wait(thread.get(), false, 50ms); + result = Wait(thread.get(), false, 1s); REQUIRE(result == WaitResult::kTimeout); thread->Terminate(-1); - result = Wait(thread.get(), false, 50ms); + result = Wait(thread.get(), false, 1s); REQUIRE(result == WaitResult::kSuccess); } @@ -869,8 +877,10 @@ TEST_CASE("Create and Run Thread", "[thread]") { thread = Thread::Create(params, [] { Thread::Exit(-1); FAIL("Function must not return"); + while (true) + ; }); - result = Wait(thread.get(), false, 50ms); + result = Wait(thread.get(), false, 1s); REQUIRE(result == WaitResult::kSuccess); } @@ -879,14 +889,16 @@ TEST_CASE("Create and Run Thread", "[thread]") { REQUIRE(result == WaitResult::kTimeout); } - SECTION("16kb stack size") { + SECTION("16Mb stack size") { params.stack_size = 16_MiB; thread = Thread::Create(params, [] { Thread::Exit(-1); FAIL("Function must not return"); + while (true) + ; }); REQUIRE(thread != nullptr); - result = Wait(thread.get(), false, 50ms); + result = Wait(thread.get(), false, 1s); REQUIRE(result == WaitResult::kSuccess); } @@ -973,18 +985,19 @@ TEST_CASE("Test Thread QueueUserCallback", "[thread]") { has_finished = -1; thread = Thread::Create(params, [&has_finished, &order] { // Not using Alertable so callback is not registered + order++; // 1 Sleep(90ms); + order++; // 2 has_finished = std::atomic_fetch_add_explicit( &order, 1, std::memory_order::memory_order_relaxed); }); - result = Wait(thread.get(), true, 50ms); - REQUIRE(result == WaitResult::kTimeout); + REQUIRE(!spin_wait_for(50ms, [&] { return order == 2; })); REQUIRE(is_modified == -1); thread->QueueUserCallback(callback); - result = Wait(thread.get(), true, 100ms); + result = Wait(thread.get(), true, 200ms); REQUIRE(result == WaitResult::kSuccess); REQUIRE(is_modified == -1); - REQUIRE(has_finished == 0); + REQUIRE(has_finished == 2); // With alertable order = 0; @@ -992,18 +1005,19 @@ TEST_CASE("Test Thread QueueUserCallback", "[thread]") { has_finished = -1; thread = Thread::Create(params, [&has_finished, &order] { // Using Alertable so callback is registered + order++; // 1 AlertableSleep(90ms); + order++; // 3 has_finished = std::atomic_fetch_add_explicit( &order, 1, std::memory_order::memory_order_relaxed); }); - result = Wait(thread.get(), true, 50ms); - REQUIRE(result == WaitResult::kTimeout); + REQUIRE(!spin_wait_for(50ms, [&] { return order == 2; })); REQUIRE(is_modified == -1); thread->QueueUserCallback(callback); - result = Wait(thread.get(), true, 100ms); + result = Wait(thread.get(), true, 200ms); REQUIRE(result == WaitResult::kSuccess); - REQUIRE(is_modified == 0); - REQUIRE(has_finished == 1); + REQUIRE(is_modified == 1); + REQUIRE(has_finished == 3); // Test Exit command with QueueUserCallback order = 0; @@ -1013,17 +1027,18 @@ TEST_CASE("Test Thread QueueUserCallback", "[thread]") { is_modified = std::atomic_fetch_add_explicit( &order, 1, std::memory_order::memory_order_relaxed); // Using Alertable so callback is registered - AlertableSleep(200ms); - has_finished = std::atomic_fetch_add_explicit( - &order, 1, std::memory_order::memory_order_relaxed); + order++; // 2 + AlertableSleep(1s); + FAIL("Thread should have been terminated during alertable sleep"); + while (true) + ; }); - result = Wait(thread.get(), true, 100ms); - REQUIRE(result == WaitResult::kTimeout); + REQUIRE(!spin_wait_for(100ms, [&] { return order == 3; })); // timeout thread->QueueUserCallback([] { Thread::Exit(0); }); result = Wait(thread.get(), true, 500ms); REQUIRE(result == WaitResult::kSuccess); REQUIRE(is_modified == 0); - REQUIRE(has_finished == -1); + REQUIRE(order == 2); // TODO(bwrsandman): Test alertable wait returning kUserCallback by using IO // callbacks. From 6bd1279fc0d93957aeaf0a95910160ad306db3bd Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Thu, 24 Feb 2022 22:22:53 +0100 Subject: [PATCH 11/24] [Base] Forward `handle=null` as nullptr for win --- src/xenia/base/threading_win.cc | 74 +++++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 18 deletions(-) diff --git a/src/xenia/base/threading_win.cc b/src/xenia/base/threading_win.cc index 60c746e55..dd2a063d8 100644 --- a/src/xenia/base/threading_win.cc +++ b/src/xenia/base/threading_win.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2020 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -12,6 +12,9 @@ #include "xenia/base/platform_win.h" #include "xenia/base/threading.h" +#define LOG_LASTERROR() \ + { XELOGI("Win32 Error 0x{:08X} in " __FUNCTION__ "(...)", GetLastError()); } + typedef HANDLE (*SetThreadDescriptionFn)(HANDLE hThread, PCWSTR lpThreadDescription); @@ -153,7 +156,9 @@ std::unique_ptr HighResolutionTimer::CreateRepeating( template class Win32Handle : public T { public: - explicit Win32Handle(HANDLE handle) : handle_(handle) {} + explicit Win32Handle(HANDLE handle) : handle_(handle) { + assert_not_null(handle); + } ~Win32Handle() override { CloseHandle(handle_); handle_ = nullptr; @@ -248,13 +253,25 @@ class Win32Event : public Win32Handle { }; std::unique_ptr Event::CreateManualResetEvent(bool initial_state) { - return std::make_unique( - CreateEvent(nullptr, TRUE, initial_state ? TRUE : FALSE, nullptr)); + HANDLE handle = + CreateEvent(nullptr, TRUE, initial_state ? TRUE : FALSE, nullptr); + if (handle) { + return std::make_unique(handle); + } else { + LOG_LASTERROR(); + return nullptr; + } } std::unique_ptr Event::CreateAutoResetEvent(bool initial_state) { - return std::make_unique( - CreateEvent(nullptr, FALSE, initial_state ? TRUE : FALSE, nullptr)); + HANDLE handle = + CreateEvent(nullptr, FALSE, initial_state ? TRUE : FALSE, nullptr); + if (handle) { + return std::make_unique(handle); + } else { + LOG_LASTERROR(); + return nullptr; + } } class Win32Semaphore : public Win32Handle { @@ -271,8 +288,14 @@ class Win32Semaphore : public Win32Handle { std::unique_ptr Semaphore::Create(int initial_count, int maximum_count) { - return std::make_unique( - CreateSemaphore(nullptr, initial_count, maximum_count, nullptr)); + HANDLE handle = + CreateSemaphore(nullptr, initial_count, maximum_count, nullptr); + if (handle) { + return std::make_unique(handle); + } else { + LOG_LASTERROR(); + return nullptr; + } } class Win32Mutant : public Win32Handle { @@ -283,8 +306,13 @@ class Win32Mutant : public Win32Handle { }; std::unique_ptr Mutant::Create(bool initial_owner) { - return std::make_unique( - CreateMutex(nullptr, initial_owner ? TRUE : FALSE, nullptr)); + HANDLE handle = CreateMutex(nullptr, initial_owner ? TRUE : FALSE, nullptr); + if (handle) { + return std::make_unique(handle); + } else { + LOG_LASTERROR(); + return nullptr; + } } class Win32Timer : public Win32Handle { @@ -344,11 +372,23 @@ class Win32Timer : public Win32Handle { }; std::unique_ptr Timer::CreateManualResetTimer() { - return std::make_unique(CreateWaitableTimer(NULL, TRUE, NULL)); + HANDLE handle = CreateWaitableTimer(NULL, TRUE, NULL); + if (handle) { + return std::make_unique(handle); + } else { + LOG_LASTERROR(); + return nullptr; + } } std::unique_ptr Timer::CreateSynchronizationTimer() { - return std::make_unique(CreateWaitableTimer(NULL, FALSE, NULL)); + HANDLE handle = CreateWaitableTimer(NULL, FALSE, NULL); + if (handle) { + return std::make_unique(handle); + } else { + LOG_LASTERROR(); + return nullptr; + } } class Win32Thread : public Win32Handle { @@ -450,15 +490,13 @@ std::unique_ptr Thread::Create(CreationParameters params, HANDLE handle = CreateThread(NULL, params.stack_size, ThreadStartRoutine, start_data, params.create_suspended ? CREATE_SUSPENDED : 0, NULL); - if (handle == INVALID_HANDLE_VALUE) { - // TODO(benvanik): pass back? - auto last_error = GetLastError(); - XELOGE("Unable to CreateThread: {}", last_error); + if (handle) { + return std::make_unique(handle); + } else { + LOG_LASTERROR(); delete start_data; return nullptr; } - - return std::make_unique(handle); } Thread* Thread::GetCurrentThread() { From 986dcf4f65af0050fcb3c571f8bf2d341b288956 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Thu, 24 Feb 2022 22:37:32 +0100 Subject: [PATCH 12/24] [Base] Check success of sync primitive creation - Mainly use `assert`s, since failure is very rare - Forward failure of `CreateSemaphore` to guests because it is more easy to trigger with invalid initial parameters. --- src/xenia/app/xenia_main.cc | 2 ++ src/xenia/apu/audio_system.cc | 6 +++- src/xenia/apu/xma_decoder.cc | 3 +- src/xenia/base/logging.cc | 3 +- src/xenia/base/socket_win.cc | 4 ++- src/xenia/base/testing/threading_test.cc | 28 ++++++++++++++++++- src/xenia/base/threading_win.cc | 3 +- src/xenia/gpu/command_processor.cc | 4 ++- src/xenia/gpu/d3d12/pipeline_cache.cc | 13 +++++++-- src/xenia/gpu/trace_player.cc | 1 + .../kernel/xboxkrnl/xboxkrnl_threading.cc | 8 +++++- src/xenia/kernel/xevent.cc | 5 +++- src/xenia/kernel/xfile.cc | 4 ++- src/xenia/kernel/xiocompletion.cc | 3 +- src/xenia/kernel/xmutant.cc | 3 +- src/xenia/kernel/xnotifylistener.cc | 4 ++- src/xenia/kernel/xobject.cc | 6 ++-- src/xenia/kernel/xsemaphore.cc | 9 ++++-- src/xenia/kernel/xsemaphore.h | 7 +++-- src/xenia/kernel/xthread.cc | 3 +- src/xenia/kernel/xtimer.cc | 3 +- 21 files changed, 96 insertions(+), 26 deletions(-) diff --git a/src/xenia/app/xenia_main.cc b/src/xenia/app/xenia_main.cc index 98ceb6578..ed6f1e9f1 100644 --- a/src/xenia/app/xenia_main.cc +++ b/src/xenia/app/xenia_main.cc @@ -17,6 +17,7 @@ #include "xenia/app/discord/discord_presence.h" #include "xenia/app/emulator_window.h" +#include "xenia/base/assert.h" #include "xenia/base/cvar.h" #include "xenia/base/debugging.h" #include "xenia/base/logging.h" @@ -371,6 +372,7 @@ bool EmulatorApp::OnInitialize() { // Setup the emulator and run its loop in a separate thread. emulator_thread_quit_requested_.store(false, std::memory_order_relaxed); emulator_thread_event_ = xe::threading::Event::CreateAutoResetEvent(false); + assert_not_null(emulator_thread_event_); emulator_thread_ = std::thread(&EmulatorApp::EmulatorThread, this); return true; diff --git a/src/xenia/apu/audio_system.cc b/src/xenia/apu/audio_system.cc index c137a4853..371b0b82b 100644 --- a/src/xenia/apu/audio_system.cc +++ b/src/xenia/apu/audio_system.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -12,6 +12,7 @@ #include "xenia/apu/apu_flags.h" #include "xenia/apu/audio_driver.h" #include "xenia/apu/xma_decoder.h" +#include "xenia/base/assert.h" #include "xenia/base/byte_stream.h" #include "xenia/base/logging.h" #include "xenia/base/math.h" @@ -45,14 +46,17 @@ AudioSystem::AudioSystem(cpu::Processor* processor) for (size_t i = 0; i < kMaximumClientCount; ++i) { client_semaphores_[i] = xe::threading::Semaphore::Create(0, kMaximumQueuedFrames); + assert_not_null(client_semaphores_[i]); wait_handles_[i] = client_semaphores_[i].get(); } shutdown_event_ = xe::threading::Event::CreateAutoResetEvent(false); + assert_not_null(shutdown_event_); wait_handles_[kMaximumClientCount] = shutdown_event_.get(); xma_decoder_ = std::make_unique(processor_); resume_event_ = xe::threading::Event::CreateAutoResetEvent(false); + assert_not_null(resume_event_); } AudioSystem::~AudioSystem() { diff --git a/src/xenia/apu/xma_decoder.cc b/src/xenia/apu/xma_decoder.cc index cd04ebd91..3ad45e821 100644 --- a/src/xenia/apu/xma_decoder.cc +++ b/src/xenia/apu/xma_decoder.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2021 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -139,6 +139,7 @@ X_STATUS XmaDecoder::Setup(kernel::KernelState* kernel_state) { worker_running_ = true; work_event_ = xe::threading::Event::CreateAutoResetEvent(false); + assert_not_null(work_event_); worker_thread_ = kernel::object_ref( new kernel::XHostThread(kernel_state, 128 * 1024, 0, [this]() { WorkerThreadMain(); diff --git a/src/xenia/base/logging.cc b/src/xenia/base/logging.cc index bed2fe43b..ed636e428 100644 --- a/src/xenia/base/logging.cc +++ b/src/xenia/base/logging.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2021 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -224,6 +224,7 @@ class Logger { write_thread_ = xe::threading::Thread::Create({}, [this]() { WriteThread(); }); + assert_not_null(write_thread_); write_thread_->set_name("Logging Writer"); } diff --git a/src/xenia/base/socket_win.cc b/src/xenia/base/socket_win.cc index 35b96d470..2d3644dda 100644 --- a/src/xenia/base/socket_win.cc +++ b/src/xenia/base/socket_win.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2015 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -91,6 +91,7 @@ class Win32Socket : public Socket { // notifications. // Set true to start so we'll force a query of the socket on first run. event_ = xe::threading::Event::CreateManualResetEvent(true); + assert_not_null(event_); WSAEventSelect(socket_, event_->native_handle(), FD_READ | FD_CLOSE); // Keepalive for a looong time, as we may be paused by the debugger/etc. @@ -279,6 +280,7 @@ class Win32SocketServer : public SocketServer { accept_callback_(std::move(client)); } }); + assert_not_null(accept_thread_); return true; } diff --git a/src/xenia/base/testing/threading_test.cc b/src/xenia/base/testing/threading_test.cc index 17a17a8e5..94d8ec11c 100644 --- a/src/xenia/base/testing/threading_test.cc +++ b/src/xenia/base/testing/threading_test.cc @@ -165,6 +165,7 @@ TEST_CASE("TlsHandle") { auto thread = Thread::Create({}, [&non_thread_local_value, &handle] { non_thread_local_value = threading::GetTlsValue(handle); }); + REQUIRE(thread); auto result = Wait(thread.get(), false, 50ms); REQUIRE(result == WaitResult::kSuccess); @@ -231,8 +232,11 @@ TEST_CASE("HighResolutionTimer") { TEST_CASE("Wait on Multiple Handles", "[wait]") { auto mutant = Mutant::Create(true); + REQUIRE(mutant); auto semaphore = Semaphore::Create(10, 10); + REQUIRE(semaphore); auto event_ = Event::CreateManualResetEvent(false); + REQUIRE(event_); auto thread = Thread::Create({}, [&mutant, &semaphore, &event_] { event_->Set(); Wait(mutant.get(), false, 25ms); @@ -259,7 +263,9 @@ TEST_CASE("Wait on Multiple Handles", "[wait]") { TEST_CASE("Signal and Wait") { WaitResult result; auto mutant = Mutant::Create(true); + REQUIRE(mutant); auto event_ = Event::CreateAutoResetEvent(false); + REQUIRE(event_); auto thread = Thread::Create({}, [&mutant, &event_] { Wait(mutant.get(), false); event_->Set(); @@ -274,6 +280,7 @@ TEST_CASE("Signal and Wait") { TEST_CASE("Wait on Event", "[event]") { auto evt = Event::CreateAutoResetEvent(false); + REQUIRE(evt); WaitResult result; // Call wait on unset Event @@ -292,6 +299,7 @@ TEST_CASE("Wait on Event", "[event]") { TEST_CASE("Reset Event", "[event]") { auto evt = Event::CreateAutoResetEvent(false); + REQUIRE(evt); WaitResult result; // Call wait on reset Event @@ -318,6 +326,9 @@ TEST_CASE("Wait on Multiple Events", "[event]") { Event::CreateAutoResetEvent(false), Event::CreateManualResetEvent(false), }; + for (auto& event : events) { + REQUIRE(event.get() != nullptr); + } std::atomic_uint threads_started(0); std::array order = {0}; @@ -398,6 +409,7 @@ TEST_CASE("Wait on Semaphore", "[semaphore]") { // Wait on semaphore with no room sem = Semaphore::Create(0, 5); + REQUIRE(sem); result = Wait(sem.get(), false, 10ms); REQUIRE(result == WaitResult::kTimeout); @@ -413,12 +425,14 @@ TEST_CASE("Wait on Semaphore", "[semaphore]") { // Set semaphore over maximum_count sem = Semaphore::Create(5, 5); + REQUIRE(sem); previous_count = -1; REQUIRE_FALSE(sem->Release(1, &previous_count)); REQUIRE(previous_count == -1); REQUIRE_FALSE(sem->Release(10, &previous_count)); REQUIRE(previous_count == -1); sem = Semaphore::Create(0, 5); + REQUIRE(sem); REQUIRE_FALSE(sem->Release(10, &previous_count)); REQUIRE(previous_count == -1); REQUIRE_FALSE(sem->Release(10, &previous_count)); @@ -432,6 +446,7 @@ TEST_CASE("Wait on Semaphore", "[semaphore]") { // Wait on fully available semaphore sem = Semaphore::Create(5, 5); + REQUIRE(sem); result = Wait(sem.get(), false, 10ms); REQUIRE(result == WaitResult::kSuccess); result = Wait(sem.get(), false, 10ms); @@ -447,6 +462,7 @@ TEST_CASE("Wait on Semaphore", "[semaphore]") { // Semaphore between threads sem = Semaphore::Create(5, 5); + REQUIRE(sem); // Occupy the semaphore with 5 threads std::atomic wait_count(0); std::atomic threads_terminate(false); @@ -505,6 +521,8 @@ TEST_CASE("Wait on Multiple Semaphores", "[semaphore]") { // Test Wait all which should fail sem0 = Semaphore::Create(0, 5); sem1 = Semaphore::Create(5, 5); + REQUIRE(sem0); + REQUIRE(sem1); all_result = WaitAll({sem0.get(), sem1.get()}, false, 10ms); REQUIRE(all_result == WaitResult::kTimeout); previous_count = -1; @@ -517,6 +535,8 @@ TEST_CASE("Wait on Multiple Semaphores", "[semaphore]") { // Test Wait all again which should succeed sem0 = Semaphore::Create(1, 5); sem1 = Semaphore::Create(5, 5); + REQUIRE(sem0); + REQUIRE(sem1); all_result = WaitAll({sem0.get(), sem1.get()}, false, 10ms); REQUIRE(all_result == WaitResult::kSuccess); previous_count = -1; @@ -529,6 +549,8 @@ TEST_CASE("Wait on Multiple Semaphores", "[semaphore]") { // Test Wait Any which should fail sem0 = Semaphore::Create(0, 5); sem1 = Semaphore::Create(0, 5); + REQUIRE(sem0); + REQUIRE(sem1); any_result = WaitAny({sem0.get(), sem1.get()}, false, 10ms); REQUIRE(any_result.first == WaitResult::kTimeout); REQUIRE(any_result.second == 0); @@ -542,6 +564,8 @@ TEST_CASE("Wait on Multiple Semaphores", "[semaphore]") { // Test Wait Any which should succeed sem0 = Semaphore::Create(0, 5); sem1 = Semaphore::Create(5, 5); + REQUIRE(sem0); + REQUIRE(sem1); any_result = WaitAny({sem0.get(), sem1.get()}, false, 10ms); REQUIRE(any_result.first == WaitResult::kSuccess); REQUIRE(any_result.second == 1); @@ -689,6 +713,7 @@ TEST_CASE("Wait on Timer", "[timer]") { // Test Manual Reset timer = Timer::CreateManualResetTimer(); + REQUIRE(timer); result = Wait(timer.get(), false, 1ms); REQUIRE(result == WaitResult::kTimeout); REQUIRE(timer->SetOnce(1ms)); // Signals it @@ -699,6 +724,7 @@ TEST_CASE("Wait on Timer", "[timer]") { // Test Synchronization timer = Timer::CreateSynchronizationTimer(); + REQUIRE(timer); result = Wait(timer.get(), false, 1ms); REQUIRE(result == WaitResult::kTimeout); REQUIRE(timer->SetOnce(1ms)); // Signals it @@ -813,7 +839,7 @@ TEST_CASE("Set and Test Current Thread ID", "[thread]") { // TODO(bwrsandman): Test on Thread object } -TEST_CASE("Set and Test Current Thread Name", "Thread") { +TEST_CASE("Set and Test Current Thread Name", "[thread]") { auto current_thread = Thread::GetCurrentThread(); REQUIRE(current_thread); auto old_thread_name = current_thread->name(); diff --git a/src/xenia/base/threading_win.cc b/src/xenia/base/threading_win.cc index dd2a063d8..cf760d603 100644 --- a/src/xenia/base/threading_win.cc +++ b/src/xenia/base/threading_win.cc @@ -66,7 +66,8 @@ static void set_name(HANDLE thread, const std::string_view name) { auto func = (SetThreadDescriptionFn)GetProcAddress(kernel, "SetThreadDescription"); if (func) { - func(thread, reinterpret_cast(xe::to_utf16(name).c_str())); + auto u16name = xe::to_utf16(name); + func(thread, reinterpret_cast(u16name.c_str())); } } raise_thread_name_exception(thread, std::string(name)); diff --git a/src/xenia/gpu/command_processor.cc b/src/xenia/gpu/command_processor.cc index 7a4ac4e23..0fe9d7cbf 100644 --- a/src/xenia/gpu/command_processor.cc +++ b/src/xenia/gpu/command_processor.cc @@ -41,7 +41,9 @@ CommandProcessor::CommandProcessor(GraphicsSystem* graphics_system, trace_writer_(graphics_system->memory()->physical_membase()), worker_running_(true), write_ptr_index_event_(xe::threading::Event::CreateAutoResetEvent(false)), - write_ptr_index_(0) {} + write_ptr_index_(0) { + assert_not_null(write_ptr_index_event_); +} CommandProcessor::~CommandProcessor() = default; diff --git a/src/xenia/gpu/d3d12/pipeline_cache.cc b/src/xenia/gpu/d3d12/pipeline_cache.cc index 01b0b9cd8..75a096f48 100644 --- a/src/xenia/gpu/d3d12/pipeline_cache.cc +++ b/src/xenia/gpu/d3d12/pipeline_cache.cc @@ -149,6 +149,7 @@ bool PipelineCache::Initialize() { creation_threads_busy_ = 0; creation_completion_event_ = xe::threading::Event::CreateManualResetEvent(true); + assert_not_null(creation_completion_event_); creation_completion_set_event_ = false; creation_threads_shutdown_from_ = SIZE_MAX; if (cvars::d3d12_pipeline_creation_threads != 0) { @@ -164,6 +165,7 @@ bool PipelineCache::Initialize() { for (size_t i = 0; i < creation_thread_count; ++i) { std::unique_ptr creation_thread = xe::threading::Thread::Create({}, [this, i]() { CreationThread(i); }); + assert_not_null(creation_thread); creation_thread->set_name("D3D12 Pipelines"); creation_threads_.push_back(std::move(creation_thread)); } @@ -540,9 +542,11 @@ void PipelineCache::InitializeShaderStorage( } while (shader_translation_threads.size() < shader_translation_threads_needed) { - shader_translation_threads.push_back(xe::threading::Thread::Create( - {}, shader_translation_thread_function)); - shader_translation_threads.back()->set_name("Shader Translation"); + auto thread = xe::threading::Thread::Create( + {}, shader_translation_thread_function); + assert_not_null(thread); + thread->set_name("Shader Translation"); + shader_translation_threads.push_back(std::move(thread)); } // Request ucode information gathering and translation of all the needed // shaders. @@ -606,6 +610,7 @@ void PipelineCache::InitializeShaderStorage( xe::threading::Thread::Create({}, [this, creation_thread_index]() { CreationThread(creation_thread_index); }); + assert_not_null(creation_thread); creation_thread->set_name("D3D12 Pipelines"); creation_threads_.push_back(std::move(creation_thread)); } @@ -771,6 +776,8 @@ void PipelineCache::InitializeShaderStorage( storage_write_thread_shutdown_ = false; storage_write_thread_ = xe::threading::Thread::Create({}, [this]() { StorageWriteThread(); }); + assert_not_null(storage_write_thread_); + storage_write_thread_->set_name("D3D12 Storage writer"); } void PipelineCache::ShutdownShaderStorage() { diff --git a/src/xenia/gpu/trace_player.cc b/src/xenia/gpu/trace_player.cc index 54db1156c..a141c9b8d 100644 --- a/src/xenia/gpu/trace_player.cc +++ b/src/xenia/gpu/trace_player.cc @@ -30,6 +30,7 @@ TracePlayer::TracePlayer(GraphicsSystem* graphics_system) kMemoryProtectRead | kMemoryProtectWrite); playback_event_ = xe::threading::Event::CreateAutoResetEvent(false); + assert_not_null(playback_event_); } TracePlayer::~TracePlayer() { delete[] edram_snapshot_; } diff --git a/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc b/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc index f913643eb..7e6c605e4 100644 --- a/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc +++ b/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc @@ -590,7 +590,13 @@ dword_result_t NtCreateSemaphore_entry(lpdword_t handle_ptr, } auto sem = object_ref(new XSemaphore(kernel_state())); - sem->Initialize((int32_t)count, (int32_t)limit); + if (!sem->Initialize((int32_t)count, (int32_t)limit)) { + if (handle_ptr) { + *handle_ptr = 0; + } + sem->ReleaseHandle(); + return X_STATUS_INVALID_PARAMETER; + } // obj_attributes may have a name inside of it, if != NULL. if (obj_attributes_ptr) { diff --git a/src/xenia/kernel/xevent.cc b/src/xenia/kernel/xevent.cc index f46fdb5fb..03c947c7e 100644 --- a/src/xenia/kernel/xevent.cc +++ b/src/xenia/kernel/xevent.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -30,6 +30,7 @@ void XEvent::Initialize(bool manual_reset, bool initial_state) { } else { event_ = xe::threading::Event::CreateAutoResetEvent(initial_state); } + assert_not_null(event_); } void XEvent::InitializeNative(void* native_ptr, X_DISPATCH_HEADER* header) { @@ -53,6 +54,7 @@ void XEvent::InitializeNative(void* native_ptr, X_DISPATCH_HEADER* header) { } else { event_ = xe::threading::Event::CreateAutoResetEvent(initial_state); } + assert_not_null(event_); } int32_t XEvent::Set(uint32_t priority_increment, bool wait) { @@ -112,6 +114,7 @@ object_ref XEvent::Restore(KernelState* kernel_state, } else { evt->event_ = xe::threading::Event::CreateAutoResetEvent(false); } + assert_not_null(evt->event_); if (signaled) { evt->event_->Set(); diff --git a/src/xenia/kernel/xfile.cc b/src/xenia/kernel/xfile.cc index b9c134c01..f46a9e137 100644 --- a/src/xenia/kernel/xfile.cc +++ b/src/xenia/kernel/xfile.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2020 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -26,10 +26,12 @@ XFile::XFile(KernelState* kernel_state, vfs::File* file, bool synchronous) file_(file), is_synchronous_(synchronous) { async_event_ = threading::Event::CreateAutoResetEvent(false); + assert_not_null(async_event_); } XFile::XFile() : XObject(kObjectType) { async_event_ = threading::Event::CreateAutoResetEvent(false); + assert_not_null(async_event_); } XFile::~XFile() { diff --git a/src/xenia/kernel/xiocompletion.cc b/src/xenia/kernel/xiocompletion.cc index 638a0a3ef..9f0411bfe 100644 --- a/src/xenia/kernel/xiocompletion.cc +++ b/src/xenia/kernel/xiocompletion.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2015 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -15,6 +15,7 @@ namespace kernel { XIOCompletion::XIOCompletion(KernelState* kernel_state) : XObject(kernel_state, kObjectType) { notification_semaphore_ = threading::Semaphore::Create(0, kMaxNotifications); + assert_not_null(notification_semaphore_); } XIOCompletion::~XIOCompletion() = default; diff --git a/src/xenia/kernel/xmutant.cc b/src/xenia/kernel/xmutant.cc index 4328fc342..0a67f2183 100644 --- a/src/xenia/kernel/xmutant.cc +++ b/src/xenia/kernel/xmutant.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -28,6 +28,7 @@ void XMutant::Initialize(bool initial_owner) { assert_false(mutant_); mutant_ = xe::threading::Mutant::Create(initial_owner); + assert_not_null(mutant_); } void XMutant::InitializeNative(void* native_ptr, X_DISPATCH_HEADER* header) { diff --git a/src/xenia/kernel/xnotifylistener.cc b/src/xenia/kernel/xnotifylistener.cc index 7c054db70..fc2d24c98 100644 --- a/src/xenia/kernel/xnotifylistener.cc +++ b/src/xenia/kernel/xnotifylistener.cc @@ -2,13 +2,14 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ #include "xenia/kernel/xnotifylistener.h" +#include "xenia/base/assert.h" #include "xenia/base/byte_stream.h" #include "xenia/base/logging.h" #include "xenia/kernel/kernel_state.h" @@ -25,6 +26,7 @@ void XNotifyListener::Initialize(uint64_t mask, uint32_t max_version) { assert_false(wait_handle_); wait_handle_ = xe::threading::Event::CreateManualResetEvent(false); + assert_not_null(wait_handle_); mask_ = mask; max_version_ = max_version; diff --git a/src/xenia/kernel/xobject.cc b/src/xenia/kernel/xobject.cc index 4909dbaa1..20b85249b 100644 --- a/src/xenia/kernel/xobject.cc +++ b/src/xenia/kernel/xobject.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -410,7 +410,9 @@ object_ref XObject::GetNativeObject(KernelState* kernel_state, case 5: // SemaphoreObject { auto sem = new XSemaphore(kernel_state); - sem->InitializeNative(native_ptr, header); + auto success = sem->InitializeNative(native_ptr, header); + // Can't report failure to the guest at late initialization: + assert_true(success); object = sem; } break; case 3: // ProcessObject diff --git a/src/xenia/kernel/xsemaphore.cc b/src/xenia/kernel/xsemaphore.cc index a582fe6d6..34b960b25 100644 --- a/src/xenia/kernel/xsemaphore.cc +++ b/src/xenia/kernel/xsemaphore.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -20,22 +20,24 @@ XSemaphore::XSemaphore(KernelState* kernel_state) XSemaphore::~XSemaphore() = default; -void XSemaphore::Initialize(int32_t initial_count, int32_t maximum_count) { +bool XSemaphore::Initialize(int32_t initial_count, int32_t maximum_count) { assert_false(semaphore_); CreateNative(sizeof(X_KSEMAPHORE)); maximum_count_ = maximum_count; semaphore_ = xe::threading::Semaphore::Create(initial_count, maximum_count); + return !!semaphore_; } -void XSemaphore::InitializeNative(void* native_ptr, X_DISPATCH_HEADER* header) { +bool XSemaphore::InitializeNative(void* native_ptr, X_DISPATCH_HEADER* header) { assert_false(semaphore_); auto semaphore = reinterpret_cast(native_ptr); maximum_count_ = semaphore->limit; semaphore_ = xe::threading::Semaphore::Create(semaphore->header.signal_state, semaphore->limit); + return !!semaphore_; } int32_t XSemaphore::ReleaseSemaphore(int32_t release_count) { @@ -85,6 +87,7 @@ object_ref XSemaphore::Restore(KernelState* kernel_state, sem->semaphore_ = threading::Semaphore::Create(free_count, sem->maximum_count_); + assert_not_null(sem->semaphore_); return object_ref(sem); } diff --git a/src/xenia/kernel/xsemaphore.h b/src/xenia/kernel/xsemaphore.h index b9e78a0b1..40261398b 100644 --- a/src/xenia/kernel/xsemaphore.h +++ b/src/xenia/kernel/xsemaphore.h @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -30,8 +30,9 @@ class XSemaphore : public XObject { explicit XSemaphore(KernelState* kernel_state); ~XSemaphore() override; - void Initialize(int32_t initial_count, int32_t maximum_count); - void InitializeNative(void* native_ptr, X_DISPATCH_HEADER* header); + [[nodiscard]] bool Initialize(int32_t initial_count, int32_t maximum_count); + [[nodiscard]] bool InitializeNative(void* native_ptr, + X_DISPATCH_HEADER* header); int32_t ReleaseSemaphore(int32_t release_count); diff --git a/src/xenia/kernel/xthread.cc b/src/xenia/kernel/xthread.cc index 9c269c6b7..b842c2c08 100644 --- a/src/xenia/kernel/xthread.cc +++ b/src/xenia/kernel/xthread.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2020 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -1057,6 +1057,7 @@ object_ref XThread::Restore(KernelState* kernel_state, // Release the self-reference to the thread. thread->ReleaseHandle(); }); + assert_not_null(thread->thread_); // Notify processor we were recreated. thread->emulator()->processor()->OnThreadCreated( diff --git a/src/xenia/kernel/xtimer.cc b/src/xenia/kernel/xtimer.cc index fe66a73b7..e98add6d3 100644 --- a/src/xenia/kernel/xtimer.cc +++ b/src/xenia/kernel/xtimer.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2013 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -35,6 +35,7 @@ void XTimer::Initialize(uint32_t timer_type) { assert_always(); break; } + assert_not_null(timer_); } X_STATUS XTimer::SetTimer(int64_t due_time, uint32_t period_ms, From fb741db2fe1bcf108e9795b0d17ab68447dde20d Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Sat, 26 Feb 2022 14:04:47 +0100 Subject: [PATCH 13/24] [Base] Fix callback threads for POSIX timers --- src/xenia/base/threading_posix.cc | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/xenia/base/threading_posix.cc b/src/xenia/base/threading_posix.cc index fac86abc8..2cab9d371 100644 --- a/src/xenia/base/threading_posix.cc +++ b/src/xenia/base/threading_posix.cc @@ -34,6 +34,19 @@ #include "xenia/base/string_util.h" #endif +#if XE_PLATFORM_LINUX +// SIGEV_THREAD_ID in timer_create(...) is a Linux extension +#define XE_HAS_SIGEV_THREAD_ID 1 +#ifdef __GLIBC__ +#define sigev_notify_thread_id _sigev_un._tid +#endif +#if __GLIBC__ == 2 && __GLIBC_MINOR__ < 30 +#define gettid() syscall(SYS_gettid) +#endif +#else +#define XE_HAS_SIGEV_THREAD_ID 0 +#endif + namespace xe { namespace threading { @@ -195,7 +208,13 @@ class PosixHighResolutionTimer : public HighResolutionTimer { } // Create timer sigevent sev{}; +#if XE_HAS_SIGEV_THREAD_ID + sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID; + sev.sigev_notify_thread_id = gettid(); +#else sev.sigev_notify = SIGEV_SIGNAL; + callback_info_->target_thread = pthread_self(); +#endif sev.sigev_signo = GetSystemSignal(SignalType::kHighResolutionTimer); sev.sigev_value.sival_ptr = (void*)&callback_; if (timer_create(CLOCK_MONOTONIC, &sev, &timer_) == -1) return false; @@ -445,7 +464,13 @@ class PosixCondition : public PosixConditionBase { // Create timer if (timer_ == nullptr) { sigevent sev{}; +#if XE_HAS_SIGEV_THREAD_ID + sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID; + sev.sigev_notify_thread_id = gettid(); +#else sev.sigev_notify = SIGEV_SIGNAL; + callback_info_->target_thread = pthread_self(); +#endif sev.sigev_signo = GetSystemSignal(SignalType::kTimer); sev.sigev_value.sival_ptr = this; if (timer_create(CLOCK_MONOTONIC, &sev, &timer_) == -1) return false; From e0f34b97fb015faea0112c70727f4a17747f9e4b Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Tue, 1 Mar 2022 23:41:57 +0100 Subject: [PATCH 14/24] [Base] Check for correct thread in HResTimer tests --- src/xenia/base/testing/threading_test.cc | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/xenia/base/testing/threading_test.cc b/src/xenia/base/testing/threading_test.cc index 94d8ec11c..3a8b119c1 100644 --- a/src/xenia/base/testing/threading_test.cc +++ b/src/xenia/base/testing/threading_test.cc @@ -180,12 +180,21 @@ TEST_CASE("HighResolutionTimer") { // Smaller values are not as precise and fail the test const auto wait_time = 500ms; + const Thread* timer_thread = nullptr; + // Time the actual sleep duration { const auto interval = 50ms; std::atomic counter(0); auto start = std::chrono::steady_clock::now(); - auto cb = [&counter] { ++counter; }; + auto cb = [&counter, &timer_thread] { + if (counter == 0) { + timer_thread = Thread::GetCurrentThread(); + } else { + REQUIRE(Thread::GetCurrentThread() == timer_thread); + } + ++counter; + }; auto pTimer = HighResolutionTimer::CreateRepeating(interval, cb); Sleep(wait_time); pTimer.reset(); @@ -206,8 +215,14 @@ TEST_CASE("HighResolutionTimer") { std::atomic counter1(0); std::atomic counter2(0); auto start = std::chrono::steady_clock::now(); - auto cb1 = [&counter1] { ++counter1; }; - auto cb2 = [&counter2] { ++counter2; }; + auto cb1 = [&counter1, timer_thread] { + ++counter1; + REQUIRE(Thread::GetCurrentThread() == timer_thread); + }; + auto cb2 = [&counter2, timer_thread] { + ++counter2; + REQUIRE(Thread::GetCurrentThread() == timer_thread); + }; auto pTimer1 = HighResolutionTimer::CreateRepeating(interval1, cb1); auto pTimer2 = HighResolutionTimer::CreateRepeating(interval2, cb2); Sleep(wait_time); From 257b904a5e2a41f00609901d80422d084f5a81df Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Tue, 1 Mar 2022 23:37:42 +0100 Subject: [PATCH 15/24] [Base] Add DelayScheduler class Schedule callbacks whith the only guarantee that they will not be run for the minimum duration specified. Useful for garbage collecting POSIX timer_create() signal info data. --- src/xenia/base/delay_scheduler.cc | 111 +++++++++++++++++++++++ src/xenia/base/delay_scheduler.h | 142 ++++++++++++++++++++++++++++++ 2 files changed, 253 insertions(+) create mode 100644 src/xenia/base/delay_scheduler.cc create mode 100644 src/xenia/base/delay_scheduler.h diff --git a/src/xenia/base/delay_scheduler.cc b/src/xenia/base/delay_scheduler.cc new file mode 100644 index 000000000..982d1d05a --- /dev/null +++ b/src/xenia/base/delay_scheduler.cc @@ -0,0 +1,111 @@ +/** + ****************************************************************************** + * Xenia : Xbox 360 Emulator Research Project * + ****************************************************************************** + * Copyright 2022 Ben Vanik. All rights reserved. * + * Released under the BSD license - see LICENSE in the root for more details. * + ****************************************************************************** + */ + +#include "xenia/base/delay_scheduler.h" +#include "xenia/base/assert.h" +#include "xenia/base/logging.h" + +namespace xe::internal { + +DelaySchedulerBase::~DelaySchedulerBase() { + if (call_on_destruct_) { + for (auto& slot : deletion_queue_) { + // No thread safety in destructors anyway + if (slot.state == State::kWaiting) { + callback_(slot.info); + } + } + } +} + +void DelaySchedulerBase::Collect() { + static_assert(atomic_state_t::is_always_lock_free, + "Locks are unsafe to use with thread suspension"); + + for (auto& slot : deletion_queue_) { + TryCollectOne_impl(slot); + } +} + +bool DelaySchedulerBase::TryCollectOne_impl(deletion_record_t& slot) { + auto now = clock::now(); + + auto state = State::kWaiting; + // Try to lock the waiting slot for reading the other fields + if (slot.state.compare_exchange_strong(state, State::kDeleting, + std::memory_order_acq_rel)) { + if (now > slot.due) { + // Time has passed, call back now + callback_(slot.info); + slot.info = nullptr; + slot.state.store(State::kFree, std::memory_order_release); + return true; + } else { + // Oops it's not yet due + slot.state.store(State::kWaiting, std::memory_order_release); + } + } + return false; +} + +void DelaySchedulerBase::ScheduleAt_impl( + void* info, const clock::time_point& timeout_time) { + bool first_pass = true; + + if (info == nullptr) { + return; + } + + for (;;) { + if (TryScheduleAt_impl(info, timeout_time)) { + return; + } + + if (first_pass) { + first_pass = false; + XELOGE( + "`DelayScheduler::ScheduleAt(...)` stalled: list is full! Find out " + "why or increase `MAX_QUEUE`."); + } + } +} + +bool DelaySchedulerBase::TryScheduleAt_impl( + void* info, const clock::time_point& timeout_time) { + if (info == nullptr) { + return false; + } + + for (auto& slot : deletion_queue_) { + // Clean up due item + TryCollectOne_impl(slot); + + if (TryScheduleAtOne_impl(slot, info, timeout_time)) { + return true; + } + } + return false; +} + +bool DelaySchedulerBase::TryScheduleAtOne_impl(deletion_record_t& slot, + void* info, + clock::time_point due) { + auto state = State::kFree; + if (slot.state.compare_exchange_strong(state, State::kInitializing, + std::memory_order_acq_rel)) { + slot.info = info; + slot.due = due; + slot.state.store(State::kWaiting, std::memory_order_release); + return true; + } + + return false; +} + +} // namespace xe::internal \ No newline at end of file diff --git a/src/xenia/base/delay_scheduler.h b/src/xenia/base/delay_scheduler.h new file mode 100644 index 000000000..33cf196c9 --- /dev/null +++ b/src/xenia/base/delay_scheduler.h @@ -0,0 +1,142 @@ +/** + ****************************************************************************** + * Xenia : Xbox 360 Emulator Research Project * + ****************************************************************************** + * Copyright 2022 Ben Vanik. All rights reserved. * + * Released under the BSD license - see LICENSE in the root for more details. * + ****************************************************************************** + */ + +#ifndef XENIA_BASE_DELAY_SCHEDULER_H_ +#define XENIA_BASE_DELAY_SCHEDULER_H_ + +#include +#include +#include +#include +#include + +namespace xe { + +namespace internal { +// Put the implementation in a non templated base class to reduce compile time +// and code duplication +class DelaySchedulerBase { + protected: // Types + enum class State : uint_least8_t { + kFree = 0, // Slot is unsued + kInitializing, // Slot is reserved and currently written to by a thread + kWaiting, // The slot contains a pointer scheduled for deletion + kDeleting // A thread is currently deleting the pointer + }; + using atomic_state_t = std::atomic; + using clock = std::chrono::steady_clock; + struct deletion_record_t { + atomic_state_t state; + void* info; + clock::time_point due; + }; + using deletion_queue_t = std::vector; + using callback_t = std::function; + + public: + /// Check all scheduled items in the queue and free any that are due using + /// `callback(info);`. Call this to reliably collect all due wait items in the + /// queue. + void Collect(); + + size_t size() { return deletion_queue_.size(); } + + protected: // Implementation + DelaySchedulerBase(size_t queue_size, callback_t callback, + bool call_on_destruct) + : deletion_queue_(queue_size), + callback_(callback), + call_on_destruct_(call_on_destruct) {} + virtual ~DelaySchedulerBase(); + + void ScheduleAt_impl(void* info, const clock::time_point& timeout_time); + [[nodiscard]] bool TryScheduleAt_impl(void* info, + const clock::time_point& timeout_time); + + /// Checks if the slot is due and if so, call back for it. + bool TryCollectOne_impl(deletion_record_t& slot); + + [[nodiscard]] bool TryScheduleAtOne_impl(deletion_record_t& slot, void* info, + clock::time_point due); + + private: + deletion_queue_t deletion_queue_; + callback_t callback_; + bool call_on_destruct_; +}; +} // namespace internal + +/// A lazy scheduler/timer. +/// Will wait at least the specified duration before invoking the callbacks but +/// might wait until it is destructed. Lockless thread-safe, will spinlock +/// though if the wait queue is full (except for `Try`... methods). Might use +/// any thread that calls any member to invoke callbacks of due wait items. +template +class DelayScheduler : internal::DelaySchedulerBase { + public: + DelayScheduler(size_t queue_size, std::function callback, + bool call_on_destruct) + : DelaySchedulerBase( + queue_size, + [callback](void* info) { callback(reinterpret_cast(info)); }, + call_on_destruct){}; + DelayScheduler(const DelayScheduler&) = delete; + DelayScheduler& operator=(const DelayScheduler&) = delete; + virtual ~DelayScheduler() {} + + // From base class: + // void Collect(); + + /// Schedule an object for deletion at some point after `timeout_time` using + /// `callback(info);`. Will collect any wait items it encounters which can be + /// 0 or all, use `Collect()` to collect all due wait items. Blocks until a + /// free wait slot is found. + template + void ScheduleAt( + INFO* info, + const std::chrono::time_point& timeout_time) { + ScheduleAt(info, + std::chrono::time_point_cast(timeout_time)); + } + /// Like `ScheduleAt` but does not block on full list. + template + [[nodiscard]] bool TryScheduleAt( + INFO* info, + const std::chrono::time_point& timeout_time) { + return TryScheduleAt( + info, std::chrono::time_point_cast(timeout_time)); + } + + void ScheduleAt(INFO* info, const clock::time_point& timeout_time) { + ScheduleAt_impl(info, timeout_time); + } + [[nodiscard]] bool TryScheduleAt(INFO* info, + const clock::time_point& timeout_time) { + return TryScheduleAt_impl(info, timeout_time); + } + + /// Schedule a callback at some point after `rel_time` has passed. + template + void ScheduleAfter(INFO* info, + const std::chrono::duration& rel_time) { + ScheduleAt(info, + clock::now() + std::chrono::ceil(rel_time)); + } + /// Like `ScheduleAfter` but does not block. + template + [[nodiscard]] bool TryScheduleAfter( + INFO* info, const std::chrono::duration& rel_time) { + return TryScheduleAt( + info, clock::now() + std::chrono::ceil(rel_time)); + } +}; + +} // namespace xe + +#endif From b72ab7b4a4356ec5a19d5678ca739f03d62b667a Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Tue, 1 Mar 2022 23:41:03 +0100 Subject: [PATCH 16/24] [Base] Refactor POSIX timers, fix user-after-free Since timer_delete does not clean up already queued signals, signal info data needs to be retained after timer deletion and object destruction in order to circumvent use-after-free bugs. --- src/xenia/base/threading_posix.cc | 175 ++++++++++++++++++++++-------- 1 file changed, 131 insertions(+), 44 deletions(-) diff --git a/src/xenia/base/threading_posix.cc b/src/xenia/base/threading_posix.cc index 2cab9d371..f0c315573 100644 --- a/src/xenia/base/threading_posix.cc +++ b/src/xenia/base/threading_posix.cc @@ -10,7 +10,7 @@ #include "xenia/base/threading.h" #include "xenia/base/assert.h" -#include "xenia/base/logging.h" +#include "xenia/base/delay_scheduler.h" #include "xenia/base/platform.h" #include @@ -23,7 +23,6 @@ #include #include #include -#include #include #include @@ -79,6 +78,47 @@ void AndroidShutdown() { } #endif +// This is separately allocated for each (`HighResolution`)`Timer` object. It +// will be cleaned up some time (`timers_garbage_collector_delay`) after the +// posix timer was canceled because posix `timer_delete(...)` does not remove +// pending timer signals. +// https://stackoverflow.com/questions/49756114/linux-timer-pending-signal +struct timer_callback_info_t { + std::atomic_bool disarmed; +#if !XE_HAS_SIGEV_THREAD_ID + pthread_t target_thread; +#endif + std::function callback; + void* userdata; + + timer_callback_info_t(std::function callback) + : disarmed(false), +#if !XE_HAS_SIGEV_THREAD_ID + target_thread(), +#endif + callback(callback), + userdata(nullptr) { + } +}; + +// GC for timer signal info structs: +constexpr uint_fast8_t timers_garbage_collector_scale_ = +#if XE_HAS_SIGEV_THREAD_ID + 1; +#else + 2; +#endif +DelayScheduler timers_garbage_collector_( + 512 * timers_garbage_collector_scale_, + [](timer_callback_info_t* info) { + assert_not_null(info); + delete info; + }, + true); +// Delay we have to assume it takes to clear all pending signals (maximum): +constexpr auto timers_garbage_collector_delay = + std::chrono::milliseconds(100 * timers_garbage_collector_scale_); + template inline timespec DurationToTimeSpec( std::chrono::duration<_Rep, _Period> duration) { @@ -195,9 +235,20 @@ bool SetTlsValue(TlsHandle handle, uintptr_t value) { class PosixHighResolutionTimer : public HighResolutionTimer { public: explicit PosixHighResolutionTimer(std::function callback) - : callback_(std::move(callback)), valid_(false) {} + : valid_(false) { + callback_info_ = new timer_callback_info_t(std::move(callback)); + } ~PosixHighResolutionTimer() override { - if (valid_) timer_delete(timer_); + if (valid_) { + callback_info_->disarmed = true; + timer_delete(timerid_); + // Deliberately leaks memory when wait queue is full instead of blogs, + // check logs + static_cast(timers_garbage_collector_.TryScheduleAfter( + callback_info_, timers_garbage_collector_delay)); + } else { + delete callback_info_; + } } bool Initialize(std::chrono::milliseconds period) { @@ -216,20 +267,23 @@ class PosixHighResolutionTimer : public HighResolutionTimer { callback_info_->target_thread = pthread_self(); #endif sev.sigev_signo = GetSystemSignal(SignalType::kHighResolutionTimer); - sev.sigev_value.sival_ptr = (void*)&callback_; - if (timer_create(CLOCK_MONOTONIC, &sev, &timer_) == -1) return false; + sev.sigev_value.sival_ptr = callback_info_; + if (timer_create(CLOCK_MONOTONIC, &sev, &timerid_) == -1) return false; // Start timer itimerspec its{}; its.it_value = DurationToTimeSpec(period); its.it_interval = its.it_value; - valid_ = timer_settime(timer_, 0, &its, nullptr) != -1; + valid_ = timer_settime(timerid_, 0, &its, nullptr) != -1; + if (!valid_) { + timer_delete(timerid_); + } return valid_; } private: - std::function callback_; - timer_t timer_; + timer_callback_info_t* callback_info_; + timer_t timerid_; bool valid_; // all values for timer_t are legal so we need this }; @@ -441,39 +495,47 @@ template <> class PosixCondition : public PosixConditionBase { public: explicit PosixCondition(bool manual_reset) - : callback_(), - timer_(nullptr), + : timer_(nullptr), + callback_info_(nullptr), signal_(false), manual_reset_(manual_reset) {} virtual ~PosixCondition() { Cancel(); } bool Signal() override { - CompletionRoutine(); + std::lock_guard lock(mutex_); + signal_ = true; + cond_.notify_all(); return true; } // TODO(bwrsandman): due_times of under 1ms deadlock under travis + // TODO(joellinn): This is likely due to deadlock on mutex_ if Signal() is + // called from signal_handler running in Thread A while thread A was still in + // Set(...) routine inside the lock bool Set(std::chrono::nanoseconds due_time, std::chrono::milliseconds period, std::function opt_callback = nullptr) { - std::lock_guard lock(mutex_); + Cancel(); - callback_ = std::move(opt_callback); + std::lock_guard lock(mutex_); + callback_info_ = new timer_callback_info_t(std::move(opt_callback)); + callback_info_->userdata = this; signal_ = false; // Create timer - if (timer_ == nullptr) { - sigevent sev{}; + sigevent sev{}; #if XE_HAS_SIGEV_THREAD_ID - sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID; - sev.sigev_notify_thread_id = gettid(); + sev.sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID; + sev.sigev_notify_thread_id = gettid(); #else - sev.sigev_notify = SIGEV_SIGNAL; - callback_info_->target_thread = pthread_self(); + sev.sigev_notify = SIGEV_SIGNAL; + callback_info_->target_thread = pthread_self(); #endif - sev.sigev_signo = GetSystemSignal(SignalType::kTimer); - sev.sigev_value.sival_ptr = this; - if (timer_create(CLOCK_MONOTONIC, &sev, &timer_) == -1) return false; + sev.sigev_signo = GetSystemSignal(SignalType::kTimer); + sev.sigev_value.sival_ptr = callback_info_; + if (timer_create(CLOCK_MONOTONIC, &sev, &timer_) == -1) { + delete callback_info_; + return false; } // Start timer @@ -483,26 +545,16 @@ class PosixCondition : public PosixConditionBase { return timer_settime(timer_, 0, &its, nullptr) == 0; } - void CompletionRoutine() { - // As the callback may reset the timer, store local. - std::function callback; - { - std::lock_guard lock(mutex_); - // Store callback - if (callback_) callback = callback_; - signal_ = true; - cond_.notify_all(); - } - // Call callback - if (callback) callback(); - } - bool Cancel() { std::lock_guard lock(mutex_); bool result = true; if (timer_) { + callback_info_->disarmed = true; result = timer_delete(timer_) == 0; timer_ = nullptr; + static_cast(timers_garbage_collector_.TryScheduleAfter( + callback_info_, timers_garbage_collector_delay)); + callback_info_ = nullptr; } return result; } @@ -518,8 +570,8 @@ class PosixCondition : public PosixConditionBase { signal_ = false; } } - std::function callback_; timer_t timer_; + timer_callback_info_t* callback_info_; volatile bool signal_; const bool manual_reset_; }; @@ -1202,15 +1254,50 @@ static void signal_handler(int signal, siginfo_t* info, void* /*context*/) { switch (GetSystemSignalType(signal)) { case SignalType::kHighResolutionTimer: { assert_not_null(info->si_value.sival_ptr); - auto callback = - *static_cast*>(info->si_value.sival_ptr); - callback(); + auto timer_info = + reinterpret_cast(info->si_value.sival_ptr); + if (!timer_info->disarmed) { +#if XE_HAS_SIGEV_THREAD_ID + { +#else + if (pthread_self() != timer_info->target_thread) { + sigval info_inner{}; + info_inner.sival_ptr = timer_info; + const auto queueres = pthread_sigqueue( + timer_info->target_thread, + GetSystemSignal(SignalType::kHighResolutionTimer), info_inner); + assert_zero(queueres); + } else { +#endif + timer_info->callback(); + } + } } break; case SignalType::kTimer: { assert_not_null(info->si_value.sival_ptr); - auto pTimer = - static_cast*>(info->si_value.sival_ptr); - pTimer->CompletionRoutine(); + auto timer_info = + reinterpret_cast(info->si_value.sival_ptr); + if (!timer_info->disarmed) { + assert_not_null(timer_info->userdata); + auto timer = static_cast*>(timer_info->userdata); +#if XE_HAS_SIGEV_THREAD_ID + { +#else + if (pthread_self() != timer_info->target_thread) { + sigval info_inner{}; + info_inner.sival_ptr = timer_info; + const auto queueres = + pthread_sigqueue(timer_info->target_thread, + GetSystemSignal(SignalType::kTimer), info_inner); + assert_zero(queueres); + } else { +#endif + timer->Signal(); + if (timer_info->callback) { + timer_info->callback(); + } + } + } } break; case SignalType::kThreadSuspend: { assert_not_null(current_thread_); From b625ef0a3838d1a247448324bb120e2f1d409ff9 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Thu, 3 Mar 2022 00:14:47 +0100 Subject: [PATCH 17/24] [CI] More verbose xenia-base-tests output --- .drone.star | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.drone.star b/.drone.star index 50b5c9ec3..7a1af7633 100644 --- a/.drone.star +++ b/.drone.star @@ -277,7 +277,7 @@ def pipeline_linux_desktop(name, image, arch, cc, build_release_all): 'image': image, 'volumes': [volume_build('premake')], 'commands': [ - 'valgrind --error-exitcode=99 ./build/bin/Linux/Debug/xenia-base-tests', + 'valgrind --error-exitcode=99 ./build/bin/Linux/Debug/xenia-base-tests --durations yes', ], 'depends_on': ['build-premake-debug-tests'], }, @@ -287,7 +287,7 @@ def pipeline_linux_desktop(name, image, arch, cc, build_release_all): 'image': image, 'volumes': [volume_build('premake')], 'commands': [ - './build/bin/Linux/Release/xenia-base-tests', + './build/bin/Linux/Release/xenia-base-tests --success --durations yes', ], 'depends_on': ['build-premake-release-tests'], }, @@ -297,7 +297,7 @@ def pipeline_linux_desktop(name, image, arch, cc, build_release_all): 'image': image, 'volumes': [volume_build('cmake')], 'commands': [ - './build/bin/Linux/Release/xenia-base-tests', + './build/bin/Linux/Release/xenia-base-tests --success --durations yes', ], 'depends_on': ['build-cmake-release-tests'], }, From 38d589d1e09a2c629deb5b99ae07bdb77bef0602 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Thu, 24 Feb 2022 22:38:55 +0100 Subject: [PATCH 18/24] [kernel] Remove unnecessary string copy --- src/xenia/kernel/util/shim_utils.h | 12 ++++++------ src/xenia/kernel/xboxkrnl/xboxkrnl_io.cc | 3 +-- src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc | 8 ++++---- src/xenia/kernel/xobject.cc | 2 +- src/xenia/kernel/xsymboliclink.cc | 4 ++-- 5 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/xenia/kernel/util/shim_utils.h b/src/xenia/kernel/util/shim_utils.h index 0b6551615..579305a72 100644 --- a/src/xenia/kernel/util/shim_utils.h +++ b/src/xenia/kernel/util/shim_utils.h @@ -84,18 +84,18 @@ inline uint64_t get_arg_64(PPCContext* ppc_context, uint8_t index) { return SHIM_MEM_64(stack_address); } -inline std::string TranslateAnsiString(const Memory* memory, - const X_ANSI_STRING* ansi_string) { +inline std::string_view TranslateAnsiString(const Memory* memory, + const X_ANSI_STRING* ansi_string) { if (!ansi_string || !ansi_string->length) { return ""; } - return std::string( + return std::string_view( memory->TranslateVirtual(ansi_string->pointer), ansi_string->length); } -inline std::string TranslateAnsiStringAddress(const Memory* memory, - uint32_t guest_address) { +inline std::string_view TranslateAnsiStringAddress(const Memory* memory, + uint32_t guest_address) { if (!guest_address) { return ""; } @@ -440,7 +440,7 @@ inline void AppendParam(StringBuffer* string_buffer, if (record) { auto name_string = kernel_memory()->TranslateVirtual(record->name_ptr); - std::string name = + std::string_view name = name_string == nullptr ? "(null)" : util::TranslateAnsiString(kernel_memory(), name_string); diff --git a/src/xenia/kernel/xboxkrnl/xboxkrnl_io.cc b/src/xenia/kernel/xboxkrnl/xboxkrnl_io.cc index 2e2022bc6..3e7a700ef 100644 --- a/src/xenia/kernel/xboxkrnl/xboxkrnl_io.cc +++ b/src/xenia/kernel/xboxkrnl/xboxkrnl_io.cc @@ -614,8 +614,7 @@ dword_result_t NtOpenSymbolicLinkObject_entry( auto object_name = kernel_memory()->TranslateVirtual(object_attrs->name_ptr); - std::string target_path = - util::TranslateAnsiString(kernel_memory(), object_name); + auto target_path = util::TranslateAnsiString(kernel_memory(), object_name); // Enforce that the path is ASCII. if (!IsValidPath(target_path, false)) { diff --git a/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc b/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc index 1eec0e430..8ecc5b3c0 100644 --- a/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc +++ b/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc @@ -144,10 +144,10 @@ DECLARE_XBOXKRNL_EXPORT1(ObDereferenceObject, kNone, kImplemented); dword_result_t ObCreateSymbolicLink_entry(pointer_t path_ptr, pointer_t target_ptr) { - auto path = util::TranslateAnsiString(kernel_memory(), path_ptr); - auto target = util::TranslateAnsiString(kernel_memory(), target_ptr); - path = xe::utf8::canonicalize_guest_path(path); - target = xe::utf8::canonicalize_guest_path(target); + auto path = xe::utf8::canonicalize_guest_path( + util::TranslateAnsiString(kernel_memory(), path_ptr)); + auto target = xe::utf8::canonicalize_guest_path( + util::TranslateAnsiString(kernel_memory(), target_ptr)); if (xe::utf8::starts_with(path, u8"\\??\\")) { path = path.substr(4); // Strip the full qualifier diff --git a/src/xenia/kernel/xobject.cc b/src/xenia/kernel/xobject.cc index 20b85249b..bf189c2af 100644 --- a/src/xenia/kernel/xobject.cc +++ b/src/xenia/kernel/xobject.cc @@ -172,7 +172,7 @@ void XObject::SetAttributes(uint32_t obj_attributes_ptr) { memory(), xe::load_and_swap( memory()->TranslateVirtual(obj_attributes_ptr + 4))); if (!name.empty()) { - name_ = std::move(name); + name_ = std::string(name); kernel_state_->object_table()->AddNameMapping(name_, handles_[0]); } } diff --git a/src/xenia/kernel/xsymboliclink.cc b/src/xenia/kernel/xsymboliclink.cc index 0d0a2ad43..e282a8879 100644 --- a/src/xenia/kernel/xsymboliclink.cc +++ b/src/xenia/kernel/xsymboliclink.cc @@ -24,8 +24,8 @@ XSymbolicLink::~XSymbolicLink() {} void XSymbolicLink::Initialize(const std::string_view path, const std::string_view target) { - path_ = path; - target_ = target; + path_ = std::string(path); + target_ = std::string(target); // TODO(gibbed): kernel_state_->RegisterSymbolicLink(this); } From 91f495496796a88bc38f074b9ac3b32648977602 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Thu, 24 Feb 2022 22:39:49 +0100 Subject: [PATCH 19/24] [kernel] Refactor uses of attribute names --- src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc | 14 ++++++++++---- src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc | 10 ++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc b/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc index 8ecc5b3c0..e09096eb0 100644 --- a/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc +++ b/src/xenia/kernel/xboxkrnl/xboxkrnl_ob.cc @@ -7,6 +7,7 @@ ****************************************************************************** */ +#include "xenia/base/assert.h" #include "xenia/base/logging.h" #include "xenia/kernel/kernel_state.h" #include "xenia/kernel/util/shim_utils.h" @@ -31,10 +32,15 @@ dword_result_t ObOpenObjectByName_entry(lpunknown_t obj_attributes_ptr, // r5 = 0 // r6 = out_ptr (handle?) - auto name = util::TranslateAnsiStringAddress( - kernel_memory(), - xe::load_and_swap(kernel_memory()->TranslateVirtual( - obj_attributes_ptr.guest_address() + 4))); + if (!obj_attributes_ptr) { + return X_STATUS_INVALID_PARAMETER; + } + + auto obj_attributes = kernel_memory()->TranslateVirtual( + obj_attributes_ptr); + assert_true(obj_attributes->name_ptr != 0); + auto name = util::TranslateAnsiStringAddress(kernel_memory(), + obj_attributes->name_ptr); X_HANDLE handle = X_INVALID_HANDLE_VALUE; X_STATUS result = diff --git a/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc b/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc index 7e6c605e4..5b3c97fc0 100644 --- a/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc +++ b/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc @@ -74,10 +74,12 @@ object_ref LookupNamedObject(KernelState* kernel_state, if (!obj_attributes_ptr) { return nullptr; } - auto name = util::TranslateAnsiStringAddress( - kernel_state->memory(), - xe::load_and_swap( - kernel_state->memory()->TranslateVirtual(obj_attributes_ptr + 4))); + auto obj_attributes = + kernel_state->memory()->TranslateVirtual( + obj_attributes_ptr); + assert_true(obj_attributes->name_ptr != 0); + auto name = util::TranslateAnsiStringAddress(kernel_state->memory(), + obj_attributes->name_ptr); if (!name.empty()) { X_HANDLE handle = X_INVALID_HANDLE_VALUE; X_RESULT result = From 7e894d10a765cf17bacaecd6f33efb9668448842 Mon Sep 17 00:00:00 2001 From: Joel Linn Date: Thu, 24 Feb 2022 22:43:47 +0100 Subject: [PATCH 20/24] [kernel] Correct status for looked up objects - The guest will check for 0x40000000 and replace it with 0xb7 (ERROR_ALREADY_EXISTS), which is the correct return value. For example, see: https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-createmutexa --- src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc b/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc index 5b3c97fc0..9da95a84d 100644 --- a/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc +++ b/src/xenia/kernel/xboxkrnl/xboxkrnl_threading.cc @@ -455,7 +455,7 @@ dword_result_t NtCreateEvent_entry( existing_object->RetainHandle(); *handle_ptr = existing_object->handle(); } - return X_STATUS_SUCCESS; + return X_STATUS_OBJECT_NAME_EXISTS; } else { return X_STATUS_INVALID_HANDLE; } @@ -576,7 +576,7 @@ DECLARE_XBOXKRNL_EXPORT1(KeReleaseSemaphore, kThreading, kImplemented); dword_result_t NtCreateSemaphore_entry(lpdword_t handle_ptr, lpvoid_t obj_attributes_ptr, dword_t count, dword_t limit) { - // Check for an existing timer with the same name. + // Check for an existing semaphore with the same name. auto existing_object = LookupNamedObject(kernel_state(), obj_attributes_ptr); if (existing_object) { @@ -585,7 +585,7 @@ dword_result_t NtCreateSemaphore_entry(lpdword_t handle_ptr, existing_object->RetainHandle(); *handle_ptr = existing_object->handle(); } - return X_STATUS_SUCCESS; + return X_STATUS_OBJECT_NAME_EXISTS; } else { return X_STATUS_INVALID_HANDLE; } @@ -647,7 +647,7 @@ dword_result_t NtCreateMutant_entry( existing_object->RetainHandle(); *handle_out = existing_object->handle(); } - return X_STATUS_SUCCESS; + return X_STATUS_OBJECT_NAME_EXISTS; } else { return X_STATUS_INVALID_HANDLE; } @@ -709,7 +709,7 @@ dword_result_t NtCreateTimer_entry(lpdword_t handle_ptr, existing_object->RetainHandle(); *handle_ptr = existing_object->handle(); } - return X_STATUS_SUCCESS; + return X_STATUS_OBJECT_NAME_EXISTS; } else { return X_STATUS_INVALID_HANDLE; } From 337f0b2948de65650d3b4ccaa45f6c13516b0a8f Mon Sep 17 00:00:00 2001 From: Wunkolo Date: Thu, 3 Feb 2022 13:19:33 -0800 Subject: [PATCH 21/24] [x64] Add AVX512 optimization for `VECTOR_ROTATE_LEFT(Int32)` `vprolvd` is an almost 1:1 analog with this opcode and can be conditionally emitted when the host supports AVX512{F,VL}. Altivec docs say that `vrl{bhw}` masks the lower log2(n) bits of the element-size. [vprold](https://www.felixcloutier.com/x86/vprold:vprolvd:vprolq:vprolvq) modulos the shift-value by the element size in bits, which is the same as masking the lower log2(n) bits. So `vrlw` maps exactly to `vprold`. --- src/xenia/cpu/backend/x64/x64_seq_vector.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/xenia/cpu/backend/x64/x64_seq_vector.cc b/src/xenia/cpu/backend/x64/x64_seq_vector.cc index 4daea260b..7cf4650b5 100644 --- a/src/xenia/cpu/backend/x64/x64_seq_vector.cc +++ b/src/xenia/cpu/backend/x64/x64_seq_vector.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2018 Xenia Developers. All rights reserved. * + * Copyright 2022 Xenia Developers. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -1287,7 +1287,6 @@ static __m128i EmulateVectorRotateLeft(void*, __m128i src1, __m128i src2) { return _mm_load_si128(reinterpret_cast<__m128i*>(value)); } -// TODO(benvanik): AVX512 has a native variable rotate (rolv). struct VECTOR_ROTATE_LEFT_V128 : Sequence> { @@ -1318,7 +1317,9 @@ struct VECTOR_ROTATE_LEFT_V128 e.vmovaps(i.dest, e.xmm0); break; case INT32_TYPE: { - if (e.IsFeatureEnabled(kX64EmitAVX2)) { + if (e.IsFeatureEnabled(kX64EmitAVX512Ortho)) { + e.vprolvd(i.dest, i.src1, i.src2); + } else if (e.IsFeatureEnabled(kX64EmitAVX2)) { Xmm temp = i.dest; if (i.dest == i.src1 || i.dest == i.src2) { temp = e.xmm2; @@ -2683,4 +2684,4 @@ EMITTER_OPCODE_TABLE(OPCODE_UNPACK, UNPACK); } // namespace x64 } // namespace backend } // namespace cpu -} // namespace xe \ No newline at end of file +} // namespace xe From f356cf5df8590e1f46612ba5e205777769923c16 Mon Sep 17 00:00:00 2001 From: Wunkolo Date: Thu, 3 Feb 2022 13:42:19 -0800 Subject: [PATCH 22/24] [x64] Add `VECTOR_ROTATE_LEFT_I32` overflow-test Edit one of the lanes in this unit-test to be larger than the width of the element-size to ensure that this case is handled correctly. It should only mask the lower `log2(32)=5` bits of the input, causing `33`(`100001`) to be `1`(`000001`). --- src/xenia/cpu/testing/vector_rotate_left_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xenia/cpu/testing/vector_rotate_left_test.cc b/src/xenia/cpu/testing/vector_rotate_left_test.cc index 406caa157..d7d76c542 100644 --- a/src/xenia/cpu/testing/vector_rotate_left_test.cc +++ b/src/xenia/cpu/testing/vector_rotate_left_test.cc @@ -2,7 +2,7 @@ ****************************************************************************** * Xenia : Xbox 360 Emulator Research Project * ****************************************************************************** - * Copyright 2014 Ben Vanik. All rights reserved. * + * Copyright 2022 Ben Vanik. All rights reserved. * * Released under the BSD license - see LICENSE in the root for more details. * ****************************************************************************** */ @@ -62,7 +62,7 @@ TEST_CASE("VECTOR_ROTATE_LEFT_I32", "[instr]") { test.Run( [](PPCContext* ctx) { ctx->v[4] = vec128i(0x00000001, 0x00000001, 0x80000000, 0x80000000); - ctx->v[5] = vec128i(0, 1, 1, 2); + ctx->v[5] = vec128i(0, 1, 33, 2); }, [](PPCContext* ctx) { auto result = ctx->v[3]; From c1de37f381f5363ed1470dd622b53159bb0fe1a6 Mon Sep 17 00:00:00 2001 From: Wunkolo Date: Thu, 3 Feb 2022 14:21:59 -0800 Subject: [PATCH 23/24] [x64] Remove usage of `xbyak_bin2hex.h` C++ has had binary-literals since C++14. There is no need for these binary enum values from xbyak. --- src/xenia/cpu/backend/x64/x64_emitter.h | 1 - src/xenia/cpu/testing/vector_rotate_left_test.cc | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/xenia/cpu/backend/x64/x64_emitter.h b/src/xenia/cpu/backend/x64/x64_emitter.h index 9117beea3..247d6175c 100644 --- a/src/xenia/cpu/backend/x64/x64_emitter.h +++ b/src/xenia/cpu/backend/x64/x64_emitter.h @@ -22,7 +22,6 @@ // NOTE: must be included last as it expects windows.h to already be included. #include "third_party/xbyak/xbyak/xbyak.h" -#include "third_party/xbyak/xbyak/xbyak_bin2hex.h" #include "third_party/xbyak/xbyak/xbyak_util.h" namespace xe { diff --git a/src/xenia/cpu/testing/vector_rotate_left_test.cc b/src/xenia/cpu/testing/vector_rotate_left_test.cc index d7d76c542..2f378246c 100644 --- a/src/xenia/cpu/testing/vector_rotate_left_test.cc +++ b/src/xenia/cpu/testing/vector_rotate_left_test.cc @@ -7,7 +7,6 @@ ****************************************************************************** */ -#include "third_party/xbyak/xbyak/xbyak_bin2hex.h" #include "xenia/cpu/testing/util.h" using namespace xe; @@ -23,16 +22,17 @@ TEST_CASE("VECTOR_ROTATE_LEFT_I8", "[instr]") { }); test.Run( [](PPCContext* ctx) { - ctx->v[4] = vec128b(B00000001); + ctx->v[4] = vec128b(0b00000001); ctx->v[5] = vec128b(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15); }, [](PPCContext* ctx) { auto result = ctx->v[3]; - REQUIRE(result == vec128b(B00000001, B00000010, B00000100, B00001000, - B00010000, B00100000, B01000000, B10000000, - B00000001, B00000010, B00000100, B00001000, - B00010000, B00100000, B01000000, B10000000)); + REQUIRE(result == + vec128b(0b00000001, 0b00000010, 0b00000100, 0b00001000, + 0b00010000, 0b00100000, 0b01000000, 0b10000000, + 0b00000001, 0b00000010, 0b00000100, 0b00001000, + 0b00010000, 0b00100000, 0b01000000, 0b10000000)); }); } From 82c1fb87aaaef40e82c31bcb0bf288b098c28f94 Mon Sep 17 00:00:00 2001 From: Triang3l Date: Mon, 14 Mar 2022 20:42:52 +0300 Subject: [PATCH 24/24] [App] Do all fullscreen entry logic for --fullscreen=true (fixes #1999) --- src/xenia/app/emulator_window.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xenia/app/emulator_window.cc b/src/xenia/app/emulator_window.cc index a4dbb8786..7e6d9e6b7 100644 --- a/src/xenia/app/emulator_window.cc +++ b/src/xenia/app/emulator_window.cc @@ -224,7 +224,7 @@ void EmulatorWindow::OnEmulatorInitialized() { // When the user can see that the emulator isn't initializing anymore (the // menu isn't disabled), enter fullscreen if requested. if (cvars::fullscreen) { - window_->SetFullscreen(true); + SetFullscreen(true); } }