cm3/cortex.h mixed bool/uint32_t usage

[karlp]

As indicated in the comments for #323 there's some somewhat unusual mixes of bool and uint32_t in the cm3/cortex.h file.

Arguably, many of the bool types should be uint32_t, as they're dealing with registers anyway. Further, the register keyword is almost entirely irrelevant, but should probably be double checked with the inline asm.

[fenugrec]

(TL;DR summary at the end)
For the functions cm_is_masked_interrupts() and similar, in include/libopencm3/cm3/cortex.h, ( https://github.com/libopencm3/libopencm3/blob/master/include/libopencm3/cm3/cortex.h#L87 ),
the cast to bool for the return value is quite OK in C99.
Ref C99 n1256, sec 6.3.1.2, "When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1.".
This is great, it amounts to " return (result !=0)? 1:0;"

I think that the functions that get/set u32 values (PRIMASK and FAULTMASK, any others?) should not be cast to/from bool. While it's true that they probably always are 1-bit, the economy is extremely dubious (and premature, in any case), not future-proof, and in my opinion provides no improvement in clarity.

For the 'register' keyword, I would agree that it should be irrelevant. Parsing the __asm__ line, the compiler already knows the instruction writes to a register, that eventually needs to be cast to bool (should be a simple "cmp rn, 0" ). Plus, original CMSIS code uses a similar

uint32_t __get_PRIMASK(void) {
  uint32_t result;
  __ASM volatile ("MRS %0, primask" : "=r" (result) );
  return(result);
}

**** however ****
tests with arm-gcc 5.2.0 give interesting results : (tested only for cortex-M0 output, since it has the most limited instruction set and may show subtle differences). Test code used: http://pastebin.ca/3119012

1- with no optimizations, as expected the generated code is silly and implements quite literally what was written, i.e. a local variable on stack if the "register" keyword is removed. Interestingly, gcc spews 5 instructions before "cmp rn, 0" :

    MRS r3, PRIMASK
    movs    r4, r3
    movs    r3, r4
    subs    r2, r3, #1
    sbcs    r3, r3, r2
    uxtb    r3, r3
    cmp r3, #0

2- with -Os and -O2, gcc takes liberties and exposes an issue: in some cases, depending on the test code, it can optimize away some "MRS rn, primask" instructions if cm_is_masked_interrupts() is called multiple times in the same context / scope. This is most probably due to the lack of "volatile" keyword on the __asm line. From the gcc docs :
" You can prevent an asm instruction from being deleted by writing the keyword volatile
after the asm. [...] The volatile keyword indicates that the instruction has important side-effects. GCC
does not delete a volatile asm if it is reachable. (The instruction can still be deleted if
GCC can prove that control flow never reaches the location of the instruction.) "

3- with -Os and -O2, the "register" keyword has no effect on the generated code.

**** TL; DR summary ****
4 recommendations:

• add "volatile" on the __asm__ line
• the "register" keyword can be kept : it changes nothing with optimizations, and has no objectionable effect at -O0
• try to use -Os at all times, unless debugging at a low level !
• alter the types of functions setting, saving or returning u32 masks.

[ChuckM]

Seems like a gcc quirk, what does clang do when you do this?

[fenugrec]

A quick inspection of the libopencm3-examples repo shows only 1 use of the cm_* functions defined in cortex.h , in examples\stm32\f4\stm32f429i-discovery\usart_irq_console/usart_irq_console.c .
So, changing bool to uint32_t would cause breakage, but very easy to fix and more future-proof should there ever be more than 1 bit defined in the PRIMASK and FAULTMASK registers.

However, for the functions named "cm_is_masked_*" , I think returning u32 is not ideal; the naming suggests a true/false result. Returning a u32 would fit better with names like "cm_get_intmask" and "cm_get_faultmask".

In any case, I'll start with a less objectionable PR that only changes some of the functions to use u32.