(I've put this back on the mailing list - I assume you meant to do that, and just pressed the wrong "reply" button.)
On 23/08/2012 01:06, William Swanson wrote: > On Wed, Aug 22, 2012 at 2:28 PM, David Brown <david.br...@hesbynett.no> wrote: >> On 22/08/12 22:15, William Swanson wrote: >>> Besides being smaller and faster, this actually does the right thing. >> >> No, it does not "do the right thing". It might do what you want - but it >> does not implement the C code you wrote. Single "state" is volatile, the >> code generated by the compiler is optimal. > > According to the ANSI standard, all writes to a "volatile" variable > must occur before the next sequence point, and reads should assume > that the value can change at any time (but a "read" is implementation > defined). Since the "|=" operator doesn't introduce a sequence point, > the code I suggested would be perfectly correct according to the > standard. The implied read from the "|=" operator could be placed next > to the final write, and the peephole optimizer could then merge the > two with no change in semantics. > > The compiler's current output is also correct, but it's certainly not optimal. > I can see your point, but I am not convinced it is correct. You are talking about a very grey area of the C specs here - "volatile" is simply not well enough specified in the C standards. There was a well-publicised paper recently which looked at code generated by a variety of compilers for different volatile constructs, which concluded that most (or all, I think) compilers got things wrong in some circumstances. The obvious conclusion to take from that is that you must keep volatile code simple. If not, then either /you/ will get it wrong, or the /compiler/ will get it wrong. And even if both are right, the hardware can still be a limitation. This very example shows the problems caused by complex volatile code - you agree the compiler's output is correct based on the source code, yet it does not do what you want it to do or think it should do. So by the coding rules I use, the code you wrote is wrong - regardless of what the compiler generates, or even whether or not it works. >> First off, you can check that the C code you wrote is what you meant to >> write - the shift operation here looks very out of place. >> >> [...] Your "desired" code is not atomic, and will cause trouble. > > That code is designed to work correctly even if "state" changes > mid-expression. Only the final "|=" operation needs to be atomic. Yes, > I realize that this code is being tricky. Then separate the two operations into two lines of code: uint8_t newBits = (state >> 1) & DIRTY_FLAG; state |= newBits; That makes the code much clearer - and it explicitly shows that the two operations can be split up. Otherwise you've got a maintenance problem for the future when a new programmer tries to figure out what works or does not work. And maybe you'll be lucky and the "|=" will be implemented atomically with that code. Simple statements are critical to getting volatile right, and it gives the compiler the best chance. There are no guarantees, however. > >> The obvious first step towards correct code is to put the "DIRTY_FLAG" in >> its own byte - or at least in a byte containing simple flags and not >> requiring shifts. > > Since relying on an atomic "|=" isn't an option, and GCC's atomic > "__sync_fetch_and_or" intrinsic doesn't exist on this platform, it > looks like I will have to re-design the algorithms along the lines you > are suggesting. If that's not possible (and it probably isn't without > a CAS or other atomic primitive), I can always fall back on > __eint()/__dint() for the critical sections. > Another option is a single inline assembly call for the "|=" if the above code split does not help. ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ Mspgcc-users mailing list Mspgcc-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mspgcc-users
