[bug #22163] Everytime ATOMIC_BLOCK(ATOMIC_RESTORESTATE) C++ compiler generates warning - unused variable 'sreg_save'

[avrs-admin]

Wed 30 Jan 2008 06:17:06 AM CET

Everytime when using ATOMIC_BLOCK(ATOMIC_RESTORESTATE) macro I have got compiler warning - warning: unused variable 'sreg_save'. I'm using winavr-20071221.

This issue was migrated from https://savannah.nongnu.org/bugs/?22163

[avrs-admin]

Eric Weddington Wed 30 Jan 2008 04:20:35 PM CET

Tomasz, Could you please post a small test case that gives this warning?

[avrs-admin]

Lars Jonsson Wed 30 Jan 2008 05:14:44 PM CET

I have not seen this warning despite having used this macro in several projects. Are you using C++ Tomasz (I only have C projects)?

[avrs-admin]

Tomasz Francuz Thu 31 Jan 2008 03:56:23 AM CET

Sorry, but mistake I didn't mention that I'm using avr-g++ compiler. When using avr-gcc everything is working perfectly, but in g++ the following code produces warning: #include <util/atomic.h>

int main() { volatile int b,a; a=0; b=0; ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { if(b==0) a++; } }

I don't know if you consider it as a bug, but it would be great if ATOMIC_BLOCK could be used in c++.

[avrs-admin]

David Brown Thu 31 Jan 2008 11:16:58 AM CET

I've just tried this code section, and there is a good reason for the "unused variable 'sreg_save'" warning - looking at the generated assembly, the variable is unused.  In other words, the SREG is not restored after the block, as it is in the C version.

Could it be that the attribute((cleanup)) attribute is not working correctly for avr c++, at least when used in this way?  If that's the case, then this is definitely a bug.

I've noticed another issue with the atomic.h functions - I don't think the memory blocks (asm("":::"memory")) are in the right places.  It's important to ensure that any changes to memory are written out before interrupts are enabled - thus the block should appear before the sei() instructions, and before restoring SREG.  The block is unnecessary before disabling interrupts.  I could be wrong, but I think that's the correct ordering.

In C++ it is normally considered better to use a class and RAII for something like this.  You should have a class such as:

class CriticalSection { public: CriticalSection() : _sreg( SREG ) { cli(); } ~CriticalSection() { asm("" ::: "memory"); SREG = _sreg; } private: uint8_t _sreg; };

Then you simply declare a "CriticalSection" variable in the block you want to protect.  Similar classes can be defined for non-critical sections, and for when you want to restore the old interrupt enable state, or to force the flag on or off.

As a workaround for the bug blocking cleanup functions, it's possible to use a RAII class within the same framework as the C-style atomic blocks.  It's a little messy, but gives identical optimal assembly code (in my brief testing, anyway).  I haven't gone through the details of patching this into atomic.h (I'm not sure what the policy of including c++ specific stuff is), but it can be added on at the end of atomic.h as it re-defines some macros:

#ifdef __cplusplus

// Override these macros: #undef ATOMIC_RESTORESTATE #undef ATOMIC_FORCEON #undef NONATOMIC_RESTORESTATE #undef NONATOMIC_FORCEOFF

class RestoreState { public: RestoreState() : _x( SREG ), _restore( true ) {} // For keeping sreg RestoreState(uint8_t x) : _x( x ), _restore( false ) {} // For __ToDo loop hack ~RestoreState() { if (_restore) SREG = _x; } operator bool() const { return _x; }; // For testing __ToDo private: uint8_t _x; bool _restore; };

class ForceOn { public: ForceOn() : _x( 0 ), _restore( true ) {} // For ignoring sreg ForceOn(uint8_t x) : _x( x ), _restore( false ) {} // For __ToDo loop hack ~ForceOn() { if (_restore) sei(); } operator bool() const { return _x; }; // For testing __ToDo private: uint8_t _x; bool _restore; };

class ForceOff { public: ForceOff() : _x( 0 ), _restore( true ) {} // For ignoring sreg ForceOff(uint8_t x) : _x( x ), _restore( false ) {} // For __ToDo loop hack ~ForceOff() { if (_restore) cli(); } operator bool() const { return _x; }; // For testing __ToDo private: uint8_t _x; bool _restore; };

#define ATOMIC_RESTORESTATE RestoreState sreg_save #define ATOMIC_FORCEON ForceOn sreg_save #define NONATOMIC_RESTORESTATE RestoreState sreg_save #define NONATOMIC_FORCEOFF ForceOff sreg_save

#endif

[avrs-admin]

Jaakko Kantojärvi Mon 03 Feb 2014 02:25:44 AM CET

Can't reproduce this with:

• avr-g++ (GCC) 4.8.1
• avr-libc (debian) 1:1.8.0-4

This bug might have been fixed in avr-gcc as avr-libc haven't changed in time passed.

Just thought it might be good to clean obsolete bugs, thus tested and commented about my resulsts.

[sprintersb]

This is no more reproducible with new versions (v5.4 and up) of the compiler.