From 9f2fff4f6614cd2ab11cf0d236ac553319e94702 Mon Sep 17 00:00:00 2001 From: Willem Jan Palenstijn Date: Fri, 1 Apr 2016 22:54:54 +0200 Subject: SCI: Remove unneeded copy --- engines/sci/engine/workarounds.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'engines/sci') diff --git a/engines/sci/engine/workarounds.cpp b/engines/sci/engine/workarounds.cpp index 073bb93983..2a2540b470 100644 --- a/engines/sci/engine/workarounds.cpp +++ b/engines/sci/engine/workarounds.cpp @@ -780,7 +780,7 @@ SciWorkaroundSolution trackOriginAndFindWorkaround(int index, const SciWorkaroun Common::List::const_iterator callIterator = state->_executionStack.end(); while (callIterator != state->_executionStack.begin()) { callIterator--; - ExecStack loopCall = *callIterator; + const ExecStack &loopCall = *callIterator; if ((loopCall.debugSelector != -1) || (loopCall.debugExportId != -1)) { lastCall->debugSelector = loopCall.debugSelector; lastCall->debugExportId = loopCall.debugExportId; -- cgit v1.2.3 From 08f1727b0813c9e20a2462402f58c4578c5902c8 Mon Sep 17 00:00:00 2001 From: Willem Jan Palenstijn Date: Sat, 2 Apr 2016 00:04:00 +0200 Subject: SCI: Improve kernel subfunction logging ExecStack now stores the kernel call number as well as the subfunction. This allows kStub and backtraces to log the actual subfunction called. The kernel call number in ExecStack used to be stored in the debugSelector field. It now has its own field, to avoid confusion. --- engines/sci/console.cpp | 5 ++++- engines/sci/engine/kernel.cpp | 14 ++++++++++++++ engines/sci/engine/kernel.h | 1 + engines/sci/engine/kmisc.cpp | 20 +++++++++++++++----- engines/sci/engine/vm.cpp | 14 +++++++------- engines/sci/engine/vm.h | 13 +++++++++---- 6 files changed, 50 insertions(+), 17 deletions(-) (limited to 'engines/sci') diff --git a/engines/sci/console.cpp b/engines/sci/console.cpp index 27ac4fac49..2c74fe4b0c 100644 --- a/engines/sci/console.cpp +++ b/engines/sci/console.cpp @@ -3094,7 +3094,10 @@ bool Console::cmdBacktrace(int argc, const char **argv) { break; case EXEC_STACK_TYPE_KERNEL: // Kernel function - debugPrintf(" %x:[%x] k%s(", i, call.debugOrigin, _engine->getKernel()->getKernelName(call.debugSelector).c_str()); + if (call.debugKernelSubFunction == -1) + debugPrintf(" %x:[%x] k%s(", i, call.debugOrigin, _engine->getKernel()->getKernelName(call.debugKernelFunction).c_str()); + else + debugPrintf(" %x:[%x] k%s(", i, call.debugOrigin, _engine->getKernel()->getKernelName(call.debugKernelFunction, call.debugKernelSubFunction).c_str()); break; case EXEC_STACK_TYPE_VARSELECTOR: diff --git a/engines/sci/engine/kernel.cpp b/engines/sci/engine/kernel.cpp index 796dea2382..4370e6de04 100644 --- a/engines/sci/engine/kernel.cpp +++ b/engines/sci/engine/kernel.cpp @@ -91,6 +91,20 @@ const Common::String &Kernel::getKernelName(uint number) const { return _kernelNames[number]; } +Common::String Kernel::getKernelName(uint number, uint subFunction) const { + // FIXME: The following check is a temporary workaround for an issue + // leading to crashes when using the debugger's backtrace command. + if (number >= _kernelFuncs.size()) + return _invalid; + const KernelFunction &kernelCall = _kernelFuncs[number]; + + if (subFunction >= kernelCall.subFunctionCount) + return _invalid; + + return kernelCall.subFunctions[subFunction].name; +} + + int Kernel::findKernelFuncPos(Common::String kernelFuncName) { for (uint32 i = 0; i < _kernelNames.size(); i++) if (_kernelNames[i] == kernelFuncName) diff --git a/engines/sci/engine/kernel.h b/engines/sci/engine/kernel.h index 1202982986..d95e228045 100644 --- a/engines/sci/engine/kernel.h +++ b/engines/sci/engine/kernel.h @@ -158,6 +158,7 @@ public: uint getKernelNamesSize() const; const Common::String &getKernelName(uint number) const; + Common::String getKernelName(uint number, uint subFunction) const; /** * Determines the selector ID of a selector by its name. diff --git a/engines/sci/engine/kmisc.cpp b/engines/sci/engine/kmisc.cpp index 0a5f26476f..065625f85f 100644 --- a/engines/sci/engine/kmisc.cpp +++ b/engines/sci/engine/kmisc.cpp @@ -605,18 +605,28 @@ reg_t kEmpty(EngineState *s, int argc, reg_t *argv) { reg_t kStub(EngineState *s, int argc, reg_t *argv) { Kernel *kernel = g_sci->getKernel(); int kernelCallNr = -1; + int kernelSubCallNr = -1; Common::List::const_iterator callIterator = s->_executionStack.end(); if (callIterator != s->_executionStack.begin()) { callIterator--; ExecStack lastCall = *callIterator; - kernelCallNr = lastCall.debugSelector; + kernelCallNr = lastCall.debugKernelFunction; + kernelSubCallNr = lastCall.debugKernelSubFunction; } - Common::String warningMsg = "Dummy function k" + kernel->getKernelName(kernelCallNr) + - Common::String::format("[%x]", kernelCallNr) + - " invoked. Params: " + - Common::String::format("%d", argc) + " ("; + Common::String warningMsg; + if (kernelSubCallNr == -1) { + warningMsg = "Dummy function k" + kernel->getKernelName(kernelCallNr) + + Common::String::format("[%x]", kernelCallNr); + } else { + warningMsg = "Dummy function k" + kernel->getKernelName(kernelCallNr, kernelSubCallNr) + + Common::String::format("[%x:%x]", kernelCallNr, kernelSubCallNr); + + } + + warningMsg += " invoked. Params: " + + Common::String::format("%d", argc) + " ("; for (int i = 0; i < argc; i++) { warningMsg += Common::String::format("%04x:%04x", PRINT_REG(argv[i])); diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp index 64e6c045db..7e708efc8b 100644 --- a/engines/sci/engine/vm.cpp +++ b/engines/sci/engine/vm.cpp @@ -240,7 +240,7 @@ ExecStack *execute_method(EngineState *s, uint16 script, uint16 pubfunct, StackP g_sci->checkExportBreakpoint(script, pubfunct); ExecStack xstack(calling_obj, calling_obj, sp, argc, argp, - seg, make_reg32(seg, exportAddr), -1, pubfunct, -1, + seg, make_reg32(seg, exportAddr), -1, -1, -1, pubfunct, -1, s->_executionStack.size() - 1, EXEC_STACK_TYPE_CALL); s->_executionStack.push_back(xstack); return &(s->_executionStack.back()); @@ -313,7 +313,7 @@ ExecStack *send_selector(EngineState *s, reg_t send_obj, reg_t work_obj, StackPt debugSelectorCall(send_obj, selector, argc, argp, varp, funcp, s->_segMan, selectorType); ExecStack xstack(work_obj, send_obj, curSP, argc, argp, - 0xFFFF, curFP, selector, -1, -1, + 0xFFFF, curFP, selector, -1, -1, -1, -1, origin, stackType); if (selectorType == kSelectorVariable) @@ -335,12 +335,12 @@ ExecStack *send_selector(EngineState *s, reg_t send_obj, reg_t work_obj, StackPt return s->_executionStack.empty() ? NULL : &(s->_executionStack.back()); } -static void addKernelCallToExecStack(EngineState *s, int kernelCallNr, int argc, reg_t *argv) { +static void addKernelCallToExecStack(EngineState *s, int kernelCallNr, int kernelSubCallNr, int argc, reg_t *argv) { // Add stack frame to indicate we're executing a callk. // This is useful in debugger backtraces if this // kernel function calls a script itself. ExecStack xstack(NULL_REG, NULL_REG, NULL, argc, argv - 1, 0xFFFF, make_reg32(0, 0), - kernelCallNr, -1, -1, s->_executionStack.size() - 1, EXEC_STACK_TYPE_KERNEL); + -1, kernelCallNr, kernelSubCallNr, -1, -1, s->_executionStack.size() - 1, EXEC_STACK_TYPE_KERNEL); s->_executionStack.push_back(xstack); } @@ -386,7 +386,7 @@ static void callKernelFunc(EngineState *s, int kernelCallNr, int argc) { // Call kernel function if (!kernelCall.subFunctionCount) { - addKernelCallToExecStack(s, kernelCallNr, argc, argv); + addKernelCallToExecStack(s, kernelCallNr, -1, argc, argv); s->r_acc = kernelCall.function(s, argc, argv); if (kernelCall.debugLogging) @@ -444,7 +444,7 @@ static void callKernelFunc(EngineState *s, int kernelCallNr, int argc) { } if (!kernelSubCall.function) error("[VM] k%s: subfunction ID %d requested, but not available", kernelCall.name, subId); - addKernelCallToExecStack(s, kernelCallNr, argc, argv); + addKernelCallToExecStack(s, kernelCallNr, subId, argc, argv); s->r_acc = kernelSubCall.function(s, argc, argv); if (kernelSubCall.debugLogging) @@ -841,7 +841,7 @@ void run_vm(EngineState *s) { ExecStack xstack(s->xs->objp, s->xs->objp, s->xs->sp, (call_base->requireUint16()) + s->r_rest, call_base, s->xs->local_segment, make_reg32(s->xs->addr.pc.getSegment(), localCallOffset), - NULL_SELECTOR, -1, localCallOffset, s->_executionStack.size() - 1, + NULL_SELECTOR, -1, -1, -1, localCallOffset, s->_executionStack.size() - 1, EXEC_STACK_TYPE_CALL); s->_executionStack.push_back(xstack); diff --git a/engines/sci/engine/vm.h b/engines/sci/engine/vm.h index 514bf58b64..dc789b6201 100644 --- a/engines/sci/engine/vm.h +++ b/engines/sci/engine/vm.h @@ -93,16 +93,19 @@ struct ExecStack { SegmentId local_segment; // local variables etc - Selector debugSelector; // The selector which was used to call or -1 if not applicable - int debugExportId; // The exportId which was called or -1 if not applicable - int debugLocalCallOffset; // Local call offset or -1 if not applicable - int debugOrigin; // The stack frame position the call was made from, or -1 if it was the initial call + Selector debugSelector; // The selector which was used to call or -1 if not applicable + int debugExportId; // The exportId which was called or -1 if not applicable + int debugLocalCallOffset; // Local call offset or -1 if not applicable + int debugOrigin; // The stack frame position the call was made from, or -1 if it was the initial call + int debugKernelFunction; // The kernel function called, or -1 if not applicable + int debugKernelSubFunction; // The kernel subfunction called, or -1 if not applicable ExecStackType type; reg_t* getVarPointer(SegManager *segMan) const; ExecStack(reg_t objp_, reg_t sendp_, StackPtr sp_, int argc_, StackPtr argp_, SegmentId localsSegment_, reg32_t pc_, Selector debugSelector_, + int debugKernelFunction_, int debugKernelSubFunction_, int debugExportId_, int debugLocalCallOffset_, int debugOrigin_, ExecStackType type_) { objp = objp_; @@ -118,6 +121,8 @@ struct ExecStack { else local_segment = pc_.getSegment(); debugSelector = debugSelector_; + debugKernelFunction = debugKernelFunction_; + debugKernelSubFunction = debugKernelSubFunction_; debugExportId = debugExportId_; debugLocalCallOffset = debugLocalCallOffset_; debugOrigin = debugOrigin_; -- cgit v1.2.3 From 7f12638763e9732bc75aab4823c43ebdf8a8c2be Mon Sep 17 00:00:00 2001 From: Willem Jan Palenstijn Date: Fri, 1 Apr 2016 23:55:09 +0200 Subject: SCI: Remove unclear &rest handling Modifying a value above the stack pointer doesn't seem to make much sense. This was added in FreeSCI back in 2002 in a pair of commits that did not make clear what the purpose of this was. My guess is that it attempted to adjust argc, but failed. This wouldn't have been noticed since argc was always set correctly by make_exec_stack_entry (which is now the ExecStack constructor). --- engines/sci/engine/vm.cpp | 1 - 1 file changed, 1 deletion(-) (limited to 'engines/sci') diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp index 7e708efc8b..273492ccc6 100644 --- a/engines/sci/engine/vm.cpp +++ b/engines/sci/engine/vm.cpp @@ -834,7 +834,6 @@ void run_vm(EngineState *s) { int argc = (opparams[1] >> 1) // Given as offset, but we need count + 1 + s->r_rest; StackPtr call_base = s->xs->sp - argc; - s->xs->sp[1].incOffset(s->r_rest); uint32 localCallOffset = s->xs->addr.pc.getOffset() + opparams[0]; -- cgit v1.2.3 From d643cb651f5661a80c7d7ea493ff397cf0bef3fd Mon Sep 17 00:00:00 2001 From: Willem Jan Palenstijn Date: Fri, 1 Apr 2016 23:54:00 +0200 Subject: SCI: Remove unexpected side effect from ExecStack constructor The ExecStack constructor set argp[0] to argc before. This is now moved to the caller, to make this action more explicit. --- engines/sci/engine/vm.cpp | 8 +++++++- engines/sci/engine/vm.h | 1 - 2 files changed, 7 insertions(+), 2 deletions(-) (limited to 'engines/sci') diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp index 273492ccc6..3e12084ed6 100644 --- a/engines/sci/engine/vm.cpp +++ b/engines/sci/engine/vm.cpp @@ -239,6 +239,7 @@ ExecStack *execute_method(EngineState *s, uint16 script, uint16 pubfunct, StackP // Check if a breakpoint is set on this method g_sci->checkExportBreakpoint(script, pubfunct); + assert(argp[0].toUint16() == argc); // The first argument is argc ExecStack xstack(calling_obj, calling_obj, sp, argc, argp, seg, make_reg32(seg, exportAddr), -1, -1, -1, pubfunct, -1, s->_executionStack.size() - 1, EXEC_STACK_TYPE_CALL); @@ -312,6 +313,7 @@ ExecStack *send_selector(EngineState *s, reg_t send_obj, reg_t work_obj, StackPt if (activeBreakpointTypes || DebugMan.isDebugChannelEnabled(kDebugLevelScripts)) debugSelectorCall(send_obj, selector, argc, argp, varp, funcp, s->_segMan, selectorType); + assert(argp[0].toUint16() == argc); // The first argument is argc ExecStack xstack(work_obj, send_obj, curSP, argc, argp, 0xFFFF, curFP, selector, -1, -1, -1, -1, origin, stackType); @@ -386,6 +388,7 @@ static void callKernelFunc(EngineState *s, int kernelCallNr, int argc) { // Call kernel function if (!kernelCall.subFunctionCount) { + argv[-1] = make_reg(0, argc); // The first argument is argc addKernelCallToExecStack(s, kernelCallNr, -1, argc, argv); s->r_acc = kernelCall.function(s, argc, argv); @@ -444,6 +447,7 @@ static void callKernelFunc(EngineState *s, int kernelCallNr, int argc) { } if (!kernelSubCall.function) error("[VM] k%s: subfunction ID %d requested, but not available", kernelCall.name, subId); + argv[-1] = make_reg(0, argc); // The first argument is argc addKernelCallToExecStack(s, kernelCallNr, subId, argc, argv); s->r_acc = kernelSubCall.function(s, argc, argv); @@ -837,8 +841,10 @@ void run_vm(EngineState *s) { uint32 localCallOffset = s->xs->addr.pc.getOffset() + opparams[0]; + int final_argc = (call_base->requireUint16()) + s->r_rest; + call_base[0] = make_reg(0, final_argc); // The first argument is argc ExecStack xstack(s->xs->objp, s->xs->objp, s->xs->sp, - (call_base->requireUint16()) + s->r_rest, call_base, + final_argc, call_base, s->xs->local_segment, make_reg32(s->xs->addr.pc.getSegment(), localCallOffset), NULL_SELECTOR, -1, -1, -1, localCallOffset, s->_executionStack.size() - 1, EXEC_STACK_TYPE_CALL); diff --git a/engines/sci/engine/vm.h b/engines/sci/engine/vm.h index dc789b6201..c41060dc32 100644 --- a/engines/sci/engine/vm.h +++ b/engines/sci/engine/vm.h @@ -115,7 +115,6 @@ struct ExecStack { fp = sp = sp_; argc = argc_; variables_argp = argp_; - *variables_argp = make_reg(0, argc); // The first argument is argc if (localsSegment_ != 0xFFFF) local_segment = localsSegment_; else -- cgit v1.2.3 From 4b72a42da93da5560ecde38010d328196ff727fd Mon Sep 17 00:00:00 2001 From: Willem Jan Palenstijn Date: Sat, 2 Jul 2016 21:31:29 +0200 Subject: SCI: Remove presumably long-outdated FIXME --- engines/sci/engine/kernel.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) (limited to 'engines/sci') diff --git a/engines/sci/engine/kernel.cpp b/engines/sci/engine/kernel.cpp index 4370e6de04..2fc338b618 100644 --- a/engines/sci/engine/kernel.cpp +++ b/engines/sci/engine/kernel.cpp @@ -84,23 +84,15 @@ uint Kernel::getKernelNamesSize() const { } const Common::String &Kernel::getKernelName(uint number) const { - // FIXME: The following check is a temporary workaround for an issue - // leading to crashes when using the debugger's backtrace command. - if (number >= _kernelNames.size()) - return _invalid; + assert(number < _kernelFuncs.size()); return _kernelNames[number]; } Common::String Kernel::getKernelName(uint number, uint subFunction) const { - // FIXME: The following check is a temporary workaround for an issue - // leading to crashes when using the debugger's backtrace command. - if (number >= _kernelFuncs.size()) - return _invalid; + assert(number < _kernelFuncs.size()); const KernelFunction &kernelCall = _kernelFuncs[number]; - if (subFunction >= kernelCall.subFunctionCount) - return _invalid; - + assert(subFunction < kernelCall.subFunctionCount); return kernelCall.subFunctions[subFunction].name; } -- cgit v1.2.3