bugAVR C Runtime Library - Bugs: bug #14616, definition of sei() in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #14616: definition of sei() in interrupts.h is faulty

Submitter:  None
Submitted:  Thu 22 Sep 2005 02:59:58 PM UTC
   
 
Category:  Header Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Invalid Assigned to:  None
Percent Complete:  0% Originator Email:  -email is unavailable-
Open/Closed:  Closed Release:  None
Fixed Release:  None

Jump to the original submission

Sat 01 Oct 2005 05:50:54 PM UTC, comment #6: 

Joerg Wunsch wrote on Montag, 26. September 2005 21:04 :

> GCC's documentation says:
>
>  The `volatile' keyword indicates that the instruction has important
> side-effects.  GCC will not delete a volatile `asm' if it is reachable.
> (The instruction can still be deleted if GCC can prove that
> control-flow will never reach the location of the instruction.)  In
> addition, GCC will not reschedule instructions across a volatile `asm'
> instruction.
>
> (Btw., I cannot find any documentation for the "memory" constraint.)

I've opened a bug report on

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24165

. Probably it's indeed a bug in gcc 4.0.0 that is already fixed in gcc 4.0.1 !

Here is the reduced test case:

/* Start of test case */

typedef unsigned char      uint8_t;
typedef unsigned short int uint16_t;

class class_with_volatile_data_structures
{
  public:

  void _attribute_ ((always_inline))
  wait_for_event (uint16_t event)
   {
     while (getval_protected () != event)
       ;
   };

  private:

  uint16_t
  getval_protected (void)
  {
    uint16_t result;

    asm volatile ("/* disable irq in cpu status */" : : );
    result = class_local_data;
    asm volatile ("/* enable irq */" : : );

    return result;
  }

  volatile uint16_t class_local_data;
};

class_with_volatile_data_structures object;

void
wait_for_42 (void)
{
  object.wait_for_event (42);
}

/* End of test case */


Compiler output reads for (buggy ?) avr-g++ (GCC) 4.0.0

_Z11wait_for_42v:
.L2:
/* #APP */
        /* disable irq in cpu status */
        /* enable irq */
/* #NOAPP */
        lds r24,object
        lds r25,(object)+1
        sbiw r24,42
        brne .L2
        ret

and for avr-g++ (GCC) 4.0.1 20050624 (prerelease)

_Z11wait_for_42v:
.L2:
/* #APP */
        /* disable irq in cpu status */
/* #NOAPP */
        lds r24,object
        lds r25,(object)+1
/* #APP */
        /* enable irq */
/* #NOAPP */
        sbiw r24,42
        brne .L2
        ret

Anonymous
Mon 26 Sep 2005 02:54:55 PM UTC, comment #5: 

The unfortunate thing is, that using volatile is not enough. Even if it works for your extremely simple test case: I am having a more complex test case example in my application code, where it does not work because gcc reorders the statements. And so as I understand the specification of gcc's inline asm statement, gcc has completely the right to reorder the statements as long as we do not communicate the possible side effects.

I am convinced that You as well will be unhappy with the present implementation if you discover one day that code segments that used to work perfectly do no longer work because you had inserted a completely unrelated line of code somewhere in the application:  A line of code that makes gcc consider it to be useful to reorder the code!

FYI: I had started with code like

{
  uint8_t temp = SREG;
  asm volatile ("cli" : :);
  volatile_declared_uint16_t_var_in_memory = value;
  SREG = temp;
}

Here gcc had reordered it such that it would read

{
  uint8_t temp = SREG;
  asm volatile ("cli" : :);
  SREG = temp;
  volatile_declared_uint16_t_var_in_memory = value;
}

I then tried to use brute force by doing
{
  asm volatile ("cli" : :);
  volatile_declared_uint16_t_var_in_memory = value;
  asm volatile ("sei" : :);
}
and also this ended up at reordered like
{
  asm volatile ("cli" : :);
  asm volatile ("sei" : :);
  volatile_declared_uint16_t_var_in_memory = value;
}
. Only when adding the "memory" clobbers to the spec. I obtained true protection. And IMO adding the clobbers is correct since indeed the sei instruction has possible unpredictable side effects on the memory, like in my case where the IRQ did write the variable!

All this is what draws me to the conclusion that the present macros are not safe. IMO one must not make the mistake to derive from the fact that simple test cases work that it works all the time :-(.

Bjoern.

Anonymous
Sat 24 Sep 2005 03:06:12 AM UTC, comment #4: 

Qualificator 'volatile' works fine in the above example.
Look, please, an expanded variant:

  int acc;
  int val;
  int volatile acc_v;
  int volatile val_v;

foo1(): no protection, loop is fully optimized:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  void foo1 (void)
  {
      unsigned char i;
      for (i = 10; i; i--) {
          asm volatile ("cli");
          acc += val;                   /* nonvolatile both     */
          asm volatile ("sei");
      }
  }

  .L6:
  /* #APP */
        cli
  /* #NOAPP */
        add r24,r18
        adc r25,r19
  /* #APP */
        sei
  /* #NOAPP */
        subi r20,lo8(-(-1))
        brne .L6

foo2(): acc_v is protected, only val fetch is optimized:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  void foo2 (void)
  {
      unsigned char i;
      for (i = 10; i; i--) {
          asm volatile ("cli");
          acc_v += val;                 /* acc_v is volatile    */
          asm volatile ("sei");
      }
  }

  .L15:
  /* #APP */
        cli
  /* #NOAPP */
        lds r24,acc_v
        lds r25,(acc_v)+1
        add r24,r18
        adc r25,r19
        sts (acc_v)+1,r25
        sts acc_v,r24
  /* #APP */
        sei
  /* #NOAPP */
        subi r20,lo8(-(-1))
        brne .L15

foo3(): val_v is protected, acc i/o is optimized:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  void foo3 (void)
  {
      unsigned char i;
      for (i = 10; i; i--) {
          asm volatile ("cli");
          acc += val_v;                 /* val_v is volatile */
          asm volatile ("sei");
      }
  }

  .L23:
  /* #APP */
        cli
  /* #NOAPP */
        lds r24,val_v
        lds r25,(val_v)+1
        add r18,r24
        adc r19,r25
  /* #APP */
        sei
  /* #NOAPP */
        subi r20,lo8(-(-1))
        brne .L23

foo4(): both are protected, all i/o is not optimized:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  void foo4 (void)
  {
      unsigned char i;
      for (i = 10; i; i--) {
          asm volatile ("cli");
          acc_v += val_v;               /* both are volatile    */
          asm volatile ("sei");
      }
  }

  .L32:
  /* #APP */
        cli
  /* #NOAPP */
        lds r24,acc_v
        lds r25,(acc_v)+1
        lds r18,val_v
        lds r19,(val_v)+1
        add r24,r18
        adc r25,r19
        sts (acc_v)+1,r25
        sts acc_v,r24
  /* #APP */
        sei
  /* #NOAPP */
        subi r20,lo8(-(-1))
        brne .L32

Is it possible to give an example, where 'volatile' variable is
not work (with simple sei()) ?

2.
Yes, sei() looks as function call. But there are many other
"almost functions", that ignore possible side effects.
For example:

  #include <stdlib.h>
  int acc, val;

  void foo (void)
  {
      unsigned char i;
      for (i = 10; i; i--) {
          val = abs(val);
          acc += val;
      }
  }

Without 'volatile': no protection, full optimization:
  .L7:
        mov r20,r24
        mov r21,r25
        sbrs r25,7
        rjmp .L6
        clr r20
        clr r21
        sub r20,r24
        sbc r21,r25
  .L6:
        mov r25,r21
        mov r24,r20
        add r18,r20
        adc r19,r21
        subi r22,lo8(-(-1))
        brne .L7

Dmitry.

Anonymous
Fri 23 Sep 2005 06:44:50 AM UTC, comment #3: 

Dimitry wrote:

>May be, if necessary it is better to add 'volatile' in
>definition of a concrete variable, than to endow speed always?


The problem is that declaring the variable to be volatile does not prevent the problem. The issue is that your code example

asm volatile ("cli");
acc += val;
asm volatile ("sei");

does not result in a protection of acc += val; ! The compiler
will possibly reorder the sei such that it ends up before the
acc += val; line. I have a test case where the reordering does not depend on whether acc is declared volatile or not! IMO, the correct specification for gcc's asm statement in order to enforce that reordering is avoided is the "clobber memory" information.

Since cli(); and sei() look like function calls, the user
(IMO) must have the right to assume that these statements are issued in chronological order and are not reordered.
I agree that the performance penilty is there for a couple of cases but I think that correctness comes first and speed second. Also because I think that sei() is issued fairly seldomly.

Yours,

Bjoern.

Anonymous
Fri 23 Sep 2005 03:07:19 AM UTC, comment #2: 

This addition will worsen quality of a code. Look an example:

int acc;
int volatile val;

void foo1 (void)
{
    unsigned char i;
    for (i = 10; i; i--) {
        asm volatile ("cli");
        acc += val;
        asm volatile ("sei");
    }
}

void foo2 (void)
{
    unsigned char i;
    for (i = 10; i; i--) {
        asm volatile ("cli");
        acc += val;
        asm volatile ("sei" ::: "memory");
    }
}

In function 'foo1' the compiler has born reading (and record) a variable
'acc' for frameworks of a cycle:
        ...
        lds r18,acc
        lds r19,(acc)+1
.L6:
/* #APP */
        cli
/* #NOAPP */
        lds r24,val
        lds r25,(val)+1
        add r18,r24
        adc r19,r25
/* #APP */
        sei
/* #NOAPP */
        subi r20,lo8(-(-1))
        brne .L6
        sts (acc)+1,r19
        sts acc,r18

In function 'foo2' reading/writing of a variable 'acc' is carried out on
everyone itteration:
        ...
.L15:
/* #APP */
        cli
/* #NOAPP */
        lds r24,acc
        lds r25,(acc)+1
        lds r18,val
        lds r19,(val)+1
        add r24,r18
        adc r25,r19
        sts (acc)+1,r25
        sts acc,r24
/* #APP */
        sei
/* #NOAPP */
        subi r20,lo8(-(-1))
        brne .L15
        ...

May be, if necessary it is better to add 'volatile' in
definition of a concrete variable, than to endow speed always?

Dmitry.

Anonymous
Thu 22 Sep 2005 03:02:20 PM UTC, comment #1: 

BTW the variable in memory was declared volatile.

Anonymous
Thu 22 Sep 2005 02:59:58 PM UTC, original submission:  

Hi,

we need to change the definition of the sei macro in interrupts.h such that it reads

#define sei()  _asm_ __volatile__ ("sei" : : : "memory")

instead of

#define sei()  _asm_ __volatile__ ("sei" ::)

I just had a situation where protecting an assignment by;

cli ();
uint16_t_variable_in_memory = value;
sei ();

did not work since avr-gcc did interchange the assignment and the sei() because it did not know anything on the possible side-effects of sei()!

For the same reason I think that we should add a macro

#define write_sreg(value)  _asm_ __volatile__ ("out _SREG_,%0" : : "r" (value) : "memory")

that signalizes gcc that writing the SREG could have possible side effects on the memory!

Yours,

Bjoern

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

Follow 3 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2005-10-06 aesok StatusNone Invalid
    Open/ClosedOpen Closed
2005-09-22 None Carbon-Copy- Added bjoern --PUNKT-- haase --AT-- de --PUNKT-- bosch --PUNKT-- com

Back to the top

Powered by Savane 3.13-d3ae.
Corresponding source code