[Scummvm-git-logs] scummvm master -> bf47637280ed9e82c5891a32f366d4bc16b3d78b
bluegr
noreply at scummvm.org
Wed Nov 30 21:13:05 UTC 2022
This automated email contains information about 2 new commits which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .
Summary:
30fad94e9a SCI: Track correct location and size of temp variables
bf47637280 SCI: Remove KQ6 black widow script patch
Commit: 30fad94e9a768d67287906c15386ae212af8fbac
https://github.com/scummvm/scummvm/commit/30fad94e9a768d67287906c15386ae212af8fbac
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2022-11-30T23:13:00+02:00
Commit Message:
SCI: Track correct location and size of temp variables
The VM has been treating the entire area between the frame pointer and
the stack pointer as temp variables for the current function. There are
two problems with this:
1. The VM hasn't been updating the frame pointer correctly when multiple
methods are called within the same send/self/super instruction.
2. The VM has been recalculating the number of temp variables on every
instruction as the difference between the two pointers.
This is incorrect, as this changes with almost every instruction in
ways have nothing to do with the number of temp variables allocated
by the link instruction. Meanwhile, the VM has not been recording
the number of variables allocated by the link instruction.
The first discrepancy caused scripts to behave differently than in SSCI
when reading parameters out of bounds in certain situations.
It also prevented our uninitialized variable detection from detecting
certain reads. The second made the temp-count used for out of bounds
detection too large, made debugger output such as `stack` incorrect,
and made stepping through the link instruction in the debugger appear
to do nothing until stepping through the following instruction.
When multiple methods are called by a send/self/super instruction, each
method's link instruction increases the stack pointer further.
Method B's variables appear after method A's. The VM has been setting
the stack pointer correctly but it kept using the previous frame
pointer, so method B would re-use method A's stack area instead of the
area the VM had just allocated for B. If a script correctly initializes
variables before using them and doesn't use out of bounds parameters or
temp variables then this discrepancy doesn't make a difference.
But a lot of scripts do these bad things and accidentally rely on the
undefined values they read.
Now we update the frame pointer correctly when "carrying over" to
subsequent method calls from the same send/self/super instruction.
This matches SSCI behavior. We also now record the number of temp
variables that have been allocated by the link instruction and use
that instead of incorrectly recalculating on every instruction.
Fixes the KQ6 black widow lockup, and other KQ6 music bugs, where
scripts call Sound:fade without the required fourth parameter.
Sound:fade expects a fourth parameter, so reads it out of bounds,
and passes it as the stop-after-completion boolean to kDoSoundFade.
Scripts that called Sound:fade as the only method in a send got the same
results as in SSCI, but scripts that called Sound:play first didn't.
Fixes bugs #5625 #5653 #6120 #6210 #6252 #13944
Changed paths:
engines/sci/console.cpp
engines/sci/engine/vm.cpp
engines/sci/engine/vm.h
diff --git a/engines/sci/console.cpp b/engines/sci/console.cpp
index 4bb14bc0d07..f18525f2b23 100644
--- a/engines/sci/console.cpp
+++ b/engines/sci/console.cpp
@@ -2995,10 +2995,18 @@ bool Console::cmdStack(int argc, const char **argv) {
int nr = atoi(argv[1]);
for (int i = nr; i > 0; i--) {
- if ((xs.sp - xs.fp - i) == 0)
+ bool isArgc = (xs.sp - xs.variables_argp - i == 0);
+ if (isArgc)
+ debugPrintf("-- parameters --\n");
+ if (xs.tempCount && ((xs.sp - xs.fp - i) == 0))
debugPrintf("-- temp variables --\n");
+ if (xs.sp - xs.fp - xs.tempCount - i == 0)
+ debugPrintf("-- local stack --\n");
if (xs.sp - i >= _engine->_gamestate->stack_base)
- debugPrintf("ST:%04x = %04x:%04x\n", (unsigned)(xs.sp - i - _engine->_gamestate->stack_base), PRINT_REG(xs.sp[-i]));
+ debugPrintf("ST:%04x = %04x:%04x%s\n",
+ (unsigned)(xs.sp - i - _engine->_gamestate->stack_base),
+ PRINT_REG(xs.sp[-i]),
+ (isArgc ? " argc" : ""));
}
return true;
diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp
index 9aa92e4bd16..fe71615bbda 100644
--- a/engines/sci/engine/vm.cpp
+++ b/engines/sci/engine/vm.cpp
@@ -596,7 +596,7 @@ void run_vm(EngineState *s) {
s->variablesSegment[VAR_LOCAL] = local_script->getLocalsSegment();
s->variablesBase[VAR_LOCAL] = s->variables[VAR_LOCAL] = local_script->getLocalsBegin();
s->variablesMax[VAR_LOCAL] = local_script->getLocalsCount();
- s->variablesMax[VAR_TEMP] = s->xs->sp - s->xs->fp;
+ s->variablesMax[VAR_TEMP] = s->xs->tempCount;
s->variablesMax[VAR_PARAM] = s->xs->argc + 1;
}
s->variables[VAR_TEMP] = s->xs->fp;
@@ -621,8 +621,6 @@ void run_vm(EngineState *s) {
error("run_vm(): stack underflow, sp: %04x:%04x, fp: %04x:%04x",
PRINT_REG(*s->xs->sp), PRINT_REG(*s->xs->fp));
- s->variablesMax[VAR_TEMP] = s->xs->sp - s->xs->fp;
-
if (s->xs->addr.pc.getOffset() >= scr->getBufSize())
error("run_vm(): program counter gone astray, addr: %d, code buffer size: %d",
s->xs->addr.pc.getOffset(), scr->getBufSize());
@@ -817,6 +815,8 @@ void run_vm(EngineState *s) {
break;
case op_link: // 0x1f (31)
+ s->variablesMax[VAR_TEMP] = s->xs->tempCount = opparams[0];
+
// We shouldn't initialize temp variables at all
// We put special segment 0xFFFF in there, so that uninitialized reads can get detected
for (int i = 0; i < opparams[0]; i++)
@@ -919,8 +919,7 @@ void run_vm(EngineState *s) {
case op_ret: // 0x24 (36)
// Return from an execution loop started by call, calle, callb, send, self or super
do {
- StackPtr old_sp2 = s->xs->sp;
- StackPtr old_fp = s->xs->fp;
+ StackPtr old_sp = s->xs->sp;
ExecStack *old_xs = &(s->_executionStack.back());
if ((int)s->_executionStack.size() - 1 == s->executionStackBase) { // Have we reached the base?
@@ -952,8 +951,8 @@ void run_vm(EngineState *s) {
if (s->xs->sp == CALL_SP_CARRY // Used in sends to 'carry' the stack pointer
|| s->xs->type != EXEC_STACK_TYPE_CALL) {
- s->xs->sp = old_sp2;
- s->xs->fp = old_fp;
+ s->xs->sp = old_sp;
+ s->xs->fp = old_sp;
}
} while (s->xs->type == EXEC_STACK_TYPE_VARSELECTOR);
diff --git a/engines/sci/engine/vm.h b/engines/sci/engine/vm.h
index 53485c129c5..7a8abe9c32b 100644
--- a/engines/sci/engine/vm.h
+++ b/engines/sci/engine/vm.h
@@ -91,6 +91,8 @@ struct ExecStack {
int argc;
StackPtr variables_argp; // Argument pointer
+ int tempCount; // Number of temp variables allocated by link opcode
+
SegmentId local_segment; // local variables etc
Selector debugSelector; // The selector which was used to call or -1 if not applicable
@@ -115,6 +117,7 @@ struct ExecStack {
fp = sp = sp_;
argc = argc_;
variables_argp = argp_;
+ tempCount = 0;
if (localsSegment_ != kUninitializedSegment)
local_segment = localsSegment_;
else
Commit: bf47637280ed9e82c5891a32f366d4bc16b3d78b
https://github.com/scummvm/scummvm/commit/bf47637280ed9e82c5891a32f366d4bc16b3d78b
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2022-11-30T23:13:00+02:00
Commit Message:
SCI: Remove KQ6 black widow script patch
The VM discrepancy that caused these buggy scripts to behave
differently has been fixed, so this patch is no longer needed
Changed paths:
engines/sci/engine/script_patches.cpp
diff --git a/engines/sci/engine/script_patches.cpp b/engines/sci/engine/script_patches.cpp
index a6b4159140b..c3d3e0fbb91 100644
--- a/engines/sci/engine/script_patches.cpp
+++ b/engines/sci/engine/script_patches.cpp
@@ -5490,57 +5490,6 @@ static const uint16 kq6PatchMacDrinkMePic[] = {
PATCH_END
};
-// WORKAROUND
-//
-// The black widow's music doesn't play, and the game locks up after reading the
-// parchment in her web. This is due to a discrepancy in the undefined values
-// that SSCI and ScummVM produce when Sound:fade reads non-existent parameters.
-// This bug also prevents catacombs music from resuming on the lower level.
-//
-// Sound:fade expects at least four parameters but KQ6 often passes only three.
-// The fourth is passed to kDoSoundFade as its boolean parameter for stopping
-// the sound after fading completes. In the original this happened to work, but
-// ScummVM produces the value three in situations where SSCI produces zero.
-// Several scripts accidentally rely on this. A non-zero value stops the sound
-// unexpectedly and this locks up the black widow parchment script.
-//
-// When a `send` opcode calls other methods before Sound:fade, the discrepancy
-// between undefined values occurs. The cause is currently unknown. There are
-// only five of these problematic calls in KQ6 and they all look the same.
-// First they set the volume to zero, then they call Sound:play which undoes
-// this by setting the volume to 127, and finally they redundantly fade to 127.
-//
-// We work around this by patching out the five problematic fades. They have no
-// effect other than breaking the game when their undefined parameter values
-// happen to be non-zero. If the VM is changed to produce the same undefined
-// values as SSCI then this patch can be removed.
-//
-// Applies to: All versions, although the floppy disk script does not lock up.
-// Responsible methods: rm460:init, lookAtParchment:changeState, blackWidowInset:init,
-// lightItUp:changeState, flyIn:changeState
-// Fixes bugs: #5625 #5653 #6120 #6210 #6252 #13944
-static const uint16 kq6SignatureBadFade[] = {
- 0x39, SIG_SELECTOR8(play), // pushi play
- 0x76, // push0
- SIG_MAGICDWORD,
- 0x38, SIG_SELECTOR16(fade), // pushi fade
- 0x39, 0x03, // pushi 03
- 0x39, 0x7f, // pushi 7f
- 0x39, 0x0a, // pushi 0a
- 0x3c, // dup
- SIG_ADDTOOFFSET(+2),
- 0x4a, 0x20, // send 20 [ ... play: fade: 127 10 10 ]
- SIG_END
-};
-
-static const uint16 kq6PatchBadFade[] = {
- PATCH_ADDTOOFFSET(+3),
- 0x32, PATCH_UINT16(0x0007), // jmp 0007
- PATCH_ADDTOOFFSET(+9),
- 0x4a, 0x16, // send 16 [ ... play: ]
- PATCH_END
-};
-
// Looking at the ribbon in inventory says that there's a hair even after it's
// been removed. This occurs after the hair has been put in the skull or is on
// a different inventory page than the ribbon.
@@ -6758,13 +6707,9 @@ static const SciScriptPatcherEntry kq6Signatures[] = {
{ true, 281, "fix pawnshop genie eye", 1, kq6SignaturePawnshopGenieEye, kq6PatchPawnshopGenieEye },
{ true, 300, "fix floating off steps", 2, kq6SignatureCliffStepFloatFix, kq6PatchCliffStepFloatFix },
{ true, 300, "fix floating off steps", 2, kq6SignatureCliffItemFloatFix, kq6PatchCliffItemFloatFix },
- { true, 301, "fix bad fade", 1, kq6SignatureBadFade, kq6PatchBadFade },
{ true, 405, "fix catacombs room message", 1, kq6SignatureRoom405LookMessage, kq6PatchRoom405LookMessage },
{ true, 406, "fix catacombs dark room inventory", 1, kq6SignatureDarkRoomInventory, kq6PatchDarkRoomInventory },
- { true, 406, "fix bad fade", 1, kq6SignatureBadFade, kq6PatchBadFade },
{ true, 407, "fix catacombs room message", 1, kq6SignatureRoom407LookMessage, kq6PatchRoom407LookMessage },
- { true, 460, "fix bad fade", 2, kq6SignatureBadFade, kq6PatchBadFade },
- { true, 461, "fix bad fade", 1, kq6SignatureBadFade, kq6PatchBadFade },
{ true, 480, "CD: fix wallflower dance", 1, kq6CDSignatureWallFlowerDanceFix, kq6CDPatchWallFlowerDanceFix },
{ true, 480, "fix picking second lettuce", 1, kq6SignatureSecondLettuceFix, kq6PatchSecondLettuceFix },
{ true, 480, "fix getting baby tears", 1, kq6SignatureGetBabyTears, kq6PatchGetBabyTears },
More information about the Scummvm-git-logs
mailing list