From 5bd1ff8066cf47132c16230976a9f83ea3abd40e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 5 Aug 2026 01:43:24 +0200 Subject: [PATCH] Move remaining manual command line parsing into CmdLine.cpp Headless.cpp, NativeApp.cpp, and SDLMain.cpp each still hand-parsed a few argv flags directly (mount/log/state/ignore/loglevel in headless and the app, xres/yres/dpi/scale in SDL), duplicating and in some cases conflicting with the shared CommandLineOptions parser. Consolidate all of it into CmdLine.cpp/.h so there's a single source of truth, and drop the now-dead remain_argc/remain_argv filtering in SDLMain.cpp since NativeInit no longer reads argv itself. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PfFvWzpHxErWgRhKqqSewN --- AGENTS.md | 22 +++++++++++++++ Core/CmdLine.cpp | 44 +++++++++++++++++++++-------- Core/CmdLine.h | 15 ++++++++++ SDL/SDLMain.cpp | 64 ++++--------------------------------------- UI/NativeApp.cpp | 44 ++--------------------------- headless/Headless.cpp | 42 ++++------------------------ unittest/UnitTest.cpp | 14 ++++++++-- 7 files changed, 95 insertions(+), 150 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6a7b4f796d..2ced817935 100644 --- a/AGENTS.md +++ b/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//`. +## 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`) 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 diff --git a/Core/CmdLine.cpp b/Core/CmdLine.cpp index e7e04d344d..bbbb672cd8 100644 --- a/Core/CmdLine.cpp +++ b/Core/CmdLine.cpp @@ -1,3 +1,5 @@ +#include + #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(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. } diff --git a/Core/CmdLine.h b/Core/CmdLine.h index ddca876adc..4d94bbc5cb 100644 --- a/Core/CmdLine.h +++ b/Core/CmdLine.h @@ -95,4 +95,19 @@ struct CommandLineOptions { std::optional screenshotFilename; std::optional screenshotFilenameSave; + + // Headless: mount an ISO/CSO on umd1:. + std::optional mountIso; + // Headless: also log through OutputDebugString (Windows). + std::optional odsLog; + // Headless: maximum allowed MSE error for screenshot comparison. + std::optional maxScreenshotError; + // Headless: test names to skip. May be specified more than once. + std::vector ignoredTests; + + // SDL only. + std::optional xres; + std::optional yres; + std::optional dpi; + std::optional scale; }; diff --git a/SDL/SDLMain.cpp b/SDL/SDLMain.cpp index 4fc1998704..6c78abc5ba 100644 --- a/SDL/SDLMain.cpp +++ b/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) { diff --git a/UI/NativeApp.cpp b/UI/NativeApp.cpp index c1ae9917d1..5f9cfb7610 100644 --- a/UI/NativeApp.cpp +++ b/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(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) diff --git a/headless/Headless.cpp b/headless/Headless.cpp index 3d2c64f6d0..01ea6f6f49 100644 --- a/headless/Headless.cpp +++ b/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::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 testFilenames; - std::vector ignoredTests; - std::string mountIso; + const std::vector &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); } diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index fc61fdd6f1..b403f8c939 100644 --- a/unittest/UnitTest.cpp +++ b/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[] = {