- Merged the SCI0 scriptRelocate() and SCI11 heapRelocate() functions inside relocate(). scriptRelocate checked one more relocation entry, which seems wrong, so we're now checking for the correct number of relocations in all SCI versions

- Re-added the error when script + heap exceed 64KB (better than an assert) - this should theoretically never happen, and it never has for the games tested
- Removed the relocated sanity check - again, it shouldn't occur (else something else is wrong)

svn-id: r49332

3 files changed, 38 insertions, 56 deletions

diff --git a/engines/sci/engine/script.cpp b/engines/sci/engine/script.cpp
index 99a567d1e7..aa975cc1d0 100644
--- a/engines/sci/engine/script.cpp
+++ b/engines/sci/engine/script.cpp
@@ -419,7 +419,7 @@ int script_instantiate_sci0(ResourceManager *resMan, SegManager *segMan, int scr
 	} while (objType != 0 && curOffset < scr->getScriptSize() - 2);
 
 	if (relocation >= 0)
-		scr->scriptRelocate(make_reg(seg_id, relocation));
+		scr->relocate(make_reg(seg_id, relocation));
 
 	return seg_id;		// instantiation successful
 }
@@ -439,7 +439,7 @@ int script_instantiate_sci11(ResourceManager *resMan, SegManager *segMan, int sc
 	int heapStart = scr->getScriptSize();
 	segMan->scriptInitialiseLocals(make_reg(seg_id, heapStart + 4));
 	segMan->scriptInitialiseObjectsSci11(seg_id);
-	scr->heapRelocate(make_reg(seg_id, READ_SCI11ENDIAN_UINT16(scr->_heapStart)));
+	scr->relocate(make_reg(seg_id, READ_SCI11ENDIAN_UINT16(scr->_heapStart)));
 
 	return seg_id;
 }
diff --git a/engines/sci/engine/segment.cpp b/engines/sci/engine/segment.cpp
index a8c4eb73f6..bdd9fbc966 100644
--- a/engines/sci/engine/segment.cpp
+++ b/engines/sci/engine/segment.cpp
@@ -100,7 +100,6 @@ Script::Script() : SegmentObj(SEG_TYPE_SCRIPT) {
 	_localsSegment = 0;
 	_localsBlock = NULL;
 
-	_relocated = false;
 	_markedAsDeleted = 0;
 }
 
@@ -125,7 +124,6 @@ void Script::init(int script_nr, ResourceManager *resMan) {
 
 	_codeBlocks.clear();
 
-	_relocated = false;
 	_markedAsDeleted = false;
 
 	_nr = script_nr;
@@ -139,6 +137,14 @@ void Script::init(int script_nr, ResourceManager *resMan) {
 	if (getSciVersion() == SCI_VERSION_0_EARLY) {
 		_bufSize += READ_LE_UINT16(script->data) * 2;
 	} else if (getSciVersion() >= SCI_VERSION_1_1) {
+		/**
+		 * In SCI11, the heap was in a separate space from the script.
+		 * We append it to the end of the script, and adjust addressing accordingly.
+		 * However, since we address the heap with a 16-bit pointer, the combined
+		 * size of the stack and the heap must be 64KB. So far this has worked
+		 * for SCI11, SCI2 and SCI21 games. SCI3 games use a different script format,
+		 * and theoretically they can exceed the 64KB boundary using relocation.
+		 */
 		Resource *heap = resMan->findResource(ResourceId(kResourceTypeHeap, script_nr), 0);
 		_bufSize += heap->size;
 		_heapSize = heap->size;
@@ -149,7 +155,11 @@ void Script::init(int script_nr, ResourceManager *resMan) {
 			_scriptSize++;
 		}
 
-		assert(_bufSize <= 65535);
+		// As mentioned above, the script and the heap together should not exceed 64KB
+		if (_bufSize > 65535)
+			error("Script and heap sizes combined exceed 64K. This means a fundamental "
+					"design bug was made regarding SCI1.1 and newer games.\nPlease "
+					"report this error to the ScummVM team");
 	}
 }
 
@@ -253,14 +263,24 @@ void Script::scriptAddCodeBlock(reg_t location) {
 	_codeBlocks.push_back(cb);
 }
 
-void Script::scriptRelocate(reg_t block) {
-	VERIFY(block.offset < (uint16)_bufSize && READ_SCI11ENDIAN_UINT16(_buf + block.offset) * 2 + block.offset < (uint16)_bufSize,
+void Script::relocate(reg_t block) {
+	byte *heap = _buf;
+	uint16 heapSize = (uint16)_bufSize;
+	uint16 heapOffset = 0;
+
+	if (getSciVersion() >= SCI_VERSION_1_1) {
+		heap = _heapStart;
+		heapSize = (uint16)_heapSize;
+		heapOffset = _scriptSize;
+	}
+
+	VERIFY(block.offset < (uint16)heapSize && READ_SCI11ENDIAN_UINT16(heap + block.offset) * 2 + block.offset < (uint16)heapSize,
 	       "Relocation block outside of script\n");
 
-	int count = READ_SCI11ENDIAN_UINT16(_buf + block.offset);
+	int count = READ_SCI11ENDIAN_UINT16(heap + block.offset);
 
-	for (int i = 0; i <= count; i++) {
-		int pos = READ_SCI11ENDIAN_UINT16(_buf + block.offset + 2 + (i * 2));
+	for (int i = 0; i < count; i++) {
+		int pos = READ_SCI11ENDIAN_UINT16(heap + block.offset + 2 + (i * 2)) + heapOffset;
 		// This occurs in SCI01/SCI1 games where every other export
 		// value is zero. I have no idea what it's supposed to mean.
 		//
@@ -281,10 +301,13 @@ void Script::scriptRelocate(reg_t block) {
 					done = true;
 			}
 
-			for (k = 0; !done && k < _codeBlocks.size(); k++) {
-				if (pos >= _codeBlocks[k].pos.offset &&
-				        pos < _codeBlocks[k].pos.offset + _codeBlocks[k].size)
-					done = true;
+			// Sanity check for SCI0-SCI1
+			if (getSciVersion() < SCI_VERSION_1_1) {
+				for (k = 0; !done && k < _codeBlocks.size(); k++) {
+					if (pos >= _codeBlocks[k].pos.offset &&
+							pos < _codeBlocks[k].pos.offset + _codeBlocks[k].size)
+						done = true;
+				}
 			}
 
 			if (!done) {
@@ -303,44 +326,6 @@ void Script::scriptRelocate(reg_t block) {
 	}
 }
 
-void Script::heapRelocate(reg_t block) {
-	VERIFY(block.offset < (uint16)_heapSize && READ_SCI11ENDIAN_UINT16(_heapStart + block.offset) * 2 + block.offset < (uint16)_bufSize,
-	       "Relocation block outside of script\n");
-
-	if (_relocated)
-		return;
-	_relocated = true;
-	int count = READ_SCI11ENDIAN_UINT16(_heapStart + block.offset);
-
-	for (int i = 0; i < count; i++) {
-		int pos = READ_SCI11ENDIAN_UINT16(_heapStart + block.offset + 2 + (i * 2)) + _scriptSize;
-
-		if (!relocateLocal(block.segment, pos)) {
-			bool done = false;
-			uint k;
-
-			ObjMap::iterator it;
-			const ObjMap::iterator end = _objects.end();
-			for (it = _objects.begin(); !done && it != end; ++it) {
-				if (it->_value.relocate(block.segment, pos, _scriptSize))
-					done = true;
-			}
-
-			if (!done) {
-				printf("While processing relocation block %04x:%04x:\n", PRINT_REG(block));
-				printf("Relocation failed for index %04x (%d/%d)\n", pos, i + 1, count);
-				if (_localsBlock)
-					printf("- locals: %d at %04x\n", _localsBlock->_locals.size(), _localsOffset);
-				else
-					printf("- No locals\n");
-				for (it = _objects.begin(), k = 0; it != end; ++it, ++k)
-					printf("- obj#%d at %04x w/ %d vars\n", k, it->_value.getPos().offset, it->_value.getVarCount());
-				error("Breakpoint in %s, line %d", __FILE__, __LINE__);
-			}
-		}
-	}
-}
-
 void Script::incrementLockers() {
 	_lockers++;
 }
diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h
index 127ab28b7e..7b0828ab6b 100644
--- a/engines/sci/engine/segment.h
+++ b/engines/sci/engine/segment.h
@@ -355,7 +355,6 @@ public:
 	LocalVariables *_localsBlock;
 
 	Common::Array<CodeBlock> _codeBlocks;
-	bool _relocated;
 	bool _markedAsDeleted;
 
 public:
@@ -409,9 +408,7 @@ public:
 	 * @param obj_pos	Location (segment, offset) of the block
 	 * @return			Location of the relocation block
 	 */
-	void scriptRelocate(reg_t block);
-
-	void heapRelocate(reg_t block);
+	void relocate(reg_t block);
 
 private:
 	bool relocateLocal(SegmentId segment, int location);