#1435 Incorrect asm for call via function pointer

closed-fixed
Maarten Brock
5
2013-05-25
2008-03-06
HarryMc
No

I have been able to recreate some incorrectly generated mcs51 assembler that has prevented execution of the qpnano statecharts framework; see:
http://www.quantum-leaps.com/downloads/index.htm#QPN

I copied the failing code from qpnano into a single file mm.c (attached) which can be built three ways:

sdcc mm.c --stack-auto --model-small --i-code-in-asm -o small

sdcc mm.c -DMEDIUM_FUNC --stack-auto --model-small --i-code-in-asm -o medium

sdcc mm.c -DLARGE_FUNC --stack-auto --model-small --i-code-in-asm -o large

sdcc -v is

SDCC : mcs51/gbz80/z80/avr/ds390/pic16/pic14/TININative/xa51/ds400/hc08 2.6.0 #4309 (Oct 15 2006) (UNIX)

on debian etch.

In mm.c, as the size of the function QHsm_dispatch() grows from small to medium, a function pointer is stored in the stack frame and is copied from the frame before being called. This results in the following asm fragment:

; mm.c:90: t = (QHsmState)((*s)(me)); /* invoke state handler s */
; [01234---] ic:12: send iTemp0 [k2 lr3:44 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct QHsmTag generic* }{ sir@ _QHsm_dispatch_me_1_1}[r2 r3 r4 ]{argreg = 1}
; [01234---] ic:13: iTemp1 [k4 lr5:37 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{void function ( struct QFsmTag generic* )
code* function ( struct QHsmTag generic* ) code* }{ sir@ _QHsm_dispatch_t_1_1}[_QHsm_dispatch_t_1_1] = pcall iTemp1 [k4 lr5:37 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{void function ( struct QFsmTag generic* ) code* function ( struct QHsmTag
generic* ) code* }{ sir@ _QHsm_dispatch_t_1_1}[_QHsm_dispatch_t_1_1]
; genPcall
push ar2
push ar3
push ar4
push ar0
push ar1
mov a,#00120$ <---- return address
push acc
mov a,#(00120$ >> 8)
push acc
push ar0
mov a,_bp
add a,#0x0d
mov r0,a
mov a,@r0
push acc <---- function pointer
inc r0
mov a,@r0
push acc
pop ar0 <------------------ fail
mov dpl,r2
mov dph,r3
mov b,r4
ret
00120$:
push ar0
push acc
mov a,_bp
add a,#0x0d
mov r0,a
pop acc
mov @r0,dpl
inc r0
mov @r0,dph
pop ar0
pop ar1
pop ar0
pop ar4
pop ar3
pop ar2

As you can see, the function pointer that has been pushed onto the stack is partially discarded when the the index register ar0 is restored.

The small function example doesn't store the function pointer in the stack frame and works fine.

The large function example shows the pointer argument "me" also being loaded off the stack into dph, dpl, and b prior to the call using "ret" but this second push and pop doesn't disrupt the stack. So over some function size where a function pointer is stored in the stack frame, the call to the pointed-to function is incorrectly made.

I have looked at src/mcs51/gen.c:genPcall (iCode * ic)
and the problem arises in:

3074: /* now push the calling address */
3075: aopOp (IC_LEFT (ic), ic, FALSE);
3076:
3077: pushSide (IC_LEFT (ic), FPTRSIZE);
3078:
3079: freeAsmop (IC_LEFT (ic), NULL, ic, TRUE);

where aopOp() saves the r0 or r1 index register which is later restored by freeAsmop()

I tried moving the aopOp() above the return address assignment and freeAsmop() below the return address label and this corrects the push and pop but also moves the load of r0 or r1 with the stack frame address which is not a solution :-/ The patch:

---------------------------------------------------------
--- gen_orig.c 2006-06-25 00:37:52.000000000 +0800
+++ gen.c 2008-03-06 11:25:16.000000000 +0900
@@ -3065,6 +3065,9 @@
}
else
{
+ /* save operand early for "push the calling address" */
+ aopOp (IC_LEFT (ic), ic, FALSE);
+
/* push the return address on to the stack */
emitcode ("mov", "a,#%05d$", (rlbl->key + 100));
emitcode ("push", "acc");
@@ -3072,12 +3075,8 @@
emitcode ("push", "acc");

/* now push the calling address */
- aopOp (IC_LEFT (ic), ic, FALSE);
-
pushSide (IC_LEFT (ic), FPTRSIZE);

- freeAsmop (IC_LEFT (ic), NULL, ic, TRUE);
-
/* if send set is not empty the assign */
if (_G.sendSet)
{
@@ -3094,6 +3093,9 @@
/* make the call */
emitcode ("ret", "");
emitLabel (rlbl);
+
+ /* restore the operand after return for "push the calling address" */
+ freeAsmop (IC_LEFT (ic), NULL, ic, TRUE);
}
}
if (swapBanks)
-----------------------------------------------------------------------

Unfortunately, looking at aopForSym(), I can't see a way to separate the stack save and loading of r0 for the ic.

If someone can suggest a way to pre-allocate an index register r0 for the ic and save as required above the return address assignment in such a way that aopOp() only loads r0 then I think moving the freeAsmop() below the return address label would release the r0 if required and behave correctly.

I don't have enough knowledge of sdcc to know if this will cause unintended consequences for other non-r0 access methods.

Latest cvs has no change in genPcall (iCode * ic).

I didn't examine all of the other target archs. Some use different code for genPcall (iCode * ic) but (at least) src/hc08/gen.c seems to be a similar implementation that would need checking.

Any ideas most welcome as this stops further work which incorporates qpnano.

Discussion

  • HarryMc
    HarryMc
    2008-03-06

    Example file to recreate bug

     
    Attachments
  • Maarten Brock
    Maarten Brock
    2008-03-07

    • assigned_to: nobody --> maartenbrock
     
  • Maarten Brock
    Maarten Brock
    2008-03-08

    Logged In: YES
    user_id=888171
    Originator: NO

    Fixed in SDCC 2.7.5 #5075.

     
  • Maarten Brock
    Maarten Brock
    2008-03-08

    • milestone: --> fixed
    • status: open --> closed-fixed
     
  • Maarten Brock
    Maarten Brock
    2008-03-08

    Logged In: YES
    user_id=888171
    Originator: NO

    The code in main also has a bug in itself. The pointer cast needs the address of AO_derived:

    QHsm_dispatch((QHsm *)&AO_derived);

    Just to be sure I have also changed hc08 and ds390, though I don't think they will ever show this bug.
    I have also added a regression test in #5077.

     
  • HarryMc
    HarryMc
    2008-03-09

    Logged In: YES
    user_id=1520229
    Originator: YES

    Thank you for responding so promptly to this bug Maarten.

    I've built a deb from trunk (now #5080) and I am able to move forward with qpnano.

    Best wishes
    Harry