SCI: Cleanup/paranoia checks

svn-id: r40711

3 files changed, 28 insertions, 30 deletions

diff --git a/engines/sci/engine/kscripts.cpp b/engines/sci/engine/kscripts.cpp
index 8610478b05..f0de8e6a5f 100644
--- a/engines/sci/engine/kscripts.cpp
+++ b/engines/sci/engine/kscripts.cpp
@@ -65,9 +65,6 @@ int invoke_selector(EngineState *s, reg_t object, int selector_id, int noinvalid
 	int slc_type;
 	StackPtr stackframe = k_argp + k_argc;
 
-	// Execution stack
-	ExecStack *xstack;
-
 	stackframe[0] = make_reg(0, selector_id);  // The selector we want to call
 	stackframe[1] = make_reg(0, argc); // Argument count
 
@@ -91,6 +88,7 @@ int invoke_selector(EngineState *s, reg_t object, int selector_id, int noinvalid
 	va_end(argp);
 
 	// Write "kernel" call to the stack, for debugging:
+	ExecStack *xstack;
 	xstack = add_exec_stack_entry(s, NULL_REG, NULL, NULL_REG, k_argc, k_argp - 1, 0, NULL_REG,
 	                              s->_executionStack.size()-1, SCI_XS_CALLEE_LOCALS);
 	xstack->selector = -42 - kfunct; // Evil debugging hack to identify kernel function
diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp
index a51a49ffb3..a9fddac956 100644
--- a/engines/sci/engine/vm.cpp
+++ b/engines/sci/engine/vm.cpp
@@ -285,19 +285,17 @@ ExecStack *execute_method(EngineState *s, uint16 script, uint16 pubfunct, StackP
 
 static void _exec_varselectors(EngineState *s) {
 	// Executes all varselector read/write ops on the TOS
-	// Now check the TOS to execute all varselector entries
-	if (!s->_executionStack.empty())
-		while (s->_executionStack.back().type == EXEC_STACK_TYPE_VARSELECTOR) {
-			// varselector access?
-			if (s->_executionStack.back().argc) { // write?
-				reg_t temp = s->_executionStack.back().variables_argp[1];
-				*(s->_executionStack.back().addr.varp) = temp;
-
-			} else // No, read
-				s->r_acc = *(s->_executionStack.back().addr.varp);
-
-			s->_executionStack.pop_back();
-		}
+	while (!s->_executionStack.empty() && s->_executionStack.back().type == EXEC_STACK_TYPE_VARSELECTOR) {
+		ExecStack &xs = s->_executionStack.back();
+		// varselector access?
+		if (xs.argc) { // write?
+			*(xs.addr.varp) = xs.variables_argp[1];
+
+		} else // No, read
+			s->r_acc = *(xs.addr.varp);
+
+		s->_executionStack.pop_back();
+	}
 }
 
 ExecStack *send_selector(EngineState *s, reg_t send_obj, reg_t work_obj, StackPtr sp, int framesize, StackPtr argp) {
@@ -364,6 +362,7 @@ ExecStack *send_selector(EngineState *s, reg_t send_obj, reg_t work_obj, StackPt
 			}
 
 			script_error_flag = script_debug_flag = 1;
+
 			break;
 
 		case kSelectorVariable:
@@ -507,6 +506,9 @@ ExecStack *add_exec_stack_entry(EngineState *s, reg_t pc, StackPtr sp, reg_t obj
 	xstack.type = EXEC_STACK_TYPE_CALL; // Normal call
 
 	s->_executionStack.push_back(xstack);
+	// FIXME: push_back can cause the storage of _executionStack to be reallocated.
+	// As a result, any pointers to a member of _executionStack becomes invalid.
+	// This can cause severe breakage since run_vm does exactly that...
 
 	return &(s->_executionStack.back());
 }
@@ -563,6 +565,8 @@ static void gc_countdown(EngineState *s) {
 static const byte _fake_return_buffer[2] = {op_ret << 1, op_ret << 1};
 
 void run_vm(EngineState *s, int restoring) {
+	assert(s);
+
 	reg_t *variables[4]; // global, local, temp, param, as immediate pointers
 	reg_t *variables_base[4]; // Used for referencing VM ops
 	SegmentId variables_seg[4]; // Same as above, contains segment IDs
@@ -592,11 +596,6 @@ void run_vm(EngineState *s, int restoring) {
 		return;
 	}
 
-	if (NULL == s) {
-		sciprintf("vm.c: run_vm(): NULL passed for \"s\"\n");
-		return;
-	}
-
 	if (!restoring)
 		s->execution_stack_base = s->_executionStack.size()-1;
 
@@ -625,12 +624,13 @@ void run_vm(EngineState *s, int restoring) {
 	while (1) {
 		byte opcode;
 		int old_pc_offset;
-		StackPtr old_sp = xs->sp;
+		StackPtr old_sp;
 		byte opnumber;
 		int var_type; // See description below
 		int var_number;
 
 		old_pc_offset = xs->addr.pc.offset;
+		old_sp = xs->sp;
 
 		if (s->_executionStackPosChanged) {
 			Script *scr;
@@ -1445,11 +1445,14 @@ void run_vm(EngineState *s, int restoring) {
 		if (s->_executionStackPosChanged) // Force initialization
 			xs = xs_new;
 
-#ifndef DISABLE_VALIDATIONS
+//#ifndef DISABLE_VALIDATIONS
 		if (xs != &(s->_executionStack.back())) {
-			sciprintf("Error: xs is stale (%d vs %d); last command was %02x\n", (int)(xs - &s->_executionStack[0]), s->_executionStack.size()-1, opnumber);
+			warning("xs is stale (%d/%p vs %d/%p); last command was %02x\n",
+					(int)(xs - &s->_executionStack[0]), (void *)xs,
+					s->_executionStack.size()-1, (void *)&(s->_executionStack.back()),
+					opnumber);
 		}
-#endif
+//#endif
 		if (script_error_flag) {
 			_debug_step_running = 0; // Stop multiple execution
 			_debug_seeking = 0; // Stop special seeks
@@ -1517,10 +1520,7 @@ static SelectorType _lookup_selector_function(EngineState *s, int seg_id, Object
 
 		if (index >= 0) {
 			if (fptr) {
-				if (s->version < SCI_VERSION_1_1)
-					*fptr = make_reg(obj->pos.segment, READ_LE_UINT16((byte *)(obj->base_method + index + obj->methods_nr + 1)));
-				else
-					*fptr = make_reg(obj->pos.segment, READ_LE_UINT16((byte *)(obj->base_method + index * 2 + 2)));
+				*fptr = VM_OBJECT_READ_FUNCTION(obj, index);
 			}
 
 			return kSelectorMethod;
diff --git a/engines/sci/gfx/font.cpp b/engines/sci/gfx/font.cpp
index 4b29aeef3d..185e32c678 100644
--- a/engines/sci/gfx/font.cpp
+++ b/engines/sci/gfx/font.cpp
@@ -63,7 +63,7 @@ bool gfxr_font_calculate_size(Common::Array<TextFragment> &fragments, gfx_bitmap
 
 	while ((foo = *text++)) {
 		if (foo >= font->chars_nr) {
-			GFXWARN("Invalid char 0x%02x (max. 0x%02x) encountered in text string '%s', font %04x\n",
+			error("Invalid char 0x%02x (max. 0x%02x) encountered in text string '%s', font %04x",
 			        foo, font->chars_nr, text, font->ID);
 			if (font->chars_nr > ' ')
 				foo = ' ';