From 53bbbd5a4fe7daf3918c2e53b13ab172dd022da7 Mon Sep 17 00:00:00 2001 From: Ryan Houdek Date: Fri, 24 Mar 2023 05:33:56 -0700 Subject: [PATCH] Review code --- External/FEXCore/Source/Common/Paths.cpp | 2 +- .../Source/Interface/Config/Config.cpp | 14 +++---- .../Source/Interface/Context/Context.h | 8 ---- .../Source/Interface/Core/LookupCache.cpp | 8 ++-- .../Source/Interface/Core/LookupCache.h | 2 +- .../Source/Utils/AllocatorOverride.cpp | 2 + External/FEXCore/Source/Utils/Telemetry.cpp | 4 +- .../FEXCore/include/FEXCore/Utils/Allocator.h | 20 +++++++++ .../include/FEXCore/fextl/memory_resource.h | 2 +- FEXHeaderUtils/FEXHeaderUtils/Filesystem.h | 4 ++ Source/Common/FEXServerClient.cpp | 2 +- Source/Tests/FEXLoader.cpp | 12 ++---- Source/Tests/LinuxSyscalls/FileManagement.cpp | 7 +--- Source/Tests/LinuxSyscalls/Syscalls.cpp | 4 +- Source/Tests/TestHarnessRunner.cpp | 8 +--- unittests/APITests/LexicallyNormal.cpp | 41 ++++++++++--------- 16 files changed, 71 insertions(+), 69 deletions(-) diff --git a/External/FEXCore/Source/Common/Paths.cpp b/External/FEXCore/Source/Common/Paths.cpp index 4f6520a27..5c648dc29 100644 --- a/External/FEXCore/Source/Common/Paths.cpp +++ b/External/FEXCore/Source/Common/Paths.cpp @@ -71,7 +71,7 @@ namespace FEXCore::Paths { EntryCache = CachePath + "/EntryCache/"; // Ensure the folder structure is created for our Data - if (!FHU::Filesystem::Exists(EntryCache.c_str()) && + if (!FHU::Filesystem::Exists(EntryCache) && !FHU::Filesystem::CreateDirectories(EntryCache)) { LogMan::Msg::DFmt("Couldn't create EntryCache directory: '{}'", EntryCache); } diff --git a/External/FEXCore/Source/Interface/Config/Config.cpp b/External/FEXCore/Source/Interface/Config/Config.cpp index c269c78a6..fc05a978d 100644 --- a/External/FEXCore/Source/Interface/Config/Config.cpp +++ b/External/FEXCore/Source/Interface/Config/Config.cpp @@ -146,7 +146,7 @@ namespace JSON { } // Ensure the folder structure is created for our configuration - if (!FHU::Filesystem::Exists(ConfigDir.c_str()) && + if (!FHU::Filesystem::Exists(ConfigDir) && !FHU::Filesystem::CreateDirectories(ConfigDir)) { // Let's go local in this case return "./"; @@ -178,7 +178,7 @@ namespace JSON { fextl::string ConfigFile = GetConfigDirectory(Global); if (!Global && - !FHU::Filesystem::Exists(ConfigFile.c_str()) && + !FHU::Filesystem::Exists(ConfigFile) && !FHU::Filesystem::CreateDirectories(ConfigFile)) { LogMan::Msg::DFmt("Couldn't create config directory: '{}'", ConfigFile); // Let's go local in this case @@ -189,7 +189,7 @@ namespace JSON { // Attempt to create the local folder if it doesn't exist if (!Global && - !FHU::Filesystem::Exists(ConfigFile.c_str()) && + !FHU::Filesystem::Exists(ConfigFile) && !FHU::Filesystem::CreateDirectories(ConfigFile)) { // Let's go local in this case return "./" + Filename + ".json"; @@ -354,7 +354,7 @@ namespace JSON { } // Only return if it exists - if (FHU::Filesystem::Exists(PathName.c_str())) { + if (FHU::Filesystem::Exists(PathName)) { return PathName; } } @@ -371,9 +371,9 @@ namespace JSON { // HostThunks: $CMAKE_INSTALL_PREFIX/lib/fex-emu/HostThunks/ // GuestThunks: $CMAKE_INSTALL_PREFIX/share/fex-emu/GuestThunks/ if (!ContainerPrefix.empty() && !PathName.empty()) { - if (!FHU::Filesystem::Exists(PathName.c_str())) { + if (!FHU::Filesystem::Exists(PathName)) { auto ContainerPath = ContainerPrefix + PathName; - if (FHU::Filesystem::Exists(ContainerPath.c_str())) { + if (FHU::Filesystem::Exists(ContainerPath)) { return ContainerPath; } } @@ -725,7 +725,7 @@ namespace JSON { EnvMap[Key] = Value; } - std::function GetVar = [](EnvMapType &EnvMap, const std::string_view id) -> std::optional { + auto GetVar = [](EnvMapType &EnvMap, const std::string_view id) -> std::optional { if (EnvMap.find(id) != EnvMap.end()) return EnvMap.at(id); diff --git a/External/FEXCore/Source/Interface/Context/Context.h b/External/FEXCore/Source/Interface/Context/Context.h index ad35e6d87..873dcaac7 100644 --- a/External/FEXCore/Source/Interface/Context/Context.h +++ b/External/FEXCore/Source/Interface/Context/Context.h @@ -72,14 +72,6 @@ namespace FEXCore::Context { class ContextImpl final : public FEXCore::Context::Context { public: - void *operator new(size_t size) { - return FEXCore::Allocator::malloc(size); - } - - void operator delete(void *ptr) { - return FEXCore::Allocator::free(ptr); - } - // Context base class implementation. bool InitializeContext() override; diff --git a/External/FEXCore/Source/Interface/Core/LookupCache.cpp b/External/FEXCore/Source/Interface/Core/LookupCache.cpp index 451e0780a..6fd47f3e5 100644 --- a/External/FEXCore/Source/Interface/Core/LookupCache.cpp +++ b/External/FEXCore/Source/Interface/Core/LookupCache.cpp @@ -15,11 +15,11 @@ $end_info$ namespace FEXCore { LookupCache::LookupCache(FEXCore::Context::ContextImpl *CTX) - : ctx {CTX} { + : BlockLinks_mbr { fextl::pmr::get_default_resource() } + , ctx {CTX} { TotalCacheSize = ctx->Config.VirtualMemSize / 4096 * 8 + CODE_SIZE + L1_SIZE; - BlockLinks_mbr = fextl::make_unique(fextl::pmr::get_default_resource()); - BlockLinks_pma = fextl::make_unique>(BlockLinks_mbr.get()); + BlockLinks_pma = fextl::make_unique>(&BlockLinks_mbr); // Setup our PMR map. BlockLinks = BlockLinks_pma->new_object(); @@ -73,8 +73,6 @@ void LookupCache::ClearCache() { // Clear L1 and L2 by clearing the full cache. madvise(reinterpret_cast(PagePointer), TotalCacheSize, MADV_DONTNEED); - // Clear the BlockLinks allocator which frees the BlockLinks map implicitly. - BlockLinks_mbr->release(); // Allocate a new pointer from the BlockLinks pma again. BlockLinks = BlockLinks_pma->new_object(); // All code is gone, clear the block list diff --git a/External/FEXCore/Source/Interface/Core/LookupCache.h b/External/FEXCore/Source/Interface/Core/LookupCache.h index 07e1ea7c4..819409e7f 100644 --- a/External/FEXCore/Source/Interface/Core/LookupCache.h +++ b/External/FEXCore/Source/Interface/Core/LookupCache.h @@ -243,7 +243,7 @@ private: // walking each block member and destructing objects. // // This makes `BlockLinks` look like a raw pointer that could memory leak, but since it is backed by the MBR, it won't. - fextl::unique_ptr BlockLinks_mbr; + std::pmr::monotonic_buffer_resource BlockLinks_mbr; using BlockLinksMapType = std::pmr::map>; fextl::unique_ptr> BlockLinks_pma; BlockLinksMapType *BlockLinks; diff --git a/External/FEXCore/Source/Utils/AllocatorOverride.cpp b/External/FEXCore/Source/Utils/AllocatorOverride.cpp index 316b0b8ff..f1f65cbaf 100644 --- a/External/FEXCore/Source/Utils/AllocatorOverride.cpp +++ b/External/FEXCore/Source/Utils/AllocatorOverride.cpp @@ -94,6 +94,8 @@ namespace FEXCore::Allocator { auto Res = fmt::format_to_n(Tmp, 512, "Allocation from 0x{:x}\n", reinterpret_cast(Return)); Tmp[Res.size] = 0; write(STDERR_FILENO, Tmp, Res.size); + + // Fault the execution to stop it in its tracks. FEX_TRAP_EXECUTION; } } diff --git a/External/FEXCore/Source/Utils/Telemetry.cpp b/External/FEXCore/Source/Utils/Telemetry.cpp index 99cc65b6b..015744052 100644 --- a/External/FEXCore/Source/Utils/Telemetry.cpp +++ b/External/FEXCore/Source/Utils/Telemetry.cpp @@ -30,7 +30,7 @@ namespace FEXCore::Telemetry { DataDirectory += "Telemetry/"; // Ensure the folder structure is created for our configuration - if (!FHU::Filesystem::Exists(DataDirectory.c_str()) && + if (!FHU::Filesystem::Exists(DataDirectory) && !FHU::Filesystem::CreateDirectories(DataDirectory)) { LogMan::Msg::IFmt("Couldn't create telemetry Folder"); } @@ -40,7 +40,7 @@ namespace FEXCore::Telemetry { auto DataDirectory = Config::GetDataDirectory(); DataDirectory += "Telemetry/" + ApplicationName + ".telem"; - if (FHU::Filesystem::Exists(DataDirectory.c_str())) { + if (FHU::Filesystem::Exists(DataDirectory)) { // If the file exists, retain a single backup auto Backup = DataDirectory + ".1"; FHU::Filesystem::CopyFile(DataDirectory, Backup, FHU::Filesystem::CopyOptions::OVERWRITE_EXISTING); diff --git a/External/FEXCore/include/FEXCore/Utils/Allocator.h b/External/FEXCore/include/FEXCore/Utils/Allocator.h index 58af7c325..5a817e9eb 100644 --- a/External/FEXCore/include/FEXCore/Utils/Allocator.h +++ b/External/FEXCore/include/FEXCore/Utils/Allocator.h @@ -27,6 +27,16 @@ namespace FEXCore::Allocator { FEX_DEFAULT_VISIBILITY ~YesIKnowImNotSupposedToUseTheGlibcAllocator(); FEX_DEFAULT_VISIBILITY static void HardDisable(); }; + + class FEX_DEFAULT_VISIBILITY GLIBCScopedFault final { + public: + GLIBCScopedFault() { + FEXCore::Allocator::SetupFaultEvaluate(); + } + ~GLIBCScopedFault() { + FEXCore::Allocator::ClearFaultEvaluate(); + } + }; #else FEX_DEFAULT_VISIBILITY inline void SetupFaultEvaluate() {} FEX_DEFAULT_VISIBILITY inline void ClearFaultEvaluate() {} @@ -37,6 +47,16 @@ namespace FEXCore::Allocator { FEX_DEFAULT_VISIBILITY ~YesIKnowImNotSupposedToUseTheGlibcAllocator() {} FEX_DEFAULT_VISIBILITY static inline void HardDisable() {} }; + + class FEX_DEFAULT_VISIBILITY GLIBCScopedFault final { + public: + GLIBCScopedFault() { + // nop + } + ~GLIBCScopedFault() { + // nop + } + }; #endif struct MemoryRegion { diff --git a/External/FEXCore/include/FEXCore/fextl/memory_resource.h b/External/FEXCore/include/FEXCore/fextl/memory_resource.h index 5857979f5..7d6009bf9 100644 --- a/External/FEXCore/include/FEXCore/fextl/memory_resource.h +++ b/External/FEXCore/include/FEXCore/fextl/memory_resource.h @@ -37,7 +37,7 @@ namespace fextl { */ class fixed_size_monotonic_buffer_resource final : public std::pmr::memory_resource { public: - fixed_size_monotonic_buffer_resource(void* Base, size_t Size) + fixed_size_monotonic_buffer_resource(void* Base, [[maybe_unused]] size_t Size) : Ptr {reinterpret_cast(Base)} #if defined(ASSERTIONS_ENABLED) && ASSERTIONS_ENABLED , PtrEnd {reinterpret_cast(Base) + Size} diff --git a/FEXHeaderUtils/FEXHeaderUtils/Filesystem.h b/FEXHeaderUtils/FEXHeaderUtils/Filesystem.h index 2832ce8d5..c76978aca 100644 --- a/FEXHeaderUtils/FEXHeaderUtils/Filesystem.h +++ b/FEXHeaderUtils/FEXHeaderUtils/Filesystem.h @@ -23,6 +23,10 @@ namespace FHU::Filesystem { return access(Path, F_OK) == 0; } + inline bool Exists(const fextl::string &Path) { + return access(Path.c_str(), F_OK) == 0; + } + /** * @brief Creates a directory at the provided path. * diff --git a/Source/Common/FEXServerClient.cpp b/Source/Common/FEXServerClient.cpp index d77ea4db0..61441e9f5 100644 --- a/Source/Common/FEXServerClient.cpp +++ b/Source/Common/FEXServerClient.cpp @@ -215,7 +215,7 @@ namespace FEXServerClient { fextl::string FEXServerPath = FHU::Filesystem::ParentPath(InterpreterPath) + "/FEXServer"; // Check if a local FEXServer next to FEXInterpreter exists // If it does then it takes priority over the installed one - if (!FHU::Filesystem::Exists(FEXServerPath.c_str())) { + if (!FHU::Filesystem::Exists(FEXServerPath)) { FEXServerPath = "FEXServer"; } diff --git a/Source/Tests/FEXLoader.cpp b/Source/Tests/FEXLoader.cpp index e8c0c18af..b4f0770aa 100644 --- a/Source/Tests/FEXLoader.cpp +++ b/Source/Tests/FEXLoader.cpp @@ -180,7 +180,7 @@ void InterpreterHandler(fextl::string *Filename, fextl::string const &RootFS, fe void RootFSRedirect(fextl::string *Filename, fextl::string const &RootFS) { auto RootFSLink = ELFCodeLoader::ResolveRootfsFile(*Filename, RootFS); - if (FHU::Filesystem::Exists(RootFSLink.c_str())) { + if (FHU::Filesystem::Exists(RootFSLink)) { *Filename = RootFSLink; } } @@ -199,7 +199,7 @@ bool IsInterpreterInstalled() { } int main(int argc, char **argv, char **const envp) { - FEXCore::Allocator::SetupFaultEvaluate(); + FEXCore::Allocator::GLIBCScopedFault GLIBFaultScope; const bool IsInterpreter = RanAsInterpreter(argv[0]); ExecutedWithFD = getauxval(AT_EXECFD) != 0; @@ -218,7 +218,6 @@ int main(int argc, char **argv, char **const envp) { if (Program.ProgramPath.empty() && !FEXFD) { // Early exit if we weren't passed an argument - FEXCore::Allocator::ClearFaultEvaluate(); return 0; } @@ -243,7 +242,6 @@ int main(int argc, char **argv, char **const envp) { // Ensure FEXServer is setup before config options try to pull CONFIG_ROOTFS if (!FEXServerClient::SetupClient(argv[0])) { LogMan::Msg::EFmt("FEXServerClient: Failure to setup client"); - FEXCore::Allocator::ClearFaultEvaluate(); return -1; } @@ -297,11 +295,10 @@ int main(int argc, char **argv, char **const envp) { RootFSRedirect(&Program.ProgramPath, LDPath()); InterpreterHandler(&Program.ProgramPath, LDPath(), &Args); - if (!ExecutedWithFD && !FEXFD && !FHU::Filesystem::Exists(Program.ProgramPath.c_str())) { + if (!ExecutedWithFD && !FEXFD && !FHU::Filesystem::Exists(Program.ProgramPath)) { // Early exit if the program passed in doesn't exist // Will prevent a crash later fmt::print(stderr, "{}: command not found\n", Program.ProgramPath); - FEXCore::Allocator::ClearFaultEvaluate(); return -ENOEXEC; } @@ -333,7 +330,6 @@ int main(int argc, char **argv, char **const envp) { fmt::print(stderr, "Use FEXRootFSFetcher to download a RootFS\n"); } #endif - FEXCore::Allocator::ClearFaultEvaluate(); return -ENOEXEC; } @@ -423,7 +419,6 @@ int main(int argc, char **argv, char **const envp) { if (!Loader.MapMemory(SyscallHandler.get())) { // failed to map LogMan::Msg::EFmt("Failed to map %d-bit elf file.", Loader.Is64BitMode() ? 64 : 32); - FEXCore::Allocator::ClearFaultEvaluate(); return -ENOEXEC; } } @@ -535,7 +530,6 @@ int main(int argc, char **argv, char **const envp) { // Allocator is now original system allocator FEXCore::Telemetry::Shutdown(Program.ProgramName); FEXCore::Profiler::Shutdown(); - FEXCore::Allocator::ClearFaultEvaluate(); if (ShutdownReason == FEXCore::Context::ExitReason::EXIT_SHUTDOWN) { return ProgramStatus; diff --git a/Source/Tests/LinuxSyscalls/FileManagement.cpp b/Source/Tests/LinuxSyscalls/FileManagement.cpp index 884cf70ea..530a03515 100644 --- a/Source/Tests/LinuxSyscalls/FileManagement.cpp +++ b/Source/Tests/LinuxSyscalls/FileManagement.cpp @@ -295,7 +295,7 @@ FileManager::FileManager(FEXCore::Context::Context *ctx) void SetupOverlay(const ThunkDBObject& DBDepend) { auto ThunkPath = fextl::fmt::format("{}/{}", ThunkGuestPath, DBDepend.LibraryName); - if (!FHU::Filesystem::Exists(ThunkPath.c_str())) { + if (!FHU::Filesystem::Exists(ThunkPath)) { if (!Is64BitMode) { // Guest libraries not existing is expected since not all libraries are thunked on 32-bit return; @@ -655,14 +655,11 @@ uint64_t FileManager::Readlinkat(int dirfd, const char *pathname, char *buf, siz dirfd != AT_FDCWD) { // Passed in a dirfd that isn't magic FDCWD // We need to get the path from the fd now - char Tmp[PATH_MAX]; + char Tmp[PATH_MAX] = ""; auto PathLength = FEX::get_fdpath(dirfd, Tmp); if (PathLength != -1) { Path = fextl::string(Tmp, PathLength); } - else { - Path = ""; - } if (pathname) { if (!Path.empty()) { diff --git a/Source/Tests/LinuxSyscalls/Syscalls.cpp b/Source/Tests/LinuxSyscalls/Syscalls.cpp index c75ea6d95..395c8dc43 100644 --- a/Source/Tests/LinuxSyscalls/Syscalls.cpp +++ b/Source/Tests/LinuxSyscalls/Syscalls.cpp @@ -257,7 +257,7 @@ uint64_t ExecveHandler(const char *pathname, char* const* argv, char* const* env // For absolute paths, check the rootfs first (if available) if (pathname[0] == '/') { auto Path = FEX::HLE::_SyscallHandler->FM.GetEmulatedPath(pathname, true); - if (!Path.empty() && FHU::Filesystem::Exists(Path.c_str())) { + if (!Path.empty() && FHU::Filesystem::Exists(Path)) { Filename = Path; } else { @@ -268,7 +268,7 @@ uint64_t ExecveHandler(const char *pathname, char* const* argv, char* const* env Filename = pathname; } - bool exists = FHU::Filesystem::Exists(Filename.c_str()); + bool exists = FHU::Filesystem::Exists(Filename); if (!exists) { return -ENOENT; } diff --git a/Source/Tests/TestHarnessRunner.cpp b/Source/Tests/TestHarnessRunner.cpp index 9ae4cf1f9..224a884ee 100644 --- a/Source/Tests/TestHarnessRunner.cpp +++ b/Source/Tests/TestHarnessRunner.cpp @@ -115,7 +115,7 @@ private: } int main(int argc, char **argv, char **const envp) { - FEXCore::Allocator::SetupFaultEvaluate(); + FEXCore::Allocator::GLIBCScopedFault GLIBFaultScope; LogMan::Throw::InstallHandler(AssertHandler); LogMan::Msg::InstallHandler(MsgHandler); FEXCore::Config::Initialize(); @@ -127,7 +127,6 @@ int main(int argc, char **argv, char **const envp) { if (Args.size() < 2) { LogMan::Msg::EFmt("Not enough arguments"); - FEXCore::Allocator::ClearFaultEvaluate(); return -1; } @@ -186,7 +185,6 @@ int main(int argc, char **argv, char **const envp) { if (TestUnsupported) { FEXCore::Context::Context::DestroyContext(CTX); - FEXCore::Allocator::ClearFaultEvaluate(); return 0; } @@ -212,7 +210,6 @@ int main(int argc, char **argv, char **const envp) { if (!Loader.MapMemory(SyscallHandler.get())) { // failed to map LogMan::Msg::EFmt("Failed to map %d-bit elf file.", Loader.Is64BitMode() ? 64 : 32); - FEXCore::Allocator::ClearFaultEvaluate(); return -ENOEXEC; } @@ -222,7 +219,6 @@ int main(int argc, char **argv, char **const envp) { bool Result1 = CTX->InitCore(Loader.DefaultRIP(), Loader.GetStackPointer()); if (!Result1) { - FEXCore::Allocator::ClearFaultEvaluate(); return 1; } @@ -242,7 +238,6 @@ int main(int argc, char **argv, char **const envp) { if (!Loader.MapMemory()) { // failed to map LogMan::Msg::EFmt("Failed to map %d-bit elf file.", Loader.Is64BitMode() ? 64 : 32); - FEXCore::Allocator::ClearFaultEvaluate(); return -ENOEXEC; } @@ -265,7 +260,6 @@ int main(int argc, char **argv, char **const envp) { LogMan::Msg::UnInstallHandlers(); FEXCore::Allocator::ClearHooks(); - FEXCore::Allocator::ClearFaultEvaluate(); return Passed ? 0 : -1; } diff --git a/unittests/APITests/LexicallyNormal.cpp b/unittests/APITests/LexicallyNormal.cpp index b9d3471c9..5ce4ed599 100644 --- a/unittests/APITests/LexicallyNormal.cpp +++ b/unittests/APITests/LexicallyNormal.cpp @@ -3,28 +3,29 @@ #include #define TestPath(Path) \ - REQUIRE(std::string_view(FHU::Filesystem::LexicallyNormal(Path)) == std::string_view(std::filesystem::path(Path).lexically_normal().string())); TEST_CASE("LexicallyNormal") { - TestPath(""); - TestPath("/"); - TestPath("/./"); - TestPath("//."); - TestPath("//./"); - TestPath("//.//"); + auto Path = GENERATE("", + "/", + "/./", + "//.", + "//./", + "//.//", - TestPath("."); - TestPath(".."); - TestPath(".//"); - TestPath("../../"); - TestPath("././"); - TestPath("./../"); - TestPath("./../"); - TestPath("./.././.././."); - TestPath("./.././.././.."); + ".", + "..", + ".//", + "../../", + "././", + "./../", + "./../", + "./.././.././.", + "./.././.././..", - TestPath("./foo/../"); - TestPath("foo/./bar/.."); - TestPath("foo/.///bar/.."); - TestPath("foo/.///bar/../"); + "./foo/../", + "foo/./bar/..", + "foo/.///bar/..", + "foo/.///bar/../"); + + REQUIRE(std::string_view(FHU::Filesystem::LexicallyNormal(Path)) == std::string_view(std::filesystem::path(Path).lexically_normal().string())); }