bug in avr-libc

Go To Last Post
44 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The code:

#include 
....
....
 cli();
 v = TicksCounter;
 sei();
....

sometime generate this (with -O2 flag):

....
     a30:	f8 94       	cli
     a32:	78 94       	sei
     a34:	c0 91 82 00 	lds	r28, 0x0082
     a38:	d0 91 83 00 	lds	r29, 0x0083
....

I use my own macros for "cli()" and "sei()" for delete bug:

#include 
#undef	cli
#undef	sei
#define cli() asm volatile( "cli" ::: "memory" )
#define sei() asm volatile( "sei" ::: "memory" )

[/code]

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You should use the atomic library (atomic.h).

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

danni wrote:
You should use the atomic library (atomic.h).

Problem is not in atomic execution. Problem is in reodering cli/sei macros with memory use by avr-gcc.

Ilya

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> Problem is not in atomic execution.

It is. Atomic execution of your assignment is *exactly* what
you want, so

  ATOMIC_BLOCK(ATOMIC_FORCEON) { v = TicksCounter; }

is the method of choice avr-libc is offering you to solve your
problem.

A general memory clobber can be such a strong pessimization that
the idea to forcibly combine cli()/sei() with a memory clobber has
been dismissed (after discussion) in the past.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I saw no mention of this potential behavior in the avr libc documentation describing cli() and sei().

This behavior seems to beg the question as to why bother to provide cli()/sei() functions/macros when it appears
that it doesn't work at all or at least not the way most people would assume it works.

Given that these functions are not yet deprecated, it would seem that even if the code is less optimal by doing the clobbers
that it is isn't as bad as silently generating code that is definitely not desired.

Is there no way to have some middle ground?
Something that forces the sei()/cli() in place without killing all the optimizations?

--- bill

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dl8dtl wrote:
> Problem is not in atomic execution.

It is. Atomic execution of your assignment is *exactly* what
you want, so

  ATOMIC_BLOCK(ATOMIC_FORCEON) { v = TicksCounter; }

Ok. In this case you are right. But when I write:

....
cli();
v = TicksCounter;
r = Mutex_TryGet( A_MUTEX );
if ( OK_RET == r ) return v;
....
sei();
....

then there is problem with cli/sei macros reodering.

Ilya

ps. All right, It is not very good idea add "memory" to cli/sei, but I do not see another way (for all cases)...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

501-q wrote:

....
cli();
v = TicksCounter;
r = Mutex_TryGet( A_MUTEX );
if ( OK_RET == r ) return v;
....
sei();
....

Pleas can you tell any practical reason, why a function should be leaved with interrupts disabled forever? :?:

With using the ATOMIC_BLOCK statement the interrupts are enabled on continue the function and also on the return expression.

This sounds an intelligent solution.

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Peter,

apart from what you see as logical, the fact is, that sei()/cli() macros are around for quite a lot of time and they are in wide use, so it would be reasonable to expect that they do produce a sequence point or whatever is needed to prevent reordering. Hence, IMHO, this does deserve a bug report and 501-q should consider filing one into the avr-libc bug tracker (potentially together with the proposed patch, to prevent the principal developers to scream at you for adding them work ;-) )

That the manual might *discourage* their usage in favour of atomic.h is another thing (and could be added as a feature request to the same tracker).

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The wrong working of cli(),sei() was not the whole story. :!:

It occurs only, if none of the variables defined as volatile.
And then, if this sequence was done inside a loop, the variable "TicksCounter" was only read once prior the loop.
I think, that this was never the intention of the code author. :cry:

So if you write your program in a right way with using "volatile", then cli(),sei() are working right also.
But "volatile" may cause unwanted code increasing inside the interrupt handler.

So the "atomic.h" was the better approach, since it handle not only the atomic access, it solve the volatile problem also.
No need to define variables volatile, if they used inside the atomic statement.

Another advantage of the atomic statement was, that the interrupt status was always restored, independent from the outgoing path (return, break, continue, "}").

So to held your code safe, readable and maintainable, you should replace any occurrence of cli(),sei() with the atomic statement. :idea:

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The prime purpose of cli() and sei() is to disable, or enable
interrupts globally. You need at least a single sei() in your
entire application initially... I could even agree that we
should perhaps completely deprecate the use of cli(). ;-)

As I said, there's not much point in filing a bug report for it.
This /has/ already been discussed among the avr-libc developers
years ago, and the result was that the idea of adding a memory
clobber has been dismissed.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Jörg,

As a compromise I wonder if it's worth someone (and I'm not volunteering!) to add a note to sei()/cli() in the manual (that is Doxygen in the interrupt.h) to point out that this issue could occur and how to implement a memory clobber to workaround it? Perhaps also a cross-reference to the relatively new atomic.h?

Hopefully someone concerned enough about this (from above?) will post a documentation patch to interrupt.h on Savannah?

Cliff

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I gave up my hope on anyone submitting documentation. ;-)

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Not surprising with the hurdles open source projects typical place in the way of those who want to contribute.

Because of these hurdles I have given up contributing to any open source project a long time ago, not being into butt-kissing. But we already had this discussion in the past.

Stealing Proteus doesn't make you an engineer.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK folks, I'll make this offer as I have a user ID on Savannah. If someone can simply post a message here with the exact text they'd like added to the interrupt.h documentation of sei()/cli() I'll do the rest to submit the patch, it's just I don't know exactly what it is you'd like it to say?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I disagree.

First, it may be a good idea to deprecate sei()/cli(), but it would certainly a bad idea to remove them altogether. Not only is there a significant codebase out there using cli()/sei(), but also those who migrate from other platforms might be accustomised to seek for these macros or equivalent.

As many pointed out, cli() and sei() now does *not* work as expected, namely it may not disable/enable interrupts exactly at the place corresponding to the position of cli()/sei() between other statements in the source code.

This defeats completely the purpose of cli()/sei().

Any pointing to inefficient translation result is thus irrelevant. Better inefficient than incorrect (as in "works other than expected") implementation.

If there is [presently] no better way to prevent reordering of code before/after cli()/sei() than memory clobbers, than that's the way to go, fullstop.

My suggestion is to:
a) add the memory clobber to sei()/cli()
b) add two new macros with the old implementation (say, pure_sei()/pure_cli() but I don't say this is a good choice)
c) update documentation of sei()/cli() to say:
"To ensure sei()/cli() to always work as expected, i.e. the surrounding statements are not reordered during optimisation to a different position relative to sei()/cli() than they are positioned in the source, these macros contain explicit memory clobbers. This results in saving/loading all variables from/to registers to/from memory, even if it may be not necessary; thus leading to always correct, but unoptimal code.

There are two other macros, pure_sei()/pure_cli() provided without this memory clobber, for those who desire more tightly optimised code and accept the risk of the surrounding instructions to be reordered.

Note, that usage of cli()/sei() macros to ensure atomicity of a sequence of operations is deprecated. For this purpose, refer to the ATOMIC_BLOCK() macro defined in ."

Please correct my bad English.
---

I do have an account on savannah, but I don't bother to file a bug report to be screamed at for not submitting a patch together with the bug report, and I don't intend to study THE proper way how to submit a patch for something the principial developers can fix in 10 minutes (wonder if it is a coincidence that it sounds quite similar to ArnoldB's experience, or just that both of us are that stupid naive).

Jan Waclawek

PS. Just for reference:
http://lists.gnu.org/archive/htm...
http://lists.gnu.org/archive/htm...
http://lists.gnu.org/archive/htm... (this is only partially related, cli()/sei() mentioned only anecdotally with little discussion and strong opinions about halfway through the thread)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> Not surprising with the hurdles open source projects typical
> place in the way of those who want to contribute.

Which "hurdles" are you referring to?

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

wek wrote:
I disagree.
PS. Just for reference:
http://lists.gnu.org/archive/htm...
http://lists.gnu.org/archive/htm...

And if you read that to the bitter end, you'll notice
Björn Haase eventually withdrew the bug report because
he analyzed the failure was elsewhere.

Also, note that has not been around there
by that time.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dl8dtl wrote:
wek wrote:
I disagree.
PS. Just for reference:
http://lists.gnu.org/archive/htm...
http://lists.gnu.org/archive/htm...

dl8dtl wrote:
And if you read that to the bitter end, you'll notice
Björn Haase eventually withdrew the bug report because
he analyzed the failure was elsewhere.

And how does that justify perpetuation of an incorrect solution?

dl8dtl wrote:
Also, note that has not been around thereby that time.

And how does that justify perpetuation of an incorrect solution?

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Would declaring them as actual inline functions instead of inline assembler alter the behavior?

--- bill

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

p.s.: What would be really a useful feature to the compiler is the
ability to declare a certain block of statements "volatile", in order
to instruct the compiler to not reorder statements across that border.
This came up here:

https://www.mikrocontroller.net/...

A memory clobber wouldn't have helped that either, because there was
no memory involved. (Note that the current GCC compiles the example
code given there the way Peter would expect, but that's by no means
guaranteed.)

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> And how does that justify perpetuation of an incorrect solution?

OK, we can stop here. You are convinced that only your view of the
world is the correct one, where everybody else, after a lengthy
discussion 5 years ago, eventually agreed that this is *not* the
solution to the problem.

If you insist on reopening that bug, I'll go ahead, and declare
cli() as deprecated because there's no real use for it in the light
of the atomic implementation we've got now.

If you think you can bear with the pessimization side effects,
then please, just add the memory clobber explicitly, but not *inside*
the cli/sei macros but as a separate macro. I wouldn't mind adding
a macro for that to avr-libc, except I don't know of a header file
to put it into.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dl8dtl wrote:
> And how does that justify perpetuation of an incorrect solution?

OK, we can stop here. You are convinced that only your view of the
world is the correct one,

Sure. So do you.

dl8dtl wrote:
where everybody else, after a lengthy
discussion 5 years ago, eventually agreed that this is *not* the
solution to the problem.

Yes, Joerg. The real solution would be to implement a mechanism preventing code reordering. This did not happen in 5 years, and, let's face it, it is highly unlikely it will happen in the following 5 years.

So, what we do have today (and had 5 years ago) is an incorrect implementation of cli()/sei() and an unoptimal method to fix it. What we may have today is also some wisdom learned in 5 years, namely that users after 5 years still complain in incorrect working of cli()/sei().

So, I keep repeating my question, how does everything you wrote justify to perpetuate an incorrect solution?

dl8dtl wrote:
If you insist on reopening that bug, I'll go ahead, and declare
cli() as deprecated because there's no real use for it in the light
of the atomic implementation we've got now.

A little bit childish, are we?

Did you read my proposal for documentation change at all?

And, if I use sei() - which you apparently find politically correct enough - to globally enable interrupts, what prevents the optimiser from moving an operation from *before* an unmodified sei() to after it?

And, based upon what consideration would you say there is no real use of cli()? Is this that you personally don't have use for it? Or can you point me to a discussion dealing with this, with some facts rather than just handwavings?

Actually, how exactly does the ATOMIC_BLOCK achieve no reordering?

dl8dtl wrote:
If you think you can bear with the pessimization side effects,
then please, just add the memory clobber explicitly, but not *inside*
the cli/sei macros but as a separate macro. I wouldn't mind adding
a macro for that to avr-libc, except I don't know of a header file
to put it into.

Joerg, I respect and admire all the work you did for avr-libc, I honestly do, and know you to be a wiser man than this. It's really a pity that you appear to get infected in the course of years by the sort of open-source-guru arrogance ArnoldB was talking about above.

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dl8dtl wrote:
p.s.: What would be really a useful feature to the compiler is the
ability to declare a certain block of statements "volatile", in order
to instruct the compiler to not reorder statements across that border.
This came up here:

https://www.mikrocontroller.net/...

A memory clobber wouldn't have helped that either, because there was
no memory involved.


The initial post appears to write to memory.

The rest of thread is quite lengthy and my German is really rusty. Can you please point out the most intereting parts of this thread in this regard?

Thanks

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dl8dtl wrote:
p.s.: What would be really a useful feature to the compiler is the
ability to declare a certain block of statements "volatile", in order
to instruct the compiler to not reorder statements across that border.
This came up here:

Not exactly quite the same but
doesn't that essentially happen with function calls?

I always assumed that the compiler was not allowed to reorder function calls.

So wouldn't the cli() and sei() stay in the proper sequence if they were real functions?

And what happens if they get turned into inline functions?

Will the ordering still be preserved?

--- bill

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

bperrybap wrote:
Not exactly quite the same but
doesn't that essentially happen with function calls?

I always assumed that the compiler was not allowed to reorder function calls.


As I understand the language, the compiler is free to reorder function calls if it can guarantee that the "observable behaviour" stays the same. Depending on how smart the compiler is it would probably be able to realize that from its standpoint, the cli() and sei() functions have no side-effects and would feel free to move them around as it saw fit.

So yeah, the "real" solution is probably to lobby for a reorder_barrier extension to GCC but good luck with that!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> The initial post appears to write to memory.

Nope, it's been registers actually (by the time of GCC 4.1.1).

> As I understand the language, the compiler is free to reorder function calls
> if it can guarantee that the "observable behaviour" stays the same.

Yes, exactly, so in particular for the case of "static" functions, the compiler
can usually determine whether they do have global side effects or not.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Above, I asked, how the ATOMIC_BLOCK prevents code reordering.

I investigated it a little bit, and in my very limited understanding, it does NOT, at least not in an expected way.

I started from the problem Joerg pointed at:

dl8dtl wrote:

https://www.mikrocontroller.net/...

A memory clobber wouldn't have helped that either, because there was
no memory involved.

wek wrote:
> The initial post appears to write to memory.
dl8dtl wrote:

Nope, it's been registers actually (by the time of GCC 4.1.1).

I tried to compile the following file with avr-gcc (GCC) 4.2.2 (WinAVR 20071221):

#include 
#include 

unsigned int ivar;


void test( unsigned int val )
{
  val = 65535U / val;

  cli();

  ivar = val;

  sei();
}


#undef   cli
#undef   sei
#define cli() asm volatile( "cli" ::: "memory" )
#define sei() asm volatile( "sei" ::: "memory" ) 


void test2( unsigned int val )
{
  val = 65535U / val;

  cli();

  ivar = val;

  sei();
}


void test3( unsigned int val )
{
  val = 65535U / val;

  ATOMIC_BLOCK(ATOMIC_FORCEON) {
    ivar = val;
  }
}

void test4( unsigned int val )
{
  val = 65535U / val;

  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    ivar = val;
  }
}

This compiles to:

00000000 :
   0:	bc 01       	movw	r22, r24
   2:	f8 94       	cli
   4:	8f ef       	ldi	r24, 0xFF	; 255
   6:	9f ef       	ldi	r25, 0xFF	; 255
   8:	0e 94 00 00 	call	0	; 0x0 
   c:	70 93 00 00 	sts	0x0000, r23
  10:	60 93 00 00 	sts	0x0000, r22
  14:	78 94       	sei
  16:	08 95       	ret

00000018 :
  18:	bc 01       	movw	r22, r24
  1a:	f8 94       	cli
  1c:	8f ef       	ldi	r24, 0xFF	; 255
  1e:	9f ef       	ldi	r25, 0xFF	; 255
  20:	0e 94 00 00 	call	0	; 0x0 
  24:	70 93 00 00 	sts	0x0000, r23
  28:	60 93 00 00 	sts	0x0000, r22
  2c:	78 94       	sei
  2e:	08 95       	ret

00000030 :
  30:	bc 01       	movw	r22, r24
  32:	2f b7       	in	r18, 0x3f	; 63
  34:	f8 94       	cli
  36:	8f ef       	ldi	r24, 0xFF	; 255
  38:	9f ef       	ldi	r25, 0xFF	; 255
  3a:	0e 94 00 00 	call	0	; 0x0 
  3e:	70 93 00 00 	sts	0x0000, r23
  42:	60 93 00 00 	sts	0x0000, r22
  46:	2f bf       	out	0x3f, r18	; 63
  48:	08 95       	ret

0000004a :
  4a:	bc 01       	movw	r22, r24
  4c:	f8 94       	cli
  4e:	8f ef       	ldi	r24, 0xFF	; 255
  50:	9f ef       	ldi	r25, 0xFF	; 255
  52:	0e 94 00 00 	call	0	; 0x0 
  56:	70 93 00 00 	sts	0x0000, r23
  5a:	60 93 00 00 	sts	0x0000, r22
  5e:	78 94       	sei
  60:	08 95       	ret

(It's funny to see how gcc swapped the functions for no obvious reason - but this of course has no influence on functionality).

As a sidenote, if ATOMIC_RESTORESTATE is used as "parameter" of ATOMIC_BLOCK, it DOES add the "memory" clobber to sei(). And, in my opinion, if you would use ATOMIC_FORCEON (which does not contain the clobber), it would result in dangerous reordering too.

Thus, the conclusion is, that ATOMIC_BLOCK is none better than a modified sei()/cli() with the memory clobber added (keeping sei()/cli() unmodified is of course still dangerous thus nonsense).

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are right, atomic.h helps not in this case. :cry:

The only solution, I found:

unsigned int ivar;

#define vu16(x) (*(volatile uint16_t*)&(x))

void test3( unsigned int val )
{
  val = 65535U / val;

  ATOMIC_BLOCK(ATOMIC_FORCEON) {
    vu16(ivar) = val;
  }
}

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

danni wrote:
The only solution, I found:

Indeed. This leads to:

  62:	bc 01       	movw	r22, r24
  64:	8f ef       	ldi	r24, 0xFF	; 255
  66:	9f ef       	ldi	r25, 0xFF	; 255
  68:	0e 94 00 00 	call	0	; 0x0 
  6c:	f8 94       	cli
  6e:	70 93 00 00 	sts	0x0000, r23
  72:	60 93 00 00 	sts	0x0000, r22
  76:	78 94       	sei
  78:	08 95       	ret

However, this is based on try-and-see work, rather than understanding of the causes and mechanisms of reordering, isn't it.

All in all, the existence of ATOMIC_BLOCK(), perfect or not (and as we see the latter is the case), still does not justifies perpetuation of broken cli()/sei() - even if there is again only an unperfect, but better (in terms of correctness), remedy to it.

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You know sometimes it is simply easier to modify your own local copies
of stuff rather than try to push the big boulder in an attempt to get something
fixed in the mainline code.
That is the beauty of having open source.
You can fix issues yourself.
This is what I've done for a gcc float promotion bug that affects the avr version of gcc.
I've also done this for the routines.

And now I'll update my own versions of cli() and sei().

It does make it tough to upgrade tool sets.

--- bill

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

danni wrote:
The wrong working of cli(),sei() was not the whole story. :!:

It occurs only, if none of the variables defined as volatile.
And then, if this sequence was done inside a loop, the variable "TicksCounter" was only read once prior the loop.
I think, that this was never the intention of the code author. :cry:

So if you write your program in a right way with using "volatile", then cli(),sei() are working right also.
But "volatile" may cause unwanted code increasing inside the interrupt handler.

So the "atomic.h" was the better approach, since it handle not only the atomic access, it solve the volatile problem also.
No need to define variables volatile, if they used inside the atomic statement.

Another advantage of the atomic statement was, that the interrupt status was always restored, independent from the outgoing path (return, break, continue, "}").

So to held your code safe, readable and maintainable, you should replace any occurrence of cli(),sei() with the atomic statement. :idea:

Joerg and I are sympathetic to those people who wish to submit fixes to problems. We're certainly not arrogant about these projects, but we do get busy (we have personal lives too). This is nothing that a little pestering won't solve.

The post above I think goes to the heart of the issue. If the test case is not using volatile variables, then what point to disable and enable interrupts around it? I also go back to what Joerg said and that interrupt should only have to be explicitly enabled once in the application (using sei()), and all other times the state of the interrupts is restored.

I'm sure we have issues with our documentation. It could always use help, and yes, patches are truly welcome. I have yet to see a test case that shows a true problem though. It always seems to come down to an application issue. No, sei() and cli() are not known to be truly immune to reordering, but when used with volatile variables, I have never seen any problems with the generated code. If volatile is used properly, i.e. for the right reasons, then there is no issue with the code size, as the generated code is correct and needed. If someone is complaining about the code size, then perhaps they don't really need volatile, and perhaps they don't really need to disable interrupts, unless they are perhaps just experimenting with the code.

We're open to a fruitful discussions about any real lack in the toolchain (compiler / library). But with a real world test case we will distinguish between a real lack and a perceived lack only based on theory.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

bperrybap wrote:

This is what I've done for a gcc float promotion bug that affects the avr version of gcc.

What was the issue, and what was the fix?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

wek wrote:
However, this is based on try-and-see work, rather than understanding of the causes and mechanisms of reordering, isn't it.

You are right, I don't understand, why it work. :?

The cast to volatile should cause, that the access was not optimized away.
Why it also prevent reordering, I have absolutely no clue.

I think, a clean and always working strictorder_on and strictorder_off pragma would be wery helpful for many realtime programming problems.

Peter

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

danni wrote:
I think, a clean and always working strictorder_on and strictorder_off pragma would be wery helpful for many realtime programming problems.

I have spent the evening with searching and reading on this problem; and this left me with a bad taste in mouth, let me not go into details why.

I noticed a new function attribute "optimize" which supposedly changes the level of optimization locally for a given function. Supposedly, lower levels of optimization don't use reordering. I tried to play with it further but it's apparently implemented only in the newer versions of gcc (4.4+? the documentation is not very clear in this) while I am a mere user left with what comes with

Can anybody who builds gcc for himself try whether this attribute is useful for the above sort of problem? Could an inline cli()/sei() function with such locally changed optimisation prevent reordering of surrounding operations?

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

whether this attribute is useful for the above sort of problem?

But first consider Eric's (prompted by danni's) post. If the OP had made the variable 'volatile' as it would have to be if shared between ISR and main where cli/sei are used then the problem goes away doesn't it? So all this sounds like a storm in a teacup.

(innocent bystander)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

EW wrote:
Joerg and I are sympathetic to those people who wish to submit fixes to problems. We're certainly not arrogant about these projects, but we do get busy (we have personal lives too). This is nothing that a little pestering won't solve.

Okay, so let me pester. (I could prehaps start with asking you to reopen the feature request I've made and you've closed without reading it to the end.)

EW wrote:
The post above I think goes to the heart of the issue. If the test case is not using volatile variables, then what point to disable and enable interrupts around it?

This is NOT the heart of the issue.

The OP did not want to disable and enable interrupts *around* a simple access to a variable. This is not the case of classical atomicity problem - the OP wanted to disable interrupts permanently under a certain condition.

You cannot simply waive such requirements; even if they may be not typical, they are fully legitimate in microcontrollers under various situations.

Thus, for the "common" problem of atomic access of a single or a few variables, let's redirect the users to the atomic.h macros - whether they solve the problem or not; and don't question the (potentially rare) need for standalone cli()/sei() anymore. Note, that this IS contained in my proposal above.

However, if cli()/sei() is KNOWN to be able to produce INCORRECT code, and if there is a fix which is KNOWN to improve on things even if not in a perfect way, that fix SHALL be imposed upon it.

At the end of the day, have a look at the ATOMIC_RESTORE "parameter" - it contains exactly the same "memory" clobber - and I guess the reason for it is exactly the same empiric observation!

EW wrote:
I have yet to see a test case that shows a true problem though.

No doubt the problem is complex enough to prevent a simple short test case and the reordering apparently occurs quite seldom - it is upon the OP whether he is willing to post a complete application exhibiting the problem or not.

As this problem comes up from time to time - and not only in the AVR port of GCC - I would not doubt its existence.

Eric wrote:
No, sei() and cli() are not known to be truly immune to reordering, but when used with volatile variables, I have never seen any problems with the generated code.

And, as we see no *evidence* not even a clean methodology how to use volatile to prevent reordering, I'd suggest not to see using volatile as a definitive solution and waive the problem away.

Thus, in addition to my recommendations for changes above, I'd also add the following to the atomic.h documentation:

"Even with ATOMIC_BLOCK, there is no guarantee the compiler won't reorder instructions in a different way than expected by the user. It is for example possible that a function call, which has otherwise no side effects, is moved by compiler from outside to inside ATOMIC_BLOCK, potentially increasing significantly the time while interrupts are disabled. Accessing volatile variables inside ATOMIC_BLOCK appears to improve things, but there no certainty in this either. As of [now], there is no mechanism to prevent the compiler of reordering, so if certainty in execution of critical sections of code is desired, it is necessary to resort to inline assembler."

(please correct spelling and wording)

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I looked at the atomic.h file and noticed the NONATOMIC_BLOCK macro, but I don't think I understand its' purpose. Isn't that the normal state of afairs?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well, for me, it *is* the normal state of affairs to not understand the purpose of things... :-)

NONATOMIC_BLOCK is provided for "symmetry" reasons, allow for a sequence of certainly interruptible instructions within a ("geographically") larger block which is potentially uninterruptible.

Now this may be not "normal", or, more precisely, not usual; yet it still might be useful. This is quite the same issue than the "normality" of standalone sei()/cli()...

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

There is another problem with ATOMIC_BLOCK macro (and all macros with open for loop):

#include 
#include 

extern volatile unsigned short TicksCounter;
extern unsigned short TheTime;
int func( void );
void func2( void );

void test( void )
{
	do {
		ATOMIC_BLOCK( ATOMIC_FORCEON ) {
			if ( TicksCounter == TheTime ) break;
		}
		func();
	} while (1);
	func2();
}

Misuse word "break" inside atomic block is the problem.

avr-gcc -mmcu=atmega64 -c --std=c99 -O2 test.c

00000000 :
   0:	f8 94       	cli
   2:	80 91 00 00 	lds	r24, 0x0000
   6:	90 91 00 00 	lds	r25, 0x0000
   a:	78 94       	sei
   c:	0e 94 00 00 	call	0	; 0x0 
  10:	00 c0       	rjmp	.+0      	; 0x12 <__zero_reg__+0x11>

There is no compare.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> (I could prehaps start with asking you to reopen the feature request
> I've made and you've closed without reading it to the end.)

I just read it again. Your request simply sounded as if *either* of
the suggested items would solve your problem, so after the patch
mentioned as the first item in your list got applied, it was just
consequent for Eric to close the request.

The problem with feature requests in general is that there are simply
much more things people would like to see than there are free
developer hours. Overall, your request was quite complex, so that's
why I expressed my doubts there will be anyone in the forseeable
future around to handle it.

Well, opensource development usually doesn't work like that, someone
requesting a particular feature, and then someone else yelling:
"Yeah!, great, will immediately do that, why did I never have an idea
like this?" Instead, most of the opensource development works by
someone having a particular problem at hand. If the problem is
annoying enough to that "someone", he eventually remembers that it's
opensource, after all, so it might be an idea to look at the source
code, and whether it would be feasible enough to start an
implementation himself. Weeks later, the first prototype is ready for
discussion... I could tell you countless stories of numerous smaller
and larger changes to opensource tools I submitted that way.

For example, WinAVR has been created like that. Eric has simply been
annoyed by the non-existance of a maintained AVR-GCC package for Win32
systems (when there was obviously ongoing development for Unix-like
environments), and when he's been further annoyed by a major
commercial compiler vendor trying to charge an arm and a leg for the
required upgrade that would have been needed in order to support some
new AVR devices, he decided to have a look himself how much work it
would be to compile all those tools under Windows.

Btw., when submitting patches, don't underestimate the effort required
by the developers to also adapt the documentation. If the patch also
submits documentation, its chances for a speedy integration into the
tree are *much* better. Same goes for adding testsuite entries, where
applicable. For example, for GCC, you won't get a single non-trivial
(and not documentation-only) patch committed without accompanying it
by testsuite entries. And right they are -- so far, I've been
contributing a total of 1 feature patches to GCC, and while the
feature has already been in wide use in AVR-GCC before, the
requirement to also submit testsuite entries uncovered a number of
bugs in it still.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dl8dtl wrote:
> (I could prehaps start with asking you to reopen the feature request
> I've made and you've closed without reading it to the end.)

I just read it again. Your request simply sounded as if *either* of
the suggested items would solve your problem, so after the patch
mentioned as the first item in your list got applied,


And, having read it, didn't my immediate complaint appear to you as an explanation that this is NOT the case?

I know my English is limited, but how exactly would you put then such a series of requests down? I proposed to submit several independent RFEs, but I suspect THEN I would be really screamed on!

dl8dtl wrote:
The problem with feature requests in general is that there are simply
much more things people would like to see than there are free
developer hours. Overall, your request was quite complex, so that's
why I expressed my doubts there will be anyone in the forseeable
future around to handle it.

Well, that was not exactly how your posts (in the subsequent discussion on the mailing list) did sound, but let it be.

I do see a significant difference between "request" and "demand". In submitting a RFE I intend to bring the existence of a certain problem into attention of those who would potentially solve it - including myself; I don't demand a solution. I of course did have >>some<< solution for my own purposes at the time of submitting the RFE, I would not wait until somebody would solve it. Eventually, I would submit parts of my solutions - maybe not exactly in the form of patches, for reasons of complexity of the problem and my inexperience with stuff like scripts for generating linker scripts. And I would do that in the hope of discussion, which would lead to an acceptable form of my solutions - which are certainly far from being perfect at the moment, fulfilling only my particular needs.

I don't do this sort of work for some personal entertainment nor do I want to perpetuate my personality in this way. I simply try to give back some of what I have received from the project. Apparently, those who feel to be in charge of the project (including you, Joerg), don't feel like the project needs this sort of help. It sounds like "do work for us, but *we* decide what work you do". Unfortunately, I am meeting with this spirit in many open source projects (fortunately not all).

Joerg wrote:
Btw., when submitting patches, don't underestimate the effort required
by the developers to also adapt the documentation. If the patch also
submits documentation, its chances for a speedy integration into the
tree are *much* better.

Do I sound that dumb stupid moron?

So, to cut it short, above, I have submitted a proposal for improvement of cli()/sei() (by adding memory clobbers to it), and also proposal for documentation enhancements; in the posts above there is also some supporting discussion.

Now, how did it help to include this to avr-libc, against your personal opposition?

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

501-q wrote:
There is another problem with ATOMIC_BLOCK macro (and all macros with open for loop)

Besides Peter Dannegger's example, this is just another example of how ATOMIC_BLOCK may fail to provide the promised effect for the unaware user.

Still, with a careful wording, I would encourage to use ATOMIC_BLOCK for its purpose, as it at least provides a way for future enhancements in a more "structured" way than cli()/sei() would.

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

In the light of previous discussion, I would like to correct my proposed changes in interrupt.h:

My suggestion is to:
a) add the memory clobber to sei()/cli()
b) add two new macros with the old implementation (say, pure_sei()/pure_cli() but I don't say this is a good choice)
c) update documentation of sei()/cli() to say:
"As of gcc version [curent version], there is no mechanism to explicitly prevent reordering of statements by the optimiser. Thus, sei()/cli() in rare cases don't work as expected, i.e. the surrounding statements may be reordered during optimisation to a different position relative to sei()/cli() than they are positioned in the source. It has been observed, that accessing volatile variables improves "fixing" of sei()/cli() position; however, there is no clear methodology how to exploit this safely.

In the current version, these macros contain explicit memory clobbers. Based on empirical evidence, this in some cases prevents reordering of sei()/cli(). As a side effect, this also results in saving/loading all variables from/to registers to/from memory, even if it may be not necessary; thus leading to correct, but potentially less optimal code.

There are two other macros, pure_sei()/pure_cli() provided without this memory clobber, for those who desire more tightly optimised code and accept the increased risk of the surrounding instructions to be reordered.

Note, that usage of cli()/sei() macros to ensure atomicity of a sequence of operations is deprecated. For this purpose, refer to macros defined in ."

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Please discuss this sei/cli thing on the avr-libc-dev mailing list.
It's been discussed there before, and the consensus last time (not
mine, I didn't participate much in the discussion) was to *not* add
the memory clobbers. If you feel there's anything to add to the old
discussion (which you already found) which makes it worthwhile to reopen
the bug report, then please do so (rather than filing a new one).
Explain why you think the former consensus is wrong, and let's see
whether other developers agree with you. (Personally, I don't care much
about it, I could live with both options quite easily.)

Of course, it would be even better if you could lobby the GCC developers
to implement some kind of instruction reordering barrier... But that will
be a lengthy process, and certainly needs a lot of personal energy if you
really want to succeed.

As of the old feature request, well, I can't remember the details offhand,
but I don't remember there was much interest in it by anyone else. As
such, it's always hard to judge what will really be useful to more than
a single person. Again, it would be better to start a discussion in the
mailing list. If nobody responds, this usually means that you are alone
with the request. In that case, you could then decide yourself whether
you'd like to go on, making it a full patch ready to be committed, or you'll
simply leave it as your local hack since apparently nobody else is
interested in it anyway.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.