SCI: Improve disassembly output

1. pushi opcode now displays decimal value and selector value (if
   one exists) in-line
2. lofsa, lofss, and super opcodes now display resolved
   object/class names
3. Opcode arguments are visually aligned

5 files changed, 99 insertions, 36 deletions

diff --git a/engines/sci/engine/script.cpp b/engines/sci/engine/script.cpp
index 098c2e56c9..8a973bd217 100644
--- a/engines/sci/engine/script.cpp
+++ b/engines/sci/engine/script.cpp
@@ -704,7 +704,7 @@ static bool relocateBlock(Common::Array<reg_t> &block, int block_location, Segme
 	return true;
 }
 
-int Script::relocateOffsetSci3(uint32 offset) {
+int Script::relocateOffsetSci3(uint32 offset) const {
 	int relocStart = READ_LE_UINT32(_buf + 8);
 	int relocCount = READ_LE_UINT16(_buf + 18);
 	const byte *seeker = _buf + relocStart;
diff --git a/engines/sci/engine/script.h b/engines/sci/engine/script.h
index 31f0a9e24d..677b367051 100644
--- a/engines/sci/engine/script.h
+++ b/engines/sci/engine/script.h
@@ -266,7 +266,7 @@ public:
 	 * Resolve a relocation in an SCI3 script
 	 * @param offset        The offset to relocate from
 	 */
-	int relocateOffsetSci3(uint32 offset);
+	int relocateOffsetSci3(uint32 offset) const;
 
 	/**
 	 * Gets an offset to the beginning of the code block in a SCI3 script
diff --git a/engines/sci/engine/scriptdebug.cpp b/engines/sci/engine/scriptdebug.cpp
index 47b360d13b..9ed97723d0 100644
--- a/engines/sci/engine/scriptdebug.cpp
+++ b/engines/sci/engine/scriptdebug.cpp
@@ -68,7 +68,7 @@ const char *opcodeNames[] = {
 #endif	// REDUCE_MEMORY_USAGE
 
 // Disassembles one command from the heap, returns address of next command or 0 if a ret was encountered.
-reg_t disassemble(EngineState *s, reg32_t pos, bool printBWTag, bool printBytecode) {
+reg_t disassemble(EngineState *s, reg32_t pos, reg_t objAddr, bool printBWTag, bool printBytecode) {
 	SegmentObj *mobj = s->_segMan->getSegment(pos.getSegment(), SEG_TYPE_SCRIPT);
 	Script *script_entity = NULL;
 	const byte *scr;
@@ -127,9 +127,11 @@ reg_t disassemble(EngineState *s, reg32_t pos, bool printBWTag, bool printByteco
 		debugN("[%c] ", opsize ? 'B' : 'W');
 
 #ifndef REDUCE_MEMORY_USAGE
-	debugN("%s", opcodeNames[opcode]);
+	debugN("%-5s", opcodeNames[opcode]);
 #endif
 
+	static const char *defaultSeparator = "\t\t; ";
+
 	i = 0;
 	while (g_sci->_opcode_formats[opcode][i]) {
 		switch (g_sci->_opcode_formats[opcode][i++]) {
@@ -140,14 +142,21 @@ reg_t disassemble(EngineState *s, reg32_t pos, bool printBWTag, bool printByteco
 		case Script_SByte:
 		case Script_Byte:
 			param_value = scr[retval.getOffset()];
-			debugN(" %02x", scr[retval.getOffset()]);
+			debugN("\t%02x", param_value);
+			if (param_value > 9) {
+				debugN("%s%u", defaultSeparator, param_value);
+			}
 			retval.incOffset(1);
 			break;
 
 		case Script_Word:
 		case Script_SWord:
 			param_value = READ_SCI11ENDIAN_UINT16(&scr[retval.getOffset()]);
-			debugN(" %04x", READ_SCI11ENDIAN_UINT16(&scr[retval.getOffset()]));
+			debugN("\t%04x", param_value);
+			if (param_value > 9) {
+				debugN("%s%u", defaultSeparator, param_value);
+			}
+
 			retval.incOffset(2);
 			break;
 
@@ -166,12 +175,38 @@ reg_t disassemble(EngineState *s, reg32_t pos, bool printBWTag, bool printByteco
 				retval.incOffset(2);
 			}
 
-			if (opcode == op_callk)
-				debugN(" %s[%x]", (param_value < kernel->_kernelFuncs.size()) ?
+			if (opcode == op_callk) {
+				debugN("\t%s[%x],", (param_value < kernel->_kernelFuncs.size()) ?
 							((param_value < kernel->getKernelNamesSize()) ? kernel->getKernelName(param_value).c_str() : "[Unknown(postulated)]")
 							: "<invalid>", param_value);
-			else
-				debugN(opsize ? " %02x" : " %04x", param_value);
+			} else if (opcode == op_super) {
+				Object *obj;
+				if (objAddr != NULL_REG && (obj = s->_segMan->getObject(objAddr)) != nullptr) {
+					debugN("\t%s", s->_segMan->getObjectName(obj->getSuperClassSelector()));
+					debugN(opsize ? "[%02x]" : "[%04x]", param_value);
+				} else {
+					debugN(opsize ? "\t%02x" : "\t%04x", param_value);
+				}
+
+				debugN(",");
+			} else {
+				const char *separator = defaultSeparator;
+
+				debugN(opsize ? "\t%02x" : "\t%04x", param_value);
+				if (param_value > 9) {
+					debugN("%s%u", separator, param_value);
+					separator = ", ";
+				}
+
+				if (param_value >= 0x20 && param_value <= 0x7e) {
+					debugN("%s'%c'", separator, param_value);
+					separator = ", ";
+				}
+
+				if (opcode == op_pushi && param_value < kernel->getSelectorNamesSize()) {
+					debugN("%s%s", separator, kernel->getSelectorName(param_value).c_str());
+				}
+			}
 
 			break;
 
@@ -183,19 +218,27 @@ reg_t disassemble(EngineState *s, reg32_t pos, bool printBWTag, bool printByteco
 				param_value = READ_SCI11ENDIAN_UINT16(&scr[retval.getOffset()]);
 				retval.incOffset(2);
 			}
-			debugN(opsize ? " %02x" : " %04x", param_value);
+
+			if (opcode == op_lofsa || opcode == op_lofss) {
+				reg_t addr = make_reg(pos.getSegment(), findOffset(param_value, script_entity, pos.getOffset()));
+				debugN("\t%s", s->_segMan->getObjectName(addr));
+				debugN(opsize ? "[%02x]" : "[%04x]", param_value);
+			} else {
+				debugN(opsize ? "\t%02x" : "\t%04x", param_value);
+			}
+
 			break;
 
 		case Script_SRelative:
 			if (opsize) {
 				int8 offset = (int8)scr[retval.getOffset()];
 				retval.incOffset(1);
-				debugN(" %02x  [%04x]", 0xff & offset, 0xffff & (retval.getOffset() + offset));
+				debugN("\t%02x  [%04x]", 0xff & offset, 0xffff & (retval.getOffset() + offset));
 			}
 			else {
 				int16 offset = (int16)READ_SCI11ENDIAN_UINT16(&scr[retval.getOffset()]);
 				retval.incOffset(2);
-				debugN(" %04x  [%04x]", 0xffff & offset, 0xffff & (retval.getOffset() + offset));
+				debugN("\t%04x  [%04x]", 0xffff & offset, 0xffff & (retval.getOffset() + offset));
 			}
 			break;
 
@@ -396,7 +439,7 @@ void SciEngine::scriptDebug() {
 	}
 
 	debugN("Step #%d\n", s->scriptStepCounter);
-	disassemble(s, s->xs->addr.pc, false, true);
+	disassemble(s, s->xs->addr.pc, s->xs->objp, false, true);
 
 	if (_debugState.runningStep) {
 		_debugState.runningStep--;
diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp
index e82128ebad..01051ebdd4 100644
--- a/engines/sci/engine/vm.cpp
+++ b/engines/sci/engine/vm.cpp
@@ -567,6 +567,31 @@ int readPMachineInstruction(const byte *src, byte &extOpcode, int16 opparams[4])
 	return offset;
 }
 
+uint32 findOffset(const int16 relOffset, const Script *scr, const uint32 pcOffset) {
+	uint32 offset;
+
+	switch (g_sci->_features->detectLofsType()) {
+	case SCI_VERSION_0_EARLY:
+		offset = (uint16)pcOffset + relOffset;
+		break;
+	case SCI_VERSION_1_MIDDLE:
+		offset = relOffset;
+		break;
+	case SCI_VERSION_1_1:
+		offset = relOffset + scr->getScriptSize();
+		break;
+	case SCI_VERSION_3:
+		// In theory this can break if the variant with a one-byte argument is
+		// used. For now, assume it doesn't happen.
+		offset = scr->relocateOffsetSci3(pcOffset - 2);
+		break;
+	default:
+		error("Unknown lofs type");
+	}
+
+	return offset;
+}
+
 void run_vm(EngineState *s) {
 	assert(s);
 
@@ -1169,38 +1194,22 @@ void run_vm(EngineState *s) {
 		}
 
 		case op_lofsa: // 0x39 (57)
-		case op_lofss: // 0x3a (58)
+		case op_lofss: { // 0x3a (58)
 			// Load offset to accumulator or push to stack
-			r_temp.setSegment(s->xs->addr.pc.getSegment());
-
-			switch (g_sci->_features->detectLofsType()) {
-			case SCI_VERSION_0_EARLY:
-				r_temp.setOffset((uint16)s->xs->addr.pc.getOffset() + opparams[0]);
-				break;
-			case SCI_VERSION_1_MIDDLE:
-				r_temp.setOffset(opparams[0]);
-				break;
-			case SCI_VERSION_1_1:
-				r_temp.setOffset(opparams[0] + local_script->getScriptSize());
-				break;
-			case SCI_VERSION_3:
-				// In theory this can break if the variant with a one-byte argument is
-				// used. For now, assume it doesn't happen.
-				r_temp.setOffset(local_script->relocateOffsetSci3(s->xs->addr.pc.getOffset() - 2));
-				break;
-			default:
-				error("Unknown lofs type");
-			}
+			Script *local_script = s->_segMan->getScriptIfLoaded(s->xs->local_segment);
 
+			r_temp.setSegment(s->xs->addr.pc.getSegment());
+			r_temp.setOffset(findOffset(opparams[0], local_script, s->xs->addr.pc.getOffset()));
 			if (r_temp.getOffset() >= scr->getBufSize())
 				error("VM: lofsa/lofss operation overflowed: %04x:%04x beyond end"
-				          " of script (at %04x)", PRINT_REG(r_temp), scr->getBufSize());
+						  " of script (at %04x)", PRINT_REG(r_temp), scr->getBufSize());
 
 			if (opcode == op_lofsa)
 				s->r_acc = r_temp;
 			else
 				PUSH32(r_temp);
 			break;
+		}
 
 		case op_push0: // 0x3b (59)
 			PUSH(0);
diff --git a/engines/sci/engine/vm.h b/engines/sci/engine/vm.h
index dd66d9d52c..cec9e1528c 100644
--- a/engines/sci/engine/vm.h
+++ b/engines/sci/engine/vm.h
@@ -36,6 +36,7 @@ class SegManager;
 struct EngineState;
 class Object;
 class ResourceManager;
+class Script;
 
 /** Number of bytes to be allocated for the stack */
 #define VM_STACK_SIZE 0x1000
@@ -386,6 +387,16 @@ SelectorType lookupSelector(SegManager *segMan, reg_t obj, Selector selectorid,
  */
 int readPMachineInstruction(const byte *src, byte &extOpcode, int16 opparams[4]);
 
+/**
+ * Finds the script-absolute offset of a relative object offset.
+ *
+ * @param[in] relOffset the relative object offset
+ * @param[in] scr the owner script object, used by SCI1.1+
+ * @param[in] pcOffset the offset of the program counter, used by SCI0early and
+ *                     SCI3
+ */
+uint32 findOffset(const int16 relOffset, const Script *scr, const uint32 pcOffset);
+
 } // End of namespace Sci
 
 #endif // SCI_ENGINE_VM_H