[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[avr-libc-dev] [bug #22163] Everytime ATOMIC_BLOCK(ATOMIC_RESTORESTATE)

From: David Brown
Subject: [avr-libc-dev] [bug #22163] Everytime ATOMIC_BLOCK(ATOMIC_RESTORESTATE) compiler generates warning - unused variable 'sreg_save'
Date: Thu, 31 Jan 2008 10:17:00 +0000
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv: Gecko/20071127 Firefox/

Follow-up Comment #4, bug #22163 (project avr-libc):

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 {
    CriticalSection() : _sreg( SREG ) { cli(); }
    ~CriticalSection() { asm("" ::: "memory"); SREG = _sreg; }
    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:

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

class ForceOn {
        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
        uint8_t _x;
        bool _restore;

class ForceOff {
        ForceOff() : _x( 0 ), _restore( true ) {}                               
        // For ignoring sreg
        ForceOff(uint8_t x) : _x( x ), _restore( false ) {}             // For 
__ToDo loop
        ~ForceOff() { if (_restore) cli(); }
        operator bool() const { return _x; };                                   
        // For testing __ToDo
        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



Reply to this item at:


  Message sent via/by Savannah

reply via email to

[Prev in Thread] Current Thread [Next in Thread]