[Bug c/33949] New: gcc-4.2.2 generates bad code on ARM

2007-10-30 Thread zero at colonel-panic dot org 
GCC generates incorrect code for the following C code in both ARM and Thumb
modes. The initialisation of param[0] is performed in the wrong leg of the if()
statement, consequently the assignemnt to DMA_ADDR_REG uses an uninitialised
value. Command line, compiler specs and object dump are included below.

I quickly tested 4.1.2 and 4.2.0 and they exhibit the same problem.

#define DMA_ADDR_REG(*(unsigned volatile *) 0x1000)

extern void panic(void);

static void inner_func(void *data, unsigned size)
{
if (!size)
panic();
else
DMA_ADDR_REG = (unsigned long) data;
}

void outer_func(unsigned offset, unsigned size)
{
unsigned char param[1];

param[0] = offset;

inner_func(param, size);
}

/*
 * gcc -Wall -O2 -fno-strict-aliasing
 *

bug.o: file format elf32-littlearm

Disassembly of section .text:

 :
   0:   b500push{lr}
   2:   b081sub sp, #4
   4:   2900cmp r1, #0
   6:   d005beq.n   14 
   8:   466amov r2, sp
   a:   4b05ldr r3, [pc, #20]   (20 <.text+0x20>)
   c:   3203addsr2, #3
   e:   601astr r2, [r3, #0]
  10:   b001add sp, #4
  12:   bd00pop {pc}
  14:   466bmov r3, sp
  16:   3303addsr3, #3
  18:   7018strbr0, [r3, #0]
  1a:   f7ff fffe   bl  0 
  1e:   e7f7b.n 10 
  20:   1000asrsr0, r0, #32
  22:   Address 0x0022 is out of bounds.

 *
 * gcc -Wall -O2 -fno-strict-aliasing -mthumb
 *

bug.o: file format elf32-littlearm

Disassembly of section .text:

 :
   0:   e52de004str lr, [sp, #-4]!
   4:   e351cmp r1, #0  ; 0x0
   8:   e24dd004sub sp, sp, #4  ; 0x4
   c:   128d2003addne   r2, sp, #3  ; 0x3
  10:   13e03a0emvnne   r3, #57344  ; 0xe000
  14:   15032fffstrne   r2, [r3, #-4095]
  18:   0a01beq 24 
  1c:   e28dd004add sp, sp, #4  ; 0x4
  20:   e8bd8000ldmia   sp!, {pc}
  24:   e5cd0003strbr0, [sp, #3]
  28:   ebfebl  0 
  2c:   eafab   1c 

 *
 * gcc -v
 *

Using built-in specs.
Target: arm-unknown-elf
Configured with: ../configure --target=arm-unknown-elf --prefix=/usr
--disable-nls --disable-shared --disable-threads --with-gnu-as --with-gnu-ld
--enable-multilib --disable-win32-registry --enable-sjlj-exceptions
--with-newlib --enable-__cxa_exit --enable-languages=c
--with-gxx-include-dir=/usr/arm-unknown-elf/include/c++
Thread model: single
gcc version 4.2.2

 */


-- 
   Summary: gcc-4.2.2 generates bad code on ARM
   Product: gcc
   Version: 4.2.2
Status: UNCONFIRMED
  Severity: normal
  Priority: P3
 Component: c
AssignedTo: unassigned at gcc dot gnu dot org
ReportedBy: zero at colonel-panic dot org
 GCC build triplet: i686-pc-linux-gnu
  GCC host triplet: i686-pc-linux-gnu
GCC target triplet: arm-unknown-elf


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33949

[Bug c/33949] gcc-4.2.2 generates bad code on ARM

2007-10-30 Thread zero at colonel-panic dot org 


--- Comment #2 from zero at colonel-panic dot org  2007-10-30 15:31 ---
Subject: Re:  gcc-4.2.2 generates bad code on ARM

rguenth at gcc dot gnu dot org wrote:
> --- Comment #1 from rguenth at gcc dot gnu dot org  2007-10-30 14:31 
> ---
> huh?  you initialize DMA_ADDR_REG to the address of param (which is on the
> stack).
> The initialization of the contents of param is unused and as such dropped
> (but due to -fno-strict-aliasing the compiler assumes it escapes to panic()).
> 

No, the initialisation of param is not unused. The address of param is 
stored in a DMA controller register which then uses this address to 
access the values in param[] (obviously in the original code param[] is 
a much larger array and there is code that follows the assignment to 
DMA_ADDR_REG that kicks off a DMA engine which copies the values out of 
param[] and to a peripheral and then waits for the DMA engine to 
complete - I reduced the code to the minimum that exhibits the fault).

Surely the assignment of the address of a variable through a "volatile" 
pointer should guarantee that the initialisation of the variable is 
complete?

I can make the code look more practical by fleshing it out with the 
following. The compiler still does the initialisation before the panic() 
rather than before the assigment to DMA_ADDR_REG.

#define DMA_COUNT_REG (*(unsigned volatile *) 0x1004)

static void inner_func(void *data, unsigned size)
{
if (!size)
panic();
else {
DMA_COUNT_REG = 1;
DMA_ADDR_REG = (unsigned long) data;
while (DMA_COUNT_REG)
;
}
}

PS - recompiling the original test case with -fstrict-aliasing does not 
affect the compiler output.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33949

[Bug c/33949] gcc-4.2.2 generates bad code on ARM

2007-10-30 Thread zero at colonel-panic dot org 


--- Comment #4 from zero at colonel-panic dot org  2007-10-30 16:45 ---
Subject: Re:  gcc-4.2.2 generates bad code on ARM

rguenth at gcc dot gnu dot org wrote:
> --- Comment #3 from rguenth at gcc dot gnu dot org  2007-10-30 15:54 
> ---
> It doesn't matter that you store the address of param to a volatile variable
> (this store is not removed), but that
> 
>   DMA_ADDR_REG = (unsigned long) data;
> 
> does not make the stack local live longer than the frame of outer_func and
> *data is not used after the assignment.  Probably your code kicking off
> the DMA engine lacks a memory barrier and a use of DMA_ADDR_REG.
> 
> 

Thanks for the explanation. Seems like an optimisation too far. I can't 
see that using the address of a variable and not it's contents is a 
particularly common practice, and having to use a GCC specific barrier 
to get simple embedded code to work sucks.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33949