This discussion has been locked.
You can no longer post new replies to this discussion. If you have a question you can start a new discussion

C51 V6.20 & V6.21 still have reported bug

On Nov. 11, 2001, I sent in a C51 V6.20 bug report via e-mail to support.us@keil.com. I never received a response, so assumed Keil's policy was to silently introduce the bug fix in a subsequent update. After downloading V6.21, I see that the bug is still present. Now I'm wondering whether my e-mailed bug report was ever acted upon, so I'm going to try the forum as the means to report the bug.

I have a section of code with pointer incrementation that compiles and executes correctly when the pointer is defined as a generic pointer or as an xdata pointer, or when the optimization setting is reduced from the default OT(8,SPEED) to OT(7,SPEED). However, when the pointer is defined as a data or idata pointer, the compiler does not emit code to increment the pointer with default optimization.

I have provided example code demonstrating the problem. The specific statement is line 50 below.

unsigned char idata u64[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
unsigned char idata s[21];

void U64ToStr( char idata *s, unsigned char idata *binP, unsigned char n )
{
    unsigned char        bcd;
    unsigned char idata *bcdP;
    unsigned char        bitMsk = 0x80;
    unsigned char        carry;
    unsigned char        i;

    /*  Zero the target buffer (NUL-terminating the string).
     */
    for (bcdP = s, i = n + 1; i != 0; *bcdP++ = 0, i--)
        ;

    /*  Convert 64 bits of binary to unpacked BCD.  For each bit, starting
     *  with the MSBit, BCD = (BCD * 2) + bit.
     */
    for (i = 64; i != 0; i--)
    {
        carry = (*binP & bitMsk) ? 1 : 0;

        for (bcdP = s + n; bcdP != s; /* Iter in body */)
        {
            bcd  = *--bcdP * 2;
            bcd += carry;

            if (bcd < 10)
                carry = 0;
            else
            {
                carry = 1;
                bcd  -= 10;
            }

            *bcdP = bcd;
        }

        if ((bitMsk >>= 1) == 0)
        {
            bitMsk = 0x80;
            binP++;
        }
    }

    /*  ASCII'fy BCD.
     */
    for (i = n; i != 0; i--)
        *bcdP++ += '0'; /* <-- C51 BUG: bcdP doesn't increment for (i)data */
}

void main( void )
{
    for (;;)
        U64ToStr( s, u64, 6 );
}

--Dan Henry

Parents
  •     /*  ASCII'fy BCD.
         */
        for (i = n; i != 0; i--)
            *bcdP++ += '0'; /* <-- C51 BUG: bcdP doesn't increment for (i)data */

    I have had problems before with packing too much stuff into a single line - particularly the pre/post increment/decrement operators.
    I have seen C51 get this wrong before!

    Have you tried separating-out the operations; eg,
        /*  ASCII'fy BCD.
         */
        for (i = n; i != 0; i--)
        {
            *bcdP += '0';
            ++bcdP;
        }

    BTW: I have found that the above is not necessarily more efficient than, eg,
        for (i = n; i != 0; i--)
        {
            bcdP[i] += '0';
        }
    You would have to check carefully with the generated assembler to verify which is really the best in your particular application!

Reply
  •     /*  ASCII'fy BCD.
         */
        for (i = n; i != 0; i--)
            *bcdP++ += '0'; /* <-- C51 BUG: bcdP doesn't increment for (i)data */

    I have had problems before with packing too much stuff into a single line - particularly the pre/post increment/decrement operators.
    I have seen C51 get this wrong before!

    Have you tried separating-out the operations; eg,
        /*  ASCII'fy BCD.
         */
        for (i = n; i != 0; i--)
        {
            *bcdP += '0';
            ++bcdP;
        }

    BTW: I have found that the above is not necessarily more efficient than, eg,
        for (i = n; i != 0; i--)
        {
            bcdP[i] += '0';
        }
    You would have to check carefully with the generated assembler to verify which is really the best in your particular application!

Children
  • Thanks for your thoughts Andrew.

    Regarding "I have had problems before with packing too much stuff into a single line - particularly the pre/post increment/decrement operators.", the compiler handles the statement fine if I change "binP" to "s":

        for (i = n; i != 0; i--)
            *s++ += '0';    /* Using 's' does work. */
    
    ;---- Variable 's' assigned to Register 'R7' ----
    ;---- Variable 'i' assigned to Register 'R4' ----
    
    0061 AC00        R     MOV     R4,n
    0063         ?C0014:
    0063 EC                MOV     A,R4
    0064 600C              JZ      ?C0017
    0066 AE07              MOV     R6,AR7
    0068 0F                INC     R7       ; <-- ***
    0069 A806              MOV     R0,AR6
    006B 7430              MOV     A,#030H
    006D 26                ADD     A,@R0
    006E F6                MOV     @R0,A
    006F 1C                DEC     R4
    0070 80F1              SJMP    ?C0014
    

    As for "separating-out the operations", interestingly, the bug is still present:

        for (i = n; i != 0; i--)
        {
            *bcdP += '0';
            ++bcdP;
        }
    
    ;---- Variable 'n' assigned to Register 'R1' ----
    ;---- Variable 'bcdP' assigned to Register 'R5' ----
    ;---- Variable 'i' assigned to Register 'R4' ----
    
    0063 AC01              MOV     R4,AR1
    0065         ?C0014:
    0065 EC                MOV     A,R4
    0066 6009              JZ      ?C0017
    0068 A805              MOV     R0,AR5
    006A 7430              MOV     A,#030H
    006C 26                ADD     A,@R0
    006D F6                MOV     @R0,A
    006E 1C                DEC     R4
    006F 80F4              SJMP    ?C0014
    0071         ?C0017:
    

    With optimization reduced by one (to 7) both of our snippets compile correctly. Here's the output of the separated-out version with optimization lowered to 7:

    ;---- Variable 'n' assigned to Register 'R1' ----
    ;---- Variable 'bcdP' assigned to Register 'R5' ----
    ;---- Variable 'i' assigned to Register 'R4' ----
    
    0064 AC01              MOV     R4,AR1
    0066         ?C0014:
    0066 EC                MOV     A,R4
    0067 600A              JZ      ?C0017
    0069 A805              MOV     R0,AR5
    006B 7430              MOV     A,#030H
    006D 26                ADD     A,@R0
    006E F6                MOV     @R0,A
    006F 0D                INC     R5       ; <-- ***
    0070 1C                DEC     R4
    0071 80F3              SJMP    ?C0014
    0073         ?C0017: