From 916404f0e4c34c5b1fad8e46d4b8cdb7d89542bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 3 Jan 2023 19:01:51 +0100 Subject: [PATCH 1/7] Try to get a bit better at detaching threads that have used JNI from the VM. --- Common/File/PathBrowser.cpp | 2 ++ Common/Thread/ThreadManager.cpp | 11 +++++++---- Common/Thread/ThreadUtil.cpp | 22 +++++++++++++++++++++- Common/Thread/ThreadUtil.h | 17 ++++++++++++++++- Core/Config.cpp | 5 +++++ Core/Util/GameManager.cpp | 6 ++++++ android/jni/app-android.cpp | 12 +++++++++++- android/jni/app-android.h | 3 +++ 8 files changed, 71 insertions(+), 7 deletions(-) diff --git a/Common/File/PathBrowser.cpp b/Common/File/PathBrowser.cpp index a86f9f246f9e..ae7d804ed1fc 100644 --- a/Common/File/PathBrowser.cpp +++ b/Common/File/PathBrowser.cpp @@ -177,6 +177,8 @@ void PathBrowser::HandlePath() { ready_ = true; } } + + DetachThreadFromJNI(); }); } diff --git a/Common/Thread/ThreadManager.cpp b/Common/Thread/ThreadManager.cpp index 0414ee5e79ed..921831fa4076 100644 --- a/Common/Thread/ThreadManager.cpp +++ b/Common/Thread/ThreadManager.cpp @@ -44,6 +44,7 @@ struct ThreadContext { std::atomic cancelled; std::atomic private_single; std::deque private_queue; + char name[16]; }; ThreadManager::ThreadManager() : global_(new GlobalThreadContext()) { @@ -124,14 +125,13 @@ bool ThreadManager::TeardownTask(Task *task, bool enqueue) { } static void WorkerThreadFunc(GlobalThreadContext *global, ThreadContext *thread) { - char threadName[16]; if (thread->type == TaskType::CPU_COMPUTE) { - snprintf(threadName, sizeof(threadName), "PoolWorker %d", thread->index); + snprintf(thread->name, sizeof(thread->name), "PoolWorker %d", thread->index); } else { _assert_(thread->type == TaskType::IO_BLOCKING); - snprintf(threadName, sizeof(threadName), "PoolWorkerIO %d", thread->index); + snprintf(thread->name, sizeof(thread->name), "PoolWorkerIO %d", thread->index); } - SetCurrentThreadName(threadName); + SetCurrentThreadName(thread->name); const bool isCompute = thread->type == TaskType::CPU_COMPUTE; const auto global_queue_size = [isCompute, &global]() -> int { @@ -185,6 +185,9 @@ static void WorkerThreadFunc(GlobalThreadContext *global, ThreadContext *thread) thread->queue_size--; } } + + // In case it got attached to JNI, detach it. Don't think this has any side effects if called redundantly. + DetachThreadFromJNI(); } void ThreadManager::Init(int numRealCores, int numLogicalCoresPerCpu) { diff --git a/Common/Thread/ThreadUtil.cpp b/Common/Thread/ThreadUtil.cpp index 73eab75e5ca3..43a44e82df5f 100644 --- a/Common/Thread/ThreadUtil.cpp +++ b/Common/Thread/ThreadUtil.cpp @@ -12,10 +12,15 @@ #elif defined(__ANDROID__) +#include "android/jni/app-android.h" + #define TLS_SUPPORTED #endif +// TODO: Many other platforms also support TLS, in fact probably nearly all that we support +// these days. + #include #include @@ -62,6 +67,13 @@ static EXCEPTION_DISPOSITION NTAPI ignore_handler(EXCEPTION_RECORD *rec, } #endif +void DetachThreadFromJNI() { +#if PPSSPP_PLATFORM(ANDROID) + Android_DetachThreadFromJNI(); +#else + // Do nothing +#endif +} #if PPSSPP_PLATFORM(WINDOWS) && !PPSSPP_PLATFORM(UWP) typedef HRESULT (WINAPI *TSetThreadDescription)(HANDLE, PCWSTR); @@ -90,7 +102,15 @@ static void InitializeSetThreadDescription() { void SetCurrentThreadNameThroughException(const char *threadName); #endif -void SetCurrentThreadName(const char* threadName) { +const char *GetCurrentThreadName() { +#ifdef TLS_SUPPORTED + return curThreadName; +#else + return ""; +#endif +} + +void SetCurrentThreadName(const char *threadName) { #if PPSSPP_PLATFORM(WINDOWS) && !PPSSPP_PLATFORM(UWP) InitializeSetThreadDescription(); if (g_pSetThreadDescription) { diff --git a/Common/Thread/ThreadUtil.h b/Common/Thread/ThreadUtil.h index 982de2a41c1e..a14c112f609d 100644 --- a/Common/Thread/ThreadUtil.h +++ b/Common/Thread/ThreadUtil.h @@ -2,11 +2,26 @@ #include -// Note that name must be a global string that lives until the end of the process, +// Note that the string pointed to must be have a lifetime until the end of the thread, // for AssertCurrentThreadName to work. void SetCurrentThreadName(const char *threadName); void AssertCurrentThreadName(const char *threadName); +// If TLS is not supported, this will return an empty string. +const char *GetCurrentThreadName(); + // Just gets a cheap thread identifier so that you can see different threads in debug output, // exactly what it is is badly specified and not useful for anything. int GetCurrentThreadIdForDebug(); + +// Call when leaving threads. On Android, calls DetachCurrentThread. +// Threads that use scoped storage I/O end up attached as JNI threads, and will thus +// need this in order to follow the rules correctly. Some devices seem to enforce this. +void DetachThreadFromJNI(); + +class AndroidJNIThreadContext { +public: + ~AndroidJNIThreadContext() { + DetachThreadFromJNI(); + } +}; diff --git a/Core/Config.cpp b/Core/Config.cpp index 4971f9a8d628..11ffc2fcd705 100644 --- a/Core/Config.cpp +++ b/Core/Config.cpp @@ -32,6 +32,7 @@ #include "Common/Log.h" #include "Common/TimeUtil.h" +#include "Common/Thread/ThreadUtil.h" #include "Common/Data/Format/IniFile.h" #include "Common/Data/Format/JSONReader.h" #include "Common/Data/Text/I18n.h" @@ -43,6 +44,7 @@ #include "Common/System/Display.h" #include "Common/System/System.h" #include "Common/StringUtils.h" +#include "Common/Thread/ThreadUtil.h" #include "Common/GPU/Vulkan/VulkanLoader.h" #include "Common/VR/PPSSPPVR.h" #include "Core/Config.h" @@ -1733,6 +1735,7 @@ void Config::RemoveRecent(const std::string &file) { void Config::CleanRecent() { private_->SetRecentIsosThread([this] { + SetCurrentThreadName("RecentISOs"); double startTime = time_now_d(); std::lock_guard guard(private_->recentIsosLock); @@ -1763,6 +1766,8 @@ void Config::CleanRecent() { INFO_LOG(SYSTEM, "CleanRecent took %0.2f", time_now_d() - startTime); recentIsos = cleanedRecent; + + DetachThreadFromJNI(); }); } diff --git a/Core/Util/GameManager.cpp b/Core/Util/GameManager.cpp index a4d192d9ed57..4c6ae7558d94 100644 --- a/Core/Util/GameManager.cpp +++ b/Core/Util/GameManager.cpp @@ -38,6 +38,7 @@ #include "Common/Log.h" #include "Common/File/FileUtil.h" #include "Common/StringUtils.h" +#include "Common/Thread/ThreadUtil.h" #include "Core/Config.h" #include "Core/Loaders.h" #include "Core/ELF/ParamSFO.h" @@ -278,11 +279,15 @@ ZipFileContents DetectZipFileContents(struct zip *z, ZipFileInfo *info) { // Parameters need to be by value, since this is a thread func. bool GameManager::InstallGame(Path url, Path fileName, bool deleteAfter) { + SetCurrentThreadName("InstallGame"); + if (installInProgress_ || installDonePending_) { ERROR_LOG(HLE, "Cannot have two installs in progress at the same time"); return false; } + AndroidJNIThreadContext context; // Destructor detaches. + if (!File::Exists(fileName)) { ERROR_LOG(HLE, "Game file '%s' doesn't exist", fileName.c_str()); return false; @@ -445,6 +450,7 @@ std::string GameManager::GetISOGameID(FileLoader *loader) const { if (!bd) { return ""; } + ISOFileSystem umd(&handles, bd); PSPFileInfo info = umd.GetFileInfo("/PSP_GAME/PARAM.SFO"); diff --git a/android/jni/app-android.cpp b/android/jni/app-android.cpp index 18ef723d445e..9164cdac6aa1 100644 --- a/android/jni/app-android.cpp +++ b/android/jni/app-android.cpp @@ -237,7 +237,12 @@ JNIEnv* getEnv() { JNIEnv *env; int status = gJvm->GetEnv((void**)&env, JNI_VERSION_1_6); if (status < 0) { - status = gJvm->AttachCurrentThread(&env, NULL); + // TODO: We should have a version of getEnv that doesn't allow auto-attach. + INFO_LOG(SYSTEM, "Thread '%s' not attached to JVM, attaching.", GetCurrentThreadName()); + JavaVMAttachArgs args{}; + args.version = JNI_VERSION_1_6; + args.name = GetCurrentThreadName(); + status = gJvm->AttachCurrentThread(&env, &args); if (status < 0) { return nullptr; } @@ -265,6 +270,11 @@ JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *pjvm, void *reserved) { return JNI_VERSION_1_6; } +void Android_DetachThreadFromJNI() { + INFO_LOG(SYSTEM, "Detaching thread from JNI: '%s'", GetCurrentThreadName()); + gJvm->DetachCurrentThread(); +} + // Only used in OpenGL mode. static void EmuThreadFunc() { JNIEnv *env; diff --git a/android/jni/app-android.h b/android/jni/app-android.h index c3d7e6b9a639..8c0deff28759 100644 --- a/android/jni/app-android.h +++ b/android/jni/app-android.h @@ -27,4 +27,7 @@ class AndroidLogger : public LogListener { }; #endif +void Android_DetachThreadFromJNI(); + #endif + From b56eef487c5558d4baf258c2b1c145c5e9ac1538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 3 Jan 2023 23:29:22 +0100 Subject: [PATCH 2/7] Strict mode checking - no way to forget detaching now. And if we forget to attach, boom. Hopefully I caught all of them. --- Common/File/PathBrowser.cpp | 4 +-- Common/Net/HTTPClient.cpp | 3 ++ Common/Thread/ThreadManager.cpp | 8 ++++- Common/Thread/ThreadUtil.cpp | 9 ++++++ Common/Thread/ThreadUtil.h | 7 +++++ Core/Config.cpp | 5 +-- Core/FileLoaders/CachingFileLoader.cpp | 2 ++ Core/FileLoaders/RamCachingFileLoader.cpp | 2 ++ Core/HLE/sceIo.cpp | 1 + Core/HW/MemoryStick.cpp | 6 ++++ Core/PSPLoaders.cpp | 8 ++++- Core/Reporting.cpp | 2 ++ Core/SaveState.cpp | 3 ++ Core/WebServer.cpp | 2 ++ UI/RemoteISOScreen.cpp | 2 ++ android/jni/app-android.cpp | 38 +++++++++++++++-------- android/jni/app-android.h | 1 + 17 files changed, 84 insertions(+), 19 deletions(-) diff --git a/Common/File/PathBrowser.cpp b/Common/File/PathBrowser.cpp index ae7d804ed1fc..f5ab1a33be5e 100644 --- a/Common/File/PathBrowser.cpp +++ b/Common/File/PathBrowser.cpp @@ -141,6 +141,8 @@ void PathBrowser::HandlePath() { pendingThread_ = std::thread([&] { SetCurrentThreadName("PathBrowser"); + AndroidJNIThreadContext jniContext; // destructor detaches + std::unique_lock guard(pendingLock_); std::vector results; Path lastPath("NONSENSE THAT WONT EQUAL A PATH"); @@ -177,8 +179,6 @@ void PathBrowser::HandlePath() { ready_ = true; } } - - DetachThreadFromJNI(); }); } diff --git a/Common/Net/HTTPClient.cpp b/Common/Net/HTTPClient.cpp index bac18da12de1..7ef0c15df1b0 100644 --- a/Common/Net/HTTPClient.cpp +++ b/Common/Net/HTTPClient.cpp @@ -526,6 +526,9 @@ std::string Download::RedirectLocation(const std::string &baseUrl) { void Download::Do() { SetCurrentThreadName("Downloader::Do"); + + AndroidJNIThreadContext jniContext; + resultCode_ = 0; std::string downloadURL = url_; diff --git a/Common/Thread/ThreadManager.cpp b/Common/Thread/ThreadManager.cpp index 921831fa4076..8aedb493e8b9 100644 --- a/Common/Thread/ThreadManager.cpp +++ b/Common/Thread/ThreadManager.cpp @@ -133,6 +133,10 @@ static void WorkerThreadFunc(GlobalThreadContext *global, ThreadContext *thread) } SetCurrentThreadName(thread->name); + if (thread->type == TaskType::IO_BLOCKING) { + AttachThreadToJNI(); + } + const bool isCompute = thread->type == TaskType::CPU_COMPUTE; const auto global_queue_size = [isCompute, &global]() -> int { return isCompute ? global->compute_queue_size.load() : global->io_queue_size.load(); @@ -187,7 +191,9 @@ static void WorkerThreadFunc(GlobalThreadContext *global, ThreadContext *thread) } // In case it got attached to JNI, detach it. Don't think this has any side effects if called redundantly. - DetachThreadFromJNI(); + if (thread->type == TaskType::IO_BLOCKING) { + DetachThreadFromJNI(); + } } void ThreadManager::Init(int numRealCores, int numLogicalCoresPerCpu) { diff --git a/Common/Thread/ThreadUtil.cpp b/Common/Thread/ThreadUtil.cpp index 43a44e82df5f..0e3716d6b6bc 100644 --- a/Common/Thread/ThreadUtil.cpp +++ b/Common/Thread/ThreadUtil.cpp @@ -67,6 +67,15 @@ static EXCEPTION_DISPOSITION NTAPI ignore_handler(EXCEPTION_RECORD *rec, } #endif +void AttachThreadToJNI() { +#if PPSSPP_PLATFORM(ANDROID) + Android_AttachThreadToJNI(); +#else + // Do nothing +#endif +} + + void DetachThreadFromJNI() { #if PPSSPP_PLATFORM(ANDROID) Android_DetachThreadFromJNI(); diff --git a/Common/Thread/ThreadUtil.h b/Common/Thread/ThreadUtil.h index a14c112f609d..ee4b46140136 100644 --- a/Common/Thread/ThreadUtil.h +++ b/Common/Thread/ThreadUtil.h @@ -14,13 +14,20 @@ const char *GetCurrentThreadName(); // exactly what it is is badly specified and not useful for anything. int GetCurrentThreadIdForDebug(); +// When you know that a thread potentially will make JNI calls, call this after setting its name. +void AttachThreadToJNI(); + // Call when leaving threads. On Android, calls DetachCurrentThread. // Threads that use scoped storage I/O end up attached as JNI threads, and will thus // need this in order to follow the rules correctly. Some devices seem to enforce this. void DetachThreadFromJNI(); +// Utility to call the above two functions. class AndroidJNIThreadContext { public: + AndroidJNIThreadContext() { + AttachThreadToJNI(); + } ~AndroidJNIThreadContext() { DetachThreadFromJNI(); } diff --git a/Core/Config.cpp b/Core/Config.cpp index 11ffc2fcd705..1a3c1ef40a28 100644 --- a/Core/Config.cpp +++ b/Core/Config.cpp @@ -1736,6 +1736,9 @@ void Config::RemoveRecent(const std::string &file) { void Config::CleanRecent() { private_->SetRecentIsosThread([this] { SetCurrentThreadName("RecentISOs"); + + AndroidJNIThreadContext jniContext; // destructor detaches + double startTime = time_now_d(); std::lock_guard guard(private_->recentIsosLock); @@ -1766,8 +1769,6 @@ void Config::CleanRecent() { INFO_LOG(SYSTEM, "CleanRecent took %0.2f", time_now_d() - startTime); recentIsos = cleanedRecent; - - DetachThreadFromJNI(); }); } diff --git a/Core/FileLoaders/CachingFileLoader.cpp b/Core/FileLoaders/CachingFileLoader.cpp index 05fa5eefba8d..da350c1d0941 100644 --- a/Core/FileLoaders/CachingFileLoader.cpp +++ b/Core/FileLoaders/CachingFileLoader.cpp @@ -269,6 +269,8 @@ void CachingFileLoader::StartReadAhead(s64 pos) { aheadThread_ = std::thread([this, pos] { SetCurrentThreadName("FileLoaderReadAhead"); + AndroidJNIThreadContext jniContext; + std::unique_lock guard(blocksMutex_); s64 cacheStartPos = pos >> BLOCK_SHIFT; s64 cacheEndPos = cacheStartPos + BLOCK_READAHEAD - 1; diff --git a/Core/FileLoaders/RamCachingFileLoader.cpp b/Core/FileLoaders/RamCachingFileLoader.cpp index 3e85777eec8a..c326bffa0b71 100644 --- a/Core/FileLoaders/RamCachingFileLoader.cpp +++ b/Core/FileLoaders/RamCachingFileLoader.cpp @@ -227,6 +227,8 @@ void RamCachingFileLoader::StartReadAhead(s64 pos) { aheadThread_ = std::thread([this] { SetCurrentThreadName("FileLoaderReadAhead"); + AndroidJNIThreadContext jniContext; + while (aheadRemaining_ != 0 && !aheadCancel_) { // Where should we look? const u32 cacheStartPos = NextAheadBlock(); diff --git a/Core/HLE/sceIo.cpp b/Core/HLE/sceIo.cpp index 06fbdbf5d324..91ea6732d028 100644 --- a/Core/HLE/sceIo.cpp +++ b/Core/HLE/sceIo.cpp @@ -579,6 +579,7 @@ static void __IoAsyncEndCallback(SceUID threadID, SceUID prevCallbackId) { static void __IoManagerThread() { SetCurrentThreadName("IO"); + AndroidJNIThreadContext jniContext; while (ioManagerThreadEnabled && coreState != CORE_BOOT_ERROR && coreState != CORE_RUNTIME_ERROR && coreState != CORE_POWERDOWN) { ioManager.RunEventsUntil(CoreTiming::GetTicks() + msToCycles(1000)); } diff --git a/Core/HW/MemoryStick.cpp b/Core/HW/MemoryStick.cpp index 4f20352bd38e..8511a9d76c0a 100644 --- a/Core/HW/MemoryStick.cpp +++ b/Core/HW/MemoryStick.cpp @@ -19,8 +19,10 @@ #include #include #include + #include "Common/Serialize/Serializer.h" #include "Common/Serialize/SerializeFuncs.h" +#include "Common/Thread/ThreadUtil.h" #include "Core/Config.h" #include "Core/CoreTiming.h" #include "Core/Compatibility.h" @@ -97,6 +99,10 @@ static void MemoryStick_CalcInitialFree() { std::unique_lock guard(freeCalcMutex); freeCalcStatus = FreeCalcStatus::RUNNING; freeCalcThread = std::thread([] { + SetCurrentThreadName("CalcInitialFree"); + + AndroidJNIThreadContext jniContext; + memstickInitialFree = pspFileSystem.FreeSpace("ms0:/") + pspFileSystem.ComputeRecursiveDirectorySize("ms0:/PSP/SAVEDATA/"); std::unique_lock guard(freeCalcMutex); diff --git a/Core/PSPLoaders.cpp b/Core/PSPLoaders.cpp index 0b14eeb8438e..ab060eb27bd4 100644 --- a/Core/PSPLoaders.cpp +++ b/Core/PSPLoaders.cpp @@ -307,7 +307,7 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string) { //in case we didn't go through EmuScreen::boot g_Config.loadGameConfig(id, g_paramSFO.GetValueString("TITLE")); host->SendUIMessage("config_loaded", ""); - INFO_LOG(LOADER,"Loading %s...", bootpath.c_str()); + INFO_LOG(LOADER, "Loading %s...", bootpath.c_str()); PSPLoaders_Shutdown(); // Note: this thread reads the game binary, loads caches, and links HLE while UI spins. @@ -319,6 +319,8 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string) { if (coreState != CORE_POWERUP) return; + AndroidJNIThreadContext jniContext; + PSP_SetLoading("Loading executable..."); // TODO: We can't use the initial error_string pointer. bool success = __KernelLoadExec(bootpath.c_str(), 0, &PSP_CoreParameter().errorString); @@ -455,6 +457,8 @@ bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string) { if (coreState != CORE_POWERUP) return; + AndroidJNIThreadContext jniContext; + bool success = __KernelLoadExec(finalName.c_str(), 0, &PSP_CoreParameter().errorString); if (success && coreState == CORE_POWERUP) { coreState = PSP_CoreParameter().startBreak ? CORE_STEPPING : CORE_RUNNING; @@ -479,6 +483,8 @@ bool Load_PSP_GE_Dump(FileLoader *fileLoader, std::string *error_string) { if (coreState != CORE_POWERUP) return; + AndroidJNIThreadContext jniContext; + bool success = __KernelLoadGEDump("disc0:/data.ppdmp", &PSP_CoreParameter().errorString); if (success && coreState == CORE_POWERUP) { coreState = PSP_CoreParameter().startBreak ? CORE_STEPPING : CORE_RUNNING; diff --git a/Core/Reporting.cpp b/Core/Reporting.cpp index 7263f6f00998..7a552a5341b6 100644 --- a/Core/Reporting.cpp +++ b/Core/Reporting.cpp @@ -111,6 +111,8 @@ namespace Reporting static int CalculateCRCThread() { SetCurrentThreadName("ReportCRC"); + AndroidJNIThreadContext jniContext; + FileLoader *fileLoader = ResolveFileLoaderTarget(ConstructFileLoader(crcFilename)); BlockDevice *blockDevice = constructBlockDevice(fileLoader); diff --git a/Core/SaveState.cpp b/Core/SaveState.cpp index 148e4307d1aa..d9282037a693 100644 --- a/Core/SaveState.cpp +++ b/Core/SaveState.cpp @@ -164,6 +164,9 @@ namespace SaveState compressThread_.join(); compressThread_ = std::thread([=]{ SetCurrentThreadName("SaveStateCompress"); + + AndroidJNIThreadContext jniContext; + Compress(*result, *state, *base); }); } diff --git a/Core/WebServer.cpp b/Core/WebServer.cpp index 4aff212ebaaf..701d4f070ca2 100644 --- a/Core/WebServer.cpp +++ b/Core/WebServer.cpp @@ -314,6 +314,8 @@ static void ForwardDebuggerRequest(const http::Request &request) { static void ExecuteWebServer() { SetCurrentThreadName("HTTPServer"); + AndroidJNIThreadContext context; // Destructor detaches. + auto http = new http::Server(new NewThreadExecutor()); http->RegisterHandler("/", &HandleListing); // This lists all the (current) recent ISOs. diff --git a/UI/RemoteISOScreen.cpp b/UI/RemoteISOScreen.cpp index 33a212f34f16..98cea5ea8556 100644 --- a/UI/RemoteISOScreen.cpp +++ b/UI/RemoteISOScreen.cpp @@ -30,6 +30,7 @@ #include "Common/Net/HTTPClient.h" #include "Common/Net/Resolve.h" #include "Common/Net/URL.h" +#include "Common/Thread/ThreadUtil.h" #include "Common/File/PathBrowser.h" #include "Common/Data/Format/JSONReader.h" @@ -366,6 +367,7 @@ RemoteISOConnectScreen::RemoteISOConnectScreen() { scanAborted = false; scanThread_ = new std::thread([](RemoteISOConnectScreen *thiz) { + SetCurrentThreadName("RemoteISOScan"); thiz->ExecuteScan(); }, this); } diff --git a/android/jni/app-android.cpp b/android/jni/app-android.cpp index 9164cdac6aa1..276e4c3f4ab8 100644 --- a/android/jni/app-android.cpp +++ b/android/jni/app-android.cpp @@ -236,17 +236,7 @@ void AndroidLogger::Log(const LogMessage &message) { JNIEnv* getEnv() { JNIEnv *env; int status = gJvm->GetEnv((void**)&env, JNI_VERSION_1_6); - if (status < 0) { - // TODO: We should have a version of getEnv that doesn't allow auto-attach. - INFO_LOG(SYSTEM, "Thread '%s' not attached to JVM, attaching.", GetCurrentThreadName()); - JavaVMAttachArgs args{}; - args.version = JNI_VERSION_1_6; - args.name = GetCurrentThreadName(); - status = gJvm->AttachCurrentThread(&env, &args); - if (status < 0) { - return nullptr; - } - } + _assert_msg_(status >= 0, "'%s': Can only call getEnv if you've attached the thread already!", GetCurrentThreadName()); return env; } @@ -270,9 +260,31 @@ JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *pjvm, void *reserved) { return JNI_VERSION_1_6; } +void Android_AttachThreadToJNI() { + JNIEnv *env; + int status = gJvm->GetEnv((void **)&env, JNI_VERSION_1_6); + if (status < 0) { + // TODO: We should have a version of getEnv that doesn't allow auto-attach. + INFO_LOG(SYSTEM, "Attaching thread '%s' (not already attached) to JNI.", GetCurrentThreadName()); + JavaVMAttachArgs args{}; + args.version = JNI_VERSION_1_6; + args.name = GetCurrentThreadName(); + status = gJvm->AttachCurrentThread(&env, &args); + + if (status < 0) { + // bad, but wh + } + } else { + WARN_LOG(SYSTEM, "Thread %s was already attached to JNI.", GetCurrentThreadName()); + } +} + void Android_DetachThreadFromJNI() { - INFO_LOG(SYSTEM, "Detaching thread from JNI: '%s'", GetCurrentThreadName()); - gJvm->DetachCurrentThread(); + if (gJvm->DetachCurrentThread() == JNI_OK) { + INFO_LOG(SYSTEM, "Detached thread from JNI: '%s'", GetCurrentThreadName()); + } else { + WARN_LOG(SYSTEM, "Failed to detach thread '%s' from JNI - never attached?", GetCurrentThreadName()); + } } // Only used in OpenGL mode. diff --git a/android/jni/app-android.h b/android/jni/app-android.h index 8c0deff28759..0dc6c03123a7 100644 --- a/android/jni/app-android.h +++ b/android/jni/app-android.h @@ -27,6 +27,7 @@ class AndroidLogger : public LogListener { }; #endif +void Android_AttachThreadToJNI(); void Android_DetachThreadFromJNI(); #endif From d352340f776f75c23d13c7373ec3d1db7c6144e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 4 Jan 2023 00:02:53 +0100 Subject: [PATCH 3/7] buildfix attempt --- android/jni/Android.mk | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/android/jni/Android.mk b/android/jni/Android.mk index 2ff923f755a2..4ab0d42b3063 100644 --- a/android/jni/Android.mk +++ b/android/jni/Android.mk @@ -32,6 +32,13 @@ ARCH_FILES := \ $(SRC)/ext/libpng17/arm/filter_neon_intrinsics.c endif +ANDROID_FILES :=\ + $(SRC)/android/jni/app-android.cpp \ + $(SRC)/android/jni/AndroidJavaGLContext.cpp \ + $(SRC)/android/jni/AndroidVulkanContext.cpp \ + $(SRC)/android/jni/AndroidAudio.cpp \ + $(SRC)/android/jni/OpenSLContext.cpp \ + NATIVE_FILES :=\ $(SRC)/Common/GPU/OpenGL/gl3stub.c \ $(SRC)/Common/GPU/OpenGL/thin3d_gl.cpp \ @@ -130,6 +137,7 @@ EXEC_AND_LIB_FILES := \ $(SPIRV_CROSS_FILES) \ $(EXT_FILES) \ $(NATIVE_FILES) \ + $(ANDROID_FILES) \ $(SRC)/Common/Buffer.cpp \ $(SRC)/Common/Crypto/md5.cpp \ $(SRC)/Common/Crypto/sha1.cpp \ @@ -680,11 +688,6 @@ LOCAL_STATIC_LIBRARIES += ppsspp_common ppsspp_core libarmips libzstd # These are the files just for ppsspp_jni LOCAL_MODULE := ppsspp_jni LOCAL_SRC_FILES := \ - $(SRC)/android/jni/app-android.cpp \ - $(SRC)/android/jni/AndroidJavaGLContext.cpp \ - $(SRC)/android/jni/AndroidVulkanContext.cpp \ - $(SRC)/android/jni/AndroidAudio.cpp \ - $(SRC)/android/jni/OpenSLContext.cpp \ $(SRC)/UI/BackgroundAudio.cpp \ $(SRC)/UI/DiscordIntegration.cpp \ $(SRC)/UI/ChatScreen.cpp \ From 7e374c932419e06bbc02418218ea2a3330e359e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 4 Jan 2023 13:48:40 +0100 Subject: [PATCH 4/7] Address feedback, more fixes --- Common/GPU/Vulkan/VulkanRenderManager.cpp | 6 ++++-- Common/Thread/ThreadUtil.h | 2 +- Core/Dialog/PSPSaveDialog.cpp | 2 ++ Core/SaveState.cpp | 3 +-- Core/WebServer.cpp | 6 ++++++ android/jni/app-android.cpp | 8 +++++--- 6 files changed, 19 insertions(+), 8 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index f26139ddd5ea..974e6899e60f 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -115,15 +115,17 @@ bool VKRGraphicsPipeline::Create(VulkanContext *vulkan, VkRenderPass compatibleR pipe.basePipelineIndex = 0; pipe.subpass = 0; + INFO_LOG(G3D, "Creating pipeline... rpType: %08x sampleBits: %d (%s)", (u32)rpType, (u32)sampleCount, tag_.c_str()); + double start = time_now_d(); VkPipeline vkpipeline; VkResult result = vkCreateGraphicsPipelines(vulkan->GetDevice(), desc->pipelineCache, 1, &pipe, nullptr, &vkpipeline); double taken_ms = (time_now_d() - start) * 1000.0; if (taken_ms < 0.1) { - DEBUG_LOG(G3D, "Pipeline creation time: %0.2f ms (fast) rpType: %08x sampleBits: %d\n(%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str()); + DEBUG_LOG(G3D, "Pipeline creation time: %0.2f ms (fast) rpType: %08x sampleBits: %d (%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str()); } else { - INFO_LOG(G3D, "Pipeline creation time: %0.2f ms rpType: %08x sampleBits: %d\n(%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str()); + INFO_LOG(G3D, "Pipeline creation time: %0.2f ms rpType: %08x sampleBits: %d (%s)", taken_ms, (u32)rpType, (u32)sampleCount, tag_.c_str()); } bool success = true; diff --git a/Common/Thread/ThreadUtil.h b/Common/Thread/ThreadUtil.h index ee4b46140136..d6c6e60c84e8 100644 --- a/Common/Thread/ThreadUtil.h +++ b/Common/Thread/ThreadUtil.h @@ -2,7 +2,7 @@ #include -// Note that the string pointed to must be have a lifetime until the end of the thread, +// Note that the string pointed to must have a lifetime until the end of the thread, // for AssertCurrentThreadName to work. void SetCurrentThreadName(const char *threadName); void AssertCurrentThreadName(const char *threadName); diff --git a/Core/Dialog/PSPSaveDialog.cpp b/Core/Dialog/PSPSaveDialog.cpp index 8e0bfcb1ba32..a1ef203606cc 100755 --- a/Core/Dialog/PSPSaveDialog.cpp +++ b/Core/Dialog/PSPSaveDialog.cpp @@ -1210,6 +1210,8 @@ void PSPSaveDialog::JoinIOThread() { static void DoExecuteIOAction(PSPSaveDialog *dialog) { SetCurrentThreadName("SaveIO"); + + AndroidJNIThreadContext jniContext; dialog->ExecuteIOAction(); } diff --git a/Core/SaveState.cpp b/Core/SaveState.cpp index d9282037a693..c28cb190a190 100644 --- a/Core/SaveState.cpp +++ b/Core/SaveState.cpp @@ -165,8 +165,7 @@ namespace SaveState compressThread_ = std::thread([=]{ SetCurrentThreadName("SaveStateCompress"); - AndroidJNIThreadContext jniContext; - + // Should do no I/O, so no JNI thread context needed. Compress(*result, *state, *base); }); } diff --git a/Core/WebServer.cpp b/Core/WebServer.cpp index 701d4f070ca2..3de0fabc7bfb 100644 --- a/Core/WebServer.cpp +++ b/Core/WebServer.cpp @@ -212,6 +212,8 @@ static void DiscHandler(const http::Request &request, const Path &filename) { } static void HandleListing(const http::Request &request) { + AndroidJNIThreadContext jniContext; + request.WriteHttpResponseHeader("1.0", 200, -1, "text/plain"); request.Out()->Printf("/\n"); if (serverFlags & (int)WebServerFlags::DISCS) { @@ -269,6 +271,8 @@ static void RedirectToDebugger(const http::Request &request) { } static void HandleFallback(const http::Request &request) { + AndroidJNIThreadContext jniContext; + if (serverFlags & (int)WebServerFlags::DISCS) { Path filename = LocalFromRemotePath(request.resource()); if (!filename.empty()) { @@ -293,6 +297,8 @@ static void HandleFallback(const http::Request &request) { } static void ForwardDebuggerRequest(const http::Request &request) { + AndroidJNIThreadContext jniContext; + if (serverFlags & (int)WebServerFlags::DEBUGGER) { // Check if this is a websocket request... std::string upgrade; diff --git a/android/jni/app-android.cpp b/android/jni/app-android.cpp index 276e4c3f4ab8..15290ddf434f 100644 --- a/android/jni/app-android.cpp +++ b/android/jni/app-android.cpp @@ -50,6 +50,9 @@ struct JNIEnv {}; #define JNI_VERSION_1_6 16 #endif +#include "Common/Log.h" +#include "Common/LogReporting.h" + #include "Common/Net/Resolve.h" #include "android/jni/AndroidAudio.h" #include "Common/GPU/OpenGL/GLCommon.h" @@ -71,7 +74,6 @@ struct JNIEnv {}; #include "Common/Data/Text/Parsers.h" #include "Common/VR/PPSSPPVR.h" -#include "Common/Log.h" #include "Common/GraphicsContext.h" #include "Common/StringUtils.h" #include "Common/TimeUtil.h" @@ -264,7 +266,6 @@ void Android_AttachThreadToJNI() { JNIEnv *env; int status = gJvm->GetEnv((void **)&env, JNI_VERSION_1_6); if (status < 0) { - // TODO: We should have a version of getEnv that doesn't allow auto-attach. INFO_LOG(SYSTEM, "Attaching thread '%s' (not already attached) to JNI.", GetCurrentThreadName()); JavaVMAttachArgs args{}; args.version = JNI_VERSION_1_6; @@ -272,7 +273,8 @@ void Android_AttachThreadToJNI() { status = gJvm->AttachCurrentThread(&env, &args); if (status < 0) { - // bad, but wh + // bad, but what can we do other than report.. + ERROR_LOG_REPORT_ONCE(threadAttachFail, SYSTEM, "Failed to attach thread %s to JNI.", GetCurrentThreadName()); } } else { WARN_LOG(SYSTEM, "Thread %s was already attached to JNI.", GetCurrentThreadName()); From a3e5e475cd22c0184d01586d7e30a195782d0495 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 4 Jan 2023 15:57:35 +0100 Subject: [PATCH 5/7] Revert "buildfix attempt" This reverts commit 97328620f16efdad7733994a0c825963675186c0. --- android/jni/Android.mk | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/android/jni/Android.mk b/android/jni/Android.mk index 4ab0d42b3063..2ff923f755a2 100644 --- a/android/jni/Android.mk +++ b/android/jni/Android.mk @@ -32,13 +32,6 @@ ARCH_FILES := \ $(SRC)/ext/libpng17/arm/filter_neon_intrinsics.c endif -ANDROID_FILES :=\ - $(SRC)/android/jni/app-android.cpp \ - $(SRC)/android/jni/AndroidJavaGLContext.cpp \ - $(SRC)/android/jni/AndroidVulkanContext.cpp \ - $(SRC)/android/jni/AndroidAudio.cpp \ - $(SRC)/android/jni/OpenSLContext.cpp \ - NATIVE_FILES :=\ $(SRC)/Common/GPU/OpenGL/gl3stub.c \ $(SRC)/Common/GPU/OpenGL/thin3d_gl.cpp \ @@ -137,7 +130,6 @@ EXEC_AND_LIB_FILES := \ $(SPIRV_CROSS_FILES) \ $(EXT_FILES) \ $(NATIVE_FILES) \ - $(ANDROID_FILES) \ $(SRC)/Common/Buffer.cpp \ $(SRC)/Common/Crypto/md5.cpp \ $(SRC)/Common/Crypto/sha1.cpp \ @@ -688,6 +680,11 @@ LOCAL_STATIC_LIBRARIES += ppsspp_common ppsspp_core libarmips libzstd # These are the files just for ppsspp_jni LOCAL_MODULE := ppsspp_jni LOCAL_SRC_FILES := \ + $(SRC)/android/jni/app-android.cpp \ + $(SRC)/android/jni/AndroidJavaGLContext.cpp \ + $(SRC)/android/jni/AndroidVulkanContext.cpp \ + $(SRC)/android/jni/AndroidAudio.cpp \ + $(SRC)/android/jni/OpenSLContext.cpp \ $(SRC)/UI/BackgroundAudio.cpp \ $(SRC)/UI/DiscordIntegration.cpp \ $(SRC)/UI/ChatScreen.cpp \ From ab6fafb6ebe1fcc6e99e18f96cb4870c1a1937c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 4 Jan 2023 16:07:39 +0100 Subject: [PATCH 6/7] Invert dependency to avoid compile issues --- Common/Thread/ThreadUtil.cpp | 38 ++++++++++++++++++++---------------- Common/Thread/ThreadUtil.h | 4 ++++ android/README.TXT | 14 +++++++++++++ android/jni/app-android.cpp | 34 +++++++++++++++++--------------- android/jni/app-android.h | 3 --- 5 files changed, 57 insertions(+), 36 deletions(-) diff --git a/Common/Thread/ThreadUtil.cpp b/Common/Thread/ThreadUtil.cpp index 0e3716d6b6bc..338dfa1a86cf 100644 --- a/Common/Thread/ThreadUtil.cpp +++ b/Common/Thread/ThreadUtil.cpp @@ -28,6 +28,27 @@ #include "Common/Thread/ThreadUtil.h" #include "Common/Data/Encoding/Utf8.h" +AttachDetachFunc g_attach; +AttachDetachFunc g_detach; + +void AttachThreadToJNI() { + if (g_attach) { + g_attach(); + } +} + + +void DetachThreadFromJNI() { + if (g_detach) { + g_detach(); + } +} + +void RegisterAttachDetach(AttachDetachFunc attach, AttachDetachFunc detach) { + g_attach = attach; + g_detach = detach; +} + #if (PPSSPP_PLATFORM(ANDROID) || PPSSPP_PLATFORM(LINUX)) && !defined(_GNU_SOURCE) #define _GNU_SOURCE #endif @@ -67,23 +88,6 @@ static EXCEPTION_DISPOSITION NTAPI ignore_handler(EXCEPTION_RECORD *rec, } #endif -void AttachThreadToJNI() { -#if PPSSPP_PLATFORM(ANDROID) - Android_AttachThreadToJNI(); -#else - // Do nothing -#endif -} - - -void DetachThreadFromJNI() { -#if PPSSPP_PLATFORM(ANDROID) - Android_DetachThreadFromJNI(); -#else - // Do nothing -#endif -} - #if PPSSPP_PLATFORM(WINDOWS) && !PPSSPP_PLATFORM(UWP) typedef HRESULT (WINAPI *TSetThreadDescription)(HANDLE, PCWSTR); diff --git a/Common/Thread/ThreadUtil.h b/Common/Thread/ThreadUtil.h index d6c6e60c84e8..6bab7d13e268 100644 --- a/Common/Thread/ThreadUtil.h +++ b/Common/Thread/ThreadUtil.h @@ -14,6 +14,10 @@ const char *GetCurrentThreadName(); // exactly what it is is badly specified and not useful for anything. int GetCurrentThreadIdForDebug(); +typedef void (*AttachDetachFunc)(); + +void RegisterAttachDetach(AttachDetachFunc attach, AttachDetachFunc detach); + // When you know that a thread potentially will make JNI calls, call this after setting its name. void AttachThreadToJNI(); diff --git a/android/README.TXT b/android/README.TXT index d8d734f3db39..082844e7aa1a 100644 --- a/android/README.TXT +++ b/android/README.TXT @@ -1,6 +1,20 @@ +================================================================ + +NOTE: These are legacy instructions for building using ndk-build. + +We mostly only use this on CI because it's faster than gradle. +There might also be some holdouts around still using eclipse. + +================================================================ + First, build the C++ static library: + > cd android > ./ab.sh +Or +> ./ab.cmd + +as appropriate. Start Eclipse, import the android directory as an existing project You need to also load the "native" project into your eclipse workspace diff --git a/android/jni/app-android.cpp b/android/jni/app-android.cpp index 15290ddf434f..855ce4d51e66 100644 --- a/android/jni/app-android.cpp +++ b/android/jni/app-android.cpp @@ -246,22 +246,6 @@ jclass findClass(const char* name) { return static_cast(getEnv()->CallObjectMethod(gClassLoader, gFindClassMethod, getEnv()->NewStringUTF(name))); } -JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *pjvm, void *reserved) { - INFO_LOG(SYSTEM, "JNI_OnLoad"); - gJvm = pjvm; // cache the JavaVM pointer - auto env = getEnv(); - //replace with one of your classes in the line below - auto randomClass = env->FindClass("org/ppsspp/ppsspp/NativeActivity"); - jclass classClass = env->GetObjectClass(randomClass); - auto classLoaderClass = env->FindClass("java/lang/ClassLoader"); - auto getClassLoaderMethod = env->GetMethodID(classClass, "getClassLoader", - "()Ljava/lang/ClassLoader;"); - gClassLoader = env->NewGlobalRef(env->CallObjectMethod(randomClass, getClassLoaderMethod)); - gFindClassMethod = env->GetMethodID(classLoaderClass, "findClass", - "(Ljava/lang/String;)Ljava/lang/Class;"); - return JNI_VERSION_1_6; -} - void Android_AttachThreadToJNI() { JNIEnv *env; int status = gJvm->GetEnv((void **)&env, JNI_VERSION_1_6); @@ -289,6 +273,24 @@ void Android_DetachThreadFromJNI() { } } +JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *pjvm, void *reserved) { + INFO_LOG(SYSTEM, "JNI_OnLoad"); + gJvm = pjvm; // cache the JavaVM pointer + auto env = getEnv(); + //replace with one of your classes in the line below + auto randomClass = env->FindClass("org/ppsspp/ppsspp/NativeActivity"); + jclass classClass = env->GetObjectClass(randomClass); + auto classLoaderClass = env->FindClass("java/lang/ClassLoader"); + auto getClassLoaderMethod = env->GetMethodID(classClass, "getClassLoader", + "()Ljava/lang/ClassLoader;"); + gClassLoader = env->NewGlobalRef(env->CallObjectMethod(randomClass, getClassLoaderMethod)); + gFindClassMethod = env->GetMethodID(classLoaderClass, "findClass", + "(Ljava/lang/String;)Ljava/lang/Class;"); + + RegisterAttachDetach(&Android_AttachThreadToJNI, &Android_DetachThreadFromJNI); + return JNI_VERSION_1_6; +} + // Only used in OpenGL mode. static void EmuThreadFunc() { JNIEnv *env; diff --git a/android/jni/app-android.h b/android/jni/app-android.h index 0dc6c03123a7..d2d7957dcbbc 100644 --- a/android/jni/app-android.h +++ b/android/jni/app-android.h @@ -27,8 +27,5 @@ class AndroidLogger : public LogListener { }; #endif -void Android_AttachThreadToJNI(); -void Android_DetachThreadFromJNI(); - #endif From 44f60ba2f0149ef6c708295be05f8047febdfbbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 5 Jan 2023 08:37:48 +0100 Subject: [PATCH 7/7] Remove overly noisy log --- Common/GPU/Vulkan/VulkanRenderManager.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index 974e6899e60f..2694febf11de 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -115,8 +115,6 @@ bool VKRGraphicsPipeline::Create(VulkanContext *vulkan, VkRenderPass compatibleR pipe.basePipelineIndex = 0; pipe.subpass = 0; - INFO_LOG(G3D, "Creating pipeline... rpType: %08x sampleBits: %d (%s)", (u32)rpType, (u32)sampleCount, tag_.c_str()); - double start = time_now_d(); VkPipeline vkpipeline; VkResult result = vkCreateGraphicsPipelines(vulkan->GetDevice(), desc->pipelineCache, 1, &pipe, nullptr, &vkpipeline);