| Author |
Message |
|
|
 Posted: Feb 19, 2010 - 07:49 AM |
|
Joined: Nov 13, 2009
Posts: 104
Location: Russia, Yekatherinburg
|
|
The code:
Code:
#include <avr/interrupt.h>
....
....
cli();
v = TicksCounter;
sei();
....
sometime generate this (with -O2 flag):
Code:
....
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:
Code:
#include <avr/interrupt.h>
#undef cli
#undef sei
#define cli() asm volatile( "cli" ::: "memory" )
#define sei() asm volatile( "sei" ::: "memory" )
[/code] |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 08:00 AM |
|
Joined: Sep 05, 2001
Posts: 2497
|
|
You should use the atomic library (atomic.h).
Peter |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 08:11 AM |
|
Joined: Nov 13, 2009
Posts: 104
Location: Russia, Yekatherinburg
|
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 09:45 AM |
|
Joined: Dec 20, 2002
Posts: 7276
Location: Dresden, Germany
|
|
> Problem is not in atomic execution.
It is. Atomic execution of your assignment is *exactly* what
you want, so
Code:
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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 10:17 AM |
|
Joined: May 03, 2009
Posts: 564
Location: Dallas, TX USA
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 11:02 AM |
|
Joined: Nov 13, 2009
Posts: 104
Location: Russia, Yekatherinburg
|
|
|
dl8dtl wrote:
> Problem is not in atomic execution.
It is. Atomic execution of your assignment is *exactly* what
you want, so
Code:
ATOMIC_BLOCK(ATOMIC_FORCEON) { v = TicksCounter; }
Ok. In this case you are right. But when I write:
Code:
....
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)... |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 11:11 AM |
|
Joined: Sep 05, 2001
Posts: 2497
|
|
|
501-q wrote:
Code:
....
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 12:25 PM |
|
Joined: Dec 16, 2005
Posts: 3086
Location: Bratislava, Slovakia
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 02:16 PM |
|
Joined: Sep 05, 2001
Posts: 2497
|
|
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. 
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. 
Peter |
|
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 02:33 PM |
|
Joined: Dec 20, 2002
Posts: 7276
Location: Dresden, Germany
|
|
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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 02:39 PM |
|
Joined: Jul 18, 2005
Posts: 62281
Location: (using avr-gcc in) Finchingfield, Essex, England
|
|
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 |
_________________
|
| |
|
|
|
|
|
|
Posted: Feb 19, 2010 - 11:48 PM |
|
Joined: Dec 20, 2002
Posts: 7276
Location: Dresden, Germany
|
|
| 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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
Posted: Feb 20, 2010 - 11:44 AM |
|
Joined: Nov 29, 2007
Posts: 3219
|
|
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. |
|
|
| |
|
|
|
|
|
|
Posted: Feb 20, 2010 - 02:29 PM |
|
Joined: Jul 18, 2005
Posts: 62281
Location: (using avr-gcc in) Finchingfield, Essex, England
|
|
| 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? |
_________________
|
| |
|
|
|
|
|
|
Posted: Feb 20, 2010 - 11:11 PM |
|
Joined: Dec 16, 2005
Posts: 3086
Location: Bratislava, Slovakia
|
|
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 <util/atomic.h>."
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/html/avr-l ... 00202.html
http://lists.gnu.org/archive/html/avr-l ... 00015.html
http://lists.gnu.org/archive/html/avr-g ... 00065.html (this is only partially related, cli()/sei() mentioned only anecdotally with little discussion and strong opinions about halfway through the thread) |
|
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 07:44 AM |
|
Joined: Dec 20, 2002
Posts: 7276
Location: Dresden, Germany
|
|
> 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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 07:49 AM |
|
Joined: Dec 20, 2002
Posts: 7276
Location: Dresden, Germany
|
|
|
wek 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.
Also, note that <util/atomic.h> 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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 08:00 AM |
|
Joined: Dec 16, 2005
Posts: 3086
Location: Bratislava, Slovakia
|
|
| [quote="dl8dtl"]
wek wrote:
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 <util/atomic.h> has not been around thereby that time.
And how does that justify perpetuation of an incorrect solution?
JW |
|
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 08:23 AM |
|
Joined: May 03, 2009
Posts: 564
Location: Dallas, TX USA
|
|
Would declaring them as actual inline functions instead of inline assembler alter the behavior?
--- bill |
|
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 08:38 AM |
|
Joined: Dec 20, 2002
Posts: 7276
Location: Dresden, Germany
|
|
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/topic/65923
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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
FAQ
Search
Register
Log in to check your private messages
Log in
Maximize
RSS
