From 0d0abe09c19c85d61ac3f2227876a0c83037d6c8 Mon Sep 17 00:00:00 2001 From: Yunus Shaikh <75296019+EagleBearerYS@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:39:06 +0000 Subject: [PATCH] fix(libSceSaveData): start the event delivery thread at the first registration sceSaveDataInitialize3 started the callback delivery thread for every title that initializes save data, though nearly none register a callback, and could throw there if scePthreadCreate failed. The thread now starts at the first successful sceSaveDataRegisterEventCallback of an initialization, under the calling, g_slots_mutex and callbacks.mutex locks that registration already takes, so a title that never registers gets no thread and sceSaveDataInitialize3 is again only the count. A replacing registration, one from inside a callback and one after an unregister reuse the running thread; a refused one starts nothing. The last sceSaveDataTerminate still takes the worker out under g_slots_mutex and joins it outside the lock, and only when one was started, so a session that registers between that terminate's detach and its join starts its own thread. The tests run two sessions with no registration, and on Linux count the process's threads to check that Initialize3 and a refused registration start none, that the first registration starts one and later ones reuse it, and that the final terminate joins it. --- .../libs/prx/libSceSaveData.native/Export.cpp | 4 +- .../libs/tests/GuestSaveDataEventCallback.cpp | 57 +++++++++++++++++++ docs/dev/debt/libSceSaveData.native.md | 2 +- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/core/libs/prx/libSceSaveData.native/Export.cpp b/core/libs/prx/libSceSaveData.native/Export.cpp index e1f14d1902..cd3f8fd5ad 100644 --- a/core/libs/prx/libSceSaveData.native/Export.cpp +++ b/core/libs/prx/libSceSaveData.native/Export.cpp @@ -122,7 +122,7 @@ void StartEventDelivery() { std::lock_guard lock(callbacks.mutex); if (callbacks.worker != nullptr) return; if (scePthreadCreate(&worker->thread, nullptr, DeliverEvents, worker.get(), "SceSaveDataEventCallback") != 0) { - throw std::runtime_error("sceSaveDataInitialize3: scePthreadCreate failed for the event callback thread"); + throw std::runtime_error("sceSaveDataRegisterEventCallback: scePthreadCreate failed for the event callback thread"); } callbacks.worker = worker.release(); } @@ -1011,7 +1011,6 @@ int APS5_VABI sceSaveDataGetSaveDataMemory2(SaveDataMemoryGet2* get_param) { int APS5_VABI sceSaveDataInitialize3(const void* init) { (void)init; std::lock_guard lock(g_slots_mutex); - if (g_initializations == 0) StartEventDelivery(); ++g_initializations; return SAVE_DATA_OK; } @@ -1770,6 +1769,7 @@ int APS5_VABI sceSaveDataRegisterEventCallback(SaveDataEventCallback callback, v if (callback == nullptr) { return SAVE_DATA_ERROR_PARAMETER; } + StartEventDelivery(); std::lock_guard lock(callbacks.mutex); callbacks.callback = callback; callbacks.userdata = userdata; diff --git a/core/libs/tests/GuestSaveDataEventCallback.cpp b/core/libs/tests/GuestSaveDataEventCallback.cpp index ea269616cb..ea1d0f208a 100644 --- a/core/libs/tests/GuestSaveDataEventCallback.cpp +++ b/core/libs/tests/GuestSaveDataEventCallback.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -134,6 +135,21 @@ private: std::thread thread; }; +#ifdef __linux__ +std::size_t TaskCount() { + return static_cast(std::distance(std::filesystem::directory_iterator("/proc/self/task"), std::filesystem::directory_iterator{})); +} + +bool WaitForTaskCount(std::size_t count) { + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (TaskCount() != count) { + if (std::chrono::steady_clock::now() > deadline) return false; + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + return true; +} +#endif + void WriteText(const std::filesystem::path& path, const std::string& text) { std::ofstream file(path, std::ios::binary | std::ios::trunc); file << text; @@ -186,6 +202,43 @@ void CheckDelivered(std::size_t index, int callback, void* userdata, std::uint32 Check(SameEvent(polled, call.event), what + " is the event the poll returns"); } +void CheckSessionsWithoutRegistration() { +#ifdef __linux__ + const auto before = TaskCount(); +#endif + for (int session = 0; session < 2; ++session) { + const auto label = "session " + std::to_string(session) + " without a registration"; + Check(sceSaveDataInitialize3(nullptr) == 0, label + ": SaveData initializes"); +#ifdef __linux__ + Check(TaskCount() == before, label + ": Initialize3 starts no thread, " + std::to_string(before) + " threads before and " + std::to_string(TaskCount()) + " after"); +#endif + Check(sceSaveDataTerminate() == 0, label + ": SaveData terminates"); + } +} + +#ifdef __linux__ +void CheckDeliveryThreadStartsAtRegistration() { + const auto before = TaskCount(); + const auto counted = [&](const std::string& what) { return what + ", " + std::to_string(before) + " threads before and " + std::to_string(TaskCount()) + " now"; }; + int tag = 0; + Check(sceSaveDataInitialize3(nullptr) == 0, "SaveData initializes for the delivery thread check"); + Check(TaskCount() == before, counted("Initialize3 starts no delivery thread")); + Check(sceSaveDataRegisterEventCallback(nullptr, &tag) == SaveDataErrorParameter, "a null callback is PARAMETER in the delivery thread check"); + Check(TaskCount() == before, counted("a refused registration starts no delivery thread")); + Check(sceSaveDataRegisterEventCallback(First, &tag) == 0, "the first callback registers in the delivery thread check"); + Check(TaskCount() == before + 1, counted("the first registration starts one delivery thread")); + Check(sceSaveDataRegisterEventCallback(Second, &tag) == 0, "the second callback registers in the delivery thread check"); + Check(sceSaveDataUnregisterEventCallback() == 0, "the second callback unregisters in the delivery thread check"); + Check(sceSaveDataRegisterEventCallback(First, &tag) == 0, "the first callback registers again in the delivery thread check"); + Check(TaskCount() == before + 1, counted("a replacing registration and one after an unregister reuse the delivery thread")); + { + Watchdog watchdog("the terminate that joins the delivery thread"); + Check(sceSaveDataTerminate() == 0, "SaveData terminates after the delivery thread check"); + } + Check(WaitForTaskCount(before), counted("the final terminate joins the delivery thread within 5 s")); +} +#endif + void CheckRegistration() { int tag = 0; Check(sceSaveDataRegisterEventCallback(First, &tag) == SaveDataErrorNotInitialized, "register before initialize is NOT_INITIALIZED"); @@ -394,6 +447,10 @@ int main() { std::filesystem::current_path(root); int firstTag = 0; int secondTag = 0; + CheckSessionsWithoutRegistration(); +#ifdef __linux__ + CheckDeliveryThreadStartsAtRegistration(); +#endif CheckRegistration(); CheckBackupDelivery(&firstTag); CheckUmountWithBackupDelivery(&firstTag); diff --git a/docs/dev/debt/libSceSaveData.native.md b/docs/dev/debt/libSceSaveData.native.md index 16e2bfba66..e9b8d29929 100644 --- a/docs/dev/debt/libSceSaveData.native.md +++ b/docs/dev/debt/libSceSaveData.native.md @@ -32,7 +32,7 @@ - [sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - saves keep no icons, so the check never fills `icon`, as shadPS4 does without `icon0.png`, and another title's `titleId` returns `SAVE_DATA_ERROR_NOT_FOUND` because only the running title's saves are kept. Backups live in `_sd_backup/`, outside the save, and `sceSaveDataDelete` removes one with its save, as shadPS4's [backup](https://github.com/shadps4-emu/shadPS4/blob/ce74386283f559f2279a7c31416bcdc9359be0b3/src/core/libraries/save_data/save_backup.cpp#L93-L134) inside the save directory goes with it. On a PS4 Pro (firmware 10.71) another title's `titleId` also returns `SAVE_DATA_ERROR_NOT_FOUND`, and `sceSaveDataDelete` drops the backup too: after it the check and the restore return `SAVE_DATA_ERROR_NOT_FOUND` ([run](https://github.com/boykopovar/AnyPS5/pull/12748#issuecomment-6499581390)). - [sceSaveDataCheckBackupData](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - a `param` buffer that isn't zeroed is not refused here. On a PS4 Pro (firmware 10.71, the system `libSceSaveData.sprx`, SDK 0x05050001) a 1,328-byte `param` filled with 0xAA gave `SAVE_DATA_ERROR_PARAMETER` on every call, also with a backup present, while a zeroed or null one gave 0 ([run](https://github.com/boykopovar/AnyPS5/pull/12748#issuecomment-6499581390)). Which bytes the console checks wasn't measured, so the check fills `param` whatever it held. Not measured on a PS5. - [sceSaveDataRestoreBackupData](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - a crash during a restore is not recovered at the next start, and what the console leaves is not measured. A failed restore leaves the live save, its `.param` and its blocks file as they were: the backup is copied to `_sd_restore_tmp/` and checked first, and a failed move puts back what was already moved; only if that fails too does the old save stay in `_sd_restore_old/`. A crash during the copy leaves the live save untouched. One during the moves can leave parts of the live save in `_sd_restore_old/` and the copy in `_sd_restore_tmp/` or `_sd`; while the save's directory is missing from `_sd`, its restore returns `SAVE_DATA_ERROR_INTERNAL` and a creating mount makes an empty save. The next restore of the save first removes both directories. -- [sceSaveDataRegisterEventCallback, sceSaveDataUnregisterEventCallback](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - not measured on a console: the behaviour follows [shadPS4 #5226](https://github.com/shadps4-emu/shadPS4/pull/5226), and PS5's signatures are assumed to be PS4's (`void (*)(const SaveDataEvent*, void*)` and a userdata pointer). Unknown: which thread delivers, its priority and affinity, when it runs relative to the call, whether unregistering drops completions not yet delivered (here it does), and what a terminate from inside the callback does (here `SAVE_DATA_ERROR_BUSY`, as in shadPS4). Here `sceSaveDataInitialize3` starts the delivering thread with `scePthreadCreate`, the last `sceSaveDataTerminate` joins it, and each backup completion (type 1 or 2) is queued for `sceSaveDataGetEventResult` before it goes to the callback registered at that moment. +- [sceSaveDataRegisterEventCallback, sceSaveDataUnregisterEventCallback](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - not measured on a console: the behaviour follows [shadPS4 #5226](https://github.com/shadps4-emu/shadPS4/pull/5226), and PS5's signatures are assumed to be PS4's (`void (*)(const SaveDataEvent*, void*)` and a userdata pointer). Unknown: which thread delivers, its priority and affinity, when it runs relative to the call, whether unregistering drops completions not yet delivered (here it does), and what a terminate from inside the callback does (here `SAVE_DATA_ERROR_BUSY`, as in shadPS4). Here the first successful `sceSaveDataRegisterEventCallback` after `sceSaveDataInitialize3` starts the delivering thread with `scePthreadCreate`, the last `sceSaveDataTerminate` joins it if one was started, and each backup completion (type 1 or 2) is queued for `sceSaveDataGetEventResult` before it goes to the callback registered at that moment. - [sceSaveDataUmountWithBackup](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - a crash during a backup is not recovered at the next start either. A failed backup keeps the previous one in `_sd_backup/`. A crash between its two moves leaves the previous backup in `_sd_backup_old/`, where the check and the restore do not find it (`SAVE_DATA_ERROR_NOT_FOUND`); the next backup of the save removes it, and any partial copy in `_sd_backup_tmp/`, before it copies. - [sceSaveDataRestoreBackupData, sceSaveDataUmountWithBackup](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - on Windows nothing shows that a backup's or a restore's renames survive a crash or power loss. Every staged file is flushed before the first rename (`FlushFileBuffers` on Windows, `fsync` on POSIX, as libc does), and on POSIX the directories on both sides of each rename are flushed after it; Windows cannot flush directory entries, so there it rests on NTFS journaling. - [sceSaveDataBackup, sceSaveDataConvert, sceSaveDataDelete, sceSaveDataDirNameSearch, sceSaveDataDirNameSearchPs4, sceSaveDataGetEventResult, sceSaveDataGetMountInfo, sceSaveDataGetParam, sceSaveDataLoadIcon, sceSaveDataMount, sceSaveDataMount3, sceSaveDataSaveIcon, sceSaveDataSetParam, sceSaveDataUmount2](../../../core/libs/prx/libSceSaveData.native/Export.cpp) (libSceSaveData.native) - null pointer arguments return `SAVE_DATA_ERROR_PARAMETER` instead of throwing C++ exceptions, an implementation assumption matching guest error expectations not checked on hardware; unmounted targets return `SAVE_DATA_ERROR_NOT_MOUNTED`, and parameter type or buffer size violations return `SAVE_DATA_ERROR_PARAMETER` after slot lookup