EmbeddedRelated.com
Forums
The 2024 Embedded Online Conference

Bug in IAR for MSP430

Started by ratishpunnoose September 23, 2010
I've come across a bug in IAR (lastest version, latest patches).
I have reduced it down to a simple one-line function.

It appears at every optimization level except "None".

I have emailed John Regehr (see his papers on C compiler correctness) and Nigel Jones (http://embeddedgurus.com/stack-overflow/) who concurred that this is a compiler bug and not some C language ambiguity. I have contacted IAR support/development but they don't acknowledge it is a bug.

I hope that the IAR gentleman (Anders) who posts in this forum can help to get this resolved. This is a very common construct and I don't know if it affects other IAR target platforms.

I found this since I tend to compile my applications with multiple compilers to ensure that they always behave identically.

Buggy function
--------
void iar_buggy_func(unsigned char ch)
{
/* Some simple steps to initialze dma omitted.
* But occurs even if they are ommitted */
/* ..... */
DMA1SZ = (uint16_t) &ch;
/* .... */
/* Other steps omitted to wait for dma completion */
}
---------
It occurs with any volatile hardware register as in
----
unsigned char volatile * volatile v3;
void iar_buggy_func3(unsigned char ch)
{
v3 = &ch;
}
---------------
What is the Bug?
Note that the address of the parameter is taken.
The compiler should put this on the stack or some place in memory and then assign the volatile hardware register this address.

The assembly generated by the above function is
------------------------------
iar_buggy_func3:
004080 8321 decd.w SP
v3 = &ch;
004082 4182 1102 mov.w SP,&v3
004086 5321 incd.w SP
004088 4130 ret
------------------------------
Note, that stack space is allocated at the beginning of the
function and released at the end of the function. But the value
of the parameter ch is not stored on the stack.
The volatile register is set to the SP (stack pointer), but
the paramter "ch" is NOT stored at this memory location.

The bug does not occur if
1) Optimization level "None"
2) Another function call f() or even intrinsic function like nop()
is present within the function.
The correct assembly should be
-------------------
iar_buggy_func:
00403E 124C push.b R12
__no_operation();
004040 4303 nop
DMA1SA = (uint16_t)src;
004042 4182 01EA mov.w SP,&DMA1SA
}
004046 5321 incd.w SP
004048 4130 ret
-----------
Note that in this case, the "push" instruction stores the parameter value (passed in R12) onto the stack.

====================================================I've contacted IAR support, but they have blown me off by saying things such as:
1) It may be a glitch that may be fixed in the next release.
2) High optimization may not produce the code intended
3) Providing me with workarounds for this specific function.
and here is the response from IAR development.

>>>>> IAR development response ----------------------------
To condense the problem, we have a function that looks like the following:

volatile unsigned char * v;
void iar_buggy_func(unsigned char ch)
{
v = & ch;
}

In the report you correctly noted that the value of "ch" was never written to the stack.

When it comes to "volatile", the basic rule is that anything that has some kind of side-effect or could be accessed by the underlying hardware should be declared to be "volatile".

In this case, when writing to "v", both "v" and "ch" is accessed by the hardware. Hence, both should be declared "volatile".

As "ch" is a parameter, it is possible to assign it to a local volatile variable before the assignment, for example:

volatile unsigned char * v;
void iar_buggy_func(unsigned char ch)
{
volatile unsigned char ch2 = ch;
v = & ch2;
}
>>>> Done IAR development response -----------------------
This is not correct. It doesn't matter if ch is volatile or not.
The correct assignment must happen.

--------------------------

----------------------
Thanks very much.
Ratish Punnoose

-------

Beginning Microcontrollers with the MSP430

I would say that the bug is that the function iar_buggy_func3 is taking
the address of an argument and then assigning that address to a global
variable, which is a very bad idea because when the function returns the
address thus taken will no longer be valid.

Preston
(the AQ430 C compiler guy)

On 10-09-23 05:15 PM, ratishpunnoose wrote:
> I've come across a bug in IAR (lastest version, latest patches).
> I have reduced it down to a simple one-line function.
>
> It appears at every optimization level except "None".
>
> I have emailed John Regehr (see his papers on C compiler correctness) and Nigel Jones (http://embeddedgurus.com/stack-overflow/) who concurred that this is a compiler bug and not some C language ambiguity. I have contacted IAR support/development but they don't acknowledge it is a bug.
>
> I hope that the IAR gentleman (Anders) who posts in this forum can help to get this resolved. This is a very common construct and I don't know if it affects other IAR target platforms.
>
> I found this since I tend to compile my applications with multiple compilers to ensure that they always behave identically.
>
> Buggy function
> --------
> void iar_buggy_func(unsigned char ch)
> {
> /* Some simple steps to initialze dma omitted.
> * But occurs even if they are ommitted */
> /* ..... */
> DMA1SZ = (uint16_t)&ch;
> /* .... */
> /* Other steps omitted to wait for dma completion */
> }
> ---------
> It occurs with any volatile hardware register as in
> ----
> unsigned char volatile * volatile v3;
> void iar_buggy_func3(unsigned char ch)
> {
> v3 =&ch;
> }
> ---------------
> What is the Bug?
> Note that the address of the parameter is taken.
> The compiler should put this on the stack or some place in memory and then assign the volatile hardware register this address.
>
> The assembly generated by the above function is
> ------------------------------
> iar_buggy_func3:
> 004080 8321 decd.w SP
> v3 =&ch;
> 004082 4182 1102 mov.w SP,&v3
> 004086 5321 incd.w SP
> 004088 4130 ret
> ------------------------------
> Note, that stack space is allocated at the beginning of the
> function and released at the end of the function. But the value
> of the parameter ch is not stored on the stack.
> The volatile register is set to the SP (stack pointer), but
> the paramter "ch" is NOT stored at this memory location.
>
> The bug does not occur if
> 1) Optimization level "None"
> 2) Another function call f() or even intrinsic function like nop()
> is present within the function.
> The correct assembly should be
> -------------------
> iar_buggy_func:
> 00403E 124C push.b R12
> __no_operation();
> 004040 4303 nop
> DMA1SA = (uint16_t)src;
> 004042 4182 01EA mov.w SP,&DMA1SA
> }
> 004046 5321 incd.w SP
> 004048 4130 ret
> -----------
> Note that in this case, the "push" instruction stores the parameter value (passed in R12) onto the stack.
>
> ====================================================> I've contacted IAR support, but they have blown me off by saying things such as:
> 1) It may be a glitch that may be fixed in the next release.
> 2) High optimization may not produce the code intended
> 3) Providing me with workarounds for this specific function.
> and here is the response from IAR development.
>
>
>>>>>> IAR development response ----------------------------
>>>>>>
> To condense the problem, we have a function that looks like the following:
>
> volatile unsigned char * v;
> void iar_buggy_func(unsigned char ch)
> {
> v =& ch;
> }
>
> In the report you correctly noted that the value of "ch" was never written to the stack.
>
> When it comes to "volatile", the basic rule is that anything that has some kind of side-effect or could be accessed by the underlying hardware should be declared to be "volatile".
>
> In this case, when writing to "v", both "v" and "ch" is accessed by the hardware. Hence, both should be declared "volatile".
>
> As "ch" is a parameter, it is possible to assign it to a local volatile variable before the assignment, for example:
>
> volatile unsigned char * v;
> void iar_buggy_func(unsigned char ch)
> {
> volatile unsigned char ch2 = ch;
> v =& ch2;
> }
>
>>>>> Done IAR development response -----------------------
>>>>>
> This is not correct. It doesn't matter if ch is volatile or not.
> The correct assignment must happen.
>
> --------------------------
>
> ----------------------
> Thanks very much.
> Ratish Punnoose
>
> -------
>

Yes, that is correct. That was just for illustration.
For actual use, you would have to have additional code at the end to ensure that the function is done with the use of that variable before exit. ..

Here is a scenario how you would use it.
Setup DMA and provide the dma controller with the address of that variable. Start dma. Wait for dma to be complete before exiting.

The bug would still be present. I just presented the smallest function that still shows the compiler bug.
Ratish

--- In m..., Preston Gurd wrote:
>
> I would say that the bug is that the function iar_buggy_func3 is taking
> the address of an argument and then assigning that address to a global
> variable, which is a very bad idea because when the function returns the
> address thus taken will no longer be valid.
>
> Preston
> (the AQ430 C compiler guy)
>
> On 10-09-23 05:15 PM, ratishpunnoose wrote:
> > I've come across a bug in IAR (lastest version, latest patches).
> > I have reduced it down to a simple one-line function.
> >
> > It appears at every optimization level except "None".
> >
> > I have emailed John Regehr (see his papers on C compiler correctness) and Nigel Jones (http://embeddedgurus.com/stack-overflow/) who concurred that this is a compiler bug and not some C language ambiguity. I have contacted IAR support/development but they don't acknowledge it is a bug.
> >
> > I hope that the IAR gentleman (Anders) who posts in this forum can help to get this resolved. This is a very common construct and I don't know if it affects other IAR target platforms.
> >
> > I found this since I tend to compile my applications with multiple compilers to ensure that they always behave identically.
> >
> > Buggy function
> > --------
> > void iar_buggy_func(unsigned char ch)
> > {
> > /* Some simple steps to initialze dma omitted.
> > * But occurs even if they are ommitted */
> > /* ..... */
> > DMA1SZ = (uint16_t)&ch;
> > /* .... */
> > /* Other steps omitted to wait for dma completion */
> > }
> > ---------
> > It occurs with any volatile hardware register as in
> > ----
> > unsigned char volatile * volatile v3;
> > void iar_buggy_func3(unsigned char ch)
> > {
> > v3 =&ch;
> > }
> > ---------------
> > What is the Bug?
> > Note that the address of the parameter is taken.
> > The compiler should put this on the stack or some place in memory and then assign the volatile hardware register this address.
> >
> > The assembly generated by the above function is
> > ------------------------------
> > iar_buggy_func3:
> > 004080 8321 decd.w SP
> > v3 =&ch;
> > 004082 4182 1102 mov.w SP,&v3
> > 004086 5321 incd.w SP
> > 004088 4130 ret
> > ------------------------------
> > Note, that stack space is allocated at the beginning of the
> > function and released at the end of the function. But the value
> > of the parameter ch is not stored on the stack.
> > The volatile register is set to the SP (stack pointer), but
> > the paramter "ch" is NOT stored at this memory location.
> >
> > The bug does not occur if
> > 1) Optimization level "None"
> > 2) Another function call f() or even intrinsic function like nop()
> > is present within the function.
> > The correct assembly should be
> > -------------------
> > iar_buggy_func:
> > 00403E 124C push.b R12
> > __no_operation();
> > 004040 4303 nop
> > DMA1SA = (uint16_t)src;
> > 004042 4182 01EA mov.w SP,&DMA1SA
> > }
> > 004046 5321 incd.w SP
> > 004048 4130 ret
> > -----------
> > Note that in this case, the "push" instruction stores the parameter value (passed in R12) onto the stack.
> >
> > ====================================================> > I've contacted IAR support, but they have blown me off by saying things such as:
> > 1) It may be a glitch that may be fixed in the next release.
> > 2) High optimization may not produce the code intended
> > 3) Providing me with workarounds for this specific function.
> > and here is the response from IAR development.
> >
> >
> >>>>>> IAR development response ----------------------------
> >>>>>>
> > To condense the problem, we have a function that looks like the following:
> >
> > volatile unsigned char * v;
> > void iar_buggy_func(unsigned char ch)
> > {
> > v =& ch;
> > }
> >
> > In the report you correctly noted that the value of "ch" was never written to the stack.
> >
> > When it comes to "volatile", the basic rule is that anything that has some kind of side-effect or could be accessed by the underlying hardware should be declared to be "volatile".
> >
> > In this case, when writing to "v", both "v" and "ch" is accessed by the hardware. Hence, both should be declared "volatile".
> >
> > As "ch" is a parameter, it is possible to assign it to a local volatile variable before the assignment, for example:
> >
> > volatile unsigned char * v;
> > void iar_buggy_func(unsigned char ch)
> > {
> > volatile unsigned char ch2 = ch;
> > v =& ch2;
> > }
> >
> >>>>> Done IAR development response -----------------------
> >>>>>
> > This is not correct. It doesn't matter if ch is volatile or not.
> > The correct assignment must happen.
> >
> > --------------------------
> >
> > ----------------------
> > Thanks very much.
> > Ratish Punnoose
> >
> >
> >
> > -------
> >
> >
> >
> >
>

Hello,

> Yes, that is correct. That was just for illustration.
> For actual use, you would have to have additional code at the end to
> ensure that the function is done with the use of that variable before
> exit. ..

By "that variable" you mean 'ch'? But there is no other use of 'ch' nor
can there be any modification to it that the compiler knows about because
DMA1SZ is not a pointer and no other pointer is stored nor accessed
indirectly. Therefore by data flow and alias analysis, the compiler does
not need to store the value of 'ch' passed in a register into memory. The
address assignment surely must be done if DMA1SZ is marked volatile, but
as 'ch' immediately goes out of scope without modification, the compiler
has complied with the user's instructions.

> Here is a scenario how you would use it.
> Setup DMA and provide the dma controller with the address of that
> variable. Start dma. Wait for dma to be complete before exiting.

I would understand if you had:

DMA1SZ = (uint16_t)ch;

Ok, that would make sense. But not the &ch. Why would you set the SIZE
of transfer to the ADDRESS of a character? Bizarre! No, I'm afraid
you're wrong, and those esteemed gentlemen are also incorrect in this
case. All that is *required* is that the address of 'ch' is converted to
an integer (and I think you're probably using the wrong integer type there
too, you could use intptr_t) and that is stored in DMA1SZ.

I'm sure you believe you are correct. However, I don't believe you are.

Regards,

> The bug would still be present. I just presented the smallest function
> that still shows the compiler bug.

The function is logically flawed, and so is the reasoning I'm afraid.
'volatile' has a very specific meaning in the ISO standard and that is to
do with ordering of accesses to volatile objects, and what must be done at
certain points in the abstract machine. Be relieved that you are not
developing on a processor where there are out-of-order reads and writes
where you need to include barrier instructions.

You need a different view of 'volatile'. The compiler does not know, and
cannot know, hardware side-effects. So, for many reasons, your example is
invalid.

A converse view is the principle of least astonishment--that is, the
compiler does what the user expects in many cases. This just leads to
pain when porting code. I've had my fill of "this compiles and runs
correctly on compiler x but it doesn't on yours" whining. Even when you
point out to customers their code is in error, they still cling to the
notion that they know better than the engineers that wrote the compiler
and eat the ISO C standard with sugar for breakfast each morning.

In short, each and every function you have presented is not a bug in the
compiler, IMO.

Regards,

-- Paul.
See Paul (CPaul?) therein lies the problem, you're diluting your Vitamin
C with sugar.

Al

On 24/09/2010 5:41 PM, Paul Curtis wrote:
> Hello,
>
> notion that they know better than the engineers that wrote the compiler
> and eat the ISO C standard with sugar for breakfast each morning.
>
> In short, each and every function you have presented is not a bug in the
> compiler, IMO.
>
> Regards,
>
> -- Paul.
>
> See Paul (CPaul?) therein lies the problem, you're diluting your Vitamin
> C with sugar.

S'okay, my elderberry wine is now ready to go into the demijohns. I can
shift my daily sugar intake from breakfast to after dinner. :-)

--
Paul Curtis, Rowley Associates Ltd http://www.rowley.co.uk
SolderCore arriving Winter 2010! http://www.soldercore.com

I mistyped the function. It should have been DMA1SA, which for the
DMA controller is the address in memory of the source to transfer.

Here is the context.
DMA1SA is defined as a volatile hardware register. It is word sized
and is intended to take the address of a place in memory to start dma transfers.

This is how it is defined in the compiler headers
#define DEFW(name, address) __no_init volatile unsigned short name @address;

#define DMA1SA_ (0x01eau)
DEFW( DMA1SA , DMA1SA_)

---------------------
void send_single_byte_over_uart(unsigned char ch)
{
if (dma_used) {
/* Other lines here to initialize dma etc.
*/

/* Provide dma controller with address
to start memory transfer */

DMA1SA = (uint16_t)&ch; /* Yup, the cast should be improved here as you suggested*/

/* Set DMA1SZ to 1 */
/* STart dma */
/* wait for end of dma completion and uart
tx completion */

} else {
/* Write "ch" directly to uart register */
/* Wait for uart tx completion */
}

}
--------------------------

--- In m..., "Paul Curtis" wrote:
> By "that variable" you mean 'ch'? But there is no other use of 'ch' nor
> can there be any modification to it that the compiler knows about because
> DMA1SZ is not a pointer and no other pointer is stored nor accessed
> indirectly. Therefore by data flow and alias analysis, the compiler does
> not need to store the value of 'ch' passed in a register into memory. The
> address assignment surely must be done if DMA1SZ is marked volatile, but
> as 'ch' immediately goes out of scope without modification, the compiler
> has complied with the user's instructions.
>
> > Here is a scenario how you would use it.
> > Setup DMA and provide the dma controller with the address of that
> > variable. Start dma. Wait for dma to be complete before exiting.
>
> I would understand if you had:
>
> DMA1SZ = (uint16_t)ch;
>
> Ok, that would make sense. But not the &ch. Why would you set the SIZE
> of transfer to the ADDRESS of a character? Bizarre! No, I'm afraid
> you're wrong, and those esteemed gentlemen are also incorrect in this
> case. All that is *required* is that the address of 'ch' is converted to
> an integer (and I think you're probably using the wrong integer type there
> too, you could use intptr_t) and that is stored in DMA1SZ.
>
> I'm sure you believe you are correct. However, I don't believe you are.

Strange place to keep your wine, I used to wear mine way back when in
the Army to keep warm.

Al

On 24/09/2010 6:36 PM, Paul Curtis wrote:
>> See Paul (CPaul?) therein lies the problem, you're diluting your Vitamin
>> C with sugar.
>
> S'okay, my elderberry wine is now ready to go into the demijohns. I can
> shift my daily sugar intake from breakfast to after dinner. :-)
>
> --
> Paul Curtis, Rowley Associates Ltd http://www.rowley.co.uk
> SolderCore arriving Winter 2010! http://www.soldercore.com
>
>
Hi,

> I mistyped the function. It should have been DMA1SA, which for the
> DMA controller is the address in memory of the source to transfer.
>
> Here is the context.
> DMA1SA is defined as a volatile hardware register. It is word sized
> and is intended to take the address of a place in memory to start dma
> transfers.
>
> This is how it is defined in the compiler headers
> #define DEFW(name, address) __no_init volatile unsigned short name
@address;
>
> #define DMA1SA_ (0x01eau)
> DEFW( DMA1SA , DMA1SA_)
> ---------------------
> void send_single_byte_over_uart(unsigned char ch)
> {
> if (dma_used) {
> /* Other lines here to initialize dma etc.
> */
>
> /* Provide dma controller with address
> to start memory transfer */
>
> DMA1SA = (uint16_t)&ch; /* Yup, the cast should be improved
> here as you suggested*/
>
> /* Set DMA1SZ to 1 */
> /* STart dma */
> /* wait for end of dma completion and uart
> tx completion */
>
> } else {
> /* Write "ch" directly to uart register */
> /* Wait for uart tx completion */
> }
>
> }

I think you should write this correctly. I think this should work for you:

void
really_send_this(unsigned char ch)
{
volatile unsigned char tx = ch; // Note: the assignment MUST happen
before the ';'
DMA1SA = (int)&tx; // I'm lazy here
DMA1SZ = 1;
start-dma-transfer;
wait-for-DMA-complete;
}

Because there is an explicit assignment to tx, that must happen. Because
it's address is taken, then tx must be stored in memory because DMA1SA is a
pointer to it and it is volatile and the assignment must happen (before the
;)

That is, tx must be assigned before its address is taken and the address
must be stored in DMA1SA.

So, like I said, no bug in the compiler, bug in user code.

The above *must* work. Really. Time to start believing that IAR, when they
told you there was no bug in the compiler were right. Pretty much. Now, if
it doesn't, come back and we'll try to reason why it doesn't.

Regards,

--
Paul Curtis, Rowley Associates Ltd http://www.rowley.co.uk
SolderCore arriving Winter 2010! http://www.soldercore.com

I know I am off the topic again.

I speak and write in Chinese. I was told that there are lots of automated translators that translate English into Chinese and a lot of other languages. But every time I looked at the translations produced by those automated translators, I could not stop laughing. Is there an ISO or ANSI standard for those translators?

--OCY

--- In m..., OneStone wrote:
>
> See Paul (CPaul?) therein lies the problem, you're diluting your Vitamin
> C with sugar.
>
> Al
>
> On 24/09/2010 5:41 PM, Paul Curtis wrote:
> > Hello,
> >
> > notion that they know better than the engineers that wrote the compiler
> > and eat the ISO C standard with sugar for breakfast each morning.
> >
> > In short, each and every function you have presented is not a bug in the
> > compiler, IMO.
> >
> > Regards,
> >
> > -- Paul.
> >
> >
> >
> >
> >
> >
> >

The 2024 Embedded Online Conference