diff --git a/.github/workflows/commit-validation.yml b/.github/workflows/commit-validation.yml index 89a29ada..1fefda4c 100644 --- a/.github/workflows/commit-validation.yml +++ b/.github/workflows/commit-validation.yml @@ -435,6 +435,7 @@ jobs: tools/tests/ui_capture_system_page.py \ tools/tests/ui_capture_system_exit.py \ tools/tests/renderer_vid_restart_route.py \ + tools/tests/cmd_args_copy.py \ tools/tests/renderer_image_recovery_owner.py \ tools/tests/renderer_image_recovery_module.py \ tools/tests/renderer_image_recovery_loader.py \ @@ -805,6 +806,7 @@ jobs: python tools/tests/ui_capture_system_page.py python tools/tests/ui_capture_system_exit.py python tools/tests/renderer_vid_restart_route.py + python tools/tests/cmd_args_copy.py python tools/tests/renderer_image_recovery_owner.py --mutations python tools/tests/renderer_image_recovery_module.py --mutations python tools/tests/renderer_image_recovery_loader.py --mutations diff --git a/.github/workflows/push-verification.yml b/.github/workflows/push-verification.yml index 7e3bc51b..d4763040 100644 --- a/.github/workflows/push-verification.yml +++ b/.github/workflows/push-verification.yml @@ -435,6 +435,7 @@ jobs: tools/tests/ui_capture_system_page.py \ tools/tests/ui_capture_system_exit.py \ tools/tests/renderer_vid_restart_route.py \ + tools/tests/cmd_args_copy.py \ tools/tests/renderer_image_recovery_owner.py \ tools/tests/renderer_image_recovery_module.py \ tools/tests/renderer_image_recovery_loader.py \ @@ -805,6 +806,7 @@ jobs: python tools/tests/ui_capture_system_page.py python tools/tests/ui_capture_system_exit.py python tools/tests/renderer_vid_restart_route.py + python tools/tests/cmd_args_copy.py python tools/tests/renderer_image_recovery_owner.py --mutations python tools/tests/renderer_image_recovery_module.py --mutations python tools/tests/renderer_image_recovery_loader.py --mutations diff --git a/docs/dev/release-completion.md b/docs/dev/release-completion.md index 07a0c5ff..d0e10398 100644 --- a/docs/dev/release-completion.md +++ b/docs/dev/release-completion.md @@ -10,6 +10,18 @@ completed release material here and publish curated notes in `releases/`. ## Post-0.14.000 corrections +- [ ] Run every startup command again (regression from f15a41f0). + `idList::Append` now copies its argument before growing, and `idCmdArgs` had + no copy constructor, so the implicit copy kept `argv` pointing into the + source and `operator=` turned that into a stray offset. The command that + landed on a growth of the startup queue (the 17th, 33rd, ... `+` command) + lost its text and was skipped silently. Dedicated-server smokes never ran + their `+quit` and timed out, the renderer startup matrix lost self-tests + (RenderGraphResource, ModernVisibility, the Vulkan render-target set), and + long client command lines dropped commands such as `+map`. `idCmdArgs` now + copy-constructs through `operator=`; `cmd_args_copy.py` checks copies and a + queue past several growths. + - [x] Complete the stage-one baseline implementation and audit: use a shared high-resolution presentation phase for each SP/MP draw and precise OpenXR smooth-turn deltas; restore imported game regression coverage; reconcile diff --git a/src/idlib/CmdArgs.h b/src/idlib/CmdArgs.h index 1b16868f..cb8376d6 100644 --- a/src/idlib/CmdArgs.h +++ b/src/idlib/CmdArgs.h @@ -14,6 +14,10 @@ class idCmdArgs { public: idCmdArgs( void ) { argc = 0; } idCmdArgs( const char *text, bool keepAsStrings ) { TokenizeString( text, keepAsStrings ); } + // argv points into this object's own tokenized buffer, so a copy + // must re-point it; the implicit member-wise copy would leave argv + // aimed at the source (idList::Append copies before it grows). + idCmdArgs( const idCmdArgs &args ) { argc = 0; *this = args; } void operator=( const idCmdArgs &args ); diff --git a/tools/tests/cmd_args_copy.py b/tools/tests/cmd_args_copy.py new file mode 100644 index 00000000..47125d21 --- /dev/null +++ b/tools/tests/cmd_args_copy.py @@ -0,0 +1,122 @@ +#!/usr/bin/env python3 +"""Copies of idCmdArgs must keep their arguments, including inside idList. + +idCmdArgs::argv points into the object's own tokenized buffer, which +operator= re-points. idList::Append copies its argument before growing the +list, so idCmdArgs needs a copy constructor that does the same: with the +implicit member-wise copy, the queued command that lands on a list growth +kept argv aimed at the caller's buffer, and operator= then turned that into +an unrelated offset. Startup commands are queued in an idList, so +the 17th "+" command on a command line silently lost its text -- the +dedicated-server smokes never ran their "+quit". + +This compiles the production idCmdArgs header with its operator= and +AppendArg bodies against idList, queues commands past several growths, and +checks every one -- and a copy-constructed one -- still reads back intact. +""" + +from pathlib import Path +import os +import shutil +import subprocess +import tempfile + +from filesystem_case_segments import function_body + +ROOT = Path(__file__).resolve().parents[2] + +SUPPORT = r''' +#include +#include +#include +#include +#define ID_INLINE inline +#define MAX_STRING_CHARS 1024 +struct idStr { + static void Copynz( char *dest, const char *src, int destsize ) { + std::strncpy( dest, src, destsize - 1 ); + dest[ destsize - 1 ] = 0; + } +}; +struct idLib { + static int SizeToInt( size_t value, const char * ) { return static_cast( value ); } +}; +#include "src/idlib/containers/List.h" +#include "src/idlib/CmdArgs.h" +''' + +MAIN = r''' +static idCmdArgs Command( int index ) { + idCmdArgs args; + args.AppendArg( "cmd" ); + args.AppendArg( std::to_string( index ).c_str() ); + return args; +} + +static bool Intact( const idCmdArgs &args, int index ) { + return args.Argc() == 2 && std::strcmp( args.Argv( 0 ), "cmd" ) == 0 && + std::strcmp( args.Argv( 1 ), std::to_string( index ).c_str() ) == 0; +} + +int main() { + int failures = 0; + const idCmdArgs source = Command( 7 ); + const idCmdArgs copied( source ); + if ( !Intact( copied, 7 ) ) { + std::fprintf( stderr, "copy-constructed idCmdArgs lost its arguments\n" ); + ++failures; + } + + // The startup queue: one Append per "+" command, growing every 16. + idList queue; + for ( int i = 0; i < 40; ++i ) { + const idCmdArgs args = Command( i ); + queue.Append( args ); + } + for ( int i = 0; i < 40; ++i ) { + if ( !Intact( queue[ i ], i ) ) { + std::fprintf( stderr, "queued command %d lost its arguments\n", i ); + ++failures; + } + } + // Drain it the way ExecuteCommandBuffer does. + for ( int i = 0; i < 40; ++i ) { + idCmdArgs next; + next = queue[ 0 ]; + queue.RemoveIndex( 0 ); + if ( !Intact( next, i ) ) { + std::fprintf( stderr, "drained command %d lost its arguments\n", i ); + ++failures; + } + } + return failures ? 1 : 0; +} +''' + + +def main() -> None: + header = (ROOT / "src/idlib/CmdArgs.h").read_text(encoding="utf-8") + if "idCmdArgs( const idCmdArgs &args ) { argc = 0; *this = args; }" not in header: + raise AssertionError("idCmdArgs must copy-construct through operator= so argv is re-pointed") + source = (ROOT / "src/idlib/CmdArgs.cpp").read_text(encoding="utf-8") + code = SUPPORT + "\n".join(function_body(source, signature) for signature in ( + "void idCmdArgs::operator=( const idCmdArgs &args )", + "void idCmdArgs::AppendArg( const char *text )", + )) + MAIN + + compiler = next((found for name in ("clang++", "g++", "c++") if (found := shutil.which(name))), None) + if not compiler: + raise RuntimeError("C++ compiler required") + (ROOT / ".tmp").mkdir(exist_ok=True) + with tempfile.TemporaryDirectory(prefix="cmd-args-copy-", dir=ROOT / ".tmp") as temp: + env = dict(os.environ, TEMP=temp, TMP=temp) + test_source = Path(temp) / "cmd_args_copy.cpp" + test_source.write_text(code, encoding="utf-8") + binary = Path(temp) / "cmd_args_copy.exe" + subprocess.run([compiler, "-std=c++17", "-I", str(ROOT), str(test_source), "-o", str(binary)], check=True, env=env) + subprocess.run([str(binary)], check=True, env=env) + print("cmd_args_copy: ok") + + +if __name__ == "__main__": + main() diff --git a/tools/validation/openq4_validate.py b/tools/validation/openq4_validate.py index b7b756da..9470aa20 100644 --- a/tools/validation/openq4_validate.py +++ b/tools/validation/openq4_validate.py @@ -729,6 +729,7 @@ def run_python_tests(args: argparse.Namespace, root: Path, env: dict[str, str]) (root / "tools" / "tests" / 'ui_capture_system_page.py', []), (root / "tools" / "tests" / 'ui_capture_system_exit.py', []), (root / "tools" / "tests" / 'renderer_vid_restart_route.py', []), + (root / "tools" / "tests" / 'cmd_args_copy.py', []), (root / "tools" / "tests" / 'renderer_image_recovery_owner.py', ['--mutations']), (root / "tools" / "tests" / 'renderer_image_recovery_module.py', ['--mutations']), (root / "tools" / "tests" / 'renderer_image_recovery_loader.py', ['--mutations']),