Dangerous compiler optimization by bit-field write

Luca,

Please declare the Flags structure as volatile and see if the problem subsides. 

Thanks

Noah

This is an interesting problem.  I don't think simple assignment (dst.bit = src.bit) is going to work even if you declare the dst as volatile.  Volatile just says to the compiler that the variable could change outside of its control.  That doesn't stop the compiler from doing whatever intermediate steps it wants to take to perform its operation.  The underlying problem here is that there is no single operation even at the assembly level that does what you need since you cannot write to just a single bit in a word.  You always have to operate on the entire word, which means somewhere along the line you need to do a read-modify-write.  If the src.bit is a 0, you need to clear the dst.bit.  If the src.bit is a 1, you need to set the dst.bit.

I think you will need to do this (in pseudo-code):

if(src.bit == 1)
{
   dst.bit = 1;
}
else
{
   dst.bit = 0;
}

The above assignments should each generate a single ATOMIC (non-interruptible) boolean ASM instructions (i.e., OR, and AND).

Regards,

David

It dawned on my here that although the compiler SHOULD generate ATOMIC boolean ASM instructions for the pseudo code assignments I showed in the previous post, there is no GUARANTEE that it will.  The compiler knows nothing about interrupts, and this is not a compiler bug or weakness.  It is simply the compiler doing what it is supposed to do, and not accounting for the intent of the programmer to guard against interrupt behavior (the compiler never considers the programmers intent).

To guarantee that your assignment to dst.bit does not have potential interrupt issues, you either need to disable/enable interrupts around the C-code doing the assignment, or you can use the compiler ATOMIC instrinsic instructions for OR and AND with the pseudo code I showed.  See the compiler user's guide SPRU514H, Table 7-6, p.144.  Something like this:

if(src.bit == 1)
{
   __or(&dst, 0x0001);
}
else
{
   __and(&dst, 0xFFFE);
}

Regards,

David

Hello,

I tried with "volatile". Here the assembler code:
00881b:   CD26FFFE    AND          AH, @0x25, #0xfffe
00881d:   761F021B    MOVW         DP, #0x21b
00881f:   9228        MOV          AL, @0x28
008820:   9001        ANDB         AL, #0x2
008821:   761F021A    MOVW         DP, #0x21a
008823:   CAA8        OR           AL, @AH
008824:   9626        MOV          @0x25, AL

I expected too that it works (as it does), but I can't understand why a compiler for embedded µC (where the interrupts are basically used often) generates assembler code that doesn't assure the data-validity of global variables at each time (in my example the content of the global variable is for a while wrong!).

The declaration as volatile each global variable that is read also in a interrupt routine is a good work-around for a small number of selected variables. But I think, the compiler should do it better!!

Thanks a lot!

   Luca

Luca,

The volatile keyword is still not guaranteed protection against interrupt issues.  The generated assembly code is showing that the compiler did a read-modify-write of the dst value.  In your case this may be acceptable since you were worried about an interrupt service routine reading an erroenous value from dst.bit.  But do any of the ISRs also modify the dst.bit?  If so, you can have problems if the interrupt strikes between the read of dst (i.e., the AND AH, @0x25, #0xFFFE), and the write back to dst (i.e., the MOV @0x25, AL).  The write back by the background routine will overwrite the write performed by the ISR.

Really, the proper way to guard against interrupt issues is to either disable interrupts around the code of concern, or in some cases you can use the atomic intrinsics as I illustrated in my earlier post.  This is exactly why the atomic instructions exist on the device at the assembly code level, and also why the intrinsics exist in the compiler.

Luca Parisi said:

The declaration as volatile each global variable that is read also in a interrupt routine is a good work-around for a small number of selected variables. But I think, the compiler should do it better!!

What would you have the compiler do?  The compiler doesn't know that the variable may be read in an ISR. You're asking the compiler to read your mind and know that this particular variable is used in both your background loop and in ISRs,  Would you have the compiler assume that every variable may be used by an ISR?  Then you'd have incredibly bloated and inefficient code, possibly to the point where you couldn't even run it on the device.  Compilers do not know anything about interrupt routines.  Where ISRs are a concern, the user needs to explicitly guard against it in their code on an as needed basis.  All compilers are going to work this way.

Regards,

David

David,

the variable ist written ONLY in the background-task! As it should be! Don't worry! ;-)

David M. Alter
All compilers are going to work this way.

That's right, but not all compilers use a global variable as temporary buffer! I worked with other µC, but I never had the problem that the content of a global variable is temporary corrupted! Normally You have just to take care about the read-modify-write issue and to avoid write access on the same variable from different contexts.

Should I now suppose that every global variable (not just bitfield-stuctures) may have the same problem? i.e.
a = b + c + d + e;
generates a "code" like
a = b + c;
a += d;
a += e;

If not: what's different here?

Regards,

    Luca

Luca,

Unless 'a' is declared volatile, the compiler COULD do exactly what you showed:

a = b + c + d + e

generates

a = b + c; // store to a
a += d; //store to a
a += e; // store to a

The compiler probably wouldn't do the above, but it COULD. There are no guarantees.  Actually, on second thought, if the compiler optimizer is off, the compiler probably does do something like the above (multiple stores to 'a').
 
- David

Later edit of this post ... It turns I am not entirely correct here.  Please see my added comment below, and my next post(s) in this thread.

David M. Alter said:

Volatile just says to the compiler that the variable could change outside of its control.

Volatile says more than that.

David M. Alter said:

That doesn't stop the compiler from doing whatever intermediate steps it wants to take to perform its operation.

Actually, it does.  Please see this wiki article about volatile.  Note in particular this sentence ...

The number of volatile reads and writes must be exactly as they appear in the source code; no more and no less, and in the same order.

So, in this case, declaring Flags volatile should solve the problem.

Added comment ... Using volatile does solve this particular problem.  But it is not a general solution to this problem.

Thanks and regards,

-George

George Mock said:

Actually, it does.  Please see this wiki article about volatile.  Note in particular this sentence ...

The number of volatile reads and writes must be exactly as they appear in the source code; no more and no less, and in the same order.

So, in this case, declaring Flags volatile should solve the problem.

That's a TI definition, isn't it? The C standard does not seem to attempt to give any such guarantee, and frankly, in the context of bit-fields, I don't even understand what it is supposed to mean. After all, the original bit-field assignment

Flags2.bit0 = Flags1.bit1;

does not imply any read-access of Flags2 in the abstract machine, yet, on any platform (without dedicated bitfield machine instructions) this cannot be implemented without a read-modify-update cycle. So there appears to be one more read than there is in the source code.

The C standard explicitly says that in the context of signals (which imo is comparable to isr's in spirit) values of objects that may be modified between the previous and the next sequence point may not be relied upon.

Markus

EDIT:
I see that the compiler documentation says

The TI compiler does not change the number of accesses to a volatile variable unless absolutely necessary. This is
significant for read-modify-write expressions such as += ; for an architecture which does not have a
corresponding read-modify-write instruction, the compiler will be forced to use two accesses, one for
the read and one for the write.

Does this imply that the compiler will only emit one write instruction for volatile accesses? It says it tries not to change the number of accesses to the variable unless necessary, but in the above situation the choice is between doing two writes or a read followed by a write, both of which are two accesses.

EDIT2:

Assuming that the bit-field struct is not larger than one word, to be completely certain, wouldn't it be better to make Flags2 volatile and write

struct Flags2_Struct localFlags2 = Flags2;
localFlags2.bit0 = Flags1.bit1;
Flags2 = localFlags2;

thereby introducing more sequence points and leaving the compiler less freedom.

C99 section 6.7.3 Type Qualifiers paragraph 6 says: "... any expression referring to such an object shall be evaluated strictly according to the rules of the abstract machine, as described in 5.1.2.3." This means that there must be exactly the right number of reads and writes, and in the same order.  However, the same paragraph says "What constitutes an access to an object that has volatile-qualified type is implementation-defined." This is where we get into trouble with bit-fields (which often don't match a native type size) and read-write-modify instructions.  The standard does not really explain what it "means" to have a volatile access; this is left to the implementation.

ISO/IEC 9899:1999 "An object that has volatile-qualified type may be modified in ways unknown to the implementation or have other unknown side effects."

In my case the variable is written in the background task and read from a ISR. From the point of view of a standard context (background), the variable shouldn't be a "volatile" because there is no other "unknown" context where the variable can be written/modified!

How I wrote: the use of "volatile" in these case is a good work-around, but my opinion is that the compiler shouldn’t use global variables as temporary buffer! Basically!
These should be the object of the discussion. :-)

Best regards

   Luca

I think this thread has raised some good points.  So I filed SDSCM00051501 in SDOWP as an enhancement request.  While it is filed in the same system, it is not a bug report.  It is a way to request some other change in the compiler.  In this particular case, it amounts to a request for the compiler development team to continue the debate on this issue.  I cannot predict what will result.  We may change the compiler.  We may improve our documentation.  Or perhaps some combination.  You are welcome to use the SDOWP link below in my signature to track this issue.

Thanks and regards,

-George

Archaeologist
From my 1st thread above:

C-code:
Flags2.bit0 = Flags1.bit1;

Assembler:
008845:   1825FFFE    AND          @0x25, #0xfffe <-- Flag2.bit0 is now 0 - corrupt!!
008847:   761F021B    MOVW         DP, #0x21b
008849:   CC280002    AND          AL, @0x28, #0x2
00884b:   761F0218    MOVW         DP, #0x218
00884d:   FFC0        LSR          AL, 1
00884e:   9825        OR           @0x25, AL <-- just now Flag2.bit0 is valid again (written with the value of Flag1.bit1)

Best regards

   Luca