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

 
 

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

Submitted by:  None
Submitted on:  Thu 22 Sep 2005 02:59:58 PM UTC  
 
Category: HeaderSeverity: 3 - Normal
Priority: 5 - NormalItem Group: None
Status: InvalidPercent Complete: 0%
Assigned to: NoneOriginator Email: -unavailable-
Open/Closed: ClosedRelease: None
Fixed Release: None

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

Please enter the title of George Orwell's famous dystopian book (it's a date):

 

 

Follow 3 latest changes.

Date Changed By Updated Field Previous Value => Replaced By
Thu 06 Oct 2005 08:16:36 PM UTCaesokStatusNone=>Invalid
  Open/ClosedOpen=>Closed
Thu 22 Sep 2005 02:59:58 PM UTCNoneCarbon-Copy-=>Added bjoern --PUNKT-- haase --AT-- de --PUNKT-- bosch --PUNKT-- com

Back to the top


Powered by Savane 3.1-cleanup1