Opened 20 months ago

Closed 9 months ago

#13661 closed defect (fixed)

SAGA: IHNM: "createThread wrong scriptEntryPointNumber" when starting with Benny

Reported by: dwatteau Owned by: johnpevensie
Priority: normal Component: Engine: SAGA
Version: Keywords: threading
Cc: Game: I Have No Mouth

Description

With either the English/Steam or French/GOG versions of IHNM (which trigger the SAMPLE.AD/SAMPLE.OPL warning), reinstalled from scratch to make sure the data is OK.

For whatever reason, I can only reproduce this with the OSX PowerPC port:

  • It also happens with ScummVM 2.2.0, so it's not a recent regression.
  • Using different compiler versions doesn't fix the problem.
  • It's systematic on this port, i.e. playing with Benny is impossible there.
  • With Linux on the same PowerPC machine, it's fine. Maybe the OSX PowerPC environment is "luckier" in triggering this bug, especially if it's related to threading?

Anyway, if I just do this:

  1. Start the game
  2. Choose Benny as a character
  3. Go down the stairs with Benny

then the following error always appears in the console when Benny starts speaking to AM:

Script::createThread wrong scriptEntryPointNumber!

Skipping the dialogue or enabling/disabling the subtitles doesn't change anything.

On Windows, I can't trigger the error message, but sometimes (1 out of 10?) if I reproduce the steps above, Benny suddendly teleports to the cemetery, and if I go back to the previous room, he will resume reacting to AM, as if nothing happened.

GDB and debug logs to be attached below.

Attachments (4)

ihnm_benny_gdb.txt (2.0 KB ) - added by dwatteau 20 months ago.
GDB log when the error happens
ihnm_benny_debug.txt (12.2 KB ) - added by dwatteau 20 months ago.
debug (-d9) log when AM teleports Benny to the first room
asan-ihnm-benny-stairs.txt (5.0 KB ) - added by dwatteau 17 months ago.
ASAN trace when falling from the stairs with Benny
tsan-saga-ihnm-amiga-aga.txt (9.2 KB ) - added by dwatteau 9 months ago.

Download all attachments as: .zip

Change History (22)

by dwatteau, 20 months ago

Attachment: ihnm_benny_gdb.txt added

GDB log when the error happens

by dwatteau, 20 months ago

Attachment: ihnm_benny_debug.txt added

debug (-d9) log when AM teleports Benny to the first room

comment:1 by dwatteau, 20 months ago

It's systematic on this port, i.e. playing with Benny is impossible there.

Well, actually there's a way of working around this on this port: look at the Psych Profile in the inventory, before you fall from the stairs with Benny.

It looks like it resets the room to a proper state, and thus prevents the wrong scriptEntryPointNumber error from happening.

comment:2 by dwatteau, 17 months ago

ASAN (with clang++ on macOS 12.6) detects a heap-use-after-free with the same Benny action. Git HEAD is 20c1dbb50d.

==13418==ERROR: AddressSanitizer: heap-use-after-free on address 0x00011d412a38 at pc 0x000100775ccc bp 0x00016fd61760 sp 0x00016fd61758
READ of size 4 at 0x00011d412a38 thread T0
    #0 0x100775cc8 in Saga::HitZone::getFlags() const objectmap.h:56
    #1 0x1006ec924 in Saga::Actor::stepZoneAction(Saga::ActorData*, Saga::HitZone const*, bool, bool) actor.cpp:551
    #2 0x100712a0c in Saga::Actor::handleActions(int, bool) actor_walk.cpp:695
    #3 0x10071da30 in Saga::Actor::direct(int) actor_walk.cpp:727
    #4 0x1007d2f48 in Saga::SagaEngine::run() saga.cpp:357
    #5 0x1000dc7e0 in runGame(Plugin const*, Plugin const*, OSystem&, Common::String const&) main.cpp:318
    #6 0x1000d8130 in scummvm_main main.cpp:619
    #7 0x1000cf19c in main macosx-main.cpp:44
    #8 0x1024b1088 in start+0x204 (dyld:arm64e+0x5088)

I'm attaching the full ASAN log below.

by dwatteau, 17 months ago

Attachment: asan-ihnm-benny-stairs.txt added

ASAN trace when falling from the stairs with Benny

comment:3 by dwatteau, 9 months ago

As per #14334 (see comment no. 12) this also happens with the ECS version of Inherit the Earth.

comment:4 by johnpevensie, 9 months ago

I meant to say the AGA Amiga version of Inherit the Earth. The ECS version of Inherit the Earth works fine.

comment:5 by PushmePullyu, 9 months ago

Thanks for investigating this and for the ASAN trace. I can not get the "Script::createThread wrong scriptEntryPointNumber!" error message on Linux x86_64, but i have attempted a fix based on the ASAN log: https://github.com/PushmePullyu/scummvm/tree/debug-ihnm-use-after-free

comment:6 by dwatteau, 9 months ago

@johnpevensie: Oops, sorry for inverting them!

Thank you very much @PushmePullyu, here's the ThreadSanitizer trace I manage to get in ITE-AGA, for reference (attaching it as .txt).

Last edited 9 months ago by dwatteau (previous) (diff)

by dwatteau, 9 months ago

comment:7 by dwatteau, 9 months ago

@PushmePullyu: I've tested your PR with IHNM: the AddressSanitizer error is now gone, and it appears to fix the createThread wrong scriptEntryPointNumber error on OSX PPC, cool! :)

The ThreadSanitizer errors in ITE are still there, though (but they look unrelated?), and I couldn't reproduce the original ITE error on OSX PPC, so I can't say if it fixes both issues yet.

@johnpevensie: Can you grab the updated SAGA plugin at <https://github.com/dwatteau/scummvm/releases/download/v0.7.1/saga.plugin>, use it to overwrite /Applications/ScummVM.app/Contents/Resources/saga.plugin (from the test build from the other Trac ticket), and report whether this fixes your error in ITE AGA too? Thanks!

Last edited 9 months ago by dwatteau (previous) (diff)

comment:8 by johnpevensie, 9 months ago

This does not fix the error on the AGA Amiga version of Inherit the Earth. I still get the same error at the same place (after the intro finishes).

comment:9 by dwatteau, 9 months ago

@johnpevensie: Thanks for your new test! I can't reproduce the error you're having with this release of Inherit the Earth on my OSX PPC systems, though.

Can you tell me at which precise moments you're hitting the Esc key (or left-clicking) during the intro, in order to trigger the issue? It's probably timing-related, as with the issue I was having in IHNM. Thanks.

in reply to:  7 comment:10 by PushmePullyu, 9 months ago

Replying to dwatteau:

@PushmePullyu: I've tested your PR with IHNM: the AddressSanitizer error is now gone, and it appears to fix the createThread wrong scriptEntryPointNumber error on OSX PPC, cool! :)

Thanks for testing. For reference: PR for this is https://github.com/scummvm/scummvm/pull/5054

I have also added debug output to https://github.com/PushmePullyu/scummvm/tree/debug-ihnm-use-after-free
Could you compile this for @johnpevensie with debugging enabled?

comment:11 by bluegr, 9 months ago

Owner: set to PushmePullyu
Resolution: fixed
Status: newclosed

The PR has been merged, so this can finally be closed. Thanks for your help on resolving this!

comment:12 by dwatteau, 9 months ago

Owner: changed from PushmePullyu to dwatteau
Resolution: fixedpending
Status: closedpending

Reopening, since the PR fixed IHNM but apparently not ITE.

I'll provide a debug test build once my tendinitis is gone

comment:13 by johnpevensie, 9 months ago

From my tests, this is not a timing issue. I get the same error no matter when I push ESC to skip the intro, or even when I let the intro play without skipping it.

comment:14 by dwatteau, 9 months ago

OK, thanks.

Here you go for the updated SAGA plugin with more debug output: <https://github.com/dwatteau/scummvm/releases/download/v0.7.1/saga-createThread-debug.plugin>

Can you move/rename it to /Applications/ScummVM.app/Contents/Resources/saga.plugin again (on top of the previous 2.7.1pre test build), then start the game from the OSX Terminal with something like this:

/Applications/ScummVM.app/Contents/MacOS/scummvm ite-cd-amiga

and then paste here the Script::createThread() lines it should output when encountering the issue? Thanks!

comment:15 by johnpevensie, 9 months ago

I tested Inherit the Earth with the new plugin. I still get the same errors. However, I did run the game from Terminal and get more output. I have also added my own annotations:

AGA AMIGA INHERIT THE EARTH OUTPUT:

Running Inherit the Earth: Quest for the Orb (AGA CD/Amiga/English)
aga.exe: e6d93bbf0f89786eb930fbc81e02810d, 202328 bytes.
ite.rtn: 749885c0f7eaab4e977dc26a41d99ad8, 18524 bytes.
ite.sounds: f09b29e3204192de7e8cc6b073cb34f5, 640284 bytes.
ite.voices: daf62750f5322da03fab395e548b0b8f, 217376825 bytes.
-the list of data files loaded by the game

WARNING: Sprite::loadList offset exceeded!
WARNING: Sprite::loadList offset exceeded!
WARNING: Sprite::loadList offset exceeded!
WARNING: Sprite::loadList offset exceeded!
WARNING: Anim::load animId=0 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=1 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=2 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=3 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=4 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=5 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=6 Excessive dimensions 320x16191, skipping!
-these warnings show during the intro. The intro plays well, with speech or without.

Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 22
Script::createThread(): _modules[8].entryPoints.size: 0
Script::createThread wrong scriptEntryPointNumber!
-These entries attempt to begin the game, but ScummVM crashes with the above error message. The below displays in the debugger:

Debugger started, type 'exit' to return to the game.
Type 'help' to see a little list of commands and variables.
ERROR: Script::createThread wrong scriptEntryPointNumber!

ECS AMIGA INHERIT THE EARTH OUTPUT:

Running Inherit the Earth: Quest for the Orb (ECS CD/Amiga/English)
ecs.exe: 29665b96c2758aec3906ebc891079234, 204228 bytes.
ecs.rtn: c9d09514839d771efdc82ad761413349, 18584 bytes.
ite.sounds: f09b29e3204192de7e8cc6b073cb34f5, 640284 bytes.
ite.voices: daf62750f5322da03fab395e548b0b8f, 217376825 bytes.
-the list of data files loaded by the game

WARNING: Sprite::loadList offset exceeded!
WARNING: Sprite::loadList offset exceeded!
WARNING: Sprite::loadList offset exceeded!
WARNING: Sprite::loadList offset exceeded!
WARNING: Anim::load animId=0 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=1 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=2 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=3 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=4 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=5 Excessive dimensions 320x16191, skipping!
WARNING: Anim::load animId=6 animation magic mismatch (0xeeea vs 0x44), skipping!
-these warnings show during the intro. The intro plays well in both versions, with speech or without.

Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 22
Script::createThread(): _modules[8].entryPoints.size: 39
-This begins the game. It is here that the AGA version crashes. The ECS version continues. In fact, I have other "Script::createThread()" entries from further gameplay (although brief)

Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 23
Script::createThread(): _modules[8].entryPoints.size: 39
Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 21
Script::createThread(): _modules[8].entryPoints.size: 39
Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 17
Script::createThread(): _modules[8].entryPoints.size: 39
Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 18
Script::createThread(): _modules[8].entryPoints.size: 39
Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 20
Script::createThread(): _modules[8].entryPoints.size: 39

in reply to:  15 comment:16 by PushmePullyu, 9 months ago

Replying to johnpevensie:

Thanks for the data.

Script::createThread(): calling loadModule(8)
Script::createThread(): scriptEntryPointNumber: 22

The numbers above look good.

Script::createThread(): _modules[8].entryPoints.size: 0

This should definitely not be 0. So we now need to figure out where that value comes from.
Checking the code, it seems to be set only once on module loading, so I suggest we check for data corruption or an incompatible version first.

Could you run the following in a terminal and post the resulting file as an attachment (warning: If you already have a file named "inheritmd5.txt" in your home directory, it will be overwritten):

( cd '/path/to/ite_data_files' && find ./ -type f -exec openssl md5 -r -- {} \; ) | tee ~/inheritmd5.txt

You will need to replace "/path/to/ite_data_files" with the location of your ITE data directory (it can be found in ScummVM (Game Options->Paths->Game Path).
This will output a list of all ITE files with their md5 hashes to the terminal and to "inheritmd5.txt", which I can then compare against my version of the game.

@dwatteau: Thanks for the build, and have a speedy recovery.

comment:17 by johnpevensie, 9 months ago

@PushmePullyu:

I followed your suggestions for testing for data corruption. The game ran fine on my Intel machine, so I thought it was a version-specific bug. However, on my Mac, it turned out I had corrupt data files. The AGA version of Inherit the Earth worked after I copied the data files all over again.

Thank you.

comment:18 by johnpevensie, 9 months ago

Owner: changed from dwatteau to johnpevensie
Resolution: pendingfixed
Status: pendingclosed
Note: See TracTickets for help on using tickets.