[Scummvm-devel] Broken Sword 2 problem

[yotam barnoy]

If anyone's interested, the problem with BS2 on the PSP was a bug in gcc.

Here's the problematic code:

void Logic::logicUp(uint32 new_script) {
	debug(5, "new pc = %d", new_script & 0xffff);

	// going up a level - and we'll keep going this cycle
	_curObjectHub.setLogicLevel(_curObjectHub.getLogicLevel() + 1);

	assert(_curObjectHub.getLogicLevel() < 3);	// Can be 0, 1, 2
	logicReplace(new_script);
}

void Logic::logicReplace(uint32 new_script) {
	uint32 level = _curObjectHub.getLogicLevel();

	_curObjectHub.setScriptId(level, new_script);
	_curObjectHub.setScriptPc(level, new_script & 0xffff);
}

G++ optimized these 2 functions into 1 and came up with this code:

8934bc0 <_ZN6Sword25Logic7logicUpEj>:
 8934bc0:	27bdfff0 	addiu	sp,sp,-16
 8934bc4:	afb20008 	sw	s2,8(sp)
 8934bc8:	afb10004 	sw	s1,4(sp)
 8934bcc:	30b2ffff 	andi	s2,a1,0xffff  # s2 = new_scip & 0xffff
 8934bd0:	00a08821 	move	s1,a1 # s1 = new_scipt
 8934bd4:	3c0508aa 	lui	a1,0x8aa # a1 = 0x8aa0000
 8934bd8:	afb00000 	sw	s0,0(sp)
 8934bdc:	24a50d68 	addiu	a1,a1,3432 # a1 = 0x8aa3432
 8934be0:	00808021 	move	s0,a0 # s0 = this
 8934be4:	02403021 	move	a2,s2 # a2 = new_script & 0xffff
 8934be8:	afbf000c 	sw	ra,12(sp)
 8934bec:	0e286377 	jal	8a18ddc <_Z5debugiPKcz>
 8934bf0:	24040005 	li	a0,5 # a0 = 5 (jump)
 8934bf4:	8e0400d8 	lw	a0,216(s0)  # a0 = *(this + 216)
 8934bf8:	88850007 	lwl	a1,7(a0) # a1 =
 8934bfc:	98850004 	lwr	a1,4(a0) # a1 = logicLevel
 8934c00:	24a20001 	addiu	v0,a1,1 # v0 = logicLevel + 1
 8934c04:	a8820007 	swl	v0,7(a0) # 7(a0) = 0.5 new logicLevel
 8934c08:	2ca30003 	sltiu	v1,a1,3 # v1 = a1 < 3?
 8934c0c:	10600011 	beqz	v1,8934c54 <_ZN6Sword25Logic7logicUpEj+0x94> # assert
 8934c10:	b8820004 	swr	v0,4(a0) # 4(a0) = 0.5 new logicLevel
 8934c14:	24a20005 	addiu	v0,a1,5 # v0 = logicLevel + 5 ???
 8934c18:	00021080 	sll	v0,v0,0x2 # v0 = 4*logicLevel + 20 (scriptId offset)
 8934c1c:	00821021 	addu	v0,a0,v0 # v0 = &scriptId
 8934c20:	24a30008 	addiu	v1,a1,8 # v1 = logicLevel + 8
 8934c24:	a8510003 	swl	s1,3(v0) # scriptId[3] = new_script
 8934c28:	00031880 	sll	v1,v1,0x2 # v1 = logicLevel * 4 + 32
 8934c2c:	b8510000 	swr	s1,0(v0) # scriptId[0] = new_script
 8934c30:	00831821 	addu	v1,a0,v1 # v1 = *(this + 216)

Note the mistake in line 8934c00. v0 is used for incrementing the
logicLevel, and v0 is indeed saved into memory (ie.
_curObjectHub.setLogicLevel(_curObjectHub.getLogicLevel() + 1); )
But the optimization gcc made prevents it from realizing it needs to
use the newly incremented logicLevel value for the other calculations,
so instead it keeps using a1 in the rest of the function. This causes
it to write the new scriptId value in the wrong place, which causes
the VM to think its next scriptId is 0.

This is a very serious mistake in gcc 4.3.3.

Yotam

2010/8/27 yotam barnoy <yotambarnoy at gmail.com>:
> OK well thanks for trying.
>
> Just to keep everyone up to date on this suspenseful bug :)
>
> I've found the problematic addition -- it was simply the code of
> BufferedWriteStream, even when completely unused(!) Once I figured
> that out, I took this patch and applied it at different points in the
> history. Bisecting yielded another innocuous addition of a couple of
> variables to the BS2 class. This is starting to look more and more
> like the 'perfect storm' theory. I bisected yet again, and found
> another addition of variables. I'm hoping that eventually I'll get to
> something more substantial. My guess is that there's something either
> from BS2 itself or from the PSP code, and I'm already digging in code
> from around a year ago.
>
> I'd appreciate it if someone could try running the current SVN code
> (~52398) on the PS2 (another MIPS target). Try restarting and skipping
> both intro videos and check if it crashes.
>
> Thanks
> Yotam
>
>
> 2010/8/26 Torbjörn Andersson <eriktorbjorn at telia.com>:
>> On 2010-08-26 10:27, yotam barnoy wrote:
>>
>>> Can someone please run valgrind on Broken Sword 2 and see if they find
>>> anything? I'm trying to debug using assembly dumps but it seems the
>>> problem is too complex to corner that way.
>>
>> I played the very beginning of the game (freeing George from the ropes)
>> under Valgrind, and it didn't report any problems. I'm afraid I have no
>> theories whatsoever.
>>
>> Torbjörn Andersson
>>
>