Opened 10 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)
Change History (21)
by , 10 years ago
| Attachment: | disable_arm_asm.patch added |
|---|
comment:1 by , 10 years ago
| Component: | Engine: ZVision → Engine: SCUMM |
|---|---|
| Game: | Zork Grand Inquisitor → Indiana Jones 4 |
| Summary: | ScummVM fails to work on armhf due to buggy ARM ASM optimizations → ARM: Various Crashes due to buggy ARM ASM optimizations |
comment:2 by , 10 years ago
comment:3 by , 10 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 , 10 years ago
| Owner: | set to |
|---|
comment:6 by , 10 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:8 by , 10 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:10 by , 7 years ago
| Component: | Engine: SCUMM → Ports |
|---|---|
| Game: | Indiana Jones 4 |
comment:11 by , 7 years ago
| Priority: | normal → blocker |
|---|
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 , 7 years ago
| Owner: | 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 , 7 years ago
| Keywords: | code-removal added |
|---|
comment:14 by , 7 years ago
| Owner: | set to |
|---|
comment:15 by , 7 years ago
| Priority: | blocker → high |
|---|
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 , 7 years ago
| Owner: | removed |
|---|
(The change was made in ddbfb8518b6fdfe58e3574421fd6b9243ad2c761.)
comment:17 by , 4 years ago
| Summary: | ARM: Various Crashes due to buggy ARM ASM optimizations → BACKNDS: ARM - Various Crashes due to buggy ARM ASM optimizations |
|---|
comment:18 by , 4 years ago
| Summary: | BACKNDS: ARM - Various Crashes due to buggy ARM ASM optimizations → BACKENDS: ARM - Various Crashes due to buggy ARM ASM optimizations |
|---|
comment:19 by , 4 years ago
| Priority: | high → normal |
|---|
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.