Opened 9 years ago

Closed 4 years ago

#6810 closed defect (fixed)

BACKENDS: ARM - Various Crashes due to buggy ARM ASM optimizations

Reported by: SF/apoleon Owned by: sev-
Priority: normal Component: Ports
Version: 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 9 years ago.

Download all attachments as: .zip

Change History (21)

by SF/apoleon, 9 years ago

Attachment: disable_arm_asm.patch added

comment:1 by digitall, 9 years ago

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 by digitall, 9 years ago

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 by digitall, 9 years ago

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 by digitall, 9 years ago

Owner: set to fuzzie

comment:5 by digitall, 9 years ago

fuzzie: Any POV on this?

comment:6 by lordhoto, 9 years ago

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 by wjp, 9 years ago

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

comment:8 by digitall, 9 years ago

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 by BenCastricum, 6 years ago

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

Last edited 6 years ago by BenCastricum (previous) (diff)

comment:10 by BenCastricum, 6 years ago

Component: Engine: SCUMMPorts
Game: Indiana Jones 4

comment:11 by csnover, 6 years ago

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 by csnover, 6 years ago

Owner: fuzzie removed

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 by csnover, 6 years ago

Keywords: code-removal added

comment:14 by csnover, 6 years ago

Owner: set to csnover

comment:15 by csnover, 6 years ago

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 by csnover, 6 years ago

Owner: csnover removed

(The change was made in ddbfb8518b6fdfe58e3574421fd6b9243ad2c761.)

comment:17 by raziel-, 4 years ago

Summary: ARM: Various Crashes due to buggy ARM ASM optimizationsBACKNDS: ARM - Various Crashes due to buggy ARM ASM optimizations

@ccawley2011
@csnover

The fix in comment:16 was merged on Nov 21, 2017.

Can this be closed?

comment:18 by raziel-, 4 years ago

Summary: BACKNDS: ARM - Various Crashes due to buggy ARM ASM optimizationsBACKENDS: ARM - Various Crashes due to buggy ARM ASM optimizations

comment:19 by sev-, 4 years ago

Priority: highnormal

comment:20 by sev-, 4 years ago

Owner: set to sev-
Resolution: fixed
Status: newclosed

Yes, closing

Note: See TracTickets for help on using tickets.