From 714713be8dabab0f4ca51b6e0100c52a4f5f2b26 Mon Sep 17 00:00:00 2001 From: Alan Russell Date: Thu, 22 Oct 2026 21:22:11 +0100 Subject: [PATCH] fix(ncs): push engine call arguments last-first (#351) Since 7c73bbb64 (merged to master via #49) the built-in NSS compiler pushed the arguments of engine routine calls first-argument-first. The NWScript VM, the original compiler and every compiled game script expect the last argument to be pushed first, so that the first argument ends up on top of the stack. Compiled scripts therefore passed swapped arguments or aborted the VM with a type error, and omitted default arguments were pushed after the supplied ones. TestNCSCompiler did not notice because it runs the compiled output through PyKotor's own Interpreter, which got a compensating change in the same commit. - EngineCallExpression.compile: iterate the arguments from last to first (param_index still indexes function.params); replace the comment that described the wrong order. - Interpreter.do_action: the first parameter is on top of the stack, so pop in parameter order and drop the args_snap.reverse() workaround. - Add byte-level regression tests next to TestNCSCompiler that compare compile_nss output with hand-assembled NCS bytes (no Interpreter involved): SetGlobalNumber("A", 7), a 3-argument call with mixed types, a call with omitted default arguments, and a call with an action argument. User-defined function calls (compile_jsr) are not changed. --- .../resource/formats/ncs/compiler/classes.py | 20 ++- .../formats/ncs/compiler/interpreter.py | 22 +--- .../tests/resource/formats/test_ncs.py | 120 ++++++++++++++++++ 3 files changed, 133 insertions(+), 29 deletions(-) diff --git a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py index a335ae384..6b5b60cde 100644 --- a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py +++ b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py @@ -1303,18 +1303,14 @@ def compile( this_stack = 0 # DEBUG: Log arguments before compilation - # Compile arguments in FORWARD order (left to right, first argument first) - # NCS bytecode pushes arguments left-to-right, so when the interpreter - # pops them (last-in-first-out), args_snap has them in reverse order. - # The interpreter then reverses args_snap to match function.params order. - # - Compile in forward: push fFloat (first param, bottom), push nWidth, push nDecimals (last param, top) - # - Stack: [fFloat, nWidth, nDecimals] (nDecimals on top) - # - Interpreter pops: nDecimals -> args_snap[0], nWidth -> args_snap[1], fFloat -> args_snap[2] - # - Before reverse: args_snap = [nDecimals, nWidth, fFloat] - # - After reverse: args_snap = [fFloat, nWidth, nDecimals] - CORRECT! - for i in range(len(self._args)): # Iterate in forward order - arg = self._args[i] - param_index = i # Parameter index in forward order + # Push the arguments LAST-first: the last parameter goes onto the stack first + # and the first parameter ends up on top, which is the order the NWScript VM + # (and the original compiler) expects for every engine action. Omitted + # trailing arguments have been filled with their defaults above, so they are + # pushed before the explicit ones. + # Example: SetGlobalNumber("A", 7) -> CONSTI 7, CONSTS "A", ACTION. + for param_index in range(len(self._args) - 1, -1, -1): + arg = self._args[param_index] param_type = DynamicDataType(self._function.params[param_index].datatype) if param_type == DataType.ACTION: after_command = NCSInstruction() diff --git a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py index 5cc5b1b9c..c167050ae 100644 --- a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py +++ b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py @@ -427,9 +427,9 @@ def do_action(self, function: ScriptFunction, args: int): # noqa: C901, PLR0912 args_snap = [] - # Pop arguments from stack in reverse order (last param popped first) - for i in range(args): - param_index = args - 1 - i # Reverse order + # The compiler pushes the last parameter first, so the first parameter is on top + # of the stack: pop in parameter order. + for param_index in range(args): if param_index >= len(function.params): msg = f"Action '{function.name}' parameter index {param_index} out of range" raise ValueError(msg) @@ -456,20 +456,8 @@ def do_action(self, function: ScriptFunction, args: int): # noqa: C901, PLR0912 msg = f"Stack underflow while popping argument for '{function.name}'" raise RuntimeError(msg) from e - # DEBUG: Log args_snap before reverse - print( - f"DEBUG: args_snap before reverse: {[f'{obj.data_type.name}={obj.value}' for obj in args_snap]}" - ) - - # We compiled arguments in reverse order (last first), so when we pop them (last-in-first-out), - # args_snap[0] is the last parameter and args_snap[-1] is the first parameter. - # We need to reverse to match function.params order (first parameter at index 0). - args_snap.reverse() - - # DEBUG: Log args_snap after reverse and function params - print( - f"DEBUG: args_snap after reverse: {[f'{obj.data_type.name}={obj.value}' for obj in args_snap]}" - ) + # DEBUG: Log args_snap (already in function.params order) and function params + print(f"DEBUG: args_snap: {[f'{obj.data_type.name}={obj.value}' for obj in args_snap]}") print(f"DEBUG: function.params: {[(p.name, p.datatype.name) for p in function.params]}") # Validate argument types diff --git a/Libraries/PyKotor/tests/resource/formats/test_ncs.py b/Libraries/PyKotor/tests/resource/formats/test_ncs.py index e8c4ed368..373bdab32 100644 --- a/Libraries/PyKotor/tests/resource/formats/test_ncs.py +++ b/Libraries/PyKotor/tests/resource/formats/test_ncs.py @@ -4,6 +4,7 @@ import importlib.util import os import pathlib +import struct import sys import tempfile import textwrap @@ -77,6 +78,40 @@ def _ncs_dencs_available() -> bool: _TSL_NCS_TEST_FILES_DIR = Path(__file__).resolve().parents[3] / "test_files" +def _hand_assembled_ncs(*instructions: bytes) -> bytes: + """Assemble an NCS file from raw instruction bytes, independent of PyKotor's writer. + + Layout: "NCS V1.0", the 0x42 size marker, the total file size as a big-endian uint32, + then the instructions. Every program starts with ``JSR main; RETN`` (``main`` follows at +8). + """ + body = b"".join(instructions) + return b"NCS V1.0" + bytes([0x42]) + (13 + len(body)).to_bytes(4, "big") + body + + +_JSR_MAIN = bytes([0x1E, 0x00]) + (8).to_bytes(4, "big", signed=True) +_RETN = bytes([0x20, 0x00]) + + +def _consti(value: int) -> bytes: + return bytes([0x04, 0x03]) + value.to_bytes(4, "big", signed=True) + + +def _constf(value: float) -> bytes: + return bytes([0x04, 0x04]) + struct.pack(">f", value) + + +def _consts(value: str) -> bytes: + return bytes([0x04, 0x05]) + len(value).to_bytes(2, "big") + value.encode("ascii") + + +def _consto(value: int) -> bytes: + return bytes([0x04, 0x06]) + value.to_bytes(4, "big", signed=True) + + +def _action(routine_id: int, argc: int) -> bytes: + return bytes([0x05, 0x00]) + routine_id.to_bytes(2, "big") + bytes([argc]) + + # ============================================================================ # Test Base Classes # ============================================================================ @@ -299,6 +334,91 @@ def test_enginecall_GetFactionEqual(self): """ ) + def test_enginecall_argument_order_bytes_set_global_number(self): + """Engine call arguments are pushed last-first, so the first argument is on top of the stack. + + NSS ``SetGlobalNumber("A", 7)`` must assemble to CONSTI 7, CONSTS "A", ACTION 581 (2 args). + The expected bytes are hand-assembled; the Interpreter is deliberately not involved, because it + used to carry a compensating bug that hid a swapped argument order. + """ + actual = bytes_ncs(compile_nss('void main() { SetGlobalNumber("A", 7); }', Game.K1)) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + _consti(7), # nValue, the last parameter, is pushed first + _consts("A"), # sIdentifier, the first parameter, ends up on top + _action(581, 2), # SetGlobalNumber + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + + def test_enginecall_argument_order_bytes_three_mixed_types(self): + """A three-argument call with object, int and float parameters pushes float, int, object.""" + actual = bytes_ncs( + compile_nss("void main() { SWMG_SetGunBankSpeed(OBJECT_SELF, 2, 1.5); }", Game.K1) + ) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + _constf(1.5), # fSpeed + _consti(2), # nGunBank + _consto(0), # oVehicle = OBJECT_SELF (on top) + _action(637, 3), # SWMG_SetGunBankSpeed + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + + def test_enginecall_argument_order_bytes_omitted_defaults(self): + """Omitted trailing arguments take their defaults and are pushed before the explicit ones. + + ``ActionMoveToObject(object oMoveTo, int bRun=FALSE, float fRange=1.0)`` called with only the + object pushes fRange (1.0), bRun (0), then oMoveTo. + """ + actual = bytes_ncs(compile_nss("void main() { ActionMoveToObject(OBJECT_SELF); }", Game.K1)) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + _constf(1.0), # default fRange + _consti(0), # default bRun + _consto(0), # oMoveTo = OBJECT_SELF (on top) + _action(22, 3), # ActionMoveToObject + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + + def test_enginecall_argument_order_bytes_action_argument(self): + """An action argument is pushed (as STORE_STATE + JMP + body + RETN) before the preceding arguments. + + ``DelayCommand(float fSeconds, action aActionToDelay)`` therefore emits the stored action first, + then the float, then ACTION 7. The body of the action is itself a last-first engine call. + """ + actual = bytes_ncs( + compile_nss('void main() { DelayCommand(1.0, SetGlobalNumber("A", 7)); }', Game.K1) + ) + action_body = b"".join( + [ + _consti(7), + _consts("A"), + _action(581, 2), # SetGlobalNumber + _RETN, + ] + ) + store_state = bytes([0x2C, 0x10]) + (0).to_bytes(4, "big") + (0).to_bytes(4, "big") + jmp_past_action = bytes([0x1D, 0x00]) + (6 + len(action_body)).to_bytes( + 4, "big", signed=True + ) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + store_state, # aActionToDelay, the last parameter, is pushed first + jmp_past_action, + action_body, + _constf(1.0), # fSeconds, the first parameter, ends up on top + _action(7, 2), # DelayCommand + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + # endregion # region Arithmetic Operator Tests