Merge pull request #22029 from hrydgard/cmdline-cleanup

Finish moving command line parsing into CmdLine.cpp

AGENTS.md

@@ -58,6 +58,28 @@ by default, but if you want to test-build it locally:
 - Example (POSIX): `cd android && NDK=/path/to/ndk ./ab.sh APP_ABI=arm64-v8a HEADLESS=1`
 - The `ppsspp_headless` executable ends up in `android/libs/<abi>/`.
 
+## Command-line parsing
+
+All command-line parsing for both the main app and headless builds belongs in `Core/CmdLine.cpp` /
+`Core/CmdLine.h` (`CommandLineOptions`), not in the platform entry points (`Windows/main.cpp`, `headless/Headless.cpp`,
+`UI/NativeApp.cpp`, etc.). Don't re-parse `argv` manually in those files - add a field to `CommandLineOptions` instead.
+
+- Most options are declared in the `g_autoParams` table in `CmdLine.cpp` as `{offsetof(...), type, longName,
+  shortName, docString, mode}`. `mode` gates the option to `CmdLineMode::Application`, `::Headless`, or `::Both`
+  (the default if the field is omitted from the initializer) - the same long name can be reused for both modes with
+  different types/meanings (e.g. `--log` is a `String` "log to FILE" option in Application mode but a `Bool` "full
+  log output" option in Headless mode; they don't collide because a given `Parse()` call only matches params whose
+  mode is `Both` or equal to the current mode).
+- Options that can be repeated (e.g. `--ignore TESTNAME`, collected into a `std::vector<std::string>`) or that don't
+  fit the generic single-value table need manual handling in the `else if` chain inside `CommandLineOptions::Parse()`,
+  similar to how `--graphics=` and `boot Filenames` are handled.
+- `ApplyToConfig()` is where parsed options get pushed into `g_Config`/`g_logManager`; prefer wiring a new option
+  through there so all platforms get it for free, rather than reading `CommandLineOptions` fields ad-hoc at each
+  call site.
+- `NativeInit()` in `UI/NativeApp.cpp` still takes `argc`/`argv` (several platform entry points pass them in), but
+  it shouldn't read them directly - by the time `NativeInit()` runs, `CommandLineOptions` should already have
+  everything.
+
 ## File formats, codecs, and other format handlers
 
 Before implementing any file format handler, decompressor, codec, or similar from scratch, search the

Core/CmdLine.cpp

@@ -1,3 +1,5 @@
+#include <cstdlib>
+
 #include "Core/Config.h"
 #include "Core/CmdLine.h"
 #include "Common/StringUtils.h"
@@ -176,10 +178,14 @@ static const CommandLineParam g_autoParams[] = {
 	{POFF(compare), CmdParamType::Bool, "compare", 'c', "Enable comparison mode", CmdLineMode::Headless},
 	{POFF(bench), CmdParamType::Bool, "bench", 'b', "Enable benchmark mode", CmdLineMode::Headless},
 	{POFF(oldAtrac), CmdParamType::Bool, "old-atrac", '\0', "Use old ATRAC decoder"},
-	{POFF(log), CmdParamType::String, "log", '\0', "Output log to FILE"},
-	{POFF(screenshotFilename), CmdParamType::String, "screenshot", '\0', "Take a screenshot and save to FILE"},
-	{POFF(screenshotFilenameSave), CmdParamType::String, "screenshot-save", '\0', "Save screenshot to specified path"},
-	{POFF(timeout), CmdParamType::Double, "timeout", '\0', "Set the timeout value"},
+	{POFF(log), CmdParamType::String, "log", '\0', "Output log to FILE", CmdLineMode::Application},
+	{POFF(enableLogging), CmdParamType::Bool, "log", '\0', "Full log output, not just emulated printfs", CmdLineMode::Headless},
+	{POFF(screenshotFilename), CmdParamType::String, "screenshot", '\0', "Compare rendered output against a reference screenshot FILE", CmdLineMode::Headless},
+	{POFF(screenshotFilenameSave), CmdParamType::String, "screenshot-save", '\0', "Save rendered screenshot to specified path", CmdLineMode::Headless},
+	{POFF(timeout), CmdParamType::Double, "timeout", '\0', "Set the timeout value", CmdLineMode::Headless},
+	{POFF(maxScreenshotError), CmdParamType::Double, "max-mse", '\0', "Maximum allowed MSE error for screenshot comparison", CmdLineMode::Headless},
+	{POFF(mountIso), CmdParamType::String, "mount", 'm', "Mount ISO/CSO on umd1:", CmdLineMode::Headless},
+	{POFF(odsLog), CmdParamType::Bool, "odslog", 'o', "Also log through OutputDebugString (Windows)", CmdLineMode::Headless},
 	{POFF(resolutionScale), CmdParamType::Int, "resolution-scale", '\0', "Set the resolution scale factor"},
 	{POFF(debuggerPort), CmdParamType::Int, "debugger", '\0', "Enable the WebSocket debugger on this port (0 = pick automatically); see docs/WebSocketDebugger.md"},
 	{POFF(bootVSH), CmdParamType::Bool, "vsh", '\0', "Boot the VSH (requires files dumped from a PSP in the flash0 directory)"},
@@ -189,6 +195,10 @@ static const CommandLineParam g_autoParams[] = {
 	{POFF(logNativeCrashes), CmdParamType::Bool, "log-native-crashes", '\0', "Log a native stack trace (Windows only) on an otherwise-unhandled crash", CmdLineMode::Both},
 	{POFF(verbose), CmdParamType::Bool, "verbose", '\0', "Enable verbose output", CmdLineMode::Both},
 	{POFF(printEqualLines), CmdParamType::Bool, "print-equal-lines", '\0', "Print lines that are equal during comparison", CmdLineMode::Headless},
+	{POFF(xres), CmdParamType::Int, "xres", '\0', "Set X resolution", CmdLineMode::Application},
+	{POFF(yres), CmdParamType::Int, "yres", '\0', "Set Y resolution", CmdLineMode::Application},
+	{POFF(dpi), CmdParamType::Double, "dpi", '\0', "Set DPI", CmdLineMode::Application},
+	{POFF(scale), CmdParamType::Double, "scale", '\0', "Set scale", CmdLineMode::Application},
 	// TODO: At some point we should maybe simply expose all config settings to be set directly from the command line automatically?
 };
 
@@ -229,7 +239,9 @@ int CommandLineOptions::PrintUsage(const char *progname, const char *situationTe
 	PRINT_STDOUT("  -d                    set the log level to debug\n");
 	PRINT_STDOUT("  -v                    set the log level to verbose\n");
 	PRINT_STDOUT("  --loglevel=INTEGER    set the log level to specified value\n");
-	PRINT_STDOUT("  --log=FILE            output log to FILE\n");
+	if (mode == CmdLineMode::Application) {
+		PRINT_STDOUT("  --log=FILE            output log to FILE\n");
+	}
 	PRINT_STDOUT("  --state=FILE          load state from FILE\n");
 
 	PRINT_STDOUT("  -i                    use the interpreter\n");
@@ -262,13 +274,10 @@ int CommandLineOptions::PrintUsage(const char *progname, const char *situationTe
 		}
 	}
 
-	// These are only available in SDL.
-	if (mode == CmdLineMode::Application) {
-		PRINT_STDOUT("  --xres PIXELS         set X resolution\n");
-		PRINT_STDOUT("  --yres PIXELS         set Y resolution\n");
-		PRINT_STDOUT("  --dpi DPI             set DPI\n");
-		PRINT_STDOUT("  --scale FACTOR        set scale\n");
+	if (mode == CmdLineMode::Headless) {
+		PRINT_STDOUT("  --ignore TESTNAME     ignore the specified test (may be repeated)\n");
 	}
+
 	PRINT_STDOUT("  --graphics=BACKEND    use a different gpu backend\n");
 	PRINT_STDOUT("                        options: gles, software, etc. (also opengl3.1, etc.)\n");
 	return 0;
@@ -282,6 +291,7 @@ CommandLineParseResult CommandLineOptions::Parse(int argc, const char *argv[], C
 	constexpr std::string_view gpuBackendStr = "--graphics=";
 	constexpr std::string_view configOption = "--config=";
 	constexpr std::string_view controlsOption = "--controlconfig=";
+	constexpr std::string_view logLevelOption = "--loglevel=";
 
 #ifdef _DEBUG
 	enableLogging = true;
@@ -411,6 +421,18 @@ CommandLineParseResult CommandLineOptions::Parse(int argc, const char *argv[], C
 			configFilename = std::string(argv[i] + configOption.size());
 		} else if (startsWith(argv[i], controlsOption)) {
 			controlsConfigFilename = std::string(argv[i] + controlsOption.size());
+		} else if (startsWith(argv[i], logLevelOption)) {
+			logLevel = static_cast<LogLevel>(atoi(argv[i] + logLevelOption.size()));
+		} else if (equals(argv[i], "--ignore")) {
+			// Headless: skip a specific test. Can be repeated, so not a regular auto-param.
+			if (i + 1 < argc) {
+				ignoredTests.emplace_back(argv[i + 1]);
+				i += 2;
+				continue;
+			} else {
+				PRINT_STDERR("Error: --ignore requires an argument.\n");
+				return CommandLineParseResult::Exit;
+			}
 		} else {
 			// Report unknown argument later once this is complete.
 		}

Core/CmdLine.h

@@ -95,4 +95,19 @@ struct CommandLineOptions {
 
 	std::optional<std::string> screenshotFilename;
 	std::optional<std::string> screenshotFilenameSave;
+
+	// Headless: mount an ISO/CSO on umd1:.
+	std::optional<std::string> mountIso;
+	// Headless: also log through OutputDebugString (Windows).
+	std::optional<bool> odsLog;
+	// Headless: maximum allowed MSE error for screenshot comparison.
+	std::optional<double> maxScreenshotError;
+	// Headless: test names to skip. May be specified more than once.
+	std::vector<std::string> ignoredTests;
+
+	// SDL only.
+	std::optional<int> xres;
+	std::optional<int> yres;
+	std::optional<double> dpi;
+	std::optional<double> scale;
 };

SDL/SDLMain.cpp

@@ -1237,29 +1237,6 @@ void System_Notify(SystemNotification notification) {
 bool System_SendDebugOutput(std::string_view data) { return false; }
 void System_SendDebugScreenshot(const uint8_t *data, int width, int height) {}
 
-// returns -1 on failure
-static int parseInt(const char *str) {
-	int val;
-	int retval = sscanf(str, "%d", &val);
-	fprintf(stderr, "%i = scanf %s\n", retval, str);
-	if (retval != 1) {
-		return -1;
-	} else {
-		return val;
-	}
-}
-
-static float parseFloat(const char *str) {
-	float val;
-	int retval = sscanf(str, "%f", &val);
-	fprintf(stderr, "%i = sscanf %s\n", retval, str);
-	if (retval != 1) {
-		return -1.0f;
-	} else {
-		return val;
-	}
-}
-
 void UpdateWindowState(SDL_Window *window) {
 	SDL_SetWindowTitle(window, g_windowState.title.c_str());
 	if (g_windowState.applyFullScreenNextFrame) {
@@ -1856,43 +1833,12 @@ int main(int argc, char *argv[]) {
 
 	const int compiled = SDL_VERSION;
 	const int linked = SDL_GetVersion();
-	int set_xres = -1;
-	int set_yres = -1;
-	float set_dpi = 0.0f;
-	float set_scale = 1.0f;
-
-	// Produce a new set of arguments with the ones we skip.
-	int remain_argc = 1;
-	const char *remain_argv[256] = { argv[0] };
-	constexpr int remain_argv_cap = (int)(sizeof(remain_argv) / sizeof(remain_argv[0]));
+	int set_xres = cmdLineOptions.xres.value_or(-1);
+	int set_yres = cmdLineOptions.yres.value_or(-1);
+	float set_dpi = (float)cmdLineOptions.dpi.value_or(0.0);
+	float set_scale = (float)cmdLineOptions.scale.value_or(1.0);
 
 	Uint32 mode = 0;
-	for (int i = 1; i < argc; i++) {
-		if (set_xres == -2)
-			set_xres = parseInt(argv[i]);
-		else if (set_yres == -2)
-			set_yres = parseInt(argv[i]);
-		else if (set_dpi == -2)
-			set_dpi = parseFloat(argv[i]);
-		else if (set_scale == -2)
-			set_scale = parseFloat(argv[i]);
-		else if (!strcmp(argv[i], "--xres"))
-			set_xres = -2;
-		else if (!strcmp(argv[i], "--yres"))
-			set_yres = -2;
-		else if (!strcmp(argv[i], "--dpi"))
-			set_dpi = -2;
-		else if (!strcmp(argv[i], "--scale"))
-			set_scale = -2;
-		else {
-			if (remain_argc < remain_argv_cap - 1) {
-				remain_argv[remain_argc++] = argv[i];
-			} else {
-				fprintf(stderr, "Too many command-line arguments, ignoring: %s\n", argv[i]);
-			}
-		}
-	}
-	remain_argv[remain_argc] = nullptr;
 
 	std::string app_name;
 	std::string app_name_nice;
@@ -2013,7 +1959,7 @@ int main(int argc, char *argv[]) {
 
 	// After NativeInit, code should no longer look at cmdLineOptions, they should have been translated
 	// into g_Config settings. This is because NativeInit may modify g_Config settings based on the command line options.
-	NativeInit(remain_argc, (const char **)remain_argv, cmdLineOptions, path, external_dir, nullptr);
+	NativeInit(argc, (const char **)argv, cmdLineOptions, path, external_dir, nullptr);
 
 	// Use the setting from the config when initing the window.
 	if (g_Config.bFullScreen) {

UI/NativeApp.cpp

@@ -583,41 +583,6 @@ void NativeInit(int argc, const char *argv[], const CommandLineOptions &cmdLineO
 		boot_filename = g_Config.flash0Directory / "vsh/module/vshmain.prx";
 	}
 
-	// Parse command line
-	LogLevel logLevel = LogLevel::LINFO;
-	bool forceLogLevel = false;
-	const auto setLogLevel = [&logLevel, &forceLogLevel](LogLevel level) {
-		logLevel = level;
-		forceLogLevel = true;
-	};
-
-	if (cmdLineOptions.logLevel.has_value()) {
-		setLogLevel(cmdLineOptions.logLevel.value());
-	}
-
-	std::string fileToLog;
-	for (int i = 1; i < argc; i++) {
-		if (argv[i][0] == '-') {
-#if defined(__APPLE__)
-			// On Apple system debugged executable may get -NSDocumentRevisionsDebugMode YES in argv.
-			if (!strcmp(argv[i], "-NSDocumentRevisionsDebugMode") && argc - 1 > i) {
-				i++;
-				continue;
-			}
-#endif
-			switch (argv[i][1]) {
-			case '-':
-				if (!strncmp(argv[i], "--loglevel=", strlen("--loglevel=")) && strlen(argv[i]) > strlen("--loglevel="))
-					setLogLevel(static_cast<LogLevel>(std::atoi(argv[i] + strlen("--loglevel="))));
-				if (!strncmp(argv[i], "--log=", strlen("--log=")) && strlen(argv[i]) > strlen("--log="))
-					fileToLog = argv[i] + strlen("--log=");
-				break;
-			}
-		} else {
-			// Ignore. Boot filename is extracted in the previous step.
-		}
-	}
-
 	if (cmdLineOptions.appendConfig.has_value()) {
 		g_Config.SetAppendedConfigIni(Path(cmdLineOptions.appendConfig.value()));
 		g_Config.LoadAppendedConfig();
@@ -671,20 +636,15 @@ void NativeInit(int argc, const char *argv[], const CommandLineOptions &cmdLineO
 		}
 	}
 
-	if (!fileToLog.empty()) {
+	if (cmdLineOptions.log.has_value() && !cmdLineOptions.log.value().empty()) {
 		// Start logging immediately.
 		g_logManager.EnableOutput(LogOutput::File);
-		g_logManager.SetFileLogPath(Path(fileToLog));
+		g_logManager.SetFileLogPath(Path(cmdLineOptions.log.value()));
 	} else {
 		// Set a default file logging path, in case the user enables it with the checkbox later.
 		g_logManager.SetFileLogPath(GetSysDirectory(DIRECTORY_DUMP) / "log.txt");
 	}
 
-	if (forceLogLevel) {
-		NOTICE_LOG(Log::System, "Setting log level to %d due to command line override", (int)logLevel);
-		g_logManager.SetAllLogLevels(logLevel);
-	}
-
 	PostLoadConfig();
 
 #if PPSSPP_PLATFORM(ANDROID)

headless/Headless.cpp

@@ -129,14 +129,6 @@ std::string NativeLoadSecret(std::string_view nameOfSecret) {
 
 int printUsage(const CommandLineOptions &options, const char *progname, const char *reason) {
 	options.PrintUsage(progname, reason);
-	fprintf(stderr, "  -m, --mount umd.cso   mount iso on umd1:\n");
-	fprintf(stderr, "  -r, --root some/path  mount path on host0: (elfs must be in here)\n");
-	fprintf(stderr, "  -l, --log             full log output, not just emulated printfs\n");
-	fprintf(stderr, "                        options: gles, software, directx9, etc.\n");
-	fprintf(stderr, "  --screenshot=FILE     compare against a screenshot\n");
-	fprintf(stderr, "  --screenshot-save=FILE  save rendered screenshot to a BMP file\n");
-	fprintf(stderr, "  --max-mse=NUMBER      maximum allowed MSE error for screenshot\n");
-	fprintf(stderr, "  --timeout=SECONDS     abort test it if takes longer than SECONDS\n");
 	fprintf(stderr, "  --ir                  use ir interpreter\n");
 	return 1;
 }
@@ -549,17 +541,18 @@ int main(int argc, const char* argv[]) {
 	testOptions.timeout = cmdLineOptions.timeout.value_or(std::numeric_limits<double>::infinity());
 	testOptions.verbose = cmdLineOptions.verbose.value_or(false);
 	testOptions.printEqualLines = cmdLineOptions.printEqualLines.value_or(false);
+	testOptions.maxScreenshotError = cmdLineOptions.maxScreenshotError.value_or(0.0);
 
-	bool fullLog = false;
-	const char *stateToLoad = 0;
+	bool fullLog = cmdLineOptions.enableLogging.value_or(false);
+	const char *stateToLoad = cmdLineOptions.stateToLoad.has_value() ? cmdLineOptions.stateToLoad.value().c_str() : nullptr;
 	GPUCore gpuCore = GPUCORE_SOFTWARE;
 	CPUCore cpuCore = CPUCore::JIT;
 	bool oldAtrac = false;
-	bool outputDebugStringLog = false;
+	bool outputDebugStringLog = cmdLineOptions.odsLog.value_or(false);
 
 	std::vector<std::string> testFilenames;
-	std::vector<std::string> ignoredTests;
-	std::string mountIso;
+	const std::vector<std::string> &ignoredTests = cmdLineOptions.ignoredTests;
+	std::string mountIso = cmdLineOptions.mountIso.value_or("");
 	std::string mountRoot;
 
 	if (cmdLineOptions.cpuCore.has_value()) {
@@ -570,29 +563,6 @@ int main(int argc, const char* argv[]) {
 		mountRoot = cmdLineOptions.root.value().c_str();
 	}
 
-	for (int i = 1; i < argc; i++) {
-		if (!strcmp(argv[i], "-m") || !strcmp(argv[i], "--mount")) {
-			if (++i >= argc)
-				return printUsage(cmdLineOptions, argv[0], "Missing argument after -m");
-			mountIso = argv[i];
-		}
-		else if (!strcmp(argv[i], "-l") || !strcmp(argv[i], "--log"))
-			fullLog = true;
-		else if (!strcmp(argv[i], "-o") || !strcmp(argv[i], "--odslog"))
-			outputDebugStringLog = true;
-		else if (!strncmp(argv[i], "--max-mse=", strlen("--max-mse=")) && strlen(argv[i]) > strlen("--max-mse="))
-			testOptions.maxScreenshotError = strtod(argv[i] + strlen("--max-mse="), nullptr);
-		else if (!strncmp(argv[i], "--state=", strlen("--state=")) && strlen(argv[i]) > strlen("--state="))
-			stateToLoad = argv[i] + strlen("--state=");
-		else if (!strcmp(argv[i], "--help") || !strcmp(argv[i], "-h"))
-			return printUsage(cmdLineOptions, argv[0], NULL);
-		else if (!strcmp(argv[i], "--ignore")) {
-			if (++i >= argc)
-				return printUsage(cmdLineOptions, argv[0], "Missing argument after --ignore");
-			ignoredTests.push_back(argv[i]);
-		}
-	}
-
 	for (const std::string &filename : cmdLineOptions.bootFilenames) {
 		AddToTestsByPath(&testFilenames, filename);
 	}

unittest/UnitTest.cpp

@@ -1330,7 +1330,6 @@ bool TestCmdLine() {
 			"--fullscreen",
 			"--graphics=d3d11",
 			"--pause-menu-exit",
-			"--timeout=3",
 			"My_Game.iso"
 		};
 		int argc = ARRAY_SIZE(argv);
@@ -1344,9 +1343,20 @@ bool TestCmdLine() {
 		EXPECT_EQ_STR(options.bootFilenames[0], std::string("My_Game.iso"));
 		EXPECT_TRUE(options.gpuBackend.has_value());
 		EXPECT_EQ_INT((int)options.gpuBackend.value_or((GPUBackend)-1), (int)GPUBackend::DIRECT3D11);
-		EXPECT_EQ_INT(options.timeout.value_or(0), 3);
 		EXPECT_TRUE(options.pauseMenuExit.value_or(false));
 	}
+	// --timeout is headless-only (only headless/Headless.cpp reads it), so it must be parsed in Headless mode.
+	{
+		const char *argv[] = {
+			"ppsspp",
+			"--timeout=3",
+			"My_Game.iso"
+		};
+		int argc = ARRAY_SIZE(argv);
+		CommandLineOptions options;
+		options.Parse(argc, argv, CmdLineMode::Headless);
+		EXPECT_EQ_INT(options.timeout.value_or(0), 3);
+	}
 	// Test GL version override
 	{
 		const char *argv[] = {