Is the connectivity framework panic() implementation correct?

[stephenlangstaf]

In the connectivity framework the implementation of panic() mixes C and assembler code like this:

void panic( panicId_t id, uint32_t location, uint32_t extra1, uint32_t extra2 )
{
#if gUsePanic_c
    /* Save the Link Register */
    volatile uint32_t savedLR = 0;
    __asm("push {r2}  ");
    __asm("push {LR} ");
    __asm("pop  {r2} ");
    __asm("str  r2, [SP, #4]");
    __asm("pop {r2}");

    panic_data.id = id;
    panic_data.location = location;
    panic_data.extra1 = extra1;
    panic_data.extra2 = extra2;
    panic_data.linkRegister = savedLR;
    panic_data.cpsr_contents   = 0;
...

This ASM access to this variable ("str r2, [SP, #4]") appears to make the assumption that the variable "savedLR" will be placed on the stack in a particular place, but I don't think that's necessarily the case under all optimisation levels.

For example, when compiling with -oS (optimise for size) I get the following disassembly (my markup in brackets):

00000000 <panic>:
   0:    b537          push    {r0, r1, r2, r4, r5, lr}
   2:    2500          movs    r5, #0
   4:    9501          str    r5, [sp, #4]
   6:    b404          push    {r2}
   8:    b500          push    {lr}
   a:    bc04          pop    {r2}
   c:    9201          str    r2, [sp, #4]
   e:    bc04          pop    {r2}
  10:    4c05          ldr    r4, [pc, #20]    ; (28 <panic+0x28>)
  12:    60e3          str    r3, [r4, #12]       (extra2)
  14:    9b01          ldr    r3, [sp, #4]
  16:    6020          str    r0, [r4, #0]        (id)
  18:    6061          str    r1, [r4, #4]        (location)
  1a:    60a2          str    r2, [r4, #8]        (extra1)
  1c:    6123          str    r3, [r4, #16]       (linkRegister)
  1e:    6165          str    r5, [r4, #20]       (cpsr_contents)
  20:    f7ff fffe     bl    0 <OSA_InterruptDisable>
            20: R_ARM_THM_CALL    OSA_InterruptDisable
  24:    46c0          nop            ; (mov r8, r8)

As I understand it the instructions at offsets 8..a (push {lr}; pop {r2}) have the effect of copying lr into r2, and the instruction at offset c (str r2, [sp, #4]) then puts the value of r2 (which is lr) onto the stack, which is mirrored by the instruction at offset 14 (ldr r3, [sp, #4]) which picks the lr value off the stack.

The problem as I see it is that the intervening instruction at offset e (pop {r2}) alters the stack pointer between the store (offset c) and the load (offset 14).

Now it could be that this code accidentally works, and it could be argued that it does not matter if we clobber the stack or read duff values as the processor is going to halt soon anyway, but what would be the right way of mixing C and ASM to get access to the link register value?

I'm thinking that the safe way to do this is as follows:

void panic( panicId_t id, uint32_t location, uint32_t extra1, uint32_t extra2 )
{
#if gUsePanic_c
    /* Save the Link Register */
    volatile uint32_t savedLR;

    __asm("mov %0, lr\n"
                : /* outputs */   "=r" (savedLR)
                : /* no inputs */
                : /* no clobbers */);

    panic_data.id = id;
    panic_data.location = location;
    panic_data.extra1 = extra1;
    panic_data.extra2 = extra2;
    panic_data.linkRegister = savedLR;
    panic_data.cpsr_contents   = 0;

...

Which compiles down to:

00000000 <panic>:
   0:    b513          push    {r0, r1, r4, lr}
   2:    4674          mov    r4, lr              (set r4 = LR)
   4:    9401          str    r4, [sp, #4]        (set stacked savedLR = r4)
   6:    4c06          ldr    r4, [pc, #24]    ; (20 <panic+0x20>)
   8:    60e3          str    r3, [r4, #12]       (extra2)
   a:    9b01          ldr    r3, [sp, #4]        (set r3 = stacked savedLR) 
   c:    6020          str    r0, [r4, #0]        (id)
   e:    6123          str    r3, [r4, #16]       (linkRegister)
  10:    2300          movs    r3, #0             (set r3 = 0)
  12:    6061          str    r1, [r4, #4]        (location)
  14:    60a2          str    r2, [r4, #8]        (extra1)
  16:    6163          str    r3, [r4, #20]       (cspr_contents = r3 = 0)

Thoughts?

[mario_castaneda]

Hi Stephen,

Than you for your feedback. 

We are checking this workaround in our stack software, however, you could implement this by your side, I will get in contact with the R&D team for this solution.

Regards,

Mario