Opened 4 years ago

Last modified 16 months ago

#6810 new defect

ARM: Various Crashes due to buggy ARM ASM optimizations

Reported by: SF/apoleon Owned by:
Priority: high Component: Ports
Keywords: code-removal Cc:
Game:

Description

Hello,

I am forwarding Debian bug report https://bugs.debian.org/779029.

It was reported that games like Beneath a Steel Sky and Indiana Jones Fate of Atlantis crash with a segfault when running Debian's ScummVM package on ARM devices. It was further claimed that the ARM asm optimizations caused these issues. The bug reporter also submitted a patch which is supposed to fix the issue.

Unfortunately I do neither possess any ARM hardware to verify this bug report nor am I able to confirm whether the patch acually works. Is this a known issue? What do you think about the patch and what are your recommendations?

Regards,

Markus

Ticket imported from: bugs/6810.

Attachments (1)

disable_arm_asm.patch (818 bytes) - added by SF/apoleon 4 years ago.

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by SF/apoleon

Attachment: disable_arm_asm.patch added

comment:1 Changed 4 years ago by digitall

Component: Engine: ZVisionEngine: SCUMM
Game: Zork Grand InquisitorIndiana Jones 4
Summary: ScummVM fails to work on armhf due to buggy ARM ASM optimizationsARM: Various Crashes due to buggy ARM ASM optimizations

comment:2 Changed 4 years ago by digitall

The attached patch should work. It modifies the configuration script to cleanly disable all ASM optimizations for ARM targets by commenting out the various defines.

comment:3 Changed 4 years ago by digitall

With the ARM ASM versions disabled, the code will use the normal C equivalents will likely be slower, but more reliable.

I would suggest merging this patch for ARM HF targets i.e. RPi etc. The team should probably look at making the ARM ASM disabled by default and only enabled for specific older ARM targets where it is absolutely necessary.

comment:4 Changed 4 years ago by digitall

Owner: set to fuzzie

comment:5 Changed 4 years ago by digitall

fuzzie: Any POV on this?

comment:6 Changed 4 years ago by lordhoto

Hmmm. I rememeber reading about something similar in the past. Wasn't there an issue with our hand crafted ASM routines and ARM targets which used some specific Thumb instruction set or something like that?

comment:7 Changed 4 years ago by wjp

Yes, but I don't think we ever got to the bottom of that.

comment:8 Changed 4 years ago by digitall

Three open bugs are likely part of the same issue:
https://sourceforge.net/p/scummvm/bugs/6316/
https://sourceforge.net/p/scummvm/bugs/6132/
https://sourceforge.net/p/scummvm/bugs/5932/

It would be nice if we could get https://github.com/scummvm/scummvm/pull/348 merged to start with...

This was last discussed in http://logs.scummvm.org/log.php?log=scummvm.log.07May2013&format=html

comment:9 Changed 18 months ago by BenCastricum

PR625 got merged, perhaps this bug report can be closed?

Last edited 18 months ago by BenCastricum (previous) (diff)

comment:10 Changed 18 months ago by BenCastricum

Component: Engine: SCUMMPorts
Game: Indiana Jones 4

comment:11 Changed 17 months ago by csnover

Priority: normalblocker

USE_ARM_SOUND_ASM has been disabled since 2015, AFAICT PR#625 did not change anything in this regard so the changes to the ARM ASM in that PR aren’t actually doing anything.

It seems unnecessarily burdensome to maintain this old hand-written ARM assembly, which might not even be as efficient as compiled C++ code on modern devices with newer architectures, so I think it probably ought to just get removed and allow the compiler to generate appropriate machine code instead (since this is what is already happening anyway).

I’m setting this to blocker to make a final decision one way or the other before the next release.

comment:12 Changed 17 months ago by csnover

Owner: fuzzie deleted

Removing all owners from release blockers so they can be reclaimed during the release process. If you are the previous owner and would graciously fix this bug for the next release, please go ahead and re-add yourself as owner.

comment:13 Changed 17 months ago by csnover

Keywords: code-removal added

comment:14 Changed 17 months ago by csnover

Owner: set to csnover

comment:15 Changed 16 months ago by csnover

Priority: blockerhigh

The ARM assembly will now only ever be enabled for specific legacy device hosts, so I am removing these tickets from blocking the release. The broken parts of the ARM code still need to be fixed and re-enabled for these hosts or else finally removed by the next-next release.

comment:16 Changed 16 months ago by csnover

Owner: csnover deleted

(The change was made in ddbfb8518b6fdfe58e3574421fd6b9243ad2c761.)

Note: See TracTickets for help on using tickets.