Fix memcpy and memset to standard return pointers

[HDC67]

as per K&R etc.

Found when debugging at91bootstrap. gcc 8.2.0 optimiser appears to expect memcpy to return the pointer to the destination as is the standard for memcpy and memset. Current code was returning pointer to (destination + length). This resulted in GPIOs etc not being set up in our board's version of sama5d4_xplained.c/at91_spi0_hw_init() in particular.

[ehristev]

Hi,

Thank you for the patch. Is the return value used in some points in the code in other way than K&R specification ? Meaning would this break anything as it is used right now ? Eugen

[HDC67]

Hello

Return code is not used anywhere in the at91bootstrap code itself that I found for memcpy or memset called intentionally by the code. We found gcc was using memcpy and expecting memcpy to return the destination pointer which is what broke things as it relied on return value still being set. It's just lucky it works at the moment - it seems the newer gcc (when we moved to Yocto thud) broke things.

So, no it should not break anything in the existing code.

Edit: I don't fully understand the mechanism here. gcc is calling memcpy as part of its compile process. That somehow gets replaced by the user implemented version rather than from a library. Because it doesn't operate exactly the same way that gcc expects, it produces bad code that can only be seen at the machine code level.

[ehristev]

Ok, it's good. To merge your patch, can you please update your commit message as following:

lib: memcpy: <your message here>
<additional message here>
Signed-off-by: <your name> <your e-mail>

[ehristev]

Which version of the GCC you are using ? meaning which ABI ? Did you try with none-eabi ? GCC should be instructed to not use any libraries. at91bootstrap must not be linked with any built-in compiler libs. Otherwise the size will be increased with useless crap, and it won't fit in our small SRAM.

[HDC67]

Gcc 8.2.0

The problem is it's using at91bootstrap memcpy (not a library) which does not work like every other memcpy. I'll have to check the ABI version but I don't think that's exactly relevant given it's not a broken library.

Whether it should be using memcpy supplied by the source it's compiling or not is another question.

This is being compiled by Yocto Thud. I somewhat expect it will be reproducible there without any of our custom layer or recipe. It didn't happen with Yocto Morty and moving to Thud exposed the problem.

[ehristev]

I know GCC 8.2.0, but arm compilers can go in different flavours, arm-linux-eabi, arm-none-eabi, arm-linux-gnueabihf- , etc. each type has a different ABI and different standard of PCS Modify the commit as requested and I will merge your patch, no worries about that.

[HDC67]

It's arm-poky-linux-gnueabi

Not sure how to easily force Yocto to use something else. Building it outside standalone with arm-linux-gnueabi (v8.3.0) from Ubuntu packages, works fine too.

Something with our Yocto build using meta-atmel/recipes-bsp/at91bootstrap is causing gcc to generate this (which is valid if memcpy does what would be normally expected). I don't see any major difference in how it builds it. -fno-pie which was recently added to the Makefile might make a difference, but it's a bit tricky going back and forth recompiling in Yocto.

So

void at91_spi0_hw_init(void)
{
    /* Configure PIN for SPI0 */
    const struct pio_desc spi0_pins[] = {
        {"SPI0_MISO",   AT91C_PIN_PC(0), 0, PIO_DEFAULT, PIO_PERIPH_A},
        {"SPI0_MOSI",   AT91C_PIN_PC(1), 0, PIO_DEFAULT, PIO_PERIPH_A},
        {"SPI0_SPCK",   AT91C_PIN_PC(2), 0, PIO_DEFAULT, PIO_PERIPH_A},
        {"SPI0_NPCS",   CONFIG_SYS_SPI_PCS, 1, PIO_DEFAULT, PIO_OUTPUT},
        {(char *)0, 0, 0, PIO_DEFAULT, PIO_PERIPH_A},
    };

    /* Configure the PIO controller */
    pio_configure(spi0_pins);

    pmc_enable_periph_clock(AT91C_ID_PIOC);
    pmc_enable_periph_clock(AT91C_ID_SPI0);
}

compiles to:

00000000 <at91_spi0_hw_init>:
   0:   e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
   4:   e59f1034        ldr     r1, [pc, #52]   ; 40 <at91_spi0_hw_init+0x40>
   8:   e24dd06c        sub     sp, sp, #108    ; 0x6c
   c:   e28d3004        add     r3, sp, #4
  10:   e08f1001        add     r1, pc, r1
  14:   e3a02064        mov     r2, #100        ; 0x64
  18:   e2811e12        add     r1, r1, #288    ; 0x120
  1c:   e1a00003        mov     r0, r3
  20:   ebfffffe        bl      0 <memcpy>
  24:   ebfffffe        bl      0 <pio_configure>
  28:   e3a00019        mov     r0, #25
  2c:   ebfffffe        bl      0 <pmc_enable_periph_clock>
  30:   e3a00025        mov     r0, #37 ; 0x25
  34:   ebfffffe        bl      0 <pmc_enable_periph_clock>
  38:   e28dd06c        add     sp, sp, #108    ; 0x6c
  3c:   e49df004        pop     {pc}            ; (ldr pc, [sp], #4)
  40:   00000028        .word   0x00000028

instead of something like

00000000 <at91_spi0_hw_init>:
   0:   e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
   4:   e24dd06c        sub     sp, sp, #108    ; 0x6c
   8:   e3a02064        mov     r2, #100        ; 0x64
   c:   e59f1024        ldr     r1, [pc, #36]   ; 38 <at91_spi0_hw_init+0x38>
  10:   e28d0004        add     r0, sp, #4
  14:   ebfffffe        bl      0 <memcpy>
  18:   e28d0004        add     r0, sp, #4
  1c:   ebfffffe        bl      0 <pio_configure>
  20:   e3a00019        mov     r0, #25
  24:   ebfffffe        bl      0 <pmc_enable_periph_clock>
  28:   e3a00025        mov     r0, #37 ; 0x25
  2c:   ebfffffe        bl      0 <pmc_enable_periph_clock>
  30:   e28dd06c        add     sp, sp, #108    ; 0x6c
  34:   e49df004        pop     {pc}            ; (ldr pc, [sp], #4)
  38:   000000b4        .word   0x000000b4

The memcpy occurs as part of setting up the memory structure. Destination is passed in r0, source in r1 and length in r2. It should return destination back in r0 which then gets used by the call to pio_configure. However, because r0 is some other value now it breaks. If r0 is reset to the location of the memory structure as in the second piece of code then it works fine of course.

[ehristev]

I took the liberty of editing the commit message a little bit and then pushed to master/at91bootstrap-3.x branches. Thank you for your contribution !