Re: [avr-gcc-list] volatile...

[Paulo Marques]

Dave Hansen wrote:
> From: Paulo Marques <address@hidden>
> > Graham Davies wrote:
> > > "David Brown" wrote:
> > > > You are missing a number of points ...
> > >
> > > Well, I think we're getting close to complete coverage now!
> >
> > Well, since we are going for complete coverage, I'll add my 2 cents, 
> > then.
>
> You've opened some new cans of worms here, but I'll only make one small 
> comment

I was afraid of that (the cans of worms, not your comment) ;)

> [...]
>
> > static uint16_t atomic_read_16(uint16_t *ptr)
> > {
> >     uint16_t ret;
> >
> >     cli();
> >     ret = (volatile)(*ptr);
>
> Your cast to volatile here is not only unnecessary, it's wrong.  You are 
> actually casting the uint16_t intermediate value to signed int, which is 
> then converted (back) to uint16_t.  If you tried to model an 
> atomic_read_32 on this function, you might not like the results.
>
> On newer conmpilers (with C99 compliance) it shouldn't even compile 
> (implied int no longer allowed).

I warned that I didn't try to compile the code, so these mistakes 
(forgetting the uint16_t in the cast to volatile) were likely to happen :)

But you're actually wrong about the volatile not being needed. Because 
the "sei" instruction doesn't claim anything about memory clobbers, 
without volatile the compiler would be free to re-order instructions and 
do the "sei" before the assignment.

This is no theoretical scenario. Just search the archives for previous 
threads over this.

> >     sei();
>
> OK, a second comment:  I like to save SREG before the CLI, then restore 
> SREG rather than blindly enabling interupts with SEI.  That way, if I 
> happen to call the function with interrupts disabled, they aren't 
> unintentionally enabled early.

I mentioned that too, but in my example the atomic_read should only be 
used _outside_ interrupts (inside interrupts you could just access the 
variable the normal way), so no harm should come from using cli/sei 
instead of store flags / restore flags.

But I agree that a generic "atomic.h" implementation should save flags / 
restore flags instead of cli/sei to be really safe to use anywhere.

--
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."