Date
1 - 12 of 12
[PATCH] pulseaudio: fix for ARM thumb + frame pointers compilation error
|
Stefan Ghinea
From: Catalin Enache <catalin.enache@windriver.com>
When compiling for Thumb or Thumb2, frame pointers _must_ be disabled since the Thumb frame pointer in r7 clashes with pulseaudio's use of inline asm to make syscalls (where r7 is used for the syscall NR). In most cases, frame pointers will be disabled automatically due to the optimisation level, but appending an explicit -fomit-frame-pointer to CFLAGS handles cases where optimisation is set to -O0 or frame pointers have been enabled by -fno-omit-frame-pointer earlier in CFLAGS, etc. References: https://www.openwall.com/lists/musl/2017/10/09/2 Signed-off-by: Catalin Enache <catalin.enache@windriver.com> Signed-off-by: Stefan Ghinea <stefan.ghinea@windriver.com> --- meta/recipes-multimedia/pulseaudio/pulseaudio.inc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc index 4e32b27087..c7f3e67022 100644 --- a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc +++ b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc @@ -63,6 +63,14 @@ DEPENDS += "speexdsp libxml-parser-perl-native libcap" inherit autotools bash-completion pkgconfig useradd gettext perlnative systemd manpages gsettings +# When compiling for Thumb or Thumb2, frame pointers _must_ be disabled since the +# Thumb frame pointer in r7 clashes with pulseaudio's use of inline asm to make syscalls +# (where r7 is used for the syscall NR). In most cases, frame pointers will be +# disabled automatically due to the optimisation level, but append an explicit +# -fomit-frame-pointer to handle cases where optimisation is set to -O0 or frame +# pointers have been enabled by -fno-omit-frame-pointer earlier in CFLAGS, etc. +CFLAGS_append_arm = " ${@bb.utils.contains('TUNE_CCARGS', '-mthumb', '-fomit-frame-pointer', '', d)}" + # *.desktop rules wont be generated during configure and build will fail # if using --disable-nls USE_NLS = "yes" -- 2.17.1
|
|
|
|
Andre McCurdy
On Thu, Mar 26, 2020 at 8:26 AM Stefan Ghinea
<stefan.ghinea@windriver.com> wrote: This is OK and safe to merge but note that it's now regarded as the "old style" fix for this problem. The newer approach is to tweak the asm code to save the frame pointer before making the syscall and restore it afterwards. e.g. https://github.com/strace/strace/commit/0c75ebaad09d6d3f2395dfe6160904af883dd0d9 If pulseaudio can be fixed in the same way then that's probably a better approach. # *.desktop rules wont be generated during configure and build will fail
|
|
|
|
Adrian Bunk
On Thu, Mar 26, 2020 at 05:26:29PM +0200, Stefan Ghinea wrote:
...How are you reproducing the problem in pulseaudio? This sounds like a workaround for a bug in musl that was fixed 2 years ago. cu Adrian
|
|
|
|
Andre McCurdy
On Thu, Mar 26, 2020 at 12:16 PM Adrian Bunk <bunk@stusta.de> wrote:
The problem can show up anywhere that inline asm is trying to use r7. In this case it looks like: https://github.com/pulseaudio/pulseaudio/blob/master/src/pulsecore/remap_neon.c#L50 Better fix there might be to use registers { r4,r5,r6,r12 } instead of { r4,r5,r6,r7 } ? cu
|
|
|
|
Adrian Bunk
On Thu, Mar 26, 2020 at 12:53:08PM -0700, Andre McCurdy wrote:
On Thu, Mar 26, 2020 at 12:16 PM Adrian Bunk <bunk@stusta.de> wrote:After looking at the pulseaudio code I suspected the patch descriptionThe problem can show up anywhere that inline asm is trying to use r7. claiming pulseaudio syscall code would be the problem was rubbish, and that this NEON code was the problem. But when I tried to reproduce the problem it built for me with both glibc and musl in master (the patch didn't mention that this was a musl-only problem). Then I saw that this was fixed in musl upstream 2 years ago: https://git.musl-libc.org/cgit/musl/commit/?id=e3c682ab5257aaa6739ef242a9676d897370e78e cu Adrian
|
|
|
|
Andre McCurdy
On Thu, Mar 26, 2020 at 1:26 PM Adrian Bunk <bunk@stusta.de> wrote:
Yes, the comment looks like it was copied and pasted and doesn't really apply in this case (since pulseaudio isn't making syscalls). That should be updated. But when I tried to reproduce the problem it built for me with bothRight, it's not related to musl or glibc. I suspect it can be reproduced by building for an ARM target which supports NEON, ensuring that DEFAULTTUNE doesn't forcefully disable Thumb (e.g. it should be armv7vethf-neon, not armv7vehf-neon), setting ARM_INSTRUCTION_SET to thumb and then compiling with frame pointers enabled (e.g. by adding -fno-omit-frame-pointer to CFLAGS). In terms of a fix, then changing the code to use r12 instead of r7 is probably the best solution (assuming it works), but would need careful testing. Appending -fomit-frame-pointer to CFLAGS for ARM machines building for Thumb is safe and should fix the issue too. Presumably limiting the -fomit-frame-pointer workaround to ARM machines which support NEON building for Thumb would be an even more targeted solution.
|
|
|
|
Adrian Bunk
On Thu, Mar 26, 2020 at 02:23:03PM -0700, Andre McCurdy wrote:
...Thanks, this was helpful in figuring out what I missed when testing. I can now confirm that the problem still exists, also with glibc. cu Adrian
|
|
|
|
Tanu Kaskinen
On Thu, 2020-03-26 at 17:26 +0200, Stefan Ghinea wrote:
From: Catalin Enache <catalin.enache@windriver.com>Do you have a pointer to the asm code that does syscalls? I'm an upstream maintainer of PulseAudio, and to me it sounds a bit strange that there would be inline asm that does syscalls. It would be great to fix this problem in upstream (either by applying this fix/workaround in the build system, or by modifying the asm code so that the problem goes away). -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk
|
|
|
|
Tanu Kaskinen
On Mon, 2020-03-30 at 19:29 +0300, Tanu Kaskinen wrote:
On Thu, 2020-03-26 at 17:26 +0200, Stefan Ghinea wrote:I forgot to check if others had already commented on this... So theFrom: Catalin Enache <catalin.enache@windriver.com>Do you have a pointer to the asm code that does syscalls? I'm an real problem is that src/pulsecore/remap_neon.c uses the r7 register in a few places. Andre McCurdy said: "In terms of a fix, then changing the code to use r12 instead of r7 is probably the best solution (assuming it works), but would need careful testing." I'll try this. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk
|
|
|
|
Tanu Kaskinen
On Thu, 2020-03-26 at 14:23 -0700, Andre McCurdy wrote:
On Thu, Mar 26, 2020 at 1:26 PM Adrian Bunk <bunk@stusta.de> wrote:I finally found time to test fixing the assembly code to use r12On Thu, Mar 26, 2020 at 12:53:08PM -0700, Andre McCurdy wrote:Yes, the comment looks like it was copied and pasted and doesn'tOn Thu, Mar 26, 2020 at 12:16 PM Adrian Bunk <bunk@stusta.de> wrote:After looking at the pulseaudio code I suspected the patch descriptionOn Thu, Mar 26, 2020 at 05:26:29PM +0200, Stefan Ghinea wrote:The problem can show up anywhere that inline asm is trying to use r7....How are you reproducing the problem in pulseaudio? instead of r7. Seems to work fine (I was first baffled by incorrect behaviour, because I changed "{r4-r7}" to "{r4-r12}" without realizing that "r4-r12" meant a range of all registers from r4 to r12). Can you enlighten me: why did you choose r12 instead of r8? Why did the original author use registers r4-r7 instead of r0-r3? Is it somehow advisable to avoid registers r0-r3 and r8-r11? The code seems to work fine with any set of registers, except r7. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk
|
|
|
|
Andre McCurdy
On Fri, Jul 17, 2020 at 3:09 AM Tanu Kaskinen <tanuk@iki.fi> wrote:
On Thu, 2020-03-26 at 14:23 -0700, Andre McCurdy wrote:The compiler will work around whichever set of registers you want toOn Thu, Mar 26, 2020 at 1:26 PM Adrian Bunk <bunk@stusta.de> wrote:I finally found time to test fixing the assembly code to use r12On Thu, Mar 26, 2020 at 12:53:08PM -0700, Andre McCurdy wrote:Yes, the comment looks like it was copied and pasted and doesn'tOn Thu, Mar 26, 2020 at 12:16 PM Adrian Bunk <bunk@stusta.de> wrote:After looking at the pulseaudio code I suspected the patch descriptionOn Thu, Mar 26, 2020 at 05:26:29PM +0200, Stefan Ghinea wrote:The problem can show up anywhere that inline asm is trying to use r7....How are you reproducing the problem in pulseaudio? use (apart from r7 in some cases) so it's expected that other combinations will work fine. Some combinations will be more efficient than others (ie the compiler will need to do less shuffling values between registers or saving register values to the stack in order to make registers you specify available to you). Using r12 instead of r8 is just an educated guess about the combination will allow the compiler to generate the most efficient code. Registers r0-r3 and r12 can be used within a function without needing to preserve their previous contents, so if a function needs registers it's more efficient to use these registers first. Other registers need to be preserved (ie saved to the stack) before use. Registers r0-r3 are also used to pass non-floating point arguments to a function, so if a function takes 4 or more non-floating point arguments, then r0-r3 will already contain values which the function will need to use. Note that in this particular function, the first argument (ie the pointer m) is never actually used, so it may be that using r0, r4, r5 and r12 will give the best result. The function is pretty trivial though so I guess with a recent compiler just writing the whole thing in C will give close to the optimal result too without all the maintenance issues.
|
|
|
|
Tanu Kaskinen
On Fri, 2020-07-17 at 12:19 -0700, Andre McCurdy wrote:
On Fri, Jul 17, 2020 at 3:09 AM Tanu Kaskinen <tanuk@iki.fi> wrote:Thanks for explaining!On Thu, 2020-03-26 at 14:23 -0700, Andre McCurdy wrote:The compiler will work around whichever set of registers you want toOn Thu, Mar 26, 2020 at 1:26 PM Adrian Bunk <bunk@stusta.de> wrote:I finally found time to test fixing the assembly code to use r12On Thu, Mar 26, 2020 at 12:53:08PM -0700, Andre McCurdy wrote:Yes, the comment looks like it was copied and pasted and doesn'tOn Thu, Mar 26, 2020 at 12:16 PM Adrian Bunk <bunk@stusta.de> wrote:After looking at the pulseaudio code I suspected the patch descriptionOn Thu, Mar 26, 2020 at 05:26:29PM +0200, Stefan Ghinea wrote:The problem can show up anywhere that inline asm is trying to use r7....How are you reproducing the problem in pulseaudio? -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk
|
|
|