Re: [avr-gcc-list] avr-g++ local variable constructor / block body / destructor execution order

[David Brown]

Oleksandr Redchuk wrote:
> Is this "overoptimization" bug or not?

I'm not really an expert in this, but I think it is a valid 
optimisation.  Access to SREG is volatile, so the compiler cannot change 
or re-order accesses, but it is free to re-arrange access to other 
(non-volatile) data around it.  Although the "crit_sect" object exists 
*logically* until after the returned expression is evaluated, and 
therefore *logically* should be destructed between reading "counter" and 
executing the return, the compiler is free to re-organise the 
non-volatile code.  Since the "crit_sect" object is not accessed later 
on, it may be destructed immediately as long as volatile accesses are 
preserved.  Similarly, if you have multiple objects being constructed or 
destructed, their ordering is *not* guaranteed by the compiler - all you 
are guaranteed is that the code will act as though they followed the 
order given, based on what the compiler knows of the code.

The solution, as far as I can see, is to either make "counter" a 
volatile (which may lead to extra code in other places), or to introduce 
a memory block which forces the compiler to synchronise accesses to 
memory before and after the block using asm("" ::: "memory")

inline uint32_t get_counter() {
        crit_sect cs;
        uint32_t x = counter;
        asm volatile("" : : : "memory");
        return x;
}

mvh.,

David



> ---------------------------------
> #include <stdint.h>
> #include <avr/io.h>
> #include <avr/interrupt.h>
>
> class crit_sect
> {
> public:
>     crit_sect() : _sreg( SREG ) { cli(); }
>     ~crit_sect() { SREG = _sreg; }
> private:
>     uint8_t _sreg;
> };
>
>
> uint32_t counter;
>
> inline uint32_t get_counter() { crit_sect cs; return counter; }
>
> uint16_t get_cnt_low() {  return (uint16_t)get_counter(); }
>
> uint32_t get_delta(uint32_t start) { return get_counter() - start; }
> ---------------------------------
>
> gcc 4.1.2 (WinAVR-20070525) and 4.2.0 both produces code like this:
>
> ---------------------------------
> _Z11get_cnt_lowv:
>         in r24,95-0x20
>         cli
>         out 95-0x20,r24
>         lds r24,counter
>         lds r25,(counter)+1
>         ret
> ---------------------------------
>
> ~crit_sect() executed *before* counter reading in both get_cnt_low()
> and get_delta()
> (full listings attached).
> Optimization level does'nt matter. -Os, -O1..-O3
>
> gcc 3.4.6 produces not so "efficient" code but with right sequence -
>
> crit_sect constructor
> counter reading
> crit_sect destructor
> read value utilization
>
>
> With
>     volatile uint32_t counter;
> gcc 4.x produces expected code, but, I think, "volatile" is not
> required in this code.
>
> destructor *must* be executed at end of block,
> *after* variable reading, either volatile variable or not.