pulp-platform / ri5cy_gnu_toolchain

22 stars 23 forks source link

Issues with compilation of inline assembly at higher levels of optimization #16

Closed deepsrc closed 6 years ago

deepsrc commented 6 years ago

We have a C function [given below] which has an inline assembly code.

uint32_t get_cpu_id_mc()
{
    uint32_t  cpu_id, stack_sz = KERNEL_PARAMS.STACK_SIZE;

#ifdef KERNEL_MODULE
    asm("addi %[result], %[inp], -1         \n\t"
        "not  %[result], %[result]          \n\t"
        "and  %[result], %[result], sp      \n\t"
        "add  %[result], %[result], %[inp]  \n\t"
        "lw   %[result], -8(%[result])      \n\t"
            :[result] "=&r" (cpu_id), [inp] "=r" (stack_sz));
#endif

    return cpu_id;
}

At higher levels of optimization [O3] the disassembly of this function looks like this -

000035f4 <get_cpu_id_mc()>:
    asm("addi %[result], %[inp], -1         \n\t"
        "not  %[result], %[result]          \n\t"
        "and  %[result], %[result], sp      \n\t"
        "add  %[result], %[result], %[inp]  \n\t"
        "lw   %[result], -8(%[result])      \n\t"
            :[result] "=&r" (cpu_id), [inp] "=r" (stack_sz));
    35f4:   fff78513            addi    a0,a5,-1
    35f8:   fff54513            not a0,a0
    35fc:   00257533            and a0,a0,sp
    3600:   00f50533            add a0,a0,a5
    3604:   ff852503            lw  a0,-8(a0)
#endif

    return cpu_id;
}
    3608:   00008067            ret

The values being passed to the inline assembly are not initialized correctly resulting into data corruption and unexpected code execution.

Without optimization the code disassembles like this -

__attribute__((optimize("O0"))) 
uint32_t get_cpu_id_mc()
{
    35f4:   fe010113            addi    sp,sp,-32
    35f8:   00812e23            sw  s0,28(sp)
    35fc:   02010413            addi    s0,sp,32
    uint32_t  cpu_id, stack_sz = KERNEL_PARAMS.STACK_SIZE;
    3600:   000017b7            lui a5,0x1
    3604:   80078793            addi    a5,a5,-2048 # 800 <pulp__PE+0x7ff>
    3608:   fef42623            sw  a5,-20(s0)
    asm("addi %[result], %[inp], -1         \n\t"
        "not  %[result], %[result]          \n\t"
        "and  %[result], %[result], sp      \n\t"
        "add  %[result], %[result], %[inp]  \n\t"
        "lw   %[result], -8(%[result])      \n\t"
            :[result] "=&r" (cpu_id), [inp] "=r" (stack_sz));
    360c:   fff78713            addi    a4,a5,-1
    3610:   fff74713            not a4,a4
    3614:   00277733            and a4,a4,sp
    3618:   00f70733            add a4,a4,a5
    361c:   ff872703            lw  a4,-8(a4)
    3620:   fee42423            sw  a4,-24(s0)
    3624:   fef42623            sw  a5,-20(s0)
#endif

    return cpu_id;
    3628:   ff040713            addi    a4,s0,-16
    362c:   ff872783            lw  a5,-8(a4)
}
    3630:   00078513            mv  a0,a5
    3634:   01c12403            lw  s0,28(sp)
    3638:   02010113            addi    sp,sp,32
    363c:   00008067            ret
atraber commented 6 years ago

I think your code contains a small error. The input constraints you are giving to the asm statement are :[result] "=&r" (cpu_id), [inp] "=r" (stack_sz) which means

So gcc is right in not setting stack_sz as it is never read anywhere. There are two options to fix this:

  1. use :[result] "=&r" (cpu_id), [inp] "+r" (stack_sz) (not the + instead of the =)
  2. use separate input and output constraints like so:
    
    uint32_t get_cpu_id_mc()
    {
    uint32_t  cpu_id, stack_sz = KERNEL_PARAMS.STACK_SIZE;

ifdef KERNEL_MODULE

asm("addi %[result], %[inp], -1         \n\t"
    "not  %[result], %[result]          \n\t"
    "and  %[result], %[result], sp      \n\t"
    "add  %[result], %[result], %[inp]  \n\t"
    "lw   %[result], -8(%[result])      \n\t"
        :[result] "=&r" (cpu_id) : [inp] "r" (stack_sz));

endif

return cpu_id;

}



Number 2 is of course the better option and also the more correct and elegant one
haugoug commented 6 years ago

Did it fix your issue ? If not could you try with the new toolchain here: https://github.com/pulp-platform/pulp-riscv-gnu-toolchain If not please reopen the issue in the new toolchain trackers.