Menu

#6345 TINSEL: DW2 is broken on big endian hardware

Discworld II
closed-fixed
5
2014-08-22
2013-06-08
canavan
No

As far as I can tell, both Discworld and Discworld II may be broken on big endian hardware since commit ce87175bede46c1bb938b73484e1db05212defbd.

TINSEL: Revert the BE -> LE resource conversion for DW1 Mac

scummvm 1.5.0 works, 1.6.0 doesn't. Both Games work if I check out af667771a918370988cc656412e2ddea3d1d44a3. I'm using N32 binaries on IRIX with the MIPSPro compilers.

discworld (detected as CD/DOS/English)fails with an assertion:

$ ./scummvm
WARNING: Could not find theme 'scummmodern' falling back to builtin!
User picked target 'dw-cd' (gameid 'tinsel')...
Looking for a plugin supporting this gameid... Tinsel
Starting 'Tinsel engine game'
Assertion failed: handle < g_numHandles, file engines/tinsel/handle.cpp, line 329, pid 65049

discworld II (detected as CD/DOS/English (GB)) dies with a segfault:

Process 65090 (scummvm) stopped on signal SIGSEGV: Segmentation violation (handler sig_fixup_mask) at [Tinsel::GetBytes(const unsigned char*,const Tinsel::WorkaroundEntry*&,int&,unsigned int):540 +0x68,0x1049ba24]
540 tmp = code[ip++ * (TinselV0 ? 4 : 1)];
(dbx) active thread 0x10000
Thread 0x10000 is active
(dbx) where

Thread 0x10000
> 0 Tinsel::GetBytes(const unsigned char*,const Tinsel::WorkaroundEntry*&,int&,unsigned int)(scriptCode = 0x113e0e38, wkEntry = 0x7ffb6b04, ip = 0x7ffb6b00, numBytes = 0) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/pcode.cpp":540, 0x1049ba24]
1 Tinsel::Interpret(Common::CoroBaseContext*&,Tinsel::INT_CONTEXT*)(coroParam = 0x10bad4c0, ic = 0x10b36f18) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/pcode.cpp":608, 0x1049bf70]
2 Tinsel::SceneTinselProcess(Common::CoroBaseContext*&,const void*)(coroParam = 0x10bbbb60, param = 0x10bbbb84) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/scene.cpp":176, 0x104d946c]
3 Common::CoroutineScheduler::schedule(void)(this = 0x10bc3950) ["/usr/people/canavan/src/scummvm/git/scummvm/common/coroutines.cpp":231, 0x1080c37c]
4 Tinsel::TinselEngine::NextGameCycle(void)(this = 0x10bb6568) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/tinsel.cpp":1032, 0x1048cd10]
5 Tinsel::TinselEngine::run(void)(this = 0x10bb6568, <no name> = 0x7ffb7538) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/tinsel.cpp":980, 0x1048b6ec]
6 ::runGame(const PluginSubclass<MetaEngine>*,OSystem&,const Common::String&)(<no name> = 0x7ffb7cf8, plugin = 0x10a22528, system = 0x10a28fb0, edebuglevels = 0x7ffb7738) ["/usr/people/canavan/src/scummvm/git/scummvm/base/main.cpp":226, 0x100e4190]
7 ::scummvm_main(argc = 1, argv = 0x7ffb7ee4) ["/usr/people/canavan/src/scummvm/git/scummvm/base/main.cpp":452, 0x100e57a4]
8 ::main(argc = 1, argv = 0x7ffb7ee4) ["/usr/people/canavan/src/scummvm/git/scummvm/backends/platform/sdl/posix/posix-main.cpp":45, 0x100e0410]
9 __start() ["/xlv55/kudzu-apr12/work/irix/lib/libc/libc_n32_M4/csu/crt1text.s":177, 0x100db1a8]

Even when compiling with -g0 (no optimization), I haven't been able to convice the debugger to provide any useable info about local/stack variables in Tinsel::GetBytes.

Discussion

1 2 > >> (Page 1 of 2)
  • Willem Jan Palenstijn

    Just to be sure, you have the DOS versions of DW1 and DW2? And do you have their 'platform' in the 'Edit game' dialog for those entries set to DOS?

    Since in theory I don't think commit ce87175bede46c1bb938b73484e1db05212defbd has any effect on DOS versions of DW1/2.

     
  • canavan

    canavan - 2013-06-08

    Autodetection has set the platform to DOS. The CDs for both DW and DW2 bear "PC CDROM" labels, the boxes list "IBM-compatible (PC|computer)" in the requirements section.

     
  • Filippos Karapetis

    I don't have a BE system to test, so could you please try and bisect this?

    As wjp said, commit b05fa7f20414d6a7571a9ba52f542e527f598c62 doesn't seem to be the cause of this...

     
  • Filippos Karapetis

    Hm... if you are talking about commit, there's one thing that I found:

    Inside palette.cpp:
    // get hardware palette pointer
    - pPalette = (const PALETTE *)LockMem(pDACtail->pal.hRGBarray);
    + pPalette = (const PALETTE *)LockMem(FROM_32(pDACtail->pal.hRGBarray));

    I tried to make sure that I didn't add any endianess handling in that commit, but it seems that line slipped through by accident :(

    If that is the offending commit, could you try changing that line to what it was? i.e.
    pPalette = (const PALETTE *)LockMem(pDACtail->pal.hRGBarray);

     
  • canavan

    canavan - 2013-06-09

    I'm not sure if I've interpreted the output of git log correctly. A round of git bisect identifies this release as the cause:

    c90d56355fa0bbcdd3122f3e376e5609422338b3 is the first bad commit
    commit c90d56355fa0bbcdd3122f3e376e5609422338b3
    Author: Filippos Karapetis <bluegr@gmail.com>
    Date: Mon Dec 10 17:18:56 2012 +0200

    TINSEL: Simplify the scene entrance handling code

    This also reverts the rest of the BE resource handling code

    :040000 040000 acfc6e90597bacc597c3673e559a69356adc1195 705e229062666e136e601323e74d8fbea6f82d36 M engines

    manually testing show that
    git checkout c90d56355fa0bbcdd3122f3e376e5609422338b3 is bad
    git checkout c6cf4827d719c1833ce4d7e108410db81f00c358 is bad.

     
  • Filippos Karapetis

    So can you verify that:

    da971e38a565a52af92c4d419546820c8af9955b works
    while
    c6cf4827d719c1833ce4d7e108410db81f00c358 doesn't work?

     
  • canavan

    canavan - 2013-06-09

    ading the FROM_32() to the pPalette assignment in HEAD doesn't change anything - scummvm still crashes.

    I may be able to provide access to a big endian system for testing, however that one would be somewhat slowish with only 2x 400MHz CPUs.

     
  • digitall

    digitall - 2013-06-09
    • summary: TINSEL broken on big endian hardware --> TINSEL: Engine broken on big endian hardware
     
  • digitall

    digitall - 2013-06-09

    There is the method described here which may be of help to allow you to run a BE VM machine on a fast LE (x86) native machine:
    http://wiki.scummvm.org/index.php/HOWTO-Debug-Endian-Issues

     
  • canavan

    canavan - 2013-06-09
    • summary: TINSEL: Engine broken on big endian hardware --> TINSEL broken on big endian hardware
     
  • canavan

    canavan - 2013-06-09

    da971e38a565a52af92c4d419546820c8af9955b
    and
    c6cf4827d719c1833ce4d7e108410db81f00c358 both work.

    My claim below that c6cf... is bad appears to be a cut-and-paste error.

    c90d56355fa0bbcdd3122f3e376e5609422338b3 doesn't work

     
  • canavan

    canavan - 2013-06-09

    Just checked with gcc 4.7, same problem as with the MIPSPro compilers.

     
  • digitall

    digitall - 2013-06-10

    canavan: Thank you for the further debugging information.

    Can you try running a Git bisection to locate the exact point where the breakage/regression was introduced please?
    If you are not familar with bisection, see:
    http://git-scm.com/book/en/Git-Tools-Debugging-with-Git

     
  • canavan

    canavan - 2013-06-10

    No new surprises running git bisect again:

    scummvm :) $ git bisect good
    c90d56355fa0bbcdd3122f3e376e5609422338b3 is the first bad commit
    commit c90d56355fa0bbcdd3122f3e376e5609422338b3
    Author: Filippos Karapetis <bluegr@gmail.com>
    Date: Mon Dec 10 17:18:56 2012 +0200

    TINSEL: Simplify the scene entrance handling code

    This also reverts the rest of the BE resource handling code

    :040000 040000 acfc6e90597bacc597c3673e559a69356adc1195 705e229062666e136e601323e74d8fbea6f82d36 M engines

     
  • digitall

    digitall - 2013-06-11

    canavan: Thanks for doing that... as suspected. Could you also try reverting / splitting down the change chunks in that commit (This will have to be a manual process as I don't think Git offers that option) and see which of the logical changes is provoking this regression?

     
  • Filippos Karapetis

    @canavan: I've fixed the regression in that commit in commit 4b69071. Please, try fetching the newest changes on master and try again. Many thanks for your bisecting work :)

     
  • Willem Jan Palenstijn

    I've just tested DW1 on debian/powerppc, and I can confirm that commit fixed the crash.

     
  • Willem Jan Palenstijn

    • assigned_to: nobody --> thebluegr
    • status: open --> closed-fixed
     
  • canavan

    canavan - 2013-06-18

    4b69071 does indeed fix the assertion when starting DW1, but DW2 still segfaults here. None of the "obvious" (to me, e.g. the handling of hProcess, numProcess, hScript, hMusicScript) other changes appear to fix DW2 while keeping DW1 usable.

     
  • Willem Jan Palenstijn

    • status: closed-fixed --> open
     
  • Filippos Karapetis

    Can you provide any more information? What kind of error are you getting? Could you please help in bisecting the last commit that works with DW2? You can apply commit 4b69071 while bisecting - IIRC, that part of the code hasn't been touched by any of the subsequent commits

     
  • Filippos Karapetis

    I assume (from the lack of responses) that DW1 has been fixed on BE systems now, thus I'm changing the bug description to only mention DW2

     
  • Filippos Karapetis

    • summary: TINSEL broken on big endian hardware --> TINSEL: DW2 is broken on big endian hardware
     
  • canavan

    canavan - 2013-09-08

    Apologies for the late response. DW1 is indeed working after 4b69071. Even with 4b69071 applied, git bisect still points at c90d56355fa0bbcdd3122f3e376e5609422338b3 as the culprit for the DW2 crash on startup.

    c90d56355fa0bbcdd3122f3e376e5609422338b3 is the first bad commit
    commit c90d56355fa0bbcdd3122f3e376e5609422338b3
    Author: Filippos Karapetis <bluegr@gmail.com>
    Date: Mon Dec 10 17:18:56 2012 +0200

    TINSEL: Simplify the scene entrance handling code

    This also reverts the rest of the BE resource handling code

    :040000 040000 acfc6e90597bacc597c3673e559a69356adc1195 705e229062666e136e601323e74d8fbea6f82d36 M engines

    The backtrace still looks the same:

    > 0 Tinsel::GetBytes(const unsigned char*,const Tinsel::WorkaroundEntry*&,int&,unsigned int)(scriptCode = <illegal>, wkEntry = <illegal>, ip = <illegal>, numBytes = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/pcode.cpp":541, 0x1016cc60]
    1 Tinsel::Interpret(Common::CoroBaseContext*&,Tinsel::INT_CONTEXT*)(coroParam = <illegal>, ic = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/pcode.cpp":608, 0x1016ce9c]
    2 Tinsel::SceneTinselProcess(Common::CoroBaseContext*&,const void*)(coroParam = <illegal>, param = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/scene.cpp":177, 0x1019d5b0]
    3 Common::CoroutineScheduler::schedule(void)(this = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/common/coroutines.cpp":231, 0x104003e8]
    4 Tinsel::TinselEngine::NextGameCycle(void)(this = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/tinsel.cpp":1031, 0x10160614]
    5 Tinsel::TinselEngine::run(void)(this = <illegal>, <no name> = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/engines/tinsel/tinsel.cpp":976, 0x1015f638]
    6 ::runGame(const PluginSubclass<MetaEngine>*,OSystem&,const Common::String&)(<no name> = <illegal>, plugin = <illegal>, system = <illegal>, edebuglevels = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/base/main.cpp":229, 0x10090a38]
    7 ::scummvm_main(argc = 0, argv = 0x1054c330) ["/usr/people/canavan/src/scummvm/git/scummvm/base/main.cpp":474, 0x10091d6c]
    8 ::main(argc = <illegal>, argv = <illegal>) ["/usr/people/canavan/src/scummvm/git/scummvm/backends/platform/sdl/posix/posix-main.cpp":45, 0x1008db88]
    9 __start() ["/xlv55/kudzu-apr12/work/irix/lib/libc/libc_n32_M4/csu/crt1text.s":177, 0x100872e8]

     
  • Willem Jan Palenstijn

    bluegr: I've tried to debug this on a BE machine a while ago, but got lost in the maze of commits and reverts mixed together around the commit canavan's bisect points to. Can you summarize what these commits are meant to do?

     
1 2 > >> (Page 1 of 2)