• State Resolved
  • Locked Locked
  • Replies 18 replies
  • Subscribers 80 subscribers
  • Views 4136 views
  • Users 0 members are here
Support feedback
Options
Options
Similar topics

This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

Apparent compiler bug when using bit-fields

Andrew Lucas
Prodigy 105 points


 

I am using the following bit-field definition for Timer A1 control register

typedef struct

{

    unsigned int iflag:   1; // interrupt flag

    unsigned int ie:      1; // interrupt enable

    unsigned int clr:     1; // clears count

    unsigned int bit3:    1; // unused

    unsigned int mode:    2; // mode

    unsigned int ckdiv:   2; // input divider

    unsigned int cksrc:   2; // clock source

    unsigned int unused:  6; // not used

}ta1ctl_reg_t;

 

I also have the following definition:

extern volatile ta1cctl0_reg_t ta1cctl0;


I then initialize it as follows:

 

static void timera1_init(void)

{

    ta1ccr0 = 32768;                      /* start timer counting (1s timer) */ 

    ta1ctl.cksrc  = TA_ACLK;                             /* set timer source */

    ta1ctl.ckdiv  = DIV_BY_1;                                 /* set divisor */

    ta1ctl.mode = UP;                                            /* set mode */

    ta1cctl0.ie = true;                              /* set interrupt enable */

}

given the following definitions:

enum{STOP, UP, CONTINUOUS, UPDOWN};

enum{TA_xCLK, TA_ACLK, TA_SMCLK, TA_TAxCLK_};

enum{DIV_BY_1, DIV_BY_2, DIV_BY_4, DIV_BY_8};

 

 

 

The bug that I found is the following.

- With no optimization turned on, the compiler produces the following output:

 

MOV.W   #0x8000,&Timer1_A3_TA1CCR0

MOV.W   #0x00fc,R15

AND.B   &0x0381,R15

BIS.B   #1,R15

MOV.B   R15,&0x0381

AND.B   #0x003f,&Timer1_A3_TA1CTL

MOV.W   #0x00cf,R15

AND.B   &Timer1_A3_TA1CTL,R15

BIS.B   #0x0010,R15

MOV.B   R15,&Timer1_A3_TA1CTL

MOV.W   #0x00ef,R15

AND.B   &Timer1_A3_TA1CCTL0,R15

BIS.B   #0x0010,R15

MOV.B   R15,&Timer1_A3_TA1CCTL0

RETA


 

- With optimization turned to 1 and optimize for speed set to 5 the compiler produces the following output:

MOV.W   #0x8000,&Timer1_A3_TA1CCR0

MOV.W   #0xfdff,R15

AND.W   &Timer1_A3_TA1CTL,R15

BIS.W   #0x0100,R15

MOV.W   R15,&Timer1_A3_TA1CTL

AND.W   #0xff3f,&Timer1_A3_TA1CTL

MOV.W   #0xffdf,R15

AND.W   &Timer1_A3_TA1CTL,R15

BIS.W   #0x0010,R15

MOV.W   R15,&Timer1_A3_TA1CTL

BIS.W   #0x0010,&Timer1_A3_TA1CCTL0

RETA

 

 

It is the first case with no optimization that has a bug.

The line of code:

    ta1ctl.cksrc  = TA_ACLK;                             /* set timer source */

should set bit 8 and clear bit 9 of the register.  Instead it is setting bit 0 and clearing bit 1.

With optimization turned on there is no problem and the correct bits are set and cleared.

I am using Code Composer Studio with version 3.3.3 of the code generation tools

Is TI aware of this?

 

  • 0
    Guru 227245 points

    Andrew Lucas said:
    should set bit 8 and clear bit 9 of the register.  Instead it is setting bit 0 and clearing bit 1.

    No. If you tae a close look, you'll see that the destination address is not Tiemr1_A3_TA1CTL, but rather 0x0381, which is the highbyte of the TA1CTL register. So indeed bit 8 is set and bit 9 of the word register is cleared.
    I only wonder why the compiler is generating an absolute address for the highbyte, but a symbolic address for th elowbyte. Both is valid, but why doing it this way here and that way there...

    Anyway, the code is correct.

  • 0 in reply to Jens-Michael Gross
    Prodigy 105 points

    I havn't actually analysed the assembly code in detail, all I can say is that running under the debugger the unoptimized code does not work.

    I am watching the timer register in the debug window and the unoptimized instructions are setting bit 1 not bit 8, subsequently the code fails to work.  When I use the optimized code it works correctly.

  • 0 in reply to Andrew Lucas
    Prodigy 105 points

    I stepped through it again and here is what happens

    After the following instructions

    MOV.W   #0x00fc,R15

    AND.B   &0x0381,R15

    BIS.B   #1,R15

    then R15 = 0x0001

     

    This next instruction then sets address 0x380 = 1 and 0x381 = 0.  I not sure I understand why, but that is what is happening.

    MOV.B   R15,&0x0381

  • 0 in reply to Andrew Lucas
    Prodigy 105 points

    I think I may have found something.  From the book MSP430 Microcontroller Basics.

     

    5.4.7 Illegal Operations

    TI's documentation says little about illegal operations. In newer devices, such as the F2013, a reset is generated if the CPU tries to fetch an instruction from the range of memory allocated to the special function and peripheral registers (addresses below 0x0200). That is about it. There are many other possible illegal operations so I experimented. Needless to say, you should never write code that relies on any form of illegal operation and the effects will be different in other variants.

    Fetching Data

    Here are three possible illegal operations.

    • Data can be fetched from a nonexistent address: empty regions of the memory map. Execution proceeds as normal but the "fetched" value is random.

    • Data for a word should be aligned to even addresses but you could attempt to fetch a word from an odd address. I found that the lsb of the address was ignored and the word was fetched from the resulting even address.

    • A related case of this is the range of peripheral registers with word access. It appears that the lsb of their addresses is ignored because they are intended to be read only as words. It is possible to read the lower byte alone but not the upper byte: An attempt to read the upper byte returns the lower byte instead.

      Would this last item be a culprit.  For peripheral registers is it valid to try and write the upper byte of a peripheral register directly?  Is the compiler doing this when it isn't supposed to?

     

  • 0 in reply to Andrew Lucas
    Guru 227245 points

    It's possible that the register is a word-access register. That means the LSB of the address is effectively ignored and so the writes to the high byte go to the low byte and clear the high byte. I had something similar with the 54xx DMA registers (but there the were optionally defined as byte registers in teh users guide and this was definitely a documentation bug)
    Normally (if not using a bitfield), the register is defined as a volatile unsigned word, so any access is done with word instructions.
    By defining it as a bitfield, however, this implicit restriction is removed. The volatile keyword only instructs the compiler to access the bits not more or less than once per C instruction, but does not make it an atomic word access. The compiler does not and cannot know that this particular register requires word access and cannot be accessed byte-wise. (well, read access to the LSB works, but a byte write to the LSB clears the MSB and a read access to the MSB returns the LSB and a write to the MSB isntead clears the MSB and writes to the LSB)

    These bitfields, however, are highly ineffective for hardware registers (even if there is no problem with word access). Just because every assignment to a member of the bitfield has to be executed as an individual read-modify-write cycle. Which is longer and slower than directly setting or modifying the register with a combined (OR) value of all bits. The compiler may not group the bit accesses to the bitfield members (if the field is volatile). If it would do so, what would you say if the compiler would group two subsequent bit toggles to a single instruction that does nothing? I hear you shouting 'compiler bug' :)

  • 0 in reply to Jens-Michael Gross
    Prodigy 105 points

    Thanks for the detailed response.

    The reason that I like using bit fields is that it is essentially abstracting away all of the bit masking, which in my mind is error prone and much less readable.  I realize that they are slightly less efficient, but if and when I need the speed or code density increase I will use masking, otherwise, I feel the advantages of bit fields outweigh their disadvantages.

    I would have thought that if I define the bit field as an unsigned int then the compiler would use word accesses to do the behind the scenes masking.  I know that it isn't standard C but with this compiler I can define bit fields as unsigned chars as well, which to my mind should cause the compiler to use byte accesses to do the masking.

    It seems to me to be a rather poor implementation of bit fields on the part of the compiler.

  • 0 in reply to Andrew Lucas
    Guru 227245 points

    Andrew Lucas said:
    It seems to me to be a rather poor implementation of bit fields on the part of the compiler.

    In theory, it allows bitfields of any length, even long or long long, without too much overhead.

    You can try adding th evolatile keyword to the type declaration or the field members rather than the variable declaration, perhaps this makes a difference.

    Andrew Lucas said:
    he reason that I like using bit fields is that it is essentially abstracting away all of the bit masking, which in my mind is error prone and much less readable.

    It's a matter of taste. Anyway, there are several hardware registers where bitfields simply not work, e.g. those with a password. Or interrupt vector registers, where reading the register removes the interrupt state, no matter which abstract bit member you accessed.
    Or other registers where it is necessary to set several bits simultaneously. Or where the additional access for each bitfield access has unwanted (and perhaps at first unniticed) side effects. MCU programming is not an abstract thing, it has to deal with real world realtime requirements and restrictions. being as close to the hardware as possible is the key to success. Seeing a physical register as a register and not as an abstract construct reduces the possibility of problems which are very difficult to track down because the code looks so 'clear' where it isn't clear at all.

    There is a reason why the people who wrote the mspgcc compiler, while being from the linux world where things like code abstration were invented, have removed the support of register bitfields from the header files. :)

  • 0 in reply to Jens-Michael Gross
    Prodigy 105 points

    Your points are well taken.  Thanks.

  • 0 in reply to Jens-Michael Gross
    Intellectual 490 points

    The result of the discussion between Andrew Lucas and Jens-Michael Gross is something like this:

    AL finds that the compiler generates invalid code for write-access of registers when registers are described by bitfields.
    JMG explains that this is because the compiler may generate an 8 bit write to the register when the register will only support 32 bit writes.
    AL thinks this might be a deficiency in the compiler but JMG says there is no way to improve it.

    I suggest that is a deficiency and could be fixed by providing a pragma which forbids 8 or 16 bit write accesses as a result of bitfield operations.

    AL believes bitfields have coding advantages for readability, but JMG is not convinced.

    I suggest that AL understates the benefits of bitfields. He might mention benefits of writeability and efficiency in debugging. In their book, K&R introduce bitfields for the benefit of programming registers.

    JMG mentions several cases where a one-line bitfield write, ie
       register.field = value;
    will not do the job. No, sometimes it does not work but often it does. When it does not you copy the register, work on the copy, then write it back. It is still better to do that with bitfields.

    The one-line read is usually all that is required.
       value = register.field;

    I have tried to reignite this discussion in a later thread because I want TI to fix the compiler and think about improving how they have been doing the CSL's. The later thread is http://e2e.ti.com/support/development_tools/compiler/f/343/p/382719/1355457.aspx

    -Robert Durkacz

  • 0 in reply to Robert Durkacz1
    Guru 227245 points

    I didn’t deny that bitfields are convenient and it looks better to write “x.y=0;”  than “x &=~(2^n);” :) Even though most people will use predefined symbols and write x &= ~SYMBOL
    However, the main usage of bitfields would be on hardware registers. And using bitfields on them means that every single change of a field member results in one single individual read/modify/write operation on the register, sometimes with unwanted side-effects (not to mention that it is impossible to do on registers with password protection), and always with increased code size and execution time.
    Yes, you can copy the register, work on it and write it back. It’s still additional code and execution time. And additional code, which counteracts the readability.
    Better waste 1 minute to write more efficient code than waste 1ms each of the trillion times the code is executed. On each built device. Especially if the system has not unlimited memory and speed.

    Making life easier is second priority after producing the best possible product. And the use of bitfields negatively affects the result.
    And as we can see every day, people usually use what is offered, without even thinking of the consequences (e.g. using double where an unsigned char would do). On a PC this is a nuisance (and customers pay with more CPU power required), on MCUs, this can be deadly. (“Why does my program that uses gazillions of unnecessary floating point divisions, can only do 10 iterations per second? I need >100, so is there a ten times faster MSP?” - don’t laugh, it was a real case here in the forum)

    Apparently, others shared my opinion about bitfields and hardware registers. There must have been a reason why the definitions have been removed from MSPGCC, IAR and CSS header files, while they have been there on the first versions.

  • 0 in reply to Jens-Michael Gross
    Expert 2505 points

    Jens-Michael Gross said:
    Apparently, others shared my opinion about bitfields and hardware registers. There must have been a reason why the definitions have been removed from MSPGCC, IAR and CSS header files, while they have been there on the first versions.

    For mspgcc, they were present in the original hand-coded headers pre-2010.  When I started maintaining the toolchain I found that manually transcribing content from data sheets and user guides across multiple families into headers was error-prone and wasted time, so I convinced TI to provide the device-specific headers for mspgcc, just as they do for IAR and CCS.

  • 0 in reply to Peter Bigot
    Intellectual 490 points

    I am not convinced by the suggestion from Jens-Michael Gross that bitfield operations on registers would lead to inefficiency. However the source code looks, the usual result should be that the register contents are copied to working memory, modified there, and copied back.

  • 0 in reply to Robert Durkacz1
    Expert 2505 points

    Robert Durkacz1 said:
    I am not convinced by the suggestion from Jens-Michael Gross that bitfield operations on registers would lead to inefficiency. However the source code looks, the usual result should be that the register contents are copied to working memory, modified there, and copied back.

    Not for the MSP430.

  • 0 in reply to Peter Bigot
    Intellectual 490 points

    How would the compiler treat it on that processor then please if I write

           register.field = value;

    ?

  • 0 in reply to Robert Durkacz1
    Expert 2505 points

    Robert Durkacz1 said:
    How would the compiler treat it on that processor then please if I write

           register.field = value;

    ?

    Assume that field is one bit wide and corresponds to the second least significant bit (mask 0x02), and that the value is a constant.  If the value is zero the instruction sequence would be:

            bic     #2, &v

    If the value is one the instruction sequence would be:

            bis     #2, &v

    If the stated assumptions are not correct, other sequences would be used.  The point is that in most cases you do not want to read the peripheral register (memory cell) into an MCU register (let alone a non-register memory location), manipulate it, then write it back, because you get race conditions and the potential of discarding state that was updated in the peripheral register while you were busy manipulating the value it used to hold.

    I don't really want to spend more time on this; suffice it to say that I agree with JMG that using bitfield structures to manipulate hardware registers unnecessarily limits expressiveness in C code and is horrifyingly complex for a compiler that intends to preserve the semantics of the C language.  I'll extend that beyond MSP430 to ARM as well.  Your effort is better spent on other battles.

  • 0 in reply to Peter Bigot
    Guru 227245 points

    If a field is more than one bit wide, then the code gets worse. And far from being an atomic instruction(so an interrupt can interfere). Even worse if the argument is not constant.

    Peter Bigot said:
    suffice it to say that I agree with JMG

    I’ll make a big red X on my calendar :)

     

  • 0 in reply to Jens-Michael Gross
    Intellectual 490 points

    In reply to Peter B and J-M Gross-

    The two contributors are in agreement. Their shared view combines user and compiler considerations. Together their opinion is surely very well-informed but I remain unconvinced and I want to make my objections known at least for posterity. (I am a DSP user but have not got much response in TI's DSP forum. This seems to be the best thread to ask about this problem.)

    --Compiler--
    PB: using bitfield structures to manipulate hardware registers ... is horrifyingly complex for a compiler that intends to preserve the semantics of the C language.

    Yet the compiler does support bitfields and as JMG and K&R say the main purpose is for registers. But there is a fixable problem (fixable by a pragma) that the compiler generates invalid code for registers when the register simply does not support, for example, byte writes. Why not provide the pragma?

    If we don't use bitfields for registers then we use shifting/masking. Why could not the compiler generate the same result if that is the best way to do it?

    --User--
    PB: The point is that in most cases you do not want to read the peripheral register (memory cell) into an MCU register (let alone a non-register memory location), manipulate it, then write it back, because you get race conditions and the potential of discarding state that was updated in the peripheral register while you were busy manipulating the value it used to hold.

    JMG: If a field is more than one bit wide, then the code gets worse. And far from being an atomic instruction(so an interrupt can interfere). Even worse if the argument is not constant.

    It looks like a bitfield write is not atomic and neither is the mask/shift procedure. Either way, as a user, I understand it is my responsibility to take that into account and I believe I can do that.

    PB: using bitfield structures to manipulate hardware registers unnecessarily limits expressiveness in C code.


    This statement is not understood. It is generally agreed that the merit of bitfields is that it is an expressive language feature.

    -Robert Durkacz

  • 0 in reply to Robert Durkacz1
    Guru 227245 points
    On the compiler side, yes, a pragma could fix it. Or generally accessing word-sized bitfields as words and byte-sized bitfields as bytes. The main problem is that the C language does not contain the concept of by-wise or word-wise-only access. For C, all is contiguous memory. While there can be a workaround using pragmas, this is unexpected and will probably lead as many people into a trap as it will help people. It's not the only such situation. A similar problem exists when mapping structs to a byte stream buffer. Since the compiler does not know about the runtime alignment of the pointers in the buffer, it needs to access the members byte-wise. What about bitfields applied to storage variables instead of the register (e.g. for register backup purposes or multiple stored configurations). Each attempt to make it transparently work for one, will make it intransparently fail for another user.
    However, if you provide bitfields, then you are responsible for making it always work for everyone. Which is next to impossible. SO don't provide them, and if a user wants them, he can do it himself. And either succeed or fail on his own account.

    From the users side, yes, as a user, you are responsible for writing working code and taking things like this into account. However, a line x.y=z obfuscates that it is a possibly complex, multi-opcode, non-atomic operation (while x=z would be atomic for byte or word).
    A line like x = (x&y)|z is obviously non-atomic.
    You'd be surprised how many people think that starting an ADC conversion does mean that the next instruction can work with the result. You should read the datasheet. But how may do this looking for problems they don't imagine the could exist?
    If people would always read and understand the whole users guide and datasheet and errata sheet before starting to write code, we wouldn't have to talk about this at all. And the forum would be perhaps 1/4 of its size, if at all. But the very most think that it is sufficient to have learned how to code in C. And then they fail and wonder why. Bitfields of hardware registers would increase their number.

**Attention** This is a public forum