Codewarrior 7.2.1 compiler issue identified

[vier_kuifjes]

In the past I have reported about an issue with a modified Coldfire Lite when compiled with Codewarrior 7.2.x. It took me a while, but I have now found the cause of the issue (and the proof that there actually is one!).

There appears to be a change in order of assembler instructions that cause the function of an IF statement to not work correctly. This change in order happens at optimisation levels 2 or higher.

This is the piece of code in which it happens. This piece of code is part of the file tcpout.c in Coldfire Lite 3.2.

   /*    * If we're sending a SYN, check the IP address of the interface    * that we will (likely) use to send the IP datagram -- if it's    * changed from what is in the template (as it might if this is    * a retransmission, and the original SYN caused PPP to start    * bringing the interface up, and PPP has got a new IP address    * via IPCP), update the template and the inpcb with the new     * address.    */   if (flags & TH_SYN)   {      ip_addr src=ip_mymach(pip->ip_src);      if (src != pip->ip_src)      {         pip->ip_src=src;         tp->t_template->ti_i.ip_src=src;         so->lhost = src;      }   }

When compiled/disassembled at optimisation level 1, the result reads...

;  410:    ptcp->th_ack = (tp->rcv_nxt);
;  411:    
;  412:       /*
;  413:        * If we're sending a SYN, check the IP address of the interface
;  414:        * that we will (likely) use to send the IP datagram -- if it's
;  415:        * changed from what is in the template (as it might if this is
;  416:        * a retransmission, and the original SYN caused PPP to start
;  417:        * bringing the interface up, and PPP has got a new IP address
;  418:        * via IPCP), update the template and the inpcb with the new 
;  419:        * address.
;  420:        */
;
0x00000262  0x256B00500008           move.l   80(a3),8(a2)
;
;  421:    if (flags & TH_SYN)
;  422:       {
;
0x00000268  0x08050001               btst     #1,d5
0x0000026C  0x6728                   beq.s    *+42                  ; 0x00000296
;
;  423:       ip_addr src=ip_mymach(pip->ip_src);
;  424:       
;
0x0000026E  0x207900000000           movea.l  _m_netp,a0
0x00000274  0x20280028               move.l   40(a0),d0
;
;  425:       if (src != pip->ip_src)
;  426:             {
;
0x00000278  0x206F0014               movea.l  20(a7),a0
0x0000027C  0xB0A8000C               cmp.l    12(a0),d0
0x00000280  0x6714                   beq.s    *+22                  ; 0x00000296
;
;  427:          pip->ip_src=src;
;
0x00000282  0x206F0014               movea.l  20(a7),a0
0x00000286  0x2140000C               move.l   d0,12(a0)
;
;  428:          tp->t_template->ti_i.ip_src=src;
;
0x0000028A  0x206B002C               movea.l  44(a3),a0
0x0000028E  0x2140000C               move.l   d0,12(a0)
;
;  429:          so->lhost = src;
;  430:                }
;  431:             }

 ...which is fine. The BEQ instruction follows directly after the BTST instruction.
But when the optimisation level is bumped one level higher, the disassembly reads...

0x0000028A  0x7002                   moveq    #2,d0
0x0000028C  0xC1AF001C               and.l    d0,28(a7)
;
;  410:    ptcp->th_ack = (tp->rcv_nxt);
;  411:    
;  412:       /*
;  413:        * If we're sending a SYN, check the IP address of the interface
;  414:        * that we will (likely) use to send the IP datagram -- if it's
;  415:        * changed from what is in the template (as it might if this is
;  416:        * a retransmission, and the original SYN caused PPP to start
;  417:        * bringing the interface up, and PPP has got a new IP address
;  418:        * via IPCP), update the template and the inpcb with the new 
;  419:        * address.
;  420:        */
;
0x00000290  0x2D6B00500008           move.l   80(a3),8(a6)
;
;  421:    if (flags & TH_SYN)
;  422:       {
;
0x00000296  0x6728                   beq.s    *+42                  ; 0x000002c0
;
;  423:       ip_addr src=ip_mymach(pip->ip_src);
;  424:       
;
0x00000298  0x207900000000           movea.l  _m_netp,a0
0x0000029E  0x20280028               move.l   40(a0),d0
;
;  425:       if (src != pip->ip_src)
;  426:             {
;
0x000002A2  0x206F002C               movea.l  44(a7),a0
0x000002A6  0xB0A8000C               cmp.l    12(a0),d0
0x000002AA  0x6714                   beq.s    *+22                  ; 0x000002c0
;
;  427:          pip->ip_src=src;
;
0x000002AC  0x206F002C               movea.l  44(a7),a0
0x000002B0  0x2140000C               move.l   d0,12(a0)
;
;  428:          tp->t_template->ti_i.ip_src=src;
;
0x000002B4  0x206B002C               movea.l  44(a3),a0
0x000002B8  0x2140000C               move.l   d0,12(a0)
;
;  429:          so->lhost = src;
;  430:                }
;  431:             }

Here the comparison is done with a MOVEQ and AND.L instruction, but the MOVE.L instruction gets in the way, influencing the Z flag of the status register and leading to incorrect branching.

In the end this causes a wrong TCP checksum to be calculated as the instructions inside the IF statement are not executed, hence a TCP communication failure.

[vier_kuifjes]

Well... the bug is obvious in the disassembly, but technical support wants some working code they can RUN and which must not be InterNiche based, as Interniche Coldfire Lite is not supposed to be used with Codewarrior 7.2. Unfortunately I cannot provide that, so I guess this is where it ends...

I know Coldfire Lite is not state of the art and tech support suggests to use MQX instead. But I have put a lot of work in Coldfire lite to make it stable and reliable. I fixed several bugs, added functionality and made it compatible with CW7.2. It's perfectly useful for me and has a smaller footprint than MQX does, which is important when using the special edition version of codewarrior. Switching my app to MQX would require a lot of additional work and studying. Some day maybe...

[vier_kuifjes]

The issue has just been confirmed by Freescale tech support.

[vier_kuifjes]

I have submitted a service request to Freescale to have this issue investigated.

In the mean time I have found a work around for the issue. Changing line 421 from...

if (flags & TH_SYN)

... to ...

if ((flags & TH_SYN) == TH_SYN)

...fixes the defective code.

[J2MEJediMaster]

Good sleuthing. And thanks for submitting an SR on this.

---Tom

[vier_kuifjes]

I hope someone will look into it. For now the service request (1-686847221) is still unassigned...

[vier_kuifjes]

I'm glad I found it, but it definitely was't easy!