Merge pull request #22176 from hrydgard/metafilesystem-savestate-compat

Fix savestate breakage related to file system mounts

Core/FileSystems/MetaFileSystem.cpp

@@ -644,20 +644,45 @@ void MetaFileSystem::DoState(PointerWrap &p) {
 
 	u32 n = (u32) fileSystems.size();
 	Do(p, n);
-	bool skipPfat0 = false;
-	if (n != (u32) fileSystems.size()) {
-		if (n == (u32) fileSystems.size() - 1) {
-			skipPfat0 = true;
-		} else {
-			p.SetError(p.ERROR_FAILURE);
-			ERROR_LOG(Log::FileSystem, "Savestate failure: number of filesystems doesn't match.");
-			return;
-		}
+
+	// The mounts are serialized positionally: one section per mount, in fileSystems order, with no
+	// length to skip by. That order is the order Mount() first saw each prefix during boot, which
+	// spans more than MountFileSystems() - booting an ISO, MountGameISO() has already added umd0:,
+	// umd1:, umd: and disc0: by the time we get there. An older build's savestate simply lacks the
+	// sections for mounts that didn't exist yet, and we have to leave out exactly those to stay
+	// lined up. We only know how many are missing (n), not which, hence the list below.
+	//
+	// ADDING A NEW MOUNT: prepend its prefix here, or every existing savestate stops loading with
+	// "Failure at <whatever section follows>". Newest first, because a state missing k mounts is
+	// missing the k most recently added ones. Where the new mount falls in the order doesn't
+	// matter - skipping it by prefix leaves the relative order of all the others unchanged.
+	//
+	// DO NOT REORDER EXISTING MOUNTS. Sections are paired with filesystems purely by position, so
+	// swapping two Mount() calls feeds every old savestate's sections to the wrong filesystems,
+	// silently and with no version to catch it.
+	//
+	// RENAMING OR REMOVING A MOUNT isn't handled here either: a section in the state we have no
+	// mount for can't be skipped, because we can't know how long it is. Either of those needs a
+	// format change - storing the prefixes, or a length per section.
+	static const char * const mountsAddedOverTime[] = { "flash1:", "pfat0:" };
+
+	const size_t missing = n < (u32)fileSystems.size() ? fileSystems.size() - n : 0;
+	if (n > (u32)fileSystems.size() || missing > ARRAY_SIZE(mountsAddedOverTime)) {
+		p.SetError(p.ERROR_FAILURE);
+		ERROR_LOG(Log::FileSystem, "Savestate failure: number of filesystems doesn't match (%d in state, %d mounted).", (int)n, (int)fileSystems.size());
+		return;
 	}
 
-	for (u32 i = 0; i < n; ++i) {
-		if (!skipPfat0 || fileSystems[i].prefix != "pfat0:") {
-			fileSystems[i].system->DoState(p);
+	for (const MountPoint &mount : fileSystems) {
+		bool skip = false;
+		for (size_t i = 0; i < missing; i++) {
+			if (mount.prefix == mountsAddedOverTime[i]) {
+				skip = true;
+				break;
+			}
+		}
+		if (!skip) {
+			mount.system->DoState(p);
 		}
 	}
 }

headless/Headless.cpp

@@ -82,6 +82,8 @@ static bool g_screenshotFailed = false;
 static std::string g_debugOutputBuffer;
 static bool g_writeFailureScreenshot = true;
 static bool g_writeDebugOutput = true;
+// Set from the savestate callback on the emu thread, read after it has been joined.
+static bool g_stateLoadFailed = false;
 
 #if PPSSPP_PLATFORM(ANDROID)
 JNIEnv *getEnv() {
@@ -339,6 +341,10 @@ static bool RunAutoTest(GraphicsContext *graphicsContext, CoreParameter &corePar
 	double deadline = time_now_d() + opt.timeout;
 	coreState = coreParameter.startBreak ? CORE_STEPPING_CPU : CORE_RUNNING_CPU;
 	while (coreState == CORE_RUNNING_CPU || coreState == CORE_STEPPING_CPU) {
+		// Savestate loads/saves are queued and applied here, same as EmuScreen::render does in the
+		// app. Without this, --state silently did nothing at all.
+		SaveState::Process();
+
 		int blockTicks = (int)usToCycles(1000000 / 10);
 		PSP_RunLoopFor(blockTicks);
 
@@ -873,7 +879,14 @@ int main(int argc, const char* argv[]) {
 	}
 
 	if (stateToLoad) {
-		SaveState::Load(Path(stateToLoad), -1);
+		// Queued now, actually applied by SaveState::Process() once the game is up and running.
+		SaveState::Load(Path(stateToLoad), -1, [](SaveState::Status status, std::string_view message, std::string_view) {
+			// The message already reads as a full sentence, e.g. "Failed to load state: <reason>".
+			fprintf(stderr, "%.*s\n", (int)message.size(), message.data());
+			if (status == SaveState::Status::FAILURE) {
+				g_stateLoadFailed = true;
+			}
+		});
 	}
 
 	std::string errorMessage;
@@ -893,6 +906,11 @@ int main(int argc, const char* argv[]) {
 		retval = 1;
 	}
 
+	if (g_stateLoadFailed && retval == 0) {
+		// Whatever the run itself reported, the state we were told to load never got applied.
+		retval = 1;
+	}
+
 	graphicsContext->ShutdownAPI();
 
 	delete graphicsContext;