Search |
 |
|
 |
| Author |
Message |
|
|
 Posted: Feb 21, 2010 - 08:43 AM |
|
Joined: Dec 20, 2002
Posts: 7279
Location: Dresden, Germany
|
|
> 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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 09:14 AM |
|
Joined: Dec 16, 2005
Posts: 3095
Location: Bratislava, Slovakia
|
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 09:21 AM |
|
Joined: Dec 16, 2005
Posts: 3095
Location: Bratislava, Slovakia
|
|
|
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/topic/65923
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 10:31 AM |
|
Joined: May 03, 2009
Posts: 564
Location: Dallas, TX USA
|
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 01:38 PM |
|
Joined: Dec 18, 2009
Posts: 95
|
|
|
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! |
|
|
| |
|
|
|
|
|
|
Posted: Feb 21, 2010 - 03:23 PM |
|
Joined: Dec 20, 2002
Posts: 7279
Location: Dresden, Germany
|
|
> 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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
Posted: Feb 22, 2010 - 08:52 AM |
|
Joined: Dec 16, 2005
Posts: 3095
Location: Bratislava, Slovakia
|
|
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/topic/65923
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):
Code:
#include <avr/interrupt.h>
#include <util/atomic.h>
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:
Code:
00000000 <test>:
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 <test>
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 <test2>:
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 <test>
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 <test4>:
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 <test>
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 <test3>:
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 <test>
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 22, 2010 - 09:54 AM |
|
Joined: Sep 05, 2001
Posts: 2509
|
|
You are right, atomic.h helps not in this case. 
The only solution, I found:
Code:
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 22, 2010 - 10:31 AM |
|
Joined: Dec 16, 2005
Posts: 3095
Location: Bratislava, Slovakia
|
|
|
danni wrote:
The only solution, I found:
Indeed. This leads to:
Code:
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 <test>
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 22, 2010 - 08:22 PM |
|
Joined: May 03, 2009
Posts: 564
Location: Dallas, TX USA
|
|
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 <util/delay.h> routines.
And now I'll update my own versions of cli() and sei().
It does make it tough to upgrade tool sets.
--- bill |
|
|
| |
|
|
|
|
|
|
Posted: Feb 23, 2010 - 08:19 AM |
|
Joined: Mar 01, 2001
Posts: 4960
Location: Rocky Mountains
|
|
|
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.
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. 
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. |
_________________
Eric Weddington
Marketing Manager
Open Source & Community
Atmel
|
| |
|
|
|
|
|
|
Posted: Feb 23, 2010 - 08:21 AM |
|
Joined: Mar 01, 2001
Posts: 4960
Location: Rocky Mountains
|
|
|
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? |
_________________
Eric Weddington
Marketing Manager
Open Source & Community
Atmel
|
| |
|
|
|
|
|
|
Posted: Feb 23, 2010 - 08:29 AM |
|
Joined: Sep 05, 2001
Posts: 2509
|
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 23, 2010 - 09:26 AM |
|
Joined: Dec 16, 2005
Posts: 3095
Location: Bratislava, Slovakia
|
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 23, 2010 - 10:02 AM |
|
Joined: Jul 18, 2005
Posts: 62939
Location: (using avr-gcc in) Finchingfield, Essex, England
|
|
|
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) |
_________________
|
| |
|
|
|
|
|
|
Posted: Feb 23, 2010 - 10:28 AM |
|
Joined: Dec 16, 2005
Posts: 3095
Location: Bratislava, Slovakia
|
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Feb 26, 2010 - 08:15 PM |
|
Joined: Aug 04, 2004
Posts: 1822
Location: Davie, FL
|
|
| 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? |
|
|
| |
|
|
|
|
|
|
Posted: Feb 26, 2010 - 08:45 PM |
|
Joined: Dec 16, 2005
Posts: 3095
Location: Bratislava, Slovakia
|
|
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 |
|
|
| |
|
|
|
|
|
|
Posted: Mar 01, 2010 - 08:23 AM |
|
Joined: Nov 13, 2009
Posts: 105
Location: Russia, Yekatherinburg
|
|
There is another problem with ATOMIC_BLOCK macro (and all macros with open for loop):
Code:
#include <avr/io.h>
#include <util/atomic.h>
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
Code:
00000000 <test>:
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 <test>
10: 00 c0 rjmp .+0 ; 0x12 <__zero_reg__+0x11>
There is no compare. |
|
|
| |
|
|
|
|
|
|
Posted: Mar 01, 2010 - 08:48 AM |
|
Joined: Dec 20, 2002
Posts: 7279
Location: Dresden, Germany
|
|
> (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.
Please read the `General information...' article before.
|
| |
|
|
|
|
|
|
|
|
FAQ
Search
Register
Log in to check your private messages
Log in
Maximize
RSS
