Menu
▾
▴
[sdcc-devel] [ sdcc-Bugs-1908493 ] Incorrect asm for call via function pointer
|
From: SourceForge.net <no...@so...> - 2008-03-07 23:39:44
|
Bugs item #1908493, was opened at 2008-03-06 07:17 Message generated for change (Settings changed) made by maartenbrock You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1908493&group_id=599 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: msc51(8051) target Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: HarryMc (harrymc) >Assigned to: Maarten Brock (maartenbrock) Summary: Incorrect asm for call via function pointer Initial Comment: 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. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1908493&group_id=599 |