6
\$\begingroup\$

I sometimes see code that disables interrupts, for example to perform a non-atomic read/write to a global variable used in an ISR. On AVR with gcc, this may look like:

ExpensiveOperation();
cli();
// Perform a non-atomic read/write
sei(); // Assume that it is acceptable to enable global interrupts here.

The cli and sei macros expand to asm volatile statements with memory barriers. The volatile keyword ensures that the cli/sei instructions are not optimized out, while the memory barriers ensure that if the non-atomic read/write is to a volatile variable, it will occur between the cli and sei instructions. However, this page suggests that nothing is preventing the compiler from putting ExpensiveOperation after the cli instruction.

Interrupts often require precise timing. If disabling interrupts in C risks having expensive operations run with interrupts disabled, should timing critical interrupts only be disabled in inline assembly (or should your program be rewritten to use only atomic reads/writes)?

\$\endgroup\$
  • 4
    \$\begingroup\$ @IgnacioVazquez-Abrams: The goal is to ensure that given something like cli(); counter++; sei();, the generated code ill only disable interrupts for a few instruction cycles. If the compiler rearranged the code to cli(); ExpensiveOperation(); counter++; sei(); and expensive operation took many milliseconds to complete, that could be disastrous. \$\endgroup\$ – supercat Aug 24 '15 at 19:31
  • 2
    \$\begingroup\$ It's only a bad idea if there is something better. I would be careful about what goes between the disable/enable. Nothing more than the minimum read or write, preferably just assignment statements rather than anything involving dynamically allocated memory or any other kind of indirection. For example, stuff it into a statically allocated (register even) variable first, then disable. Or do it all in inline assembly (safer). \$\endgroup\$ – Spehro Pefhany Aug 24 '15 at 19:38
  • 1
    \$\begingroup\$ Any compiler that rearranged things such that an optimizer moved things across a memory barrier would have all sorts of bugs filed against it. While it may not be in the standard, that's just something they shouldn't be doing. \$\endgroup\$ – Ignacio Vazquez-Abrams Aug 24 '15 at 19:38
  • 1
    \$\begingroup\$ @IgnacioVazquez-Abrams The example in the link I posted, in which a slow division call is moved across the cli, still occurs on avr-gcc 5.2.0. The memory barrier doesn't protect against it because the division involves a local variable and the call doesn't have side effects. \$\endgroup\$ – varactyl Aug 24 '15 at 20:17
  • 1
    \$\begingroup\$ @Lundin But if the specific interrupt you disable is high priority and time-critical, your write/read could be interrupted by other lower priority interrupts, increasing the time that the more important interrupt is disabled. \$\endgroup\$ – varactyl Aug 25 '15 at 18:45
2
\$\begingroup\$

Depending upon what kind of side-effects ExpensiveOperation() has, it may be illegitimate for a compiler to move it beyond a call to a function that the compiler can't "see" into if, from the compiler's point of view, there would be a possibility of such a function making use of such side-effects. It would be helpful if there were a standard function, typically implemented by an intrinsic, which would in actuality do nothing, but which a compiler would be required to treat as though it might do anything. Compilers which treated such a function as an intrinsic could avoid having to actually generate a useless "call" instruction [or any other code] for it beyond the fact required to ensure that any global variables which are cached in registers would get flushed before the "call".

Unfortunately, while calling an outside function would probably force a compiler to process ExpensiveOperation() in its entirety first, I don't know of anything that would 100% eliminate the possibility of a compiler identifying portions of that function whose execution could be deferred. Still, adding a sequenced side effect to ExpensiveOperation() and calling an outside function that would require any such side effect would need to have been completed may be the best one can hope for.

\$\endgroup\$
3
\$\begingroup\$

The core problem here is that this compiler apparently decided to re-order inline assembler instructions just as if they were any C code, without having a clue about what those instructions do. So while those instructions might not access any C level variables, they access the very fundamental condition code register!

The compiler is explicitly not allowed to re-order instructions if it would affect the program behavior, period. And it is quite hard to find anything that would affect program behavior more than writes to the most fundamental register in the CPU core.

So this is nonsense behavior and can only be regarded as a compiler bug, likely in the AVR port.


A possible work-around for this compiler bug might be to introduce a side effect, as specified by the C standard, before the cli() call. One way to do this is to implement ExpensiveOperation() as an actual C function. Note that in the linked example, the expensive operation was not a function. Another way would be to write some dummy code like

ExpensiveOperation();
volatile uint8_t invoke_side_effect = 0;
cli();

Otherwise, standard procedure when encountering a buggy optimizer is to disable that optimizer. If you can find the particular option that allows the compiler to re-order instructions and disable it, that is the best thing to do.

\$\endgroup\$
1
\$\begingroup\$

You might try putting the cli()/write/sei() sequence in its own function. I've found that optimizers tend to be reluctant to move code across functions, especially when optimizing for size. I'm not sure about AVR-GCC specifically, though.

\$\endgroup\$
  • \$\begingroup\$ If link time optimization is enabled (and some compilers may one day enable it by default), this strategy is less likely to work. At least in gcc, it looks like the only way to ensure that swapping won't happen is to do it all in inline assembly. The compiler mentioned in this post seems to be able to do it in C, so not all compilers have this problem. \$\endgroup\$ – varactyl Aug 25 '15 at 6:09
  • \$\begingroup\$ Yeah, I suspect the real solution is to use a compiler that's designed for embedded programming and leave the aggressive optimizations off. \$\endgroup\$ – Adam Haun Aug 25 '15 at 14:20
1
\$\begingroup\$

There are several issues here:

  1. AFAIK, there is only one bit to control interrupts on an AVR. So discussion about more precise interrupt control does not apply to this CPU.
  2. The example illustrates a strong case for looking at the assembler for compiler generated code.
    It should be practical to spot all uses of cli to sei with a small edit script applied to objdump output (to make it easy to monitor). A program-based script could highlight all examples of 'call' between them.
    This might show there is no problem to solve in your code.
  3. Controlling interrupts using cli()/sei() is the easiest way for a developer to ensure atomic access, and hence maintain memory consistency, for multi-byte values on an AVR.
    Having to write assembler may be so error prone, or negatively impact optimisation, that it is a poor approach. I wouldn't do that until the result of step 1 shows there is a problem to solve.
  4. The example shows that the compiler has generated code for the integer divide by inserting a subroutine call for val = 65535U / val, and that call is the code that has been 'optimised' between cli() to sei().
    However, it does not prove that user-generated subroutines are ever moved into the code between cli() and sei(). So this may be a none-issue for the, potentially, much more significant case.

  5. This example is fixed by introducing a new volatile variable:

    #define cli() __asm volatile( "cli" ::: "memory" )
    #define sei() __asm volatile( "sei" ::: "memory" )
    unsigned int ivar;  
    void test2( unsigned int val ) {  
       volatile unsigned int val1 = 65535U / val;
       cli();
       ivar = val1;
       sei();
    }
    

The generated code became:

  92:   cf 93           push    r28
  94:   df 93           push    r29
  96:   00 d0           rcall   .+0         ; 0x98 <_Z5test2j+0x6>
  98:   cd b7           in  r28, 0x3d   ; 61
  9a:   de b7           in  r29, 0x3e   ; 62
  9c:   bc 01           movw    r22, r24
  9e:   8f ef           ldi r24, 0xFF   ; 255
  a0:   9f ef           ldi r25, 0xFF   ; 255
  a2:   0e 94 fb 00     call    0x1f6   ; 0x1f6 <__udivmodhi4>
  a6:   7a 83           std Y+2, r23    ; 0x02
  a8:   69 83           std Y+1, r22    ; 0x01
  aa:   f8 94           cli  <----------------------- switch off interrupts
  ac:   89 81           ldd r24, Y+1    ; 0x01
  ae:   9a 81           ldd r25, Y+2    ; 0x02
  b0:   90 93 01 01     sts 0x0101, r25
  b4:   80 93 00 01     sts 0x0100, r24
  b8:   78 94           sei  <----------------------- switch on interrupts
  ba:   0f 90           pop r0
  bc:   0f 90           pop r0
  be:   df 91           pop r29
  c0:   cf 91           pop r28
  c2:   08 95           ret

This is not pretty, but it is relatively easy, and may be sufficient. Of course, avoid premature optimisations; don't do subtle changes to control the compiler until it becomes important, and preferably after the code is working and stable.

I would check generated code, and wait to see a problem, then solve that specific case.

I think I would raise a bug report if my user-level code is moved between the cli()/sei(). That would give the compiler developers the opportunity to identify work-arounds or develop fixes. Compiler developers are free to invent solutions, often using pragma's, and they might respond to a bug report by offering a robust fix.

In the meantime, it would be easier to continue along the easy path, rather than make development harder, until there is evidence of a significant problem.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.