#4154 mcs51: spillThis() frees registers that a dead live range no longer owns

[d00f]

With 4.5.0 and trunk r17011, with or without the patch from #4153, -mmcs51 can store through a pointer whose high byte register has been given to another value. The attached test fails on info_while[2] == 2 in mcs51-small, mcs51-large and mcs51-small-stack-auto.

void rcv(uint8_t port) __reentrant {
  if (info_while[port] &&
          cmp((__xdata uint8_t *)((__xdata struct pkt *)buf)->bridge.mac,
                   &dbridge[port].mac, 1) ||
      dpid[port])
    return;
  rxage[port] = rxfwd[port] = clamp(
      ((__xdata struct pkt *)&buf[rxbody])->fwd_delay, 4, 30, 15);
  info_while[port] = port;
}

With sdcc -mmcs51 -S on the test (plus -I for testfwk.h) the end of rcv() is:

lcall _clamp
mov   r4, dpl
...
mov   dpl,r3
mov   dph,r4
movx  @dptr,a

The address of info_while[port] is block-spilt around the cmp() call and gets r3:r4 back at the IPOP. A later stack spill makes spillLRWithPtrReg() call spillThis() on temps whose live ranges have ended, and spillThis() frees every register left in their regs[], r4 included.

The patch frees them only while the symbol is still in _G.regAssigned. It does not depend on [#4153]. ds390 has the same code.

With it the new test no longer fails in those three models or on host, and the mcs51-small, mcs51-small-stack-auto and mcs51-large regression runs show no new failures.

[d00f]

The patch against r17011 is attached. It has the three-line change in src/mcs51/ralloc.c and the test as support/regression/tests/bug-4154.c. Without the change the test fails on info_while[2] in mcs51-small, mcs51-large and mcs51-small-stack-auto. It applies on its own and together with the patch from #4153.

[d00f]

A short follow-up in case it makes this easier to look at. Below is a standalone version of the test, without the regression framework. It prints PASS or FAIL through the ucsim simif interface:

sdcc -mmcs51 repro-4154.c
printf 'run\nquit\n' | s51 -t 8052 -I 'if=xram[0x7654]' repro-4154.ihx

With plain 4.5.0 it prints FAIL in the small, large and stack-auto models, and with the patch from this ticket it prints PASS. I also tried shrinking it with cvise, but the function under test barely got smaller, so it seems to need all of those statements to run out of registers.

#include <stdint.h>

struct bridge {
  uint8_t mac;
};
__xdata uint8_t buf[16];
__xdata struct bridge dbridge[4];
__xdata uint16_t dpid[4];
__xdata uint8_t rxage[4];
__xdata uint8_t rxfwd[4];
__xdata uint16_t info_while[4];
uint8_t rxbody;
struct pkt {
  struct bridge bridge;
  uint16_t fwd_delay;
};
int8_t cmp(__xdata uint8_t *, __xdata uint8_t *, uint8_t) __reentrant;
uint8_t clamp(uint8_t, uint8_t, uint8_t, uint8_t);
void rcv(uint8_t port) __reentrant {
  if (info_while[port] &&
          cmp((__xdata uint8_t *)((__xdata struct pkt *)buf)->bridge.mac,
                   &dbridge[port].mac, 1) ||
      dpid[port])
    return;
  rxage[port] = rxfwd[port] = clamp(
      ((__xdata struct pkt *)&buf[rxbody])->fwd_delay, 4, 30, 15);
  info_while[port] = port;
}

int8_t cmp(__xdata uint8_t *a, __xdata uint8_t *b, uint8_t n) __reentrant
{
  while (n--)
    if (*a++ != *b++)
      return 1;
  return 0;
}

uint8_t clamp(uint8_t v, uint8_t lo, uint8_t hi, uint8_t d)
{
  return (v < lo || v > hi) ? d : v;
}

#define SIMIF (*(volatile __xdata char *)0x7654)

static void say(const char *s)
{
  while (*s) {
    SIMIF = 'p';
    SIMIF = *s++;
  }
}

void main(void)
{
  ((__xdata struct pkt *)buf)->fwd_delay = 9;
  rxbody = 0;
  rcv(2);
  say(info_while[2] == 2 ? "PASS\n" : "FAIL: info_while[2] not stored\n");
  SIMIF = 's';
  for (;;)
    ;
}

If another form of report or patch would be easier for you to review, just let me know and I'll redo it. Thanks for taking a look.

[Philipp Klaus Krause]

I can reproduce the issue using SDCC built from current trunk on my Debian GNU/Linux testing system.
Clearly an issue in register allocation, as we can see in the asm (compiled with --no-peep --i-code-in-asm), since two variables (iTemp3 and iTemp30) with overlapping live-ranges both are (partially) allocated to r4.

;   [---34567] ic:50:   iTemp30 [k50 lr40:42 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-char fixed}[r4 ] = call _clamp [k36 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-char function (unsigned-char fixed, unsigned-char fixed, unsigned-char fixed, unsigned-char fixed) fixed}
;   genCall
;   genSend
    mov dpl, r4
    push    ar7
    push    ar6
    push    ar5
    push    ar3
    lcall   _clamp
    mov r4, dpl
    pop ar3
    pop ar5
    pop ar6
    pop ar7
;   [---34567] ic:51:   *(iTemp22 [k35 lr30:41 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-char __xdata* fixed}{ sir@ _rcv_sloc4_1_0}[_rcv_sloc4_1_0]) := iTemp30 [k50 lr40:42 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-char fixed}[r4 ]
;   genPointerSet
;   genFarPointerSet
    mov a,_bp
    add a,#0x07
    mov r0,a
    mov dpl,@r0
    inc r0
    mov dph,@r0
    mov a,r4
    movx    @dptr,a
;   [---34567] ic:52:   *(iTemp20 [k32 lr28:42 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-char __xdata* fixed}[r5 r6 ]) := iTemp30 [k50 lr40:42 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-char fixed}[r4 ]
;   genPointerSet
;   genFarPointerSet
    mov dpl,r5
    mov dph,r6
    mov a,r4
    movx    @dptr,a
;   test.c:27: info_while[port] = port;
;   [---34567] ic:56:   iTemp34 [k54 lr43:44 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-int fixed}[r5 r6 ] = (unsigned-int fixed)iTemp0 [k2 lr3:43 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-char auto}{ sir@ _rcv_port_10000_3}[r7 ]
;   genCast
    mov ar5,r7
    mov r6,#0x00
;   [---3456-] ic:57:   *(iTemp3 [k7 lr6:44 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-int __xdata* fixed}[r3 r4 ]) := iTemp34 [k54 lr43:44 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-int fixed}[r5 r6 ]
;   genPointerSet
;   genFarPointerSet
    mov dpl,r3
    mov dph,r4
    mov a,r5
    movx    @dptr,a
    mov a,r6
    inc dptr
    movx    @dptr,a

[d00f]

The r4 overlap comes from spillThis() freeing registers that the symbol no longer owns.

Traced on r17021, where the call result is iTemp29 instead of iTemp30: at ic 40 spilSomething() puts iTemp0 on the stack. As the first stack spill it calls spillLRWithPtrReg(iTemp0), which spills every range using r0/r1 that does not end before iTemp0 starts (3). That includes iTemp13 [r0 r4], range 13:16, which had already ended and given its registers back; r4 then went to iTemp3 [r3 r4], range 6:43. spillThis(iTemp13) frees r0 and r4 again, so r4 counts as free while iTemp3 still holds it, and the call result gets r4.

The patch frees registers in spillThis() only while the symbol is still in _G.regAssigned; it still clears regs[] and spills the symbol as before. If you'd rather filter ended ranges in spillLRWithPtrReg() instead, just say so and I'll rework it.

[Maarten Brock]

Patch is applied in [r17039], thanks..