From bf4db22165c2a7e2ba96dcef5e02535aa644c54c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 2 Aug 2020 17:11:09 +0200 Subject: [PATCH] SaveState: On section version failure, log out the name of the bad section. Also some other minor improvements to logging and comments. --- Common/ChunkFile.cpp | 22 +++++++++++++--------- Common/ChunkFile.h | 27 +++++++++++++++++---------- Core/SaveState.cpp | 38 ++++++++++++++++++++------------------ Core/SaveState.h | 2 +- UI/EmuScreen.cpp | 23 ++++------------------- 5 files changed, 55 insertions(+), 57 deletions(-) diff --git a/Common/ChunkFile.cpp b/Common/ChunkFile.cpp index cbfec7ffe7..8954de5c45 100644 --- a/Common/ChunkFile.cpp +++ b/Common/ChunkFile.cpp @@ -30,23 +30,26 @@ PointerWrapSection PointerWrap::Section(const char *title, int minVer, int ver) char marker[16] = {0}; int foundVersion = ver; - // This is strncpy because we rely on its weird non-null-terminating truncation behaviour. + // This is strncpy because we rely on its weird non-null-terminating zero-filling truncation behaviour. // Can't replace it with the more sensible truncate_cpy because that would break savestates. strncpy(marker, title, sizeof(marker)); - if (!ExpectVoid(marker, sizeof(marker))) - { + if (!ExpectVoid(marker, sizeof(marker))) { // Might be before we added name markers for safety. - if (foundVersion == 1 && ExpectVoid(&foundVersion, sizeof(foundVersion))) + if (foundVersion == 1 && ExpectVoid(&foundVersion, sizeof(foundVersion))) { DoMarker(title); - // Wasn't found, but maybe we can still load the state. - else + } else { + // Wasn't found, but maybe we can still load the state. foundVersion = 0; - } - else + } + } else { Do(foundVersion); + } if (error == ERROR_FAILURE || foundVersion < minVer || foundVersion > ver) { - WARN_LOG(SAVESTATE, "Savestate failure: wrong version %d found for %s", foundVersion, title); + if (!firstBadSectionTitle_) { + firstBadSectionTitle_ = title; + } + WARN_LOG(SAVESTATE, "Savestate failure: wrong version %d found for section '%s'", foundVersion, title); SetError(ERROR_FAILURE); return PointerWrapSection(*this, -1, title); } @@ -58,6 +61,7 @@ void PointerWrap::SetError(Error error_) { error = error_; } if (error > ERROR_WARNING) { + // For the rest of this run, just measure. mode = PointerWrap::MODE_MEASURE; } } diff --git a/Common/ChunkFile.h b/Common/ChunkFile.h index 42d5b38675..e8a41901c5 100644 --- a/Common/ChunkFile.h +++ b/Common/ChunkFile.h @@ -76,6 +76,7 @@ private: // Wrapper class class PointerWrap { +private: // This makes it a compile error if you forget to define DoState() on non-POD. // Which also can be a problem, for example struct tm is non-POD on linux, for whatever reason... #ifdef _MSC_VER @@ -111,6 +112,8 @@ class PointerWrap } }; + const char *firstBadSectionTitle_ = nullptr; + public: enum Mode { MODE_READ = 1, // load @@ -127,11 +130,11 @@ public: u8 **ptr; Mode mode; - Error error; + Error error = ERROR_NONE; public: - PointerWrap(u8 **ptr_, Mode mode_) : ptr(ptr_), mode(mode_), error(ERROR_NONE) {} - PointerWrap(unsigned char **ptr_, int mode_) : ptr((u8**)ptr_), mode((Mode)mode_), error(ERROR_NONE) {} + PointerWrap(u8 **ptr_, Mode mode_) : ptr(ptr_), mode(mode_) {} + PointerWrap(unsigned char **ptr_, int mode_) : ptr((u8**)ptr_), mode((Mode)mode_) {} PointerWrapSection Section(const char *title, int ver); @@ -145,6 +148,10 @@ public: u8 **GetPPtr() {return ptr;} void SetError(Error error_); + const char *GetBadSectionTitle() const { + return firstBadSectionTitle_; + } + // Same as DoVoid, except doesn't advance pointer if it doesn't match on read. bool ExpectVoid(void *data, int size); void DoVoid(void *data, int size); @@ -586,7 +593,7 @@ public: // May fail badly if ptr doesn't point to valid data. template - static Error LoadPtr(u8 *ptr, T &_class) + static Error LoadPtr(u8 *ptr, T &_class, std::string *errorString) { PointerWrap p(&ptr, PointerWrap::MODE_READ); _class.DoState(p); @@ -594,6 +601,7 @@ public: if (p.error != p.ERROR_FAILURE) { return ERROR_NONE; } else { + *errorString = std::string("Failure at ") + p.GetBadSectionTitle(); return ERROR_BROKEN_STATE; } } @@ -630,14 +638,13 @@ public: u8 *ptr = nullptr; size_t sz; Error error = LoadFile(filename, gitVersion, ptr, sz, failureReason); - if (error == ERROR_NONE) { - error = LoadPtr(ptr, _class); - delete [] ptr; - } - - INFO_LOG(SAVESTATE, "ChunkReader: Done loading %s", filename.c_str()); if (error == ERROR_NONE) { failureReason->clear(); + error = LoadPtr(ptr, _class, failureReason); + delete [] ptr; + INFO_LOG(SAVESTATE, "ChunkReader: Done loading '%s'", filename.c_str()); + } else { + WARN_LOG(SAVESTATE, "ChunkReader: Error found during load of '%s'", filename.c_str()); } return error; } diff --git a/Core/SaveState.cpp b/Core/SaveState.cpp index 4630b0d9a3..c6cd4fe469 100644 --- a/Core/SaveState.cpp +++ b/Core/SaveState.cpp @@ -91,9 +91,9 @@ namespace SaveState return CChunkFileReader::SavePtr(&data[0], state); } - CChunkFileReader::Error LoadFromRam(std::vector &data) { + CChunkFileReader::Error LoadFromRam(std::vector &data, std::string *errorString) { SaveStart state; - return CChunkFileReader::LoadPtr(&data[0], state); + return CChunkFileReader::LoadPtr(&data[0], state, errorString); } struct StateRingbuffer @@ -135,7 +135,7 @@ namespace SaveState return err; } - CChunkFileReader::Error Restore() + CChunkFileReader::Error Restore(std::string *errorString) { std::lock_guard guard(lock_); @@ -149,7 +149,7 @@ namespace SaveState static std::vector buffer; LockedDecompress(buffer, states_[n], bases_[baseMapping_[n]]); - return LoadFromRam(buffer); + return LoadFromRam(buffer, errorString); } void ScheduleCompress(std::vector *result, const std::vector *state, const std::vector *base) @@ -632,13 +632,14 @@ namespace SaveState return copy; } - bool HandleFailure() + bool HandleLoadFailure() { // Okay, first, let's give the rewind state a shot - maybe we can at least not reset entirely. // Even if this was a rewind, maybe we can still load a previous one. CChunkFileReader::Error result; do { - result = rewindStates.Restore(); + std::string errorString; + result = rewindStates.Restore(&errorString); } while (result == CChunkFileReader::ERROR_BROKEN_STATE); if (result == CChunkFileReader::ERROR_NONE) { @@ -728,7 +729,6 @@ namespace SaveState Status callbackResult; bool tempResult; std::string callbackMessage; - std::string reason; std::string title; auto sc = GetI18NCategory("Screen"); @@ -740,13 +740,14 @@ namespace SaveState i18nSaveFailure = sc->T("Failed to save state"); std::string slot_prefix = op.slot >= 0 ? StringFromFormat("(%d) ", op.slot + 1) : ""; + std::string errorString; switch (op.type) { case SAVESTATE_LOAD: INFO_LOG(SAVESTATE, "Loading state from '%s'", op.filename.c_str()); // Use the state's latest version as a guess for saveStateInitialGitVersion. - result = CChunkFileReader::Load(op.filename, &saveStateInitialGitVersion, state, &reason); + result = CChunkFileReader::Load(op.filename, &saveStateInitialGitVersion, state, &errorString); if (result == CChunkFileReader::ERROR_NONE) { callbackMessage = slot_prefix + sc->T("Loaded State"); callbackResult = Status::SUCCESS; @@ -777,12 +778,13 @@ namespace SaveState } #endif } else if (result == CChunkFileReader::ERROR_BROKEN_STATE) { - HandleFailure(); - callbackMessage = i18nLoadFailure; - ERROR_LOG(SAVESTATE, "Load state failure: %s", reason.c_str()); + HandleLoadFailure(); + callbackMessage = std::string(i18nLoadFailure) + ": " + errorString; + ERROR_LOG(SAVESTATE, "Load state failure: %s", errorString.c_str()); callbackResult = Status::FAILURE; } else { - callbackMessage = sc->T(reason.c_str(), i18nLoadFailure); + // ? + callbackMessage = sc->T(errorString.c_str(), i18nLoadFailure); callbackResult = Status::FAILURE; } break; @@ -812,9 +814,9 @@ namespace SaveState } #endif } else if (result == CChunkFileReader::ERROR_BROKEN_STATE) { - HandleFailure(); + // TODO: What else might we want to do here? This should be very unusual. callbackMessage = i18nSaveFailure; - ERROR_LOG(SAVESTATE, "Save state failure: %s", reason.c_str()); + ERROR_LOG(SAVESTATE, "Save state failure"); callbackResult = Status::FAILURE; } else { callbackMessage = i18nSaveFailure; @@ -834,24 +836,24 @@ namespace SaveState case SAVESTATE_REWIND: INFO_LOG(SAVESTATE, "Rewinding to recent savestate snapshot"); - result = rewindStates.Restore(); + result = rewindStates.Restore(&errorString); if (result == CChunkFileReader::ERROR_NONE) { callbackMessage = sc->T("Loaded State"); callbackResult = Status::SUCCESS; hasLoadedState = true; } else if (result == CChunkFileReader::ERROR_BROKEN_STATE) { // Cripes. Good news is, we might have more. Let's try those too, better than a reset. - if (HandleFailure()) { + if (HandleLoadFailure()) { // Well, we did rewind, even if too much... callbackMessage = sc->T("Loaded State"); callbackResult = Status::SUCCESS; hasLoadedState = true; } else { - callbackMessage = i18nLoadFailure; + callbackMessage = std::string(i18nLoadFailure) + ": " + errorString; callbackResult = Status::FAILURE; } } else { - callbackMessage = i18nLoadFailure; + callbackMessage = std::string(i18nLoadFailure) + ": " + errorString; callbackResult = Status::FAILURE; } break; diff --git a/Core/SaveState.h b/Core/SaveState.h index 8a205ee579..98b5d9b7d9 100644 --- a/Core/SaveState.h +++ b/Core/SaveState.h @@ -69,7 +69,7 @@ namespace SaveState void Save(const std::string &filename, int slot, Callback callback = Callback(), void *cbUserData = 0); CChunkFileReader::Error SaveToRam(std::vector &state); - CChunkFileReader::Error LoadFromRam(std::vector &state); + CChunkFileReader::Error LoadFromRam(std::vector &state, std::string *errorString); // For testing / automated tests. Runs a save state verification pass (async.) // Warning: callback will be called on a different thread. diff --git a/UI/EmuScreen.cpp b/UI/EmuScreen.cpp index 31936b64dd..b517d6c206 100644 --- a/UI/EmuScreen.cpp +++ b/UI/EmuScreen.cpp @@ -1439,13 +1439,15 @@ void EmuScreen::render() { return; } + // Freeze-frame functionality (loads a savestate on every frame). if (PSP_CoreParameter().freezeNext) { PSP_CoreParameter().frozen = true; PSP_CoreParameter().freezeNext = false; SaveState::SaveToRam(freezeState_); } else if (PSP_CoreParameter().frozen) { - if (CChunkFileReader::ERROR_NONE != SaveState::LoadFromRam(freezeState_)) { - ERROR_LOG(SAVESTATE, "Failed to load freeze state. Unfreezing."); + std::string errorString; + if (CChunkFileReader::ERROR_NONE != SaveState::LoadFromRam(freezeState_, &errorString)) { + ERROR_LOG(SAVESTATE, "Failed to load freeze state (%s). Unfreezing.", errorString.c_str()); PSP_CoreParameter().frozen = false; } } @@ -1503,23 +1505,6 @@ void EmuScreen::render() { screenManager()->getUIContext()->BeginFrame(); renderUI(); } - - // We have no use for backbuffer depth or stencil, so let tiled renderers discard them after tiling. - /* - if (gl_extensions.GLES3 && glInvalidateFramebuffer != nullptr) { - GLenum attachments[2] = { GL_DEPTH, GL_STENCIL }; - glInvalidateFramebuffer(GL_FRAMEBUFFER, 2, attachments); - } else if (!gl_extensions.GLES3) { -#ifdef USING_GLES2 - // Tiled renderers like PowerVR should benefit greatly from this. However - seems I can't call it? - bool hasDiscard = gl_extensions.EXT_discard_framebuffer; // TODO - if (hasDiscard) { - //const GLenum targets[3] = { GL_COLOR_EXT, GL_DEPTH_EXT, GL_STENCIL_EXT }; - //glDiscardFramebufferEXT(GL_FRAMEBUFFER, 3, targets); - } -#endif - } - */ } bool EmuScreen::hasVisibleUI() {