#3381 Incorrect compilation starting in r10263

[Clayton Smith]

Firmware for the RfCat project ( https://github.com/atlas0fd00m/rfcat/ ) fails to run properly when built with SDCC 3.8.0 or later. I did some investigation, and I believe this is due to a bug in SDCC. My findings are summarized in https://github.com/atlas0fd00m/rfcat/issues/121.

I bisected the issue, and it appears that SDCC began producing invalid output in r10263. The problem can be reproduced as follows:

git clone https://github.com/atlas0fd00m/rfcat.git
cd rfcat/firmware
git checkout 7106b017cc1568b451410a4e23a7aa3b83c07b7b
sdcc -Iinclude -DBUILD_VERSION=`../revision.sh` -DDONSDONGLES -DCC1111 -DUSBDEVICE -DUSB_DEVICE_SERIAL_NUMBER="`./new_serial.py`" -c chipcon_usb.c

Observe in the resulting chipcon_usb.asm that line 1014 appears to be compiled incorrectly:

;       chipcon_usb.c:1014: ep5.OUTlen += len;
        mov     a,_handleOUTEP5_len_65536_175
        add     a,r7
        mov     r6,a
        mov     a,(_handleOUTEP5_len_65536_175 + 1)
        addc    a,r6
        mov     r7,a
        mov     dptr,#(_ep5 + 0x0008)
        mov     a,r6
        movx    @dptr,a
        mov     a,r7
        inc     dptr
        movx    @dptr,a

At the start of this line, r7 contains the least significant byte of ep5.OUTlen, and r6 contains the most significant byte. The first two instructions add the least significant bytes of len and ep5.OUTlen, but the result is then stored in r6, clobbering the most significant byte of ep5.OUTlen and corrupting the most significant byte of the sum.

This code was produced with SDCC : mcs51/z80/z180/r2k/r2ka/r3ka/sm83/tlcs90/ez80_z80/z80n/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8/pdk13/pdk14/pdk15/mos6502 4.2.0 #13081 (Linux).

Prior to revision 10263, r7 caches the most significant byte of ep5.OUTlen and r6 caches the least significant byte, and the line compiles correctly:

;   chipcon_usb.c:1014: ep5.OUTlen += len;
    mov a,_handleOUTEP5_len_1_175
    add a,r6
    mov r6,a
    mov a,(_handleOUTEP5_len_1_175 + 1)
    addc    a,r7
    mov r7,a
    mov dptr,#(_ep5 + 0x0008)
    mov a,r6
    movx    @dptr,a
    mov a,r7
    inc dptr
    movx    @dptr,a

[Philipp Klaus Krause]

Could you provide a small, compileable code sample to reproduce the issue? SDCC has nearly no inter-procedural optimizations, so putting the affected function together with the necessary declarations into a source file is usually a good starting point.

[Konstantin Kim]

I cannot reproduce it. For #10526 #13384 it looks ok for MINGW64.

3.7.2 #10526

;   chipcon_usb.c:1014: ep5.OUTlen += len;
    mov dptr,#(_ep5 + 0x0008)
    movx    a,@dptr
    mov r6,a
    inc dptr
    movx    a,@dptr
    mov r7,a
    mov a,_handleOUTEP5_len_65536_175
    add a,r6
    mov r6,a
    mov a,(_handleOUTEP5_len_65536_175 + 1)
    addc    a,r7
    mov r7,a
    mov dptr,#(_ep5 + 0x0008)
    mov a,r6
    movx    @dptr,a
    mov a,r7
    inc dptr
    movx    @dptr,a
;   chipcon_usb.c:1016: while (!(DMAIRQ & usbdmaarm));

4.2.2 #13384

;   chipcon_usb.c:1014: ep5.OUTlen += len;
    mov dptr,#(_ep5 + 0x0008)
    movx    a,@dptr
    mov r6,a
    inc dptr
    movx    a,@dptr
    mov r7,a
    mov a,_handleOUTEP5_len_65536_175
    add a,r6
    mov r6,a
    mov a,(_handleOUTEP5_len_65536_175 + 1)
    addc    a,r7
    mov r7,a
    mov dptr,#(_ep5 + 0x0008)
    mov a,r6
    movx    @dptr,a
    mov a,r7
    inc dptr
    movx    @dptr,a
;   chipcon_usb.c:1016: while (!(DMAIRQ & usbdmaarm));

[Konstantin Kim]

may you show chipcon_usb.asm output file with "-S --fverbose-asm" options instead of "-c" ?

[Konstantin Kim]

btw SDCC emit some (offtopic) warnings at lines 680 and 766. bug?

    if (req.wLength)
        ep0.epstatus == EP_STATE_RX;

    switch(req.bmRequestType & USB_BM_REQTYPE_TYPEMASK)
    ...

[Clayton Smith]

Konstantin Kim: Did you check out revision 7106b017cc1568b451410a4e23a7aa3b83c07b7b of rfcat before compiling? The master branch has a workaround which marks ep5.OUTlen as volatile to prevent it from being cached to registers.

[Clayton Smith]

I was able to distill the problematic code down to this single file which still demonstrates the issue (with SDCC 4.2.0 #13081):

#include <cc1110.h>

SFRX(USBCNTL,   0xDE16);
SFRX(USBCNTH,   0xDE17);

typedef struct {
    unsigned int  event;
} USB_STATE;

typedef struct {
    unsigned char* OUTbuf;
    unsigned int   OUTlen;
    unsigned int   OUTbytesleft;
    volatile unsigned char flags;
} USB_EP_IO_BUF;

typedef struct DMA_DESC_S {
    unsigned char destAddrH;
    unsigned char destAddrL;
} DMA_DESC;

USB_STATE usb_data;
__xdata USB_EP_IO_BUF     ep5;
__xdata DMA_DESC *usbdma;

int main(void)
{
    unsigned int len;
    __xdata unsigned char* ptr;

    len = USBCNTL;
    len += (unsigned int)(USBCNTH<<8);

    ptr = &ep5.OUTbuf[0] + ep5.OUTlen;

    usbdma->destAddrH = ((unsigned int)ptr)>>8;
    usbdma->destAddrL = ((unsigned int)ptr)&0xff;

    if (len + ep5.OUTlen > 516)
        len = 516 - ep5.OUTlen;

    ep5.OUTlen += len;

    if (ep5.OUTlen >= ep5.OUTbytesleft)
    {
        ep5.flags |= 2;
        ep5.OUTbytesleft = 0;
        usb_data.event &= ~(unsigned int)0x4000;
        return 1;
    }

    return 0;
}

The compilation of line 42 still looks broken:

;   bug3381.c:42: ep5.OUTlen += len;
    mov a,_main_len_65536_2
    add a,r7
    mov r6,a
    mov a,(_main_len_65536_2 + 1)
    addc    a,r6
    mov r7,a
    mov dptr,#(_ep5 + 0x0003)
    mov a,r6
    movx    @dptr,a
    mov a,r7
    inc dptr
    movx    @dptr,a

[Clayton Smith]

Further simplified:

#include <cc1110.h>

__xdata struct {
    unsigned char* a;
    unsigned int b;
} foo;

struct {
    unsigned char c;
} bar;

int main()
{
    unsigned int len = ADDR << 8;
    __xdata unsigned char* ptr = &foo.a[0] + foo.b;
    bar.c = (unsigned int)ptr;
    if (len + foo.b > 516)
        len = 516 - foo.b;
    foo.b += len;
    return 0;
}

This produces the following:

;   bug3381.c:19: foo.b += len;
    mov a,_main_len_65536_1
    add a,r7
    mov r6,a
    mov a,(_main_len_65536_1 + 1)
    addc    a,r6
    mov r7,a
    mov dptr,#(_foo + 0x0003)
    mov a,r6
    movx    @dptr,a
    mov a,r7
    inc dptr
    movx    @dptr,a

[Philipp Klaus Krause]

Thanks. I can reproduce the issue:

~~~~
; test.c:41: foo.b += len;
; [------67] ic:21: iTemp30 [k44 lr21:22 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{unsigned-int fixed}[r6 r7 ] = iTemp18 [k29 lr15:21 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{unsigned-int fixed}[r7 r6 ] + _main_len_65536_1 [k2 lr5:21 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{unsigned-int fixed}{ sir@ _main_len_65536_1}[_main_len_65536_1]
; genPlus
mov a,_main_len_65536_1
add a,r7
mov r6,a
mov a,(_main_len_65536_1 + 1)
addc a,r6
mov r7,a
~~~~

Apparently code generation can't deal with what the register allocator does here (i.e. addition where the result uses the same registers as an operand, but swapped). I've fixed similar issues in backends that use the new register allocator, but mcs51 is still using the old one, so this would be different. If no mcs51 expert finds the time to look into this, I'll try sometime.

[Konstantin Kim]

a bit more of simplifications:

__xdata char ADDR, *ptr;

__xdata struct {
    char* a;
    int b;
} foo;

struct {
    char c;
} bar;

void test() {
    int len = ADDR&1;
    ptr = &foo.a[foo.b];
    bar.c++;
    if (foo.b) len -= foo.b;
    foo.b += len;
}

[Maarten Brock]

I once gave the register allocator a preference to allocate the LSB in a lower register than the MSB so it could give a normal address for a little-endian variable/itemp to a debugger. Is that failing here? Or was it removed?