Gcc disable interrupt movement about spinnaker_tools HOT 5 CLOSED

Apparently, gcc 7.2 has the fix (and it was a general bug too). It would be nice to not have the function call overhead in these very critical bits of code!

from spinnaker_tools.

Please be very careful. There is no straight-forward way to 'prove' that any changes are correct. Usually we can only state that no issues manifest in the tests we run.

I believe that this issue may be different from the one that we experimented. We only looked at it using 'armcc'.

Our issue is not a bug. In our case, the compiler reorders instructions that it is entitled to reorder because we do not establish a sequence point when we 'inline' the functions that enable/disable interrupts. The function call is the sequence point.

The issue manifests in the following code.

void event_schedule (event_t *e, uint t) {
uint cpsr = int_off ();
... code that inserts event in event queue ...
mode_set (cpsr);
}

In the compiled code, an instruction that reads a variable into a register, which should have been located between the 'int_off()' and 'mode_set()', was placed by the compiler before the 'int_off()' call WHEN the functions were inlined. The compiler is entitled to do it, unless the variable is declared as volatile or a sequence point is established by means other than the function call. If an interrupt occurs at the wrong time, the variable gets the wrong value. This is very difficult to diagnose.

We tried to set the sequence point as follows:

void event_schedule (event_t *e, uint t) {
uint cpsr = int_off ();
__schedule_barrier();
... code that inserts event in event queue ...
__schedule_barrier();
mode_set (cpsr);
}

ARM indicated that this is not the correct way to set a sequence point even thought the 'armcc' documentation states:

__schedule_barrier intrinsic

This intrinsic creates a sequence point where operations before and operations after the sequence point are not merged by the compiler.

They said that the correct way to do it would be:

void event_schedule (event_t *e, uint t) {
uint cpsr = int_off ();
__memory_changed();
... code that inserts event in event queue ...
__force_stores();
mode_set (cpsr);
}

This seems to work, but I think that these intrinsics only apply to 'armcc'.

from spinnaker_tools.

I think that means that we should do this (syntactically compressing it a bit from the real code to improve the readability of this page!):

static __inline __attribute__((always_inline)) void spin1_store_barrier(void) {
    asm volatile("" : : : "cc", "memory");
}

static __inline __attribute__((always_inline)) void spin1_memory_barrier(void) {
    asm volatile("" : : : "cc", "memory");
}

__inline __attribute__((always_inline)) uint spin1_irq_disable(void) {
    uint old_val, new_val;
    asm volatile (
    "mrs	%[old_val], cpsr \n\
     orr	%[new_val], %[old_val], #0x80 \n\
     msr	cpsr_c, %[new_val] \n"
     : [old_val] "=r" (old_val), [new_val] "=r" (new_val) : : );
    spin1_memory_barrier();
    return old_val;
}

__inline __attribute__((always_inline)) void spin1_mode_restore(uint cpsr) {
    spin1_store_barrier();
    asm volatile (
    "msr	cpsr_c, %[cpsr]"
    : : [cpsr] "r" (cpsr) : );
}

Now, spin1_memory_barrier() and spin1_store_barrier() correspond to the __memory_changed() and __force_stores() intrinsics, and I'm calling them in here so that we don't have to modify everywhere that uses the other calls. They're the same in GCC (so far as I can tell, after a very confusing morning of searching online!) but different in ArmCC (where they actually just call the real intrinsics).

from spinnaker_tools.

References:

• http://infocenter.arm.com/help/topic/com.arm.doc.dui0491c/DUI0491C_arm_compiler_reference.pdf
• https://stackoverflow.com/questions/18088873/how-to-protect-a-global-variable-shared-by-isr-and-regular-function
• https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers

from spinnaker_tools.

I believe that #107 will put this all to bed. And somehow we've ended up with the code already being inline (but outside of SCAMP; SCAMP is built in THUMB mode…)

from spinnaker_tools.