[Scummvm-devel] Broken Sword 2 problem

[yotam barnoy]

For the (hopefully) last episode in this bug saga, see commit 52473.
Yotam


On Tue, Aug 31, 2010 at 2:24 PM, yotam barnoy <yotambarnoy at gmail.com> wrote:
> 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
>>>
>>
>