A strange error which destroys array content

simonsan
2007-07-16
2013-03-12
  • simonsan

    simonsan - 2007-07-16

    Hi!

    I use an 89S52 I/O port to read a keypad and store the inputs in a static key buffer array.

    My code is something like this:

    if(R1 & R2 & R3 & R4)
    do_something();
    else do_other_thing();

    Where R1~R4 are I/O pins.

    I found whenever a key press caused R4 to become low,  a particular key buffer array element's content would be destroyed. The stack started at 0x62, so it was not caused by stack overflow.

    I complied it with version 2.6 and 2.7, same error occurred.
    If I use logical AND(&&) instead of bitwise AND(&), then everything seems alright.
    I noted bitwise AND(&) employed a local variable to handle the intermediate AND result, while logical AND(&&) used none.

    I also compiled the program with KEIL compiler, and found no problem at all.

    Is this a bug of the SDCC?

     
    • Maarten Brock

      Maarten Brock - 2007-07-16

      How are R1~R4 declared?
      Can you show the generated asm code?

      Maarten

       
      • simonsan

        simonsan - 2007-07-17

        Hi! Maarten

        My declaration is as follows:

        sbit R1 = 0x90; // R1~R4 are rows, and C1~C3 are columns of a 4x3 keypad
        sbit R2 = 0x92;
        sbit R3 = 0x93;
        sbit R4 = 0x94;
        sbit C1 = 0x95;
        sbit C2 = 0x96;
        sbit C3 = 0x97;

        The C code is as follows:

            P1 = 0xff;            /* set rows and columns high */
            C1 = C2 = C3 = 0;        /* all columns low */
            if (R1 & R2 & R3 & R4)
             {
                if (kp_flag == 1)    // no key press, clear flag
                 {                      // and return
                    kp_flag = 0;
                     delay_50ms();
                 }
                return 0xff;
             }
            else if (kp_flag == 1)
                     return 0xff;
                ... key processing here

        The ASM generated is as follows:

           02AB                    1100 _read_key:
                                   1101 ;    wop.c:546: byte row_status = 0, key_code = 0;
           02AB 7A 00              1102     mov    r2,#0x00
                                   1103 ;    wop.c:549: feed_dog();  /* feed watch dog */
           02AD C0 02              1104     push    ar2
           02AF 12 10 7B           1105     lcall    _feed_dog
           02B2 D0 02              1106     pop    ar2
                                   1107 ;    wop.c:550: P1 = 0xff;    /* set rows and columns high */
           02B4 75 90 FF           1108     mov    _P1,#0xFF
                                   1109 ;    wop.c:551: C1 = C2 = C3 = 0;    /* all columns low */
           02B7 C2 97              1110     clr    _C3
           02B9 C2 96              1111     clr    _C2
           02BB C2 95              1112     clr    _C1
                                   1113 ;    wop.c:552: if (R1 & R2 & R3 & R4)
           02BD A2 90              1114     mov    c,_R1
           02BF 82 92              1115     anl    c,_R2
           02C1 92 62              1116     mov    _read_key_sloc0_1_0,c
           02C3 A2 93              1117     mov    c,_R3
           02C5 82 62              1118     anl    c,_read_key_sloc0_1_0
           02C7 92 62              1119     mov    _read_key_sloc0_1_0,c
           02C9 A2 62              1120     mov    c,_read_key_sloc0_1_0
           02CB 82 94              1121     anl    c,_R4
           02CD 50 0F              1122     jnc    00106$
                                   1123 ;    wop.c:554: if (kp_flag == 1)    // no key press, clear flag
           02CF 74 01              1124     mov    a,#0x01
           02D1 B5 29 06           1125     cjne    a,_kp_flag,00102$
                                   1126 ;    wop.c:556: kp_flag = 0;
           02D4 75 29 00           1127     mov    _kp_flag,#0x00
                                   1128 ;    wop.c:557: delay_50ms();
           02D7 12 03 85           1129     lcall    _delay_50ms
           02DA                    1130 00102$:
                                   1131 ;    wop.c:559: return 0xff;
           02DA 75 82 FF           1132     mov    dpl,#0xFF
           02DD 22                 1133     ret
           02DE                    1134 00106$:
                                   1135 ;    wop.c:561: else if (kp_flag == 1)
           02DE 74 01              1136     mov    a,#0x01
           02E0 B5 29 04           1137     cjne    a,_kp_flag,00107$
                                   1138 ;    wop.c:562: return 0xff;
           02E3 75 82 FF           1139     mov    dpl,#0xFF
           02E6 22                 1140     ret
           02E7                    1141 00107$:
                                                ... key processing here

         
        • Raphael Neider

          Raphael Neider - 2007-07-17

          I cannot read 8051 assembler, but probably you want

          sbit __at(0x90) R1;

          instead of (Keil'ish?)

          sbit R1 = 0x90; // R1~R4 are rows, and C1~C3 are columns of a 4x3 keypad

          The latter would create a bit variable somewhere and initialize it with 0x90... See compiler.h for a vendor-independant way do declare bits.

          HTH,
          Raphael

           
          • simonsan

            simonsan - 2007-07-17

            Thank you,  tecodev.

            The declaration on my last post is incorrect.

            The actual declaration is as follows:
            sbit at 0x90 R1;
            sbit at 0x92 R2;
            sbit at 0x93 R3;
            sbit at 0x94 R4;
            sbit at 0x95 C1;
            sbit at 0x96 C2;
            sbit at 0x97 C3;

             
    • wek

      wek - 2007-07-17

      I think this fully qualifies as an error. The _read_key_sloc0_1_0
      automatic variable is created apparently as a sfr bit and the assembler/linker allocates it as standard data.

      There are two errors here: one is the compiler creating a temporary variable as sbit, two is the assembler/linker accepting it as data without throwing an error.

      Of course I am too stupid to tell how to proceed, that's upon the gurus... :-)

      JW

       
      • Maarten Brock

        Maarten Brock - 2007-07-17

        Hmm, this is weird indeed. The sloc should have been a __bit in BSEG. And RSEG should normally only contain EQU or = definitions (for either sfr or sbit) in which case it doesn't really matter what memory space is chosen.

        How to proceed should now be obvious: file a bug report in the bug tracker.

         
        • wek

          wek - 2007-07-17

          As I said already, I would see it as 2 problems:
          1. the compiler should generate the sloc properly
          2. the assembler should throw an error when non-EQU/= definition appears in RSEG.

          Testing, I came to 3. too:
          3. the compiler should not allow sbit without __at (there is no reason for the linker to allocate it).

          A crappy test "program" below...

          JW
          -----
          #include<8052.h>

          sbit at 0x90 R1;
          sbit at 0x92 R2;
          sbit at 0x93 R3;
          sbit at 0x94 R4;
          sbit at 0x95 C1;
          sbit at 0x96 C2;
          sbit at 0x97 C3;

          unsigned char kp_flag;

          unsigned char key_scan(void){
          sbit tmp;
          volatile bit tmp2;

                  tmp = C1;
                  tmp2 = C2;
                  C3 = tmp2;

              P1 = 0xff;            /* set rows and columns high */
              C1 = C2 = C3 = 0;        /* all columns low */
              if (R1 & R2 & R3 & R4)
               {
                  if (kp_flag == 1)    // no key press, clear flag
                   {                      // and return
                      kp_flag = 0;
          //             delay_50ms();
                   }
                  return 0xff;
               }
              else if (kp_flag == 1)
                       return 0xff;
                  return 0xff;
          }

          void main(void) {
            key_scan();
          }

           
          • Maarten Brock

            Maarten Brock - 2007-07-17

            Re 3. I think it is already on the Requested Features list not to accept sfr and sbit definitions without an absolute address (unless maybe extern).

             
            • simonsan

              simonsan - 2007-07-18

              Wow, the gurus have identified the bug in no time!
              Thank you very much!

              I will file a bug report accordingly.

               
            • Maarten Brock

              Maarten Brock - 2007-07-18

              Re 1. I've fixed this in SDCC 2.7.3 #4886.

               
              • simonsan

                simonsan - 2007-07-18

                maartenbrock, you are amazing!
                Many thanks for your great works!

                 

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks