From e1a48d74c405ab66817148dac8daf7f8e741b2bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 1 Jan 2023 21:12:04 +0100 Subject: [PATCH] A bit more GetPointer cleanup. Probably not worth it for performance reasons, but some semantic cleanup is good, especially the accidental GetPointer -> writable casts without using GetPointerWrite. Using Unchecked on already checked pointers, or when we'd crash anyway if it returned nullptr, is good for clarity. --- Core/HLE/sceKernelInterrupt.cpp | 18 +++++++++--------- Core/HLE/sceKernelModule.cpp | 10 +++++----- Core/HLE/sceMpeg.cpp | 1 + Core/HLE/sceP3da.cpp | 4 ++-- Core/HLE/sceRtc.cpp | 5 +++-- Core/HLE/sceUtility.cpp | 11 ++++++++--- Core/MIPS/JitCommon/JitBlockCache.cpp | 7 ++++++- Core/MemMap.h | 4 ++++ Core/TextureReplacer.cpp | 2 +- GPU/Common/DrawEngineCommon.cpp | 2 +- GPU/Common/FramebufferManagerCommon.cpp | 4 ++-- 11 files changed, 42 insertions(+), 26 deletions(-) diff --git a/Core/HLE/sceKernelInterrupt.cpp b/Core/HLE/sceKernelInterrupt.cpp index 3dadc4dc77..c1ff47ee2f 100644 --- a/Core/HLE/sceKernelInterrupt.cpp +++ b/Core/HLE/sceKernelInterrupt.cpp @@ -728,7 +728,7 @@ static u32 sysclib_strcpy(u32 dst, u32 src) { static u32 sysclib_strlen(u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strlen(src=%08x)", src); if (Memory::IsValidAddress(src)) { - return (u32)strlen(Memory::GetCharPointer(src)); + return (u32)strlen(Memory::GetCharPointerUnchecked(src)); } else { // What to do? Crash, probably. return 0; @@ -738,7 +738,7 @@ static u32 sysclib_strlen(u32 src) { static int sysclib_memcmp(u32 dst, u32 src, u32 size) { ERROR_LOG(SCEKERNEL, "Untested sysclib_memcmp(dest=%08x, src=%08x, size=%i)", dst, src, size); if (Memory::IsValidRange(dst, size) && Memory::IsValidRange(src, size)) { - return memcmp(Memory::GetCharPointer(dst), Memory::GetCharPointer(src), size); + return memcmp(Memory::GetCharPointerUnchecked(dst), Memory::GetCharPointerUnchecked(src), size); } else { // What to do? Crash, probably. return 0; @@ -749,7 +749,7 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { ERROR_LOG(SCEKERNEL, "Unimpl sysclib_sprintf(dest=%08x, src=%08x)", dst, fmt); if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(fmt)) { // TODO: Properly use the format string with more parameters. - return sprintf((char *)Memory::GetPointer(dst), "%s", Memory::GetCharPointer(fmt)); + return sprintf((char *)Memory::GetPointerUnchecked(dst), "%s", Memory::GetCharPointerUnchecked(fmt)); } else { // What to do? Crash, probably. return 0; @@ -768,8 +768,8 @@ static u32 sysclib_memset(u32 destAddr, int data, int size) { static int sysclib_strstr(u32 s1, u32 s2) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strstr(%08x, %08x)", s1, s2); if (Memory::IsValidAddress(s1) && Memory::IsValidAddress(s2)) { - std::string str1 = Memory::GetCharPointer(s1); - std::string str2 = Memory::GetCharPointer(s2); + std::string str1 = Memory::GetCharPointerUnchecked(s1); + std::string str2 = Memory::GetCharPointerUnchecked(s2); size_t index = str1.find(str2); if (index == str1.npos) { return 0; @@ -782,8 +782,8 @@ static int sysclib_strstr(u32 s1, u32 s2) { static int sysclib_strncmp(u32 s1, u32 s2, u32 size) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strncmp(%08x, %08x, %08x)", s1, s2, size); if (Memory::IsValidAddress(s1) && Memory::IsValidAddress(s2)) { - const char * str1 = Memory::GetCharPointer(s1); - const char * str2 = Memory::GetCharPointer(s2); + const char * str1 = Memory::GetCharPointerUnchecked(s1); + const char * str2 = Memory::GetCharPointerUnchecked(s2); return strncmp(str1, str2, size); } return 0; @@ -810,8 +810,8 @@ static u32 sysclib_strncpy(u32 dest, u32 src, u32 size) { // This is just regular strncpy, but being explicit to avoid warnings/safety fixes on missing null. u32 i = 0; u32 srcSize = Memory::ValidSize(src, size); - const u8 *srcp = Memory::GetPointer(src); - u8 *destp = Memory::GetPointerWrite(dest); + const u8 *srcp = Memory::GetPointerUnchecked(src); + u8 *destp = Memory::GetPointerWriteUnchecked(dest); for (i = 0; i < srcSize; ++i) { u8 c = *srcp++; if (c == 0) diff --git a/Core/HLE/sceKernelModule.cpp b/Core/HLE/sceKernelModule.cpp index f1c4b67155..dc4b53032d 100644 --- a/Core/HLE/sceKernelModule.cpp +++ b/Core/HLE/sceKernelModule.cpp @@ -1042,7 +1042,7 @@ static bool KernelImportModuleFuncs(PSPModule *module, u32 *firstImportStubAddr, strncpy(func.moduleName, modulename, KERNELOBJECT_MAX_NAME_LENGTH); func.moduleName[KERNELOBJECT_MAX_NAME_LENGTH] = '\0'; - u32_le *nidDataPtr = (u32_le *)Memory::GetPointer(entry->nidData); + u32_le *nidDataPtr = (u32_le *)Memory::GetPointerUnchecked(entry->nidData); for (int i = 0; i < entry->numFuncs; ++i) { // This is the id of the import. func.nid = nidDataPtr[i]; @@ -1080,7 +1080,7 @@ static bool KernelImportModuleFuncs(PSPModule *module, u32 *firstImportStubAddr, } WriteVarSymbolState state; - u32_le *varRef = (u32_le *)Memory::GetPointer(varRefsPtr); + u32_le *varRef = (u32_le *)Memory::GetPointerUnchecked(varRefsPtr); for (; *varRef != 0; ++varRef) { var.nid = nid; var.stubAddr = (*varRef & 0x03FFFFFF) << 2; @@ -1106,7 +1106,7 @@ static bool KernelImportModuleFuncs(PSPModule *module, u32 *firstImportStubAddr, char temp[512]; const char *modulename; if (Memory::IsValidAddress(entry->name)) { - modulename = Memory::GetCharPointer(entry->name); + modulename = Memory::GetCharPointerUnchecked(entry->name); } else { modulename = "(invalidname)"; } @@ -1316,7 +1316,7 @@ static PSPModule *__KernelLoadELFFromPtr(const u8 *ptr, size_t elfSize, u32 load return nullptr; } - modinfo = (PspModuleInfo *)Memory::GetPointer(modinfoaddr); + modinfo = (PspModuleInfo *)Memory::GetPointerUnchecked(modinfoaddr); module->nm.nsegment = reader.GetNumSegments(); module->nm.attribute = modinfo->moduleAttrs; @@ -1498,7 +1498,7 @@ static PSPModule *__KernelLoadELFFromPtr(const u8 *ptr, size_t elfSize, u32 load continue; } - u32_le *residentPtr = (u32_le *)Memory::GetPointer(ent->resident); + u32_le *residentPtr = (u32_le *)Memory::GetPointerUnchecked(ent->resident); u32_le *exportPtr = residentPtr + ent->fcount + variableCount; if (ent->size != 4 && ent->unknown1 != 0 && ent->unknown2 != 0) { diff --git a/Core/HLE/sceMpeg.cpp b/Core/HLE/sceMpeg.cpp index 02c66b6268..2f8cb86416 100644 --- a/Core/HLE/sceMpeg.cpp +++ b/Core/HLE/sceMpeg.cpp @@ -2175,6 +2175,7 @@ static u32 convertABGRToYCbCr(u32 abgr) { return (y << 16) | (cb << 8) | cr; } +// bufferOutputAddr is checked in the caller. static int __MpegAvcConvertToYuv420(const void *data, u32 bufferOutputAddr, int width, int height) { u32 *imageBuffer = (u32*)data; int sizeY = width * height; diff --git a/Core/HLE/sceP3da.cpp b/Core/HLE/sceP3da.cpp index de5ce004dc..e9cd6aa1c4 100644 --- a/Core/HLE/sceP3da.cpp +++ b/Core/HLE/sceP3da.cpp @@ -48,11 +48,11 @@ static u32 sceP3daBridgeCore(u32 p3daCoreAddr, u32 channelsNum, u32 samplesNum, DEBUG_LOG(SCEAUDIO, "sceP3daBridgeCore(%08x, %08x, %08x, %08x, %08x)", p3daCoreAddr, channelsNum, samplesNum, inputAddr, outputAddr); if (Memory::IsValidAddress(inputAddr) && Memory::IsValidAddress(outputAddr)) { int scaleval = getScaleValue(channelsNum); - s16_le *outbuf = (s16_le *)Memory::GetPointer(outputAddr); + s16_le *outbuf = (s16_le *)Memory::GetPointerWriteUnchecked(outputAddr); memset(outbuf, 0, samplesNum * sizeof(s16) * 2); for (u32 k = 0; k < channelsNum; k++) { u32 inaddr = Memory::Read_U32(inputAddr + k * 4); - const s16 *inbuf = (const s16 *)Memory::GetPointer(inaddr); + const s16 *inbuf = (const s16 *)Memory::GetPointerUnchecked(inaddr); if (!inbuf) continue; for (u32 i = 0; i < samplesNum; i++) { diff --git a/Core/HLE/sceRtc.cpp b/Core/HLE/sceRtc.cpp index 5ec9690300..1b243dd6fc 100644 --- a/Core/HLE/sceRtc.cpp +++ b/Core/HLE/sceRtc.cpp @@ -879,6 +879,7 @@ static int sceRtcSetAlarmTick(u32 unknown1, u32 unknown2) return 0; } +// Caller must check outPtr and srcTickPtr. static int __RtcFormatRFC2822(u32 outPtr, u32 srcTickPtr, int tz) { u64 srcTick = Memory::Read_U64(srcTickPtr); @@ -897,7 +898,7 @@ static int __RtcFormatRFC2822(u32 outPtr, u32 srcTickPtr, int tz) local.tm_min += tz; rtc_timegm(&local); - char *out = (char *)Memory::GetPointer(outPtr); + char *out = (char *)Memory::GetPointerWriteUnchecked(outPtr); char *end = out + 32; out += strftime(out, end - out, "%a, %d %b ", &local); out += snprintf(out, end - out, "%04d", pt.year); @@ -928,7 +929,7 @@ static int __RtcFormatRFC3339(u32 outPtr, u32 srcTickPtr, int tz) local.tm_min += tz; rtc_timegm(&local); - char *out = (char *)Memory::GetPointer(outPtr); + char *out = (char *)Memory::GetPointerWriteUnchecked(outPtr); char *end = out + 32; out += snprintf(out, end - out, "%04d", pt.year); out += strftime(out, end - out, "-%m-%dT%H:%M:%S.00", &local); diff --git a/Core/HLE/sceUtility.cpp b/Core/HLE/sceUtility.cpp index f31c2291bd..5cffd414af 100644 --- a/Core/HLE/sceUtility.cpp +++ b/Core/HLE/sceUtility.cpp @@ -816,15 +816,20 @@ static u32 sceUtilitySetSystemParamString(u32 id, u32 strPtr) return 0; } -static u32 sceUtilityGetSystemParamString(u32 id, u32 destaddr, int destSize) +static u32 sceUtilityGetSystemParamString(u32 id, u32 destAddr, int destSize) { - DEBUG_LOG(SCEUTILITY, "sceUtilityGetSystemParamString(%i, %08x, %i)", id, destaddr, destSize); - char *buf = (char *)Memory::GetPointer(destaddr); + if (!Memory::IsValidRange(destAddr, destSize)) { + // TODO: What error code? + return -1; + } + DEBUG_LOG(SCEUTILITY, "sceUtilityGetSystemParamString(%i, %08x, %i)", id, destAddr, destSize); + char *buf = (char *)Memory::GetPointerWriteUnchecked(destAddr); switch (id) { case PSP_SYSTEMPARAM_ID_STRING_NICKNAME: // If there's not enough space for the string and null terminator, fail. if (destSize <= (int)g_Config.sNickName.length()) return PSP_SYSTEMPARAM_RETVAL_STRING_TOO_LONG; + // TODO: should we zero-pad the output as strncpy does? And what are the semantics for the terminating null if destSize == length? strncpy(buf, g_Config.sNickName.c_str(), destSize); break; diff --git a/Core/MIPS/JitCommon/JitBlockCache.cpp b/Core/MIPS/JitCommon/JitBlockCache.cpp index 5dfb580bb7..6ed9bc6427 100644 --- a/Core/MIPS/JitCommon/JitBlockCache.cpp +++ b/Core/MIPS/JitCommon/JitBlockCache.cpp @@ -65,7 +65,12 @@ static uint64_t HashJitBlock(const JitBlock &b) { PROFILE_THIS_SCOPE("jithash"); if (JIT_USE_COMPILEDHASH) { // Includes the emuhack (or emuhacks) in memory. - return XXH3_64bits(Memory::GetPointerRange(b.originalAddress, b.originalSize * 4), b.originalSize * 4); + if (Memory::IsValidRange(b.originalAddress, b.originalSize * 4)) { + return XXH3_64bits(Memory::GetPointerUnchecked(b.originalAddress), b.originalSize * 4); + } else { + // Hm, this would be bad. + return 0; + } } return 0; } diff --git a/Core/MemMap.h b/Core/MemMap.h index d9ef31c8ae..b2edbe0290 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -280,6 +280,10 @@ inline const char* GetCharPointer(const u32 address) { } } +inline const char *GetCharPointerUnchecked(const u32 address) { + return (const char *)GetPointerUnchecked(address); +} + inline void MemcpyUnchecked(void *to_data, const u32 from_address, const u32 len) { memcpy(to_data, GetPointerUnchecked(from_address), len); } diff --git a/Core/TextureReplacer.cpp b/Core/TextureReplacer.cpp index 635812c302..97f1cc2529 100644 --- a/Core/TextureReplacer.cpp +++ b/Core/TextureReplacer.cpp @@ -402,7 +402,7 @@ u32 TextureReplacer::ComputeHash(u32 addr, int bufw, int w, int h, GETextureForm } } - const u8 *checkp = Memory::GetPointer(addr); + const u8 *checkp = Memory::GetPointerUnchecked(addr); if (reduceHash_) { reduceHashSize = LookupReduceHashRange(w, h); // default to reduceHashGlobalValue which default is 0.5 diff --git a/GPU/Common/DrawEngineCommon.cpp b/GPU/Common/DrawEngineCommon.cpp index f675a255b6..139d47613c 100644 --- a/GPU/Common/DrawEngineCommon.cpp +++ b/GPU/Common/DrawEngineCommon.cpp @@ -422,7 +422,7 @@ bool DrawEngineCommon::GetCurrentSimpleVertices(int count, std::vector simpleVertices; temp_buffer.resize(std::max((int)indexUpperBound, 8192) * 128 / sizeof(u32)); simpleVertices.resize(indexUpperBound + 1); - NormalizeVertices((u8 *)(&simpleVertices[0]), (u8 *)(&temp_buffer[0]), Memory::GetPointer(gstate_c.vertexAddr), indexLowerBound, indexUpperBound, gstate.vertType); + NormalizeVertices((u8 *)(&simpleVertices[0]), (u8 *)(&temp_buffer[0]), Memory::GetPointerUnchecked(gstate_c.vertexAddr), indexLowerBound, indexUpperBound, gstate.vertType); float world[16]; float view[16]; diff --git a/GPU/Common/FramebufferManagerCommon.cpp b/GPU/Common/FramebufferManagerCommon.cpp index 65d12b522e..0a145b6781 100644 --- a/GPU/Common/FramebufferManagerCommon.cpp +++ b/GPU/Common/FramebufferManagerCommon.cpp @@ -1103,7 +1103,7 @@ void FramebufferManagerCommon::UpdateFromMemory(u32 addr, int size) { // TODO: This doesn't seem quite right anymore. fmt = displayFormat_; } - DrawPixels(vfb, 0, 0, Memory::GetPointer(addr), fmt, vfb->fb_stride, vfb->width, vfb->height, RASTER_COLOR, "UpdateFromMemory_DrawPixels"); + DrawPixels(vfb, 0, 0, Memory::GetPointerUnchecked(addr), fmt, vfb->fb_stride, vfb->width, vfb->height, RASTER_COLOR, "UpdateFromMemory_DrawPixels"); SetColorUpdated(vfb, gstate_c.skipDrawReason); } else { INFO_LOG(FRAMEBUF, "Invalidating FBO for %08x (%dx%d %s)", vfb->fb_address, vfb->width, vfb->height, GeBufferFormatToString(vfb->fb_format)); @@ -1474,7 +1474,7 @@ void FramebufferManagerCommon::CopyDisplayToOutput(bool reallyDirty) { if (Memory::IsValidAddress(fbaddr)) { // The game is displaying something directly from RAM. In GTA, it's decoded video. if (!vfb) { - DrawFramebufferToOutput(Memory::GetPointer(fbaddr), displayStride_, displayFormat_); + DrawFramebufferToOutput(Memory::GetPointerUnchecked(fbaddr), displayStride_, displayFormat_); return; } } else {