#5704 closed defect (fixed)
Tinsel: DW 1 & 2 Segfault on start
| Reported by: | SF/ezekiel000 | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: Tinsel |
| Version: | Keywords: | ||
| Cc: | Game: | Discworld |
Description
Both Discworld 1 & 2 crash ScummVM with this on the terminal: User picked target 'dw' (gameid 'tinsel')... Looking for a plugin supporting this gameid... Tinsel Starting 'Tinsel engine game' Connected to Alsa sequencer client [14:0] ALSA client initialised [128:0] Segmentation fault
And Discworld 2: User picked target 'dw2-gb' (gameid 'tinsel')... Looking for a plugin supporting this gameid... Tinsel Starting 'Tinsel engine game' Segmentation fault
Running a my own compiled ScummVM built 17/05 on Ubuntu 11.04 amd64: ScummVM 1.4.0git (May 17 2011 22:58:32) Features compiled in: Vorbis ALSA SEQ TiMidity RGB zLib Theora
With these compile options: ./configure --enable-myst --enable-riven --enable-sword25 --disable-he --disable-agi --disable-agos --disable-agos2 --disable-cine --disable-cruise --disable-draci --disable-drascula --disable-gob --disable-groovie --disable-hugo --disable-kyra --disable-lure --disable-parallaction --disable-queen --disable-saga --disable-ihnm --disable-sci --disable-sky --disable-teenagent --disable-toon --disable-touche --disable-tucker --disable-debug --disable-mt32emu --disable-mad --disable-flac --disable-fluidsynth --disable-readline --enable-release
Ticket imported from: #3303799. Ticket imported from: bugs/5704.
Change History (8)
comment:1 by , 13 years ago
| Owner: | set to |
|---|
comment:2 by , 13 years ago
comment:3 by , 13 years ago
Can't replicate this crash with the latest Git master.
Could you: a) try to compile with just "./configure && make clean && make" and see if the crash still occurs. b) confirm the git revision you are using i.e. ./scummvm --version from a build without --enable-release.
If the crash still occurs even with the options in a), then please try updating to the latest Git master and repeating a) and b).
P.S. If you are trying to disable all engines but a small selection, "./configure --disable-all-engines --enable-myst --enable-riven --enable-sword25" would make for a far more readable command...
comment:4 by , 13 years ago
I updated to the latest git master and did just: ./configure && make clean && make
Version: ezekiel@alice:~/scummvm-scummvm-f830d68$ ./scummvm -v ScummVM 1.4.0git (May 18 2011 12:31:02) Features compiled in: Vorbis ALSA SEQ TiMidity RGB zLib Theora
And still got segfaults on both discworld games.
P.S. Thanks for pointing out the disable all engines flag, it makes my command a bit shorter: ./configure --disable-all-engines --enable-made --enable-mohawk --enable-myst --enable-riven --enable-scumm --enable-scumm-7-8 --enable-sword1 --enable-sword2 --enable-sword25 --enable-tinsel --disable-debug --disable-mt32emu --disable-mad --disable-flac --disable-fluidsynth --disable-readline --enable-release
comment:5 by , 13 years ago
dreammaster, you are right about deducing the commit which is the cause of this crash, and I reverted it. However, the code it touches is most definitely *not* correct: It casts an OBJECT** to OBJECT*. This is *very* fishy, and should be analyzed on its own.
comment:6 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:7 by , 13 years ago
Ah, I see: The PLAYFIELD struct has as first entry an OBJECT pointer, just like the OBJECT itself. So in fact, the dirty trick used by tinsel here is to pretend the PLAYFIELD is an OBJECT, using it as the imaginary HEAD of the linked list of objects PLAYFIELD::pDispList points to.
I'll document this more explicitly in the code, and we probably still should rewrite the code to avoid that, as it could cause problem with aliasing analysis in compilers.
comment:8 by , 13 years ago
Thanks for looking into further. That makes sense, and explains why the cast was being done, which definitely looked weird.
Hi Fingolfin,
I think the problem was caused by your commit 'Remove unnecessary casts'. This has removed pointer to object operations which are necessary. In background.cpp:165, it's meant to return a pointer to the pDispList field, rather than the pDispList value itself.
I'm not sure if there's some cool way with git to undo that single commit, so I'll leave it up to you to decide how to proceed.