Hello, after some discussion in #debian-arm, I have to revise this.
On 04/26/2017 10:30 AM, Uwe Kleine-König wrote:
> On Mon, Apr 17, 2017 at 05:02:32PM +0100, Paul Brook wrote:
>> Package: libsbc1
>> Version: 1.3-1+b2
>> Followup-For: Bug #856487
>>
>> Not a stack corruption.
>>
>> This is miscompilation of sbc_analyze_4b_8s_armv6. gcc appears to look
>> into the asm function and decides that it does not clobber r3 (which the
>> normal ARM ABI says is call clobbered). The last out += out_stride ends
>> up incrementing the pointer by an arbitrary amount.
>>
>> The attached patch works around the bug.
>>
>> I'm not entirely sure whether this is a gcc bug or not, but at best it's
>> surprising behavior from gcc. I've attached a reduced testcase for the
>> toolchain
>> folks to argue over (compile with gcc -O2, tested with gcc 6.3.0-2 from
>> sid).
>
> AFAICT this is not a gcc bug. gcc is not supposed (and I'm sure this is
> even impossible in general) to determine the set of clobbered registers.
>
>> diff -ur clean/sbc/sbc_primitives_armv6.c sbc-1.3/sbc/sbc_primitives_armv6.c
>> --- clean/sbc/sbc_primitives_armv6.c 2013-04-30 17:19:23.000000000 +0100
>> +++ sbc-1.3/sbc/sbc_primitives_armv6.c 2017-04-17 16:43:49.918809345
>> +0100
>> @@ -102,6 +102,7 @@
>> "pop {r8-r11}\n"
>> "stmia r1, {r4, r5, r6, r7}\n"
>> "pop {r1, r4-r7, pc}\n"
>> + :::"r0", "r2", "r3", "ip"
>
> While this might fix the problem at hand, this is not a complete fix.
naked functions must only use basic assembler according to
https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html
.
> So there is no guarantee that r0 still has the value passed to the
> function when it returns.
>
> So r0, r1 and r2 must be declared inputs, r0 also an output.
So this is wrong.
> Also I'd
> recommend to fix the prototype of the functions to match reality.
This however stays correct.
> Other than that the code is hard to review and I wonder if it's worth to
> have this as hand coded assembly.
ditto.
>> /* Compile with -O2 on arm */
>> #include <stdint.h>
>> #include <stdlib.h>
>>
>> static void __attribute__((naked)) frob(int16_t *a, int32_t *b, const
>> int16_t *c)
>> {
>> /* The explicit clobber of r3 should not be necessary because that it
>> is implied by the function call?
>
> No it's not.
ok, AAPCS[1] says:
A subroutine must preserve the contents of the registers r4-
r8, r10, r11 and SP (and r9 in PCS variants that
designate r9 as v6).
So r3 can be clobbered and gcc must assume that this is the case.
BTW, the relevant assembly output (generated on abel.d.o) is:
000006cc <frob>:
6cc: e3a03102 mov r3, #-2147483648 ; 0x80000000
6d0: e5813000 str r3, [r1]
6d4: e12fff1e bx lr
000006d8 <test>:
6d8: e92d41f0 push {r4, r5, r6, r7, r8, lr}
6dc: e1a04001 mov r4, r1
6e0: e1a01002 mov r1, r2
6e4: e59f2054 ldr r2, [pc, #84] ; 740 <test+0x68>
6e8: e59f0054 ldr r0, [pc, #84] ; 744 <test+0x6c>
6ec: e08f2002 add r2, pc, r2
6f0: e7925000 ldr r5, [r2, r0]
6f4: e1a03103 lsl r3, r3, #2
6f8: e1a02005 mov r2, r5
6fc: e2840030 add r0, r4, #48 ; 0x30
700: e0817003 add r7, r1, r3
704: ebfffff0 bl 6cc <frob>
708: e1a01007 mov r1, r7
70c: e0876003 add r6, r7, r3
710: e1a02005 mov r2, r5
714: e2840020 add r0, r4, #32
718: ebffffeb bl 6cc <frob>
71c: e1a01006 mov r1, r6
720: e1a02005 mov r2, r5
724: e2840010 add r0, r4, #16
728: ebffffe7 bl 6cc <frob>
72c: e1a02005 mov r2, r5
730: e0861003 add r1, r6, r3
734: e1a00004 mov r0, r4
738: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
73c: eaffffe2 b 6cc <frob>
740: 0001090c .word 0x0001090c
744: 00000034 .word 0x00000034
And at address 70c r3 is assumed to still hold the initial value but
frob already clobbered it when being called at address 704.
When I change frob to:
void frob(int16_t *a, int32_t *b, const int16_t *c);
(and compile with -c) the code is correct. So it would indeed be
interesting to get some input from the gcc people.
Best regards
Uwe
[1]
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf
signature.asc
Description: OpenPGP digital signature
