Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
ARM: 7668/1: fix memset-related crashes caused by recent GCC (4.7.2) …
…optimizations Recent GCC versions (e.g. GCC-4.7.2) perform optimizations based on assumptions about the implementation of memset and similar functions. The current ARM optimized memset code does not return the value of its first argument, as is usually expected from standard implementations. For instance in the following function: void debug_mutex_lock_common(struct mutex *lock, struct mutex_waiter *waiter) { memset(waiter, MUTEX_DEBUG_INIT, sizeof(*waiter)); waiter->magic = waiter; INIT_LIST_HEAD(&waiter->list); } compiled as: 800554d0 <debug_mutex_lock_common>: 800554d0: e92d4008 push {r3, lr} 800554d4: e1a00001 mov r0, r1 800554d8: e3a02010 mov r2, #16 ; 0x10 800554dc: e3a01011 mov r1, #17 ; 0x11 800554e0: eb04426e bl 80165ea0 <memset> 800554e4: e1a03000 mov r3, r0 800554e8: e583000c str r0, [r3, #12] 800554ec: e5830000 str r0, [r3] 800554f0: e5830004 str r0, [r3, #4] 800554f4: e8bd8008 pop {r3, pc} GCC assumes memset returns the value of pointer 'waiter' in register r0; causing register/memory corruptions. This patch fixes the return value of the assembly version of memset. It adds a 'mov' instruction and merges an additional load+store into existing load/store instructions. For ease of review, here is a breakdown of the patch into 4 simple steps: Step 1 ====== Perform the following substitutions: ip -> r8, then r0 -> ip, and insert 'mov ip, r0' as the first statement of the function. At this point, we have a memset() implementation returning the proper result, but corrupting r8 on some paths (the ones that were using ip). Step 2 ====== Make sure r8 is saved and restored when (! CALGN(1)+0) == 1: save r8: - str lr, [sp, #-4]! + stmfd sp!, {r8, lr} and restore r8 on both exit paths: - ldmeqfd sp!, {pc} @ Now <64 bytes to go. + ldmeqfd sp!, {r8, pc} @ Now <64 bytes to go. (...) tst r2, #16 stmneia ip!, {r1, r3, r8, lr} - ldr lr, [sp], #4 + ldmfd sp!, {r8, lr} Step 3 ====== Make sure r8 is saved and restored when (! CALGN(1)+0) == 0: save r8: - stmfd sp!, {r4-r7, lr} + stmfd sp!, {r4-r8, lr} and restore r8 on both exit paths: bgt 3b - ldmeqfd sp!, {r4-r7, pc} + ldmeqfd sp!, {r4-r8, pc} (...) tst r2, #16 stmneia ip!, {r4-r7} - ldmfd sp!, {r4-r7, lr} + ldmfd sp!, {r4-r8, lr} Step 4 ====== Rewrite register list "r4-r7, r8" as "r4-r8". Signed-off-by: Ivan Djelic <ivan.djelic@parrot.com> Reviewed-by: Nicolas Pitre <nico@linaro.org> Signed-off-by: Dirk Behme <dirk.behme@gmail.com> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
- Loading branch information
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch destroyed manual update in OpenElec. After update SYSTEM file is corrupted and system not loading. "final_check. Could not find system." I checked this 8-10 times. Every time SYSTEM file is corrupted. I remove this patch and manual update is fine. No more corrupted SYSTEM file after update.
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is surprising as this is an upstream patch which should be well tested.
Any else tried this and observed problems?
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that I have been running it on an Apachebench tested Pi for several days without issue - I can't understand what OpenELEC might do with memset to trigger the above problem.
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problems here.
I've built stock OpenELEC (with my own auto-update patch), and dropped this memset patch into the OpenELEC.tv/projects/RPi/patches/linux folder and rebuilt linux.
I now boot successfully, and have also auto-updated the system 5 times, all without incident.
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also confirm the corruption of SYSTEM file in openELEC. After latest kernel patches has been reverted its back to normal.
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you testing with any other patches? I'm using just stock OE builds, plus a modified init script which shouldn't affect this.
@tuxen: After auto-updating, do you definitely have the new kernel on your SD card? Are you setup to boot from SD or USB/NFS/SMB?
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I definitely have the new kernel but SYSTEM file is corrupted and filename looks the same everytime SYSTEM plus some that looks like quarter squares from a c64 and underscores that I do not know the ASCII code for in my head. I tested both USB, harddrive (main setup) and sdcard only. I tested 6-8 times like rbej. It's confirmed by 3 other users getting the same error. It is with other patches though, the latest added to official. So here I'm trusting rbej. (Hope it can be of help anyway) The moment they where reverted everything was good again also for the 3 other unlucky users (had to format fat and copy the files by hand first ofcause then manually update to test bug was gone)
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't understand though, why the new SYSTEM would be transferred in a corrupt state, when it should be copied to the SD card[1] by the old kernel - unless it is being corrupted when the new kernel is started.
[1] If booting stock OE (but not rbej builds) when boot= is set to anything other than /dev/mmcblk0p1, the OE auto-update process will copy the kernel and firmware files to the boot disk (ie. /dev/sda1, or NFS or SMB) and not /dev/mmcblk0p1, so some users may end up booting with old kernel/firmware after auto-updating stock OE.
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I know about issue [1]. If I copy the files by hand the build will run. But then again the manual update seems to go fine, it's copying the files like nothing is wrong which is weird if its copied in a corrupt state but then reboots and SYSTEM is trashed. So it seems to be like something in between. If someone could make a build that wrote a log about the state of the files we could narrow it down?
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been a number of updates to the stock initramfs init script which I'm not running, wonder if it was one of those patches? Not sure if Rbej has reverted to this stock init, or continues to run my patched version from waaay back.
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the Easter Holliday's I will look into setting up a build environment so I can be of more support and help in the future.
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the status of this patch, has it been pulled? I'm building stock OE from the 3.0.0 branch and have now been bitten by this bug while updating the SYSTEM file (always the SYSTEM file...hmm).
690c641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been reverted.