[Scummvm-git-logs] scummvm master -> a092a8a08edf7d179198c3ac86bb77e192d66c00
bluegr
bluegr at gmail.com
Sat Oct 16 22:37:56 UTC 2021
This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .
Summary:
a092a8a08e SCI: Remove the unused and unnecessary VM hooks mechanism
Commit: a092a8a08edf7d179198c3ac86bb77e192d66c00
https://github.com/scummvm/scummvm/commit/a092a8a08edf7d179198c3ac86bb77e192d66c00
Author: Filippos Karapetis (bluegr at gmail.com)
Date: 2021-10-17T01:37:33+03:00
Commit Message:
SCI: Remove the unused and unnecessary VM hooks mechanism
This was introduced as an extra mechanism of patching game scripts.
However, it's completely hardcoded with offsets for specific game
versions, with no endianess handling. Furthermore, the only patch
it's used for at the moment (input prompt for SQ3) has no visible
changes.
The SCI engine's script patcher has pattern matching functionality,
which allows it to be used in a variety of game versions. Furthermore,
it supports endianess handling for BE versions. Thus, it makes no
sense to keep a separate and complex script patching functionality
for a single patch that is hardcoded for a single game version with
no actual change in functionality. Since this is a fan patch, all of
these script changes can be added as part of the patch without any
changes to the engine code. We've discussed this with @sluicebox and
decided to remove all of this code, which should not have been part
of the engine's codebase because it clashes with the existing script
patcher
Changed paths:
R engines/sci/engine/vm_hooks.cpp
R engines/sci/engine/vm_hooks.h
engines/sci/engine/scriptdebug.cpp
engines/sci/engine/scriptdebug.h
engines/sci/engine/vm.cpp
engines/sci/module.mk
diff --git a/engines/sci/engine/scriptdebug.cpp b/engines/sci/engine/scriptdebug.cpp
index 93bc908f2d..02e5e733bd 100644
--- a/engines/sci/engine/scriptdebug.cpp
+++ b/engines/sci/engine/scriptdebug.cpp
@@ -39,8 +39,7 @@ namespace Sci {
// This table is only used for debugging. Don't include it for devices
// with not enough available memory (e.g. phones), where REDUCE_MEMORY_USAGE
// is defined
-// Update: This is used in the VM hooks mechanism. TODO: Readd the memory check?
-//#ifndef REDUCE_MEMORY_USAGE
+#ifndef REDUCE_MEMORY_USAGE
const char *opcodeNames[] = {
"bnot", "add", "sub", "mul", "div",
"mod", "shr", "shl", "xor", "and",
@@ -69,7 +68,7 @@ const char *opcodeNames[] = {
"-agi", "-ali", "-ati", "-api", "-sgi",
"-sli", "-sti", "-spi"
};
-//#endif // REDUCE_MEMORY_USAGE
+#endif // REDUCE_MEMORY_USAGE
void DebugState::updateActiveBreakpointTypes() {
int type = 0;
@@ -150,9 +149,9 @@ reg_t disassemble(EngineState *s, reg_t pos, const Object *obj, bool printBWTag,
return retval;
}
-//#ifndef REDUCE_MEMORY_USAGE
+#ifndef REDUCE_MEMORY_USAGE
debugN("%-5s", opcodeNames[opcode]);
-//#endif
+#endif
static const char *defaultSeparator = "\t\t; ";
diff --git a/engines/sci/engine/scriptdebug.h b/engines/sci/engine/scriptdebug.h
index df6bcef34f..3d54b327d3 100644
--- a/engines/sci/engine/scriptdebug.h
+++ b/engines/sci/engine/scriptdebug.h
@@ -25,9 +25,9 @@
namespace Sci {
-//#ifndef REDUCE_MEMORY_USAGE
+#ifndef REDUCE_MEMORY_USAGE
extern const char *opcodeNames[];
-//#endif
+#endif
void debugSelectorCall(reg_t send_obj, Selector selector, int argc, StackPtr argp, ObjVarRef &varp, reg_t funcp, SegManager *segMan, SelectorType selectorType);
diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp
index a70d52e7a0..2ccd379bc5 100644
--- a/engines/sci/engine/vm.cpp
+++ b/engines/sci/engine/vm.cpp
@@ -37,7 +37,6 @@
#include "sci/engine/gc.h"
#include "sci/engine/workarounds.h"
#include "sci/engine/scriptdebug.h"
-#include "sci/engine/vm_hooks.h"
namespace Sci {
@@ -578,8 +577,6 @@ void run_vm(EngineState *s) {
StackPtr s_temp; // Temporary stack pointer
int16 opparams[4]; // opcode parameters
- VmHooks vmHooks;
-
s->r_rest = 0; // &rest adjusts the parameter count by this value
// Current execution data:
s->xs = &(s->_executionStack.back());
@@ -605,8 +602,6 @@ void run_vm(EngineState *s) {
#endif
while (1) {
- vmHooks.vm_hook_before_exec(s);
-
int var_type; // See description below
int var_number;
@@ -664,12 +659,7 @@ void run_vm(EngineState *s) {
// Get opcode
byte extOpcode;
- if (!vmHooks.isActive(s))
- s->xs->addr.pc.incOffset(readPMachineInstruction(scr->getBuf(s->xs->addr.pc.getOffset()), extOpcode, opparams));
- else {
- int offset = readPMachineInstruction(vmHooks.data(), extOpcode, opparams);
- vmHooks.advance(offset);
- }
+ s->xs->addr.pc.incOffset(readPMachineInstruction(scr->getBuf(s->xs->addr.pc.getOffset()), extOpcode, opparams));
const byte opcode = extOpcode >> 1;
//debug("%s: %d, %d, %d, %d, acc = %04x:%04x, script %d, local script %d", opcodeNames[opcode], opparams[0], opparams[1], opparams[2], opparams[3], PRINT_REG(s->r_acc), scr->getScriptNumber(), local_script->getScriptNumber());
@@ -804,44 +794,30 @@ void run_vm(EngineState *s) {
case op_bt: // 0x17 (23)
// Branch relative if true
- if (!vmHooks.isActive(s)) {
- if (s->r_acc.getOffset() || s->r_acc.getSegment())
- s->xs->addr.pc.incOffset(opparams[0]);
+ if (s->r_acc.getOffset() || s->r_acc.getSegment())
+ s->xs->addr.pc.incOffset(opparams[0]);
- if (s->xs->addr.pc.getOffset() >= local_script->getScriptSize())
- error("[VM] op_bt: request to jump past the end of script %d (offset %d, script is %d bytes)",
- local_script->getScriptNumber(), s->xs->addr.pc.getOffset(), local_script->getScriptSize());
- } else {
- if (s->r_acc.getOffset() || s->r_acc.getSegment())
- vmHooks.advance(opparams[0]);
- }
+ if (s->xs->addr.pc.getOffset() >= local_script->getScriptSize())
+ error("[VM] op_bt: request to jump past the end of script %d (offset %d, script is %d bytes)",
+ local_script->getScriptNumber(), s->xs->addr.pc.getOffset(), local_script->getScriptSize());
break;
case op_bnt: // 0x18 (24)
// Branch relative if not true
- if (!vmHooks.isActive(s)) {
- if (!(s->r_acc.getOffset() || s->r_acc.getSegment()))
- s->xs->addr.pc.incOffset(opparams[0]);
+ if (!(s->r_acc.getOffset() || s->r_acc.getSegment()))
+ s->xs->addr.pc.incOffset(opparams[0]);
- if (s->xs->addr.pc.getOffset() >= local_script->getScriptSize())
- error("[VM] op_bnt: request to jump past the end of script %d (offset %d, script is %d bytes)",
- local_script->getScriptNumber(), s->xs->addr.pc.getOffset(), local_script->getScriptSize());
- } else {
- if (!(s->r_acc.getOffset() || s->r_acc.getSegment()))
- vmHooks.advance(opparams[0]);
- }
+ if (s->xs->addr.pc.getOffset() >= local_script->getScriptSize())
+ error("[VM] op_bnt: request to jump past the end of script %d (offset %d, script is %d bytes)",
+ local_script->getScriptNumber(), s->xs->addr.pc.getOffset(), local_script->getScriptSize());
break;
case op_jmp: // 0x19 (25)
- if (!vmHooks.isActive(s)) {
- s->xs->addr.pc.incOffset(opparams[0]);
+ s->xs->addr.pc.incOffset(opparams[0]);
- if (s->xs->addr.pc.getOffset() >= local_script->getScriptSize())
- error("[VM] op_jmp: request to jump past the end of script %d (offset %d, script is %d bytes)",
- local_script->getScriptNumber(), s->xs->addr.pc.getOffset(), local_script->getScriptSize());
- } else {
- vmHooks.advance(opparams[0]);
- }
+ if (s->xs->addr.pc.getOffset() >= local_script->getScriptSize())
+ error("[VM] op_jmp: request to jump past the end of script %d (offset %d, script is %d bytes)",
+ local_script->getScriptNumber(), s->xs->addr.pc.getOffset(), local_script->getScriptSize());
break;
case op_ldi: // 0x1a (26)
diff --git a/engines/sci/engine/vm_hooks.cpp b/engines/sci/engine/vm_hooks.cpp
deleted file mode 100644
index 81abcfcaea..0000000000
--- a/engines/sci/engine/vm_hooks.cpp
+++ /dev/null
@@ -1,197 +0,0 @@
-/* ScummVM - Graphic Adventure Engine
- *
- * ScummVM is the legal property of its developers, whose names
- * are too numerous to list here. Please refer to the COPYRIGHT
- * file distributed with this source distribution.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
- *
- */
-
-#include "common/hashmap.h"
-#include "common/array.h"
-#include "common/language.h"
-#include "sci/engine/vm_hooks.h"
-#include "sci/engine/vm.h"
-#include "sci/engine/state.h"
-#include "sci/engine/kernel.h"
-#include "sci/engine/scriptdebug.h"
-
-namespace Sci {
-
-/*****************************************************************************************************************
- *
- * This mechanism allows inserting new instructions, and not only replace existing code.
- * Note that when using hooks, the regular PC is frozen, and doesn't advance.
- * Therefore, 'jmp', 'bt' and 'bnt' are used to locally move around inside the patch.
- *
- ******************************************************************************************************************/
-
-// SCI0 Hebrew translations need to modify and relocate the "Enter input:" prompt
-// currently SQ3 is the only Hebrew SCI0 game, but this patch (or similar) should work for the future games as well
-
-static const byte sci0_hebrew_input_prompt[] = {
- // in (procedure (Print ...
- // ...
- // (#edit
- // (++ paramCnt)
- // adding:
- // (hDText moveTo: 104 4)
- // (= hDText (DText new:))
- // (hDText
- // text: ''
- // moveTo: 4 4
- // font: gDefaultFont
- // setSize:
- // )
- // (hDialog add: hDText)
- 0x38, 0x92, 0, // pushi #moveTo
- 0x39, 2, // pushi 2
- 0x38, 210, 0, // pushi 220 ;x
- 0x39, 4, // pushi 4 ;y
- 0x85, 1, // lat temp1
- 0x4a, 8, // send 8
- 0x38, 0x59, 0, // pushi #new
- 0x39, 0, // pushi 0
- 0x51, 0xd, // class DText
- 0x4a, 4, // send 4
- 0xa5, 1, // sat temp1
- 0x39, 0x1a, // pushi #text
- 0x39, 1, // pushi 1
- 0x75, 25, // lofss '' ; that location has '\0'
- 0x38, 146, 0, // pushi 146
- 0x39, 2, // pushi 2
- 0x39, 4, // pushi 4 ;x
- 0x39, 14, // pushi 12 ;y
- 0x39, 33, // pushi 33
- 0x39, 1, // pushi 1
- 0x89, 0x16, // lsg global22
- 0x38, 144, 0, // pushi 144
- 0x39, 0, // pushi 0
- 0x85, 1, // lat temp1
- 0x4a, 24, // send 24
- 0x38, 0x64, 0, // pushi #add
- 0x39, 1, // pushi 1
- 0x8d, 1, // lst temp1
- 0x85, 0, // lat temp0
- 0x4a, 6, // send 6
- 0x85, 5 // lat temp5
-};
-
-
-
-/** Write here all games hooks
- * From this we'll build _hooksMap, which contains only relevant hooks to current game
- * The match is performed according to PC, script number, opcode (only opcode name, as seen in ScummVM debugger),
- * and either:
- * - objName and selector (and then externID is -1)
- * - external function ID (and then selector is "")
- * = in that case, if objName == "" it will be ignored, otherwise, it will be also used to match
- */
-
-static const GeneralHookEntry allGamesHooks[] = {
- // GID, script, lang, PC.offset, objName, selector, externID, opcode, hook array
- {GID_SQ3, Common::HE_ISR, {255, 0x1103}, {"User", "", -1 , "pushi", HOOKARRAY(sci0_hebrew_input_prompt)}}
-};
-
-
-VmHooks::VmHooks() {
- // build _hooksMap
- for (uint i = 0; i < ARRAYSIZE(allGamesHooks); i++) {
- if (allGamesHooks[i].gameId == g_sci->getGameId())
- if (allGamesHooks[i].language == g_sci->getLanguage() || allGamesHooks[i].language == Common::UNK_LANG)
- _hooksMap.setVal(allGamesHooks[i].key, allGamesHooks[i].entry);
- }
-
- _lastPc = NULL_REG;
- _just_finished = false;
- _location = 0;
-}
-
-uint64 HookHashKey::hash() {
- return ((uint64)scriptNumber << 32) + offset;
-}
-
-//#ifndef REDUCE_MEMORY_USAGE
-extern const char* opcodeNames[];
-//#endif
-
-// returns true if entry is matching to current state
-bool hook_exec_match(Sci::EngineState *s, HookEntry entry) {
- Script *scr = s->_segMan->getScript(s->xs->addr.pc.getSegment());
- const char *objName = s->_segMan->getObjectName(s->xs->objp);
- Common::String selector;
- if (s->xs->debugSelector != -1)
- selector = g_sci->getKernel()->getSelectorName(s->xs->debugSelector);
- byte opcode = (scr->getBuf(s->xs->addr.pc.getOffset())[0]) >> 1;
-
- bool objMatch;
- if (entry.exportId != -1 && strcmp(entry.objName, "") == 0)
- objMatch = true;
- else
- objMatch = strcmp(objName, entry.objName) == 0;
-
- return objMatch && selector == entry.selector &&
- s->xs->debugExportId == entry.exportId && strcmp(entry.opcodeName, opcodeNames[opcode]) == 0;
-}
-
-void VmHooks::vm_hook_before_exec(Sci::EngineState *s) {
- if (_just_finished) {
- _just_finished = false;
- _lastPc = NULL_REG;
- return;
- }
- Script *scr = s->_segMan->getScript(s->xs->addr.pc.getSegment());
- int scriptNumber = scr->getScriptNumber();
- HookHashKey key = { scriptNumber, s->xs->addr.pc.getOffset() };
- if (_hookScriptData.empty() && _lastPc != s->xs->addr.pc && _hooksMap.contains(key)) {
- _lastPc = s->xs->addr.pc;
- HookEntry entry = _hooksMap[key];
- if (hook_exec_match(s, entry)) {
- debugC(kDebugLevelPatcher, "vm_hook: patching script: %d, PC: %04x:%04x, obj: %s, selector: %s, extern: %d, opcode: %s", scriptNumber, PRINT_REG(s->xs->addr.pc), entry.objName, entry.selector, entry.exportId, entry.opcodeName);
- Common::Array<byte> buffer(entry.patch, entry.patchSize);
-
- _hookScriptData = buffer;
- } else {
- debugC(kDebugLevelPatcher, "vm_hook: failed to match! script: %d, PC: %04x:%04x, obj: %s, selector: %s, extern: %d, opcode: %s", scriptNumber, PRINT_REG(s->xs->addr.pc), entry.objName, entry.selector, entry.exportId, entry.opcodeName);
- }
- }
-}
-
-byte *VmHooks::data() {
- return _hookScriptData.data() + _location;
-}
-
-bool VmHooks::isActive(Sci::EngineState *s) {
- // if PC has changed, then we're temporary inactive - went to some other call, or send, etc.
- return !_hookScriptData.empty() && _lastPc == s->xs->addr.pc;
-}
-
-void VmHooks::advance(int offset) {
- int newLocation = _location + offset;
- if (newLocation < 0)
- error("VmHooks: requested to change offset before start of patch");
- else if ((uint)newLocation > _hookScriptData.size())
- error("VmHooks: requested to change offset after end of patch");
- else if ((uint)newLocation == _hookScriptData.size()) {
- _hookScriptData.clear();
- _just_finished = true;
- _location = 0;
- } else
- _location = newLocation;
-}
-
-
-} // End of namespace Sci
diff --git a/engines/sci/engine/vm_hooks.h b/engines/sci/engine/vm_hooks.h
deleted file mode 100644
index ce60c28da6..0000000000
--- a/engines/sci/engine/vm_hooks.h
+++ /dev/null
@@ -1,107 +0,0 @@
-/* ScummVM - Graphic Adventure Engine
- *
- * ScummVM is the legal property of its developers, whose names
- * are too numerous to list here. Please refer to the COPYRIGHT
- * file distributed with this source distribution.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
- *
- */
-
-#ifndef SCI_ENGINE_VM_HOOKS_H
-#define SCI_ENGINE_VM_HOOKS_H
-
-#include "sci/engine/vm.h"
-
-namespace Sci {
-
-
-#define ARRAYSIZE(x) ((int)(sizeof(x) / sizeof(x[0])))
-#define HOOKARRAY(x) x, ARRAYSIZE(x)
-
-/** _hooksMap keys are built from script number and PC's offset */
-struct HookHashKey {
- int scriptNumber;
- uint32 offset;
-
- uint64 hash();
-
- bool operator==(const HookHashKey &other) const {
- return scriptNumber == other.scriptNumber && offset == other.offset;
- }
-
-
-};
-
-/** _hooksMap value entry */
-struct HookEntry {
- /** These are used to make sure that the PC is indeed the requested place */
- const char *objName;
- const char *selector;
- int exportId;
- const char *opcodeName;
-
- /** If all the previous match, patch */
- const byte *patch;
- uint patchSize;
-};
-
-/** Used for allGamesHooks - from it we build the specific _hooksMap */
-struct GeneralHookEntry {
- SciGameId gameId;
- Common::Language language; // language to be patched. UNK_LANG means to patch all languages
- HookHashKey key;
- HookEntry entry;
-};
-
-/** Hash key equality function */
-struct HookHash : public Common::UnaryFunction<HookHashKey, uint64> {
- uint64 operator()(HookHashKey val) const { return val.hash(); }
-};
-
-/** VM Hook mechanism */
-class VmHooks {
-public:
- VmHooks();
-
- /** Called just before executing opcode, to check if there is a requried hook */
- void vm_hook_before_exec(Sci::EngineState *s);
-
- byte *data();
-
- bool isActive(Sci::EngineState *s);
-
- void advance(int offset);
-
-private:
- /** Hash map of all game's hooks */
- Common::HashMap<HookHashKey, HookEntry, HookHash> _hooksMap;
-
- Common::Array<byte> _hookScriptData;
-
- /** Used to avoid double patching in row, and to support `call`ing */
- reg_t _lastPc;
-
- /** Raised after patch has ended, to avoid confusion with situation of returning from a `call` to the patch, and continue execution of original code */
- bool _just_finished;
-
- /** Location inside patch */
- int _location;
-};
-
-
-} // End of namespace Sci
-
-#endif // SCI_ENGINE_VM_HOOKS_H
diff --git a/engines/sci/module.mk b/engines/sci/module.mk
index 62fa3f3753..a9129b4804 100644
--- a/engines/sci/module.mk
+++ b/engines/sci/module.mk
@@ -38,7 +38,6 @@ MODULE_OBJS := \
engine/state.o \
engine/static_selectors.o \
engine/vm.o \
- engine/vm_hooks.o \
engine/vm_types.o \
engine/workarounds.o \
graphics/animate.o \
More information about the Scummvm-git-logs
mailing list