SCI: PHANT1: Crash on startup

[dafioram]

ScummVM: 2.1.0git-2295-gce586aa95c
Game: Phant1 DOS

When starting up the game it crashes. Mentioned by eriktorbjorn on IRC. Phant2 works just fine.

#0  0x000055555570578c in Sci::SCALER_Scale<false, Sci::READER_Compressed>::read() (this=0x7ffffffb6df0) at engines/sci/graphics/celobj32.cpp:303
#1  0x0000555555704705 in Sci::RENDERER<Sci::MAPPER_NoMD, Sci::SCALER_Scale<false, Sci::READER_Compressed>, false>::draw(Graphics::Surface&, Common::Rect const&, Common::Point const&) const (this=0x7ffffffb6dd0, target=..., targetRect=..., scaledPosition=...) at engines/sci/graphics/celobj32.cpp:737
#2  0x00005555556fe4e5 in Sci::CelObj::render<Sci::MAPPER_NoMD, Sci::SCALER_Scale<false, Sci::READER_Compressed> >(Graphics::Surface&, Common::Rect const&, Common::Point const&, Common::Rational const&, Common::Rational const&) const (this=0x555556e1ae80, target=..., targetRect=..., scaledPosition=..., scaleX=..., scaleY=...) at engines/sci/graphics/celobj32.cpp:764
#3  0x00005555556f8db3 in Sci::CelObj::scaleDrawNoMD(Graphics::Surface&, Common::Rational const&, Common::Rational const&, Common::Rect const&, Common::Point const&) const (this=0x555556e1ae80, target=..., scaleX=..., scaleY=..., targetRect=..., scaledPosition=...) at engines/sci/graphics/celobj32.cpp:868
#4  0x00005555556f80a0 in Sci::CelObj::draw(Graphics::Surface&, Sci::ScreenItem const&, Common::Rect const&) const (this=0x555556e1ae80, target=..., screenItem=..., targetRect=...) at engines/sci/graphics/celobj32.cpp:574
#5  0x00005555556f80eb in Sci::CelObj::draw(Graphics::Surface&, Sci::ScreenItem const&, Common::Rect const&, bool) (this=0x555556e1ae80, target=..., screenItem=..., targetRect=..., mirrorX=false) at engines/sci/graphics/celobj32.cpp:584
#6  0x000055555570ee19 in Sci::GfxFrameout::drawScreenItemList(Sci::DrawList const&) (this=0x555556bb0270, screenItemList=...)
    at engines/sci/graphics/frameout.cpp:930
#7  0x000055555570cabb in Sci::GfxFrameout::frameOut(bool, Common::Rect const&) (this=0x555556bb0270, shouldShowBits=true, eraseRect=...)
    at engines/sci/graphics/frameout.cpp:432
#8  0x0000555555723630 in Sci::GfxTransitions32::processShowStyles() (this=0x555556bb0130) at engines/sci/graphics/transitions32.cpp:126
#9  0x000055555570fd1a in Sci::GfxFrameout::kernelFrameOut(bool) (this=0x555556bb0270, shouldShowBits=true) at engines/sci/graphics/frameout.cpp:1137
#10 0x00005555556f347f in Sci::kFrameOut(Sci::EngineState*, int, Sci::reg_t*) (s=0x555556aabdd0, argc=0, argv=0x555556b4937c)
    at engines/sci/engine/kgraphics32.cpp:235
#11 0x0000555555691e81 in Sci::callKernelFunc(Sci::EngineState*, int, int) (s=0x555556aabdd0, kernelCallNr=33, argc=0) at engines/sci/engine/vm.cpp:376
#12 0x0000555555694130 in Sci::run_vm(Sci::EngineState*) (s=0x555556aabdd0)
    at engines/sci/engine/vm.cpp:896
#13 0x000055555562ac7c in Sci::SciEngine::runGame() (this=0x55555604d3f0)
    at engines/sci/sci.cpp:692
#14 0x00005555556298b9 in Sci::SciEngine::run() (this=0x55555604d3f0)
    at engines/sci/sci.cpp:459
#15 0x00005555555fa1a2 in runGame(Plugin const*, OSystem&, Common::String const&) (plugin=0x555555f8eb80, system=..., edebuglevels=...) at base/main.cpp:264
#16 0x00005555555fb3c5 in scummvm_main(int, char const* const*) (argc=1, argv=0x7fffffffdfb8) at base/main.cpp:530
#17 0x00005555555f832c in main(int, char**) (argc=1, argv=0x7fffffffdfb8)
    at backends/platform/sdl/posix/posix-main.cpp:45

[eriktorbjorn]

This seems to be the offending commit, according to git bisect:

f8c32b07e7bc219e81df6d1f27209ac28d3ced6b is the first bad commit
commit f8c32b07e7bc219e81df6d1f27209ac28d3ced6b
Author: Daniel Wolf <dwolf@dannad.de>
Date:   Fri Mar 16 11:57:26 2018 +0100

    SCI: Use LarryScale cel scaler instead of nearest-neighbor

:040000 040000 81511969972e66a2ec46f2600a9f2e16dfc7bc50 ed855858ae8c5a8b1e67be7c9d302dd07993b6de M	engines

[eriktorbjorn]

Here's a Valgrind log:

==32727== Memcheck, a memory error detector
==32727== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==32727== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==32727== Command: ./scummvm
==32727== 
User picked target 'phantasmagoria' (gameid 'sci')...
  Looking for a plugin supporting this gameid... SCI [SCI0, SCI01, SCI10, SCI11, SCI32]
  Starting 'Sierra SCI Game'
Skipping blacklisted patch file 65535.map
==32727== Invalid read of size 1
==32727==    at 0x1944F6C: Sci::SCALER_Scale<false, Sci::READER_Compressed>::read() (celobj32.cpp:303)
==32727==    by 0x1943F08: Sci::RENDERER<Sci::MAPPER_NoMD, Sci::SCALER_Scale<false, Sci::READER_Compressed>, false>::draw(Graphics::Surface&, Common::Rect const&, Common::Point const&) const (celobj32.cpp:737)
==32727==    by 0x193E08F: void Sci::CelObj::render<Sci::MAPPER_NoMD, Sci::SCALER_Scale<false, Sci::READER_Compressed> >(Graphics::Surface&, Common::Rect const&, Common::Point const&, Common::Rational const&, Common::Rational const&) const (celobj32.cpp:764)
==32727==    by 0x193920E: Sci::CelObj::scaleDrawNoMD(Graphics::Surface&, Common::Rational const&, Common::Rational const&, Common::Rect const&, Common::Point const&) const (celobj32.cpp:868)
==32727==    by 0x1938573: Sci::CelObj::draw(Graphics::Surface&, Sci::ScreenItem const&, Common::Rect const&) const (celobj32.cpp:574)
==32727==    by 0x19385BE: Sci::CelObj::draw(Graphics::Surface&, Sci::ScreenItem const&, Common::Rect const&, bool) (celobj32.cpp:584)
==32727==    by 0x194DDDE: Sci::GfxFrameout::drawScreenItemList(Sci::DrawList const&) (frameout.cpp:930)
==32727==    by 0x194BDBA: Sci::GfxFrameout::frameOut(bool, Common::Rect const&) (frameout.cpp:432)
==32727==    by 0x1961622: Sci::GfxTransitions32::processShowStyles() (transitions32.cpp:126)
==32727==    by 0x194EAD4: Sci::GfxFrameout::kernelFrameOut(bool) (frameout.cpp:1137)
==32727==    by 0x1933E56: Sci::kFrameOut(Sci::EngineState*, int, Sci::reg_t*) (kgraphics32.cpp:235)
==32727==    by 0x18D97EA: Sci::callKernelFunc(Sci::EngineState*, int, int) (vm.cpp:376)
==32727==  Address 0xffffffffffffffff is not stack'd, malloc'd or (recently) free'd
==32727== 
==32727== 
==32727== Process terminating with default action of signal 11 (SIGSEGV)
==32727==  Access not within mapped region at address 0xFFFFFFFFFFFFFFFF
==32727==    at 0x1944F6C: Sci::SCALER_Scale<false, Sci::READER_Compressed>::read() (celobj32.cpp:303)
==32727==    by 0x1943F08: Sci::RENDERER<Sci::MAPPER_NoMD, Sci::SCALER_Scale<false, Sci::READER_Compressed>, false>::draw(Graphics::Surface&, Common::Rect const&, Common::Point const&) const (celobj32.cpp:737)
==32727==    by 0x193E08F: void Sci::CelObj::render<Sci::MAPPER_NoMD, Sci::SCALER_Scale<false, Sci::READER_Compressed> >(Graphics::Surface&, Common::Rect const&, Common::Point const&, Common::Rational const&, Common::Rational const&) const (celobj32.cpp:764)
==32727==    by 0x193920E: Sci::CelObj::scaleDrawNoMD(Graphics::Surface&, Common::Rational const&, Common::Rational const&, Common::Rect const&, Common::Point const&) const (celobj32.cpp:868)
==32727==    by 0x1938573: Sci::CelObj::draw(Graphics::Surface&, Sci::ScreenItem const&, Common::Rect const&) const (celobj32.cpp:574)
==32727==    by 0x19385BE: Sci::CelObj::draw(Graphics::Surface&, Sci::ScreenItem const&, Common::Rect const&, bool) (celobj32.cpp:584)
==32727==    by 0x194DDDE: Sci::GfxFrameout::drawScreenItemList(Sci::DrawList const&) (frameout.cpp:930)
==32727==    by 0x194BDBA: Sci::GfxFrameout::frameOut(bool, Common::Rect const&) (frameout.cpp:432)
==32727==    by 0x1961622: Sci::GfxTransitions32::processShowStyles() (transitions32.cpp:126)
==32727==    by 0x194EAD4: Sci::GfxFrameout::kernelFrameOut(bool) (frameout.cpp:1137)
==32727==    by 0x1933E56: Sci::kFrameOut(Sci::EngineState*, int, Sci::reg_t*) (kgraphics32.cpp:235)
==32727==    by 0x18D97EA: Sci::callKernelFunc(Sci::EngineState*, int, int) (vm.cpp:376)
==32727==  If you believe this happened as a result of a stack
==32727==  overflow in your program's main thread (unlikely but
==32727==  possible), you can try to increase the size of the
==32727==  main thread stack using the --main-stacksize= flag.
==32727==  The main thread stack size used in this run was 8388608.
==32727== 
==32727== HEAP SUMMARY:
==32727==     in use at exit: 19,947,013 bytes in 41,110 blocks
==32727==   total heap usage: 122,545 allocs, 81,435 frees, 50,501,332 bytes allocated
==32727== 
==32727== LEAK SUMMARY:
==32727==    definitely lost: 176 bytes in 3 blocks
==32727==    indirectly lost: 176 bytes in 4 blocks
==32727==      possibly lost: 496,499 bytes in 3,190 blocks
==32727==    still reachable: 19,450,162 bytes in 37,913 blocks
==32727==                       of which reachable via heuristic:
==32727==                         multipleinheritance: 24 bytes in 1 blocks
==32727==         suppressed: 0 bytes in 0 blocks
==32727== Rerun with --leak-check=full to see details of leaked memory
==32727== 
==32727== For counts of detected and suppressed errors, rerun with: -v
==32727== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 1)
Segmentation fault

[eriktorbjorn]

Apparently when it reaches read(), _row is null. It seems to be set to null by setTarget(). There is a _sourceBuffer, but getBasePtr() returns null. Or something. I'm way out of my depth here.

[DanielSWolf]

Commit 999cf71dcf adds a game option to enable/disable LarryScale. The idea was that for LSL7, the option should be available and default to true. For all other SCI32 games, the option should not be available and the value should default to false.

The options system in ScummVM is a bit complicated. It may be that the option defaults to true for all games, not just LSL7. This would certainly explain the bug, since I didn't test the code with other games.

In engines/sci/graphics/celobj32.cpp, there's a line reading const bool useLarryScale = ConfMan.getBool("enable_larryscale");. Could you place a breakpoint just after that line and tell me the value of the variable?

[eriktorbjorn]

The option is true.

From what I understand, runGame() in base/main.cpp registers defaults for all of the engine's specific settings, including enable_larryscale. At least it does for me.

[eriktorbjorn]

Looks like it may be by design. It calls getExtraGuiOptions(), which does take a target name as input. But when called from base/main.cpp, it always passes an empty String because:

// Set default values for all of the custom engine options
// Apparently some engines query them in their constructor, thus we
// need to set this up before instance creation.

So i guess the SCI engine has to ensure that the game-specific settings are harmless for other games.

[DanielSWolf]

I must confess I don't really understand how ScummVM options work. In order to add this single option, I had to modify five (!) different files (commit 999cf71dcf) and did so by copying from existing options.

A quick fix would be setting the default value to false. But that would affect LSL7, too. What we need is a way of setting the default value to true if the option is available, false otherwise.

Is there anyone who knows more about the ScummVM options?

[DanielSWolf]

I think I've figured it out. Try changing that line in celobj32.cpp to say

const bool useLarryScale = ConfMan.hasKey("enable_larryscale") && ConfMan.getBool("enable_larryscale");

If I'm right, hasKey should indicate whether the option actually exists for the current game. I already checked that it returns true for LSL7 even if no explicit option is set.

[eriktorbjorn]

That makes useLarryScale false for me in both games. I hadn't seen this function before, but judging by how it's done in other parts of the SCI engine perhaps something like this would work:

#include "common/gui_options.h"

...

const bool useLarryScale = Common::checkGameGUIOption(GAMEOPTION_LARRYSCALE, ConfMan.get("guioptions")) && ConfMan.getBool("enable_larryscale");

[DanielSWolf]

My bad. I removed and re-added LSL7, but apparently this didn't clear the configuration entirely. It's a good thing you have both games!

I assumed that hasKey indicates whether a given option exists for the current game, but after what you wrote, this doesn't seem to be the case. It's certainly possible that checkGameGUIOption does exactly this. At least it seems to do the right thing both on your machine and mine.

I'm still a bit skeptical because I just don't fully understand ScummVM's code for handling options. What's more, there are some usages of checkGameGUIOption that mirror your code. But there are also multiple places in code where this function seems to be used to determine not whether the option exists, but whether the flag is set. Take this excerpt from engines/sci/engine/guest_additions.cpp:

if (Common::checkGameGUIOption(GUIO_LINKMUSICTOSFX, ConfMan.get("guioptions"))) {
    ConfMan.setInt("music_volume", volume);
}

This looks as if checkGameGUIOption returned the actual option value. Then again, maybe we're not the only ones who don't grok the options system. Maybe this usage and others are actually bugs?

[dafioram]

PR1216

-		const bool useLarryScale = ConfMan.getBool("enable_larryscale");
+		const bool useLarryScale = (g_sci->getGameId() == GID_LSL7) && ConfMan.getBool("enable_larryscale");

[rsn8887]

Fixed via PR1216. Once Larry scale is enabled for other games, a better solution has to be found, which might be along the line what @eriktorbjorn suggested here.

[eriktorbjorn]

I could be wrong, but if I understand it correctly...

Any game engine that uses the "advanced detector" will automatically maintain a "guioptions" setting for each game it handles. In my case, LSL7 has

guioptions=sndNoMIDI noAspect gameOption2 gameOption9 gameOptionA gameOptionC lang_English

GAMEOPTION_LARRYSCALE is defined as GUIO_GAMEOPTIONS12, which has "gameOptionC" as its description. The checkGameGUIOption() function simply checks to see if a string contains the description for a specified option. So it should answer the question "should this game use the LarryScale option".

I guess in your other example GUIO_LINKMUSICTOSFX means that adjusting sound effects volume should also adjust the music volume, or something like that. It's using it to see if a setting should be updated, rather than to see if it shold be queried, but I can't think of anything obviously wrong with that.

So the proposed fix still feels right to me, though of course I wouldn't mind of someone could confirm it.

[DanielSWolf]

You're right. So I'm pretty confident that your code does exactly what I was going for. It answers the question: Does the current game offer a LarryScale option and, if so, is that option enabled?

As chance will have it, a different solution was merged just two hours ago: ​https://github.com/scummvm/scummvm/pull/1216 I feel that your solution is far cleaner and should be used instead. Can you create a MR? Given that it was you who came up with the code, I feel that it is more appropriate if you do it.

[criezy]

It’s difficult for me to check the code on my phone, and I can check when I am back home in a couple of days, but from what I remember the solution proposed by eriktorbjorn should work.

If I remember correctly he guioptions for the games is set when the game is added and lists the options for that game. Using Common::checkGameGUIOptions should then return true only for games that have this options, so currently LSL7. The caveat is that this means users that have added their game to ScummVM before the LarryScale option was added will never use the option (although I am not even sure it appears in the GUI, so it might be for the best).

[rsn8887]

I merged the other solution, I apologize since I violated the guidelines. Of course I agree that if there's a new, cleaner solution that works, it should be merged via a new PR.

[eriktorbjorn]

AdvancedMetaEngine::createInstance() calls Common::updateGameGUIOptions(), which I think is there to ensure that the "guioptions" list is refreshed and saved to the config file whenever a game that supports it is started.